Skip to content
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.

Sign up for GitHub

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

Log warning on same version upgrade to prevent failed status #6273

Merged
merged 18 commits into from
Jan 2, 2025
Merged

Log warning on same version upgrade to prevent failed status #6273

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
29ea961
Log warning on same version upgrade to prevent failed status
michel-laterman Dec 10, 2024
96deaee
Fix linter
michel-laterman Dec 10, 2024
299ab0b
Fix status reporting
michel-laterman Dec 13, 2024
1abfd1f
Merge branch 'main' into fix-same-ver-upgrade
michel-laterman Dec 13, 2024
6cac6df
Update internal/pkg/agent/application/upgrade/upgrade.go
michel-laterman Dec 13, 2024
ea65977
Detect same version upgrade in version check
michel-laterman Dec 13, 2024
5688de5
remove double clear
michel-laterman Dec 13, 2024
1ea127e
Remove pre-check
michel-laterman Dec 17, 2024
72a5106
Add new func for released version comparisons
michel-laterman Dec 17, 2024
14e7875
do not return an error
michel-laterman Dec 18, 2024
4ba234c
Merge branch 'main' into fix-same-ver-upgrade
michel-laterman Dec 18, 2024
b467845
Review feedback
michel-laterman Dec 19, 2024
29ba576
Merge branch 'main' into fix-same-ver-upgrade
michel-laterman Dec 26, 2024
167701b
Merge branch 'main' into fix-same-ver-upgrade
michel-laterman Dec 26, 2024
eff680d
Add comments
michel-laterman Dec 30, 2024
bef8ff6
Merge branch 'main' into fix-same-ver-upgrade
michel-laterman Dec 30, 2024
0232a31
Remove extra log line
michel-laterman Jan 2, 2025
aeb6ee0
Merge branch 'main' into fix-same-ver-upgrade
michel-laterman Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion internal/pkg/agent/application/coordinator/coordinator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,6 @@ func (c *Coordinator) Upgrade(ctx context.Context, version string, sourceURI str
det.SetState(details.StateCompleted)
return err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
c.ClearOverrideState()
det.Fail(err)
return err
}
Expand Down
4 changes: 2 additions & 2 deletions internal/pkg/agent/application/upgrade/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string
// check version before download
same, _ := isSameVersion(u.log, currentVersion, packageMetadata{}, version)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check will not work for snapshot upgrades.
The packageMetadata is needed to check on the hash for snapshots in the check below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it should work for non-snapshot attempts, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

@pchila pchila Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're correct, I forgot that comparing currentVersion vs version would also compare the hashes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a new func that does this comparison correctly.

if same {
u.log.Warnf("Upgrade action skipped: %v", ErrUpgradeSameVersion)
u.log.Warnf("Upgrade action skipped because agent is already at version %s", currentVersion)
return nil, ErrUpgradeSameVersion
}

Expand Down Expand Up @@ -223,7 +223,7 @@ func (u *Upgrader) Upgrade(ctx context.Context, version string, sourceURI string
// Recheck version here in case of a snapshot->snapshot upgrade on the same version.
same, newVersion := isSameVersion(u.log, currentVersion, metadata, version)
if same {
u.log.Warnf("Upgrade action skipped: %v", ErrUpgradeSameVersion)
u.log.Warnf("Upgrade action skipped because agent is already at version %s", currentVersion)
return nil, ErrUpgradeSameVersion
}

Expand Down
2 changes: 1 addition & 1 deletion testing/integration/upgrade_standalone_same_commit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func TestStandaloneUpgradeSameCommit(t *testing.T) {
upgradetest.WithUnprivileged(unprivilegedAvailable),
upgradetest.WithDisableHashCheck(true),
)
assert.ErrorContains(t, err, "failed to find watcher:")
assert.ErrorContains(t, err, "upgrade did not occur because it is the same version")
})

t.Run(fmt.Sprintf("Upgrade on a repackaged version of agent %s (%s)", currentVersion, unPrivilegedString), func(t *testing.T) {
Expand Down
4 changes: 4 additions & 0 deletions testing/upgradetest/upgrader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,11 +332,15 @@ func PerformUpgrade(

upgradeOutput, err := startFixture.Exec(ctx, upgradeCmdArgs)
if err != nil {
logger.Logf("Upgrade err: %v, output: %s", err, upgradeOutput)
// Sometimes the gRPC server shuts down before replying to the command which is expected
// we can determine this state by the EOF error coming from the server.
// If the server is just unavailable/not running, we should not succeed.
// Starting with version 8.13.2, this is handled by the upgrade command itself.
outputString := string(upgradeOutput)
if strings.Contains(outputString, "upgrade did not occur because it is the same version") {
michel-laterman marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("upgrade did not occur because it is the same version")
}
isConnectionInterrupted := strings.Contains(outputString, "Unavailable") && strings.Contains(outputString, "EOF")
if !isConnectionInterrupted {
return fmt.Errorf("failed to start agent upgrade to version %q: %w\n%s", endVersionInfo.Binary.Version, err, upgradeOutput)
Expand Down
Loading