Log warning on same version upgrade to prevent failed status #6273
Conversation
michel-laterman
added
bug
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
| } | ||
|
|
||
| // check version before download | ||
| same, _ := isSameVersion(u.log, currentVersion, packageMetadata{}, version) |
There was a problem hiding this comment.
This check will not work for snapshot upgrades.
The packageMetadata is needed to check on the hash for snapshots in the check below.
There was a problem hiding this comment.
Yes, but it should work for non-snapshot attempts, right?
There was a problem hiding this comment.
@pchila so in this case this will fail because hash is unpopulated right? but then later it is extracted and checked again on line 224. so we lose a bit of time but will not perform upgrade
There was a problem hiding this comment.
Here we are relying on the fact that the current version has a not empty hash compared to the zero value in packageMetadata{} so I don't see how it can ever detect this to be the same version as the current one if the hash in packageMetadata is ""
I guess we will never get true from here... Did you test this ? Is there a case where this returns true ?
There was a problem hiding this comment.
you're correct, I forgot that comparing currentVersion vs version would also compare the hashes
There was a problem hiding this comment.
I've added a new func that does this comparison correctly.
You don't want it to return an error. It should be treated like an idempotent API call and return success because the upgrade was already done successfully in the past. This is to handle the case where someone uses an orchestration tool to ensure all of their agents are a specific version. Without an error, they can unconditionally run Edit: this is also true for the Fleet upgrade action and is what caused the bug. Someone unconditionally upgraded all Fleet managed agents to a specific version regardless of what version they currently were as a way of driving consistency. |
| if errors.Is(err, upgrade.ErrUpgradeSameVersion) { | ||
| // Set upgrade state to completed, but return an error if a same version-upgrade is attempted. | ||
| det.SetState(details.StateCompleted) | ||
| return err |
There was a problem hiding this comment.
I don't think we want to consider this an error, that is what lead to the bug this is fixing in the first place. It needs to be like an idempotent API call. It returns success if the action has already completed successfully once.
|
What does this PR do?
Log warning on same version upgrade and filter error to prevent upgrade status reporting.
If a user attempts to upgrade to the same version, the
elastic-agent upgradecall will exit with an error, but the agent's status will be cleared of any upgrade markers.Include an earlier check for same-version upgrades before artifacts are downloaded to prevent unnecessary downloads
Why is it important?
Currently upgrading to the same version results in the agent reporting a failed state until a successful upgrade can be done.
Testing instructions
Install a stand-alone agent and use
elastic-agent upgrade 9.0.0-SNAPSHOT --source-uri file://path/to/dir --skip-verificationto upgrade the agentFor example:
Checklist
I have made corresponding changes to the documentationI have made corresponding change to the default configuration files./changelog/fragmentsusing the changelog toolDisruptive User Impact
N/A
Related issues