Repository navigation
Conversation
FindLatestMachine started with the first listed machine and skipped the receiver only inside the loop. When the receiver was both first in the list and the newest machine, no other machine could replace it and the function returned nil, although other machines existed. With the same machines in a different order it returned the newest other machine. swapNICInFailoverGroup uses this result to move the failover IP to another NIC. A nil result leaves the failover group pointing at the NIC of the machine that is being deleted. Skip the receiver up front and return the newest of the remaining machines, or nil if there is none. Signed-off-by: Lucas Barros <lucasbarros.kam@gmail.com>
lbarrosby
requested review from
WassimKallel,
gauravgahlot,
gfariasalves-ionos,
jriedel-ionos,
mcbenjemaa,
mneverov,
piepmatz,
pulish,
schegi,
tinashe-mundangepfupfu and
wikkyk
as code owners
November 6, 2026 02:26
|
This branch is waiting to be deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of this pull request/Why do we need it?
Machine.FindLatestMachinereturns a different result for the same set of machines depending on the order in which the list comes back.swapNICInFailoverGrouprelies on it to pick the machine that takes over the failover IP when a machine in the failover group is deleted.Issue #, if available:
None.
Description of changes:
The loop started with
latestMachine := machines[0]and only skipped the receiver inside the loop. If the receiver is the first listed machine and also the newest one, no other machine passes the!Before(latest)check,latestMachinestays the receiver and the function returns nil, although other machines exist. With the same machines listed in another order (receiver last), it returns the newest of the others.Example with three control plane machines where the one being deleted is the newest:
cp-test-1: returns nilcp-test-3: returnscp-test-2When it returns nil,
swapNICInFailoverGroupdoes nothing, so the failover group keeps the NIC of the machine that is being deleted, for example when a MachineDeployment with theNewestdelete policy scales down.The fix skips the receiver up front and returns the newest of the remaining machines, or nil when there is none.
Special notes for your reviewer:
TestFindLatestMachineMachineIsReceiverexpected nil for exactly this case (receivercp-test-1is newer thancp-test-2). I changed it to expectcp-test-2, since the doc comment says only machines other than the receiver are considered and nil is returned only when the receiver is the only one. If returning nil there was intended, let me know and I will rework this.Added
TestFindLatestMachineReceiverIsNewest, which runs the same scenario with the receiver in each list position. On main thecp-test-1case fails and the other two pass; with the fix all pass.go test ./scope/ ./internal/service/cloud/passes locally.AI disclosure: written with the help of Claude Code, reviewed and tested by me.
Checklist: