[Ingest Manager] Agent fix snapshot download for upgrade by michalpristas · Pull Request #22175 · elastic/beats

michalpristas · 2020-10-27T09:33:05Z

What does this PR do?

There was a version mismatch for snapshot downloads. WHen upgrading it tried to download current version snapshot instead of desired one.

Also as my connection is acting funny today and is not the fastest i run into issue when timeout occurred frequently when downloading elastic-agent snapshot so i increased timeout for this scenario.

Why is it important?

Snapshot upgrades

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 · 2020-10-27T09:33:07Z

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

…shot

elasticmachine · 2020-10-27T10:09:46Z

💚 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 #22175 updated]
Start Time: 2020-10-27T15:48:49.478+0000
Duration: 36 min 7 sec

Test stats 🧪

Test	Results
Failed	0
Passed	1382
Skipped	6
Total	1388

ruflin · 2020-10-27T10:43:33Z

Is there an easy way to test this change?

ruflin · 2020-10-27T10:48:13Z

x-pack/elastic-agent/pkg/artifact/download/http/downloader.go

 		return "", errors.New(err, "fetching package failed", errors.TypeNetwork, errors.M(errors.MetaKeyURI, sourceURI))
 	}

+	destinationFile, err := os.OpenFile(fullPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, packagePermissions)


This was moved to take the local file into account first? Not sure I fully follow this part here.

I think it was to ensure that the file path to write to can be opened before even starting the HTTP connection.

yes exactly, just a micro optimization

blakerouse

Overall looks good, just a few questions. No blockers.

blakerouse · 2020-10-27T14:00:48Z

x-pack/elastic-agent/pkg/agent/application/upgrade/step_download.go

 	settings := *u.settings
+
+	// agent binaries are a bit larger and do not fit into normal timeout on slower connections
+	settings.Timeout = 120 * time.Second


Is this still enough time? How are you deciding this number?

i was also thining about N x standard timeout from config which seems more ok

blakerouse · 2020-10-27T14:01:38Z

x-pack/elastic-agent/pkg/artifact/config.go

 		SourceURI:       "https://artifacts.elastic.co/downloads/",
 		TargetDirectory: filepath.Join(homePath, "downloads"),
-		Timeout:         30 * time.Second,
+		Timeout:         60 * time.Second, // binaries are a bit larger it might take >30s to download them


You used 120 above and 60 here, why the difference?

this one is used for beats and endpoint, these binaries a smaller so it enables us to fail faster in case somethigns wrong.
i'm considering 2 approaches: have agent timeout *2 of what this regular is set to
or set this to higher number but slow down processing loop in case server responds very slowly

Will it really be slower? A TCP reset will not rely on timeout, it's only in the case the server stops pushing data in the HTTP response. I see that as a very rare-case.

not really slower, but it takes more time to download 100megs than 40, we use this as a timecap for execution of the whole request. not only getting a response

blakerouse · 2020-10-27T14:02:12Z

x-pack/elastic-agent/pkg/artifact/download/http/downloader.go

 		return "", errors.New(err, "fetching package failed", errors.TypeNetwork, errors.M(errors.MetaKeyURI, sourceURI))
 	}

+	destinationFile, err := os.OpenFile(fullPath, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, packagePermissions)


I think it was to ensure that the file path to write to can be opened before even starting the HTTP connection.

[Ingest Manager] Agent fix snapshot download for upgrade (elastic#22175)

…2200) [Ingest Manager] Agent fix snapshot download for upgrade (#22175) (#22200)

…2201) [Ingest Manager] Agent fix snapshot download for upgrade (#22175) (#22201)

* upstream/master: (93 commits) Update commands used in the quick start (elastic#22248) Add interval documentation to `monitor` metricset (elastic#22152) [CI] enable x-pack/packetbeat in the CI (elastic#22252) Fix awscloudwatch input documentation (elastic#22247) Add support for different Azure Cloud environments in the metricbeat azure module (elastic#21044) [CI] support windows-2008-r2 (elastic#19791) protect against accessing undefined variables in sysmon module (elastic#22236) [CI] archive only if failed steps (elastic#22220) Add pe fields to Sysmon module (elastic#22217) [CI][flaky] Support 7.x branches and PRs (elastic#22197) Perfmon - Fix regular expressions to comply to multiple parentheses in instance name and object (elastic#22146) ci: improve linting speed (elastic#22103) Move cloudfoundry tags with metadata to common metadata fields (elastic#22150) [Docs] Update custom beat docs (elastic#22194) [Ingest Manager] Agent fix snapshot download for upgrade (elastic#22175) Update shared-autodiscover.asciidoc (elastic#21827) [DOCS] Warn about compression and Azure Event Hub for Kafka (elastic#21578) [CI][flaky] reporting for PRs in GitHub (elastic#21853) [Packetbeat] Create x-pack magefile (elastic#21979) [Elastic Agent] Fix deb/rpm installation (elastic#22153) ...

michalpristas added bug needs_backport PR is waiting to be backported to other branches. Team:Ingest Management Ingest Management:beta2 Group issues for ingest management beta2 labels Oct 27, 2020

michalpristas self-assigned this Oct 27, 2020

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 Oct 27, 2020

step

6a5a2b5

michalpristas force-pushed the agent-fix-snapshot branch from a26d157 to 6a5a2b5 Compare October 27, 2020 09:33

Merge branch 'master' of github.com:elastic/beats into agent-fix-snap…

6a93606

…shot

ruflin reviewed Oct 27, 2020

View reviewed changes

blakerouse approved these changes Oct 27, 2020

View reviewed changes

unified timeout

fafe325

michalpristas merged commit d395a44 into elastic:master Oct 27, 2020

michalpristas added a commit to michalpristas/beats that referenced this pull request Oct 27, 2020

[Ingest Manager] Agent fix snapshot download for upgrade (elastic#22175)

90eed06

[Ingest Manager] Agent fix snapshot download for upgrade (elastic#22175)

michalpristas added a commit to michalpristas/beats that referenced this pull request Oct 27, 2020

[Ingest Manager] Agent fix snapshot download for upgrade (elastic#22175)

bc6379e

[Ingest Manager] Agent fix snapshot download for upgrade (elastic#22175)

This was referenced Oct 27, 2020

Cherry-pick #22175 to 7.10: Agent fix snapshot download for upgrade #22200

Merged

Cherry-pick #22175 to 7.x: Agent fix snapshot download for upgrade #22201

Merged

michalpristas added a commit that referenced this pull request Oct 27, 2020

[Ingest Manager] Agent fix snapshot download for upgrade (#22175) (#2…

9dab96b

…2200) [Ingest Manager] Agent fix snapshot download for upgrade (#22175) (#22200)

michalpristas added a commit that referenced this pull request Oct 27, 2020

[Ingest Manager] Agent fix snapshot download for upgrade (#22175) (#2…

c05c5ef

…2201) [Ingest Manager] Agent fix snapshot download for upgrade (#22175) (#22201)

Conversation

michalpristas commented Oct 27, 2020

What does this PR do?

Why is it important?

Checklist

Uh oh!

elasticmachine commented Oct 27, 2020

Uh oh!

elasticmachine commented Oct 27, 2020 • edited by jenkins-beats-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Uh oh!

ruflin commented Oct 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blakerouse left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elasticmachine commented Oct 27, 2020 •

edited by jenkins-beats-ci bot

Loading