Skip to content

fix ProviderData.Update() semantics wrt controller-runtime client cache #2008

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

Open
multi-io wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix ProviderData.Update() semantics wrt controller-runtime client cache #2008

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 12 additions & 5 deletions pkg/cloudprovider/types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,20 +99,27 @@ func GetMachineUpdater(ctx context.Context, client ctrlruntimeclient.Client) Mac
// Store name here, because the machine can be nil if an update failed.
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

The comment says the passed-in machine can be nil, but this function dereferences machine immediately (Namespace/Name) and later calls DeepCopyInto(machine), which would panic if it were nil. Either adjust the comment to reflect the actual contract (non-nil required) or add an explicit nil check that returns a clear error.

Suggested change
// Store name here, because the machine can be nil if an update failed.
if machine == nil {
return fmt.Errorf("machine must not be nil when applying modifiers")
}
// Store name here for use in the retry loop; machine is required to be non-nil.

Copilot uses AI. Check for mistakes.
namespacedName := types.NamespacedName{Namespace: machine.Namespace, Name: machine.Name}
return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
if err := client.Get(ctx, namespacedName, machine); err != nil {
cachedMachine := &clusterv1alpha1.Machine{}
if err := client.Get(ctx, namespacedName, cachedMachine); err != nil {
return fmt.Errorf("failed to get machine: %w", err)
}

// Check if we actually change something and only update if that is the case.
unmodifiedMachine := machine.DeepCopy()
unmodifiedMachine := cachedMachine.DeepCopy()
for _, modify := range modifiers {
modify(machine)
modify(cachedMachine)
}
if equality.Semantic.DeepEqual(unmodifiedMachine, machine) {
if equality.Semantic.DeepEqual(unmodifiedMachine, cachedMachine) {
Comment on lines 107 to +112
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

This change also alters modifier execution semantics: modifiers now run against cachedMachine, while the caller-provided machine remains stale until after a successful Update. Any modifier that closes over and reads machine (instead of only using the updatedMachine argument) will now make decisions on stale data and may introduce unintended diffs (e.g. pkg/cloudprovider/provider/azure/provider.go:769 checks HasFinalizer(machine, ...) inside the modifier). Consider preserving the old “modifiers see the freshly-Get()'d object” behavior by copying cachedMachine into machine before invoking modifiers and restoring the original machine value on no-op / error, or otherwise updating affected call sites and documenting that modifiers must only use their argument.

Copilot uses AI. Check for mistakes.
return nil
}
Comment on lines +112 to 114
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

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

This is a subtle behavior change (caller object is no longer refreshed on no-op, and only updated on successful writes). Please add targeted unit tests for GetMachineUpdater to lock in the new semantics (no mutation on no-op / on update error; mutation on successful update) and to prevent regressions in conflict-retry behavior.

Copilot uses AI. Check for mistakes.

return client.Update(ctx, machine)
if err := client.Update(ctx, cachedMachine); err != nil {
return err
}

cachedMachine.DeepCopyInto(machine)

return nil
})
}
}
Loading