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.

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

fix for duplicate applying nodes #133

Merged
merged 1 commit into from
Aug 25, 2021
Merged

fix for duplicate applying nodes #133

merged 1 commit into from
Aug 25, 2021

Conversation

dweomer
Copy link
Contributor

@dweomer dweomer commented Aug 25, 2021

Make duplicate entries for .status.applying on plans magically disappear
by correctly sorting the return value for SelectConcurrentNodes.

Fixes #132

Additionally, address a logic bug introduced in #126 that could also
spin out of control.

See
90e3d08

Signed-off-by: Jacob Blain Christen jacob@rancher.com

@dweomer
Copy link
Contributor Author

dweomer commented Aug 25, 2021

See also, rancher/rancher#32050 (comment)

@@ -177,7 +189,7 @@ func SelectConcurrentNodes(plan *upgradeapiv1.Plan, nodeCache corectlv1.NodeCach
}
}
sort.Slice(selected, func(i, j int) bool {
return selected[i].Name < selected[i].Name
return selected[i].Name < selected[j].Name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amazing!

Fixes rancher#132

Additionally, address a logic bug introduced in rancher#126 that could also
spin out of control.

See
rancher@90e3d08

Signed-off-by: Jacob Blain Christen <jacob@rancher.com>
}
obj.Status.Applying = concurrentNodeNames[:]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sort issue was spurious, this was where the dupes were coming from all along.

@dweomer dweomer merged commit 32a31b2 into rancher:master Aug 25, 2021
@dweomer dweomer deleted the fix-132 branch August 25, 2021 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

duplicated, every-growing .status.applying on plans
3 participants