[Ingest Manager] Make installer atomic on windows #24253

michalpristas · 2021-02-25T19:42:03Z

What does this PR do?

PR fixes issue on windows when on restart while installing beats we end up with partial data.
awaitable installler was introduced which forces app wait for installer finish its job

and

sync is forced for windows after rename is called during install.

Why is it important?

Fixes #24180

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

elasticmachine · 2021-02-25T19:42:05Z

Pinging @elastic/agent (Team:Agent)

elasticmachine · 2021-02-25T19:42:05Z

Pinging @elastic/ingest-management (Team:Ingest Management)

ph · 2021-02-25T20:04:46Z

/package

elasticmachine · 2021-02-25T20:36:19Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: Pull request #24253 updated
Start Time: 2021-03-02T15:29:13.479+0000
Duration: 54 min 4 sec
Commit: 57116a1

Test stats 🧪

Test	Results
Failed	0
Passed	6548
Skipped	16
Total	6564

Trends 🧪

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test	Results
Failed	0
Passed	6548
Skipped	16
Total	6564

blakerouse

Maybe I don't understand how this fixes it, but see my inline comments on what I think this is doing.

I also wonder if all this ensuring that its fully extracted before shutdown is really even worth it. I think we should just think about always extracting before executing the program.

That would ensure:

Elastic Agent is always running what it expects (because the binary was not replaced in the directory)
Because of the point above the window of time to inject a bad binary from extract to starting the program is a extremely small window. (currently because we don't re-extract the binary can be changed as we only check the .asc and .sha512 of the compessed artifact vs the binary itself).

blakerouse · 2021-02-25T20:44:37Z

x-pack/elastic-agent/pkg/artifact/install/zip/zip_installer.go

@@ -120,6 +120,11 @@ func (i *Installer) unzip(artifactPath string) error {
 	}

 	for _, f := range r.File {
+		// if we were cancelled in between
+		if err := ctx.Err(); err != nil {
+			return err


Won't this still leave it broken? If the context is cancelled, then this will stop extracting.

this will propagate error and prevent install to continue
also speeds up cancellation as otherwise it tries to finish loop

This will prevent all of the files in the zip file from being extracted. So that mean that if a SIGTERM occurs it will cancel the context. When the context is cancelled and its in this loop then only part of the zip file will be extracted.

What happens when this returns a context.Cancelled? Does the zip installer then remove the directory because the context was cancelled during the extraction? If that is the case, then this is okay. If it accepts context.Cancelled as an acceptable error, or does not remove the directory then it would still be left in a bad state.

yes that's the case, atomic installer removes the extracted files because error was returned

blakerouse · 2021-02-25T20:46:13Z

x-pack/elastic-agent/pkg/artifact/install/awaitable/awaitable_installer.go

+	i.wg.Add(1)
+	defer i.wg.Done()
+
+	return i.installer.Install(ctx, spec, version, installDir)


Because of the context cancelling won't this still leave it in a bad state? Because if the context is cancelled then the waitgroup would still be marked with Done() and the Wait() would not actually wait for this to finish.

when the context is cancelled we should finish install and return error here, then wg should be mark with done.
if context is not cancelled this should work as usuual.
please elaborate a bit more, i'm not seeing what you see atm

michalpristas · 2021-02-26T06:37:31Z

@blakerouse there are 2 things happening now at this PR
first one is proper cancellation of the task which does not ensure archive is fully unpacked but other way around, it ensures task is cancelled as soon as we know about cancellation and artifact is removed by atomic installer (because error is propagated during unpack and atomic removes on error). for this we're awaiting.

how it works atm:

installer unpacking
shutdown is proposed
installer unpacking
operator shutdown is called, context cancelled, operator stopped
installer still installing because it;s not even aware it should stop
agent shuts down
installer interupted

how this pr changes the flow

installer unpacking
shutdown proposed
installer unpacking
operator shutdown called, context cancelled, wait for installer
zip installed registers cancelled context, exits loop with err
atomic installer sees error, cleans up
awaitable installer returns done when cleanup done
operator shutdown continues
agent exits

the second one is sync of FS operations in Install dir
with unpack we sync file by file
but with remove we need to sync entire directory because when agent exits close to remove, it gets corrupted.
this is done in atomic installer and agent using await makes sure it runs ok before exiting agent

i'm for debating about complete remove before first install but we need to think it through and solve even for endpoint usecase, i see benefits there definitely, but as this needs to be backported to 7.12 i would rather postpone such a change to x

blakerouse

Thank you for taking the time to explain how this is working. I just wanted to be sure that it worked as you where expecting. Glad to hear it is.

michalpristas · 2021-03-02T08:15:36Z

/package

michalpristas · 2021-03-02T11:26:24Z

/package

michalpristas · 2021-03-02T12:06:14Z

finding out whether test failures are related

EricDavisX · 2021-03-02T14:29:57Z

Hi - @dikshachauhan-qasource used an early version of the artifact from our GCP Observability builds and tried out the PR against current 8.0 master cloud build, and reports they did not see the problem (so that is good, if not a 100% definitive confirmation): Her words:

Policies used separately:

default having only system
new policy having all core 4 integrations [ system, linux, windows, Endpoint]

It may not fix all of the e2e-testing failures but may be worthwhile to push in and iterate over. @michalpristas thanks.

[Ingest Manager] Make installer atomic on windows (elastic#24253)

[Ingest Manager] Make installer atomic on windows (#24253)

Cherry-pick #24253 to 7.x: Make installer atomic on windows (#24297)

michalpristas added 6 commits February 25, 2021 17:01

is that it?

1a9e7d5

a

9c2ca6c

files

8a3cff1

tests

2684d9c

polish

d09eb7b

polish

f2d31c9

michalpristas added bug needs_backport PR is waiting to be backported to other branches. Team:Ingest Management Team:Elastic-Agent Label for the Agent team labels Feb 25, 2021

michalpristas self-assigned this Feb 25, 2021

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Feb 25, 2021

michalpristas changed the title ~~Fix installer~~ [Ingest Manager] Make installer atomic on windows Feb 25, 2021

changelog

e22fac2

blakerouse reviewed Feb 25, 2021

View reviewed changes

fmt

e69e68a

blakerouse approved these changes Mar 1, 2021

View reviewed changes

michalpristas added 2 commits March 2, 2021 09:10

tar installer cancells as well

f297533

Merge branch 'master' of github.com:elastic/beats into fix-installer

6404d4d

resolved conflicts with master

9726d00

michalpristas mentioned this pull request Mar 2, 2021

[Ingest Manager]: Agent stays in 'Updating' state on 7.12.0 https://staging.found.no/login build #24150

Closed

Merge branch 'master' of github.com:elastic/beats into fix-installer

57116a1

michalpristas merged commit 09ac584 into elastic:master Mar 2, 2021

michalpristas added a commit to michalpristas/beats that referenced this pull request Mar 2, 2021

[Ingest Manager] Make installer atomic on windows (elastic#24253)

ce90edb

[Ingest Manager] Make installer atomic on windows (elastic#24253)

michalpristas added a commit to michalpristas/beats that referenced this pull request Mar 2, 2021

[Ingest Manager] Make installer atomic on windows (elastic#24253)

5ebb425

[Ingest Manager] Make installer atomic on windows (elastic#24253)

This was referenced Mar 2, 2021

Cherry-pick #24253 to 7.12: Make installer atomic on windows #24296

Merged

Cherry-pick #24253 to 7.x: Make installer atomic on windows #24297

Merged

EricDavisX pushed a commit that referenced this pull request Mar 2, 2021

Cherry-pick #24253 to 7.12: Make installer atomic on windows (#24296)

d931545

[Ingest Manager] Make installer atomic on windows (#24253)

michalpristas added a commit that referenced this pull request Mar 3, 2021

Cherry-pick #24253 to 7.x: Make installer atomic on windows (#24297)

096598e

Cherry-pick #24253 to 7.x: Make installer atomic on windows (#24297)

This was referenced Mar 4, 2021

[Ingest-Manager] Fix windows installer during enroll #24343

Merged

Cherry-pick #24343 to 7.x: Fix windows installer during enroll #24388

Merged

Cherry-pick #24343 to 7.12: Fix windows installer during enroll #24389

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Ingest Manager] Make installer atomic on windows #24253

[Ingest Manager] Make installer atomic on windows #24253

michalpristas commented Feb 25, 2021 •

edited

Loading

elasticmachine commented Feb 25, 2021

elasticmachine commented Feb 25, 2021

ph commented Feb 25, 2021

elasticmachine commented Feb 25, 2021 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

Trends 🧪

Test stats 🧪

blakerouse left a comment

blakerouse Feb 25, 2021

michalpristas Feb 25, 2021 •

edited

Loading

blakerouse Feb 26, 2021

michalpristas Feb 26, 2021

blakerouse Feb 25, 2021

michalpristas Feb 25, 2021 •

edited

Loading

michalpristas commented Feb 26, 2021 •

edited

Loading

blakerouse left a comment

michalpristas commented Mar 2, 2021

michalpristas commented Mar 2, 2021

michalpristas commented Mar 2, 2021

EricDavisX commented Mar 2, 2021

[Ingest Manager] Make installer atomic on windows #24253

[Ingest Manager] Make installer atomic on windows #24253

Conversation

michalpristas commented Feb 25, 2021 • edited Loading

What does this PR do?

Why is it important?

Checklist

elasticmachine commented Feb 25, 2021

elasticmachine commented Feb 25, 2021

ph commented Feb 25, 2021

elasticmachine commented Feb 25, 2021 • edited by jenkins-beats-ci bot Loading

💚 Build Succeeded

Build stats

Test stats 🧪

Trends 🧪

💚 Flaky test report

Test stats 🧪

blakerouse left a comment

Choose a reason for hiding this comment

blakerouse Feb 25, 2021

Choose a reason for hiding this comment

michalpristas Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

blakerouse Feb 26, 2021

Choose a reason for hiding this comment

michalpristas Feb 26, 2021

Choose a reason for hiding this comment

blakerouse Feb 25, 2021

Choose a reason for hiding this comment

michalpristas Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

michalpristas commented Feb 26, 2021 • edited Loading

blakerouse left a comment

Choose a reason for hiding this comment

michalpristas commented Mar 2, 2021

michalpristas commented Mar 2, 2021

michalpristas commented Mar 2, 2021

EricDavisX commented Mar 2, 2021

michalpristas commented Feb 25, 2021 •

edited

Loading

elasticmachine commented Feb 25, 2021 •

edited by jenkins-beats-ci bot

Loading

michalpristas Feb 25, 2021 •

edited

Loading

michalpristas Feb 25, 2021 •

edited

Loading

michalpristas commented Feb 26, 2021 •

edited

Loading