-
Notifications
You must be signed in to change notification settings - Fork 148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restore downloads directory before downloading #2222
Restore downloads directory before downloading #2222
Conversation
This pull request does not have a backport label. Could you fix it @michalpristas? 🙏
NOTE: |
🌐 Coverage report
|
@@ -54,6 +57,10 @@ func (u *Upgrader) downloadArtifact(ctx context.Context, version, sourceURI stri | |||
return "", errors.New(err, "initiating fetcher") | |||
} | |||
|
|||
if err := os.MkdirAll(paths.Downloads(), 0755); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do we really need 0755? why not 0750 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
If we can lower the permissions as @aleksmaus mentioned that would be a plus+
Just to clarify this, particularly the "upgrade is fine or rolled back" part, do we only hit this bug when an upgrade is tried and then rolled back? |
Additional question, is there anyway to automatically remediate agents in this situation without requiring users to run mkdir 100s of times on all affected agent machines? |
@cmacknz yes, after upgrade we cleanup download. so once you start another upgrade you fail. |
also about remedy, this version will be ok without creating it. previous not. this version creates it and ignores path during cleanup |
(cherry picked from commit fdd1465)
(cherry picked from commit fdd1465) Co-authored-by: Michal Pristas <[email protected]>
We need QA validation that the following upgrade path is broken:
Let's also test this path and confirm if the results are the same:
We should then be able to confirm that the following upgrade path works since it includes the fix from this PR:
We should create a regression test case for this situation as a result of this testing as well covering an upgrade from the last minor, to the current minor, to the snapshot version of the next minor release. |
@dikshachauhan-qasource @amolnater-qasource would you be able to look at Craig's comment above and test the mentioned upgrade path? |
if err := os.MkdirAll(paths.Downloads(), 0750); err != nil { | ||
return "", errors.New(err, fmt.Sprintf("failed to create download directory at %s", paths.Downloads())) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michalpristas the downloader implementation looks like it does this internally:
elastic-agent/internal/pkg/agent/application/upgrade/artifact/download/http/downloader.go
Lines 179 to 183 in b8553c5
if destinationDir := filepath.Dir(fullPath); destinationDir != "" && destinationDir != "." { | |
if err := os.MkdirAll(destinationDir, 0755); err != nil { | |
return "", err | |
} | |
} |
What am I missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the change here may not have actually been necessary speaking with Michal.
Hi Team, Thank you for the updates. Thanks! |
Hi Team,
Further we are unable to test this PR on snapshot build as Snapshot versions are not available under agent upgrade flyout, also reported under elastic/kibana#139174 Build details: Please let us know if anything else is required from our end. |
The snapshot versions should be available in specific cloud regions, for example GCP us-west-2. If the official release versions work I am not that concerned about testing the snapshots if this does allow you to test it. |
What does this PR do?
During upgrade we cleanup
downloads
directory after ourselves.This makes troubles when upgrade is fine or rolled back, and then next upgrade needs to happen.
Downloads directory does not exist and upgrade fails
Related PR here: #752
Why is it important?
Running multiple upgrades not possible
Logs from an example failure
Checklist
./changelog/fragments
using the changelog toolNot fixing an actual issue, just one of the issues related to upgrade to 8.6