Skip to content

OCPCLOUD-3104: Fix v1beta2 condition setting #385

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

Merged
merged 3 commits into from
Oct 15, 2025
Merged

OCPCLOUD-3104: Fix v1beta2 condition setting #385

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87a2b73
machineset: always use v1beta2 condition utils to ensure lastTransiti…
chrischdi Oct 8, 2025
682e9ff
machineset: rewrite status setting
chrischdi Oct 13, 2025
ae094eb
machinesetsync: also adjust MAPItoCAPI
chrischdi Oct 13, 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
56 changes: 33 additions & 23 deletions pkg/controllers/machinesetsync/machineset_sync_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -996,35 +996,33 @@ func (r *MachineSetSyncReconciler) ensureMAPIMachineSetStatusUpdated(ctx context
}

// setChangedCAPIMachineSetStatusFields sets the updated fields in the CAPI machine set status.
// Note: ObservedGeneration is handled after calling this function.
func setChangedCAPIMachineSetStatusFields(existingCAPIMachineSet, convertedCAPIMachineSet *clusterv1.MachineSet) {
// convertedCAPIMachineSet holds the computed and desired status changes, so apply them to the existing existingCAPIMachineSet.
// Set the changed v1beta1 fields.
existingCAPIMachineSet.Status.Replicas = convertedCAPIMachineSet.Status.Replicas
existingCAPIMachineSet.Status.ReadyReplicas = convertedCAPIMachineSet.Status.ReadyReplicas
existingCAPIMachineSet.Status.AvailableReplicas = convertedCAPIMachineSet.Status.AvailableReplicas
existingCAPIMachineSet.Status.FullyLabeledReplicas = convertedCAPIMachineSet.Status.FullyLabeledReplicas
existingCAPIMachineSet.Status.FailureReason = convertedCAPIMachineSet.Status.FailureReason
existingCAPIMachineSet.Status.FailureMessage = convertedCAPIMachineSet.Status.FailureMessage

for i := range convertedCAPIMachineSet.Status.Conditions {
conditions.Set(existingCAPIMachineSet, &convertedCAPIMachineSet.Status.Conditions[i])
// convertedCAPIMachine holds the computed and desired status changes converted from the source MAPI machine, so apply them to the existing existingCAPIMachine.
// Merge the v1beta1 conditions.
for _, condition := range convertedCAPIMachineSet.Status.Conditions {
conditions.Set(existingCAPIMachineSet, &condition)
}

// Set the changed v1beta2 fields.
switch {
case convertedCAPIMachineSet.Status.V1Beta2 == nil:
existingCAPIMachineSet.Status.V1Beta2 = nil
case existingCAPIMachineSet.Status.V1Beta2 == nil:
existingCAPIMachineSet.Status.V1Beta2 = convertedCAPIMachineSet.Status.V1Beta2
default:
existingCAPIMachineSet.Status.V1Beta2.UpToDateReplicas = convertedCAPIMachineSet.Status.V1Beta2.UpToDateReplicas
existingCAPIMachineSet.Status.V1Beta2.AvailableReplicas = convertedCAPIMachineSet.Status.V1Beta2.AvailableReplicas
existingCAPIMachineSet.Status.V1Beta2.ReadyReplicas = convertedCAPIMachineSet.Status.V1Beta2.ReadyReplicas
// Copy them back to the convertedCAPIMachine.
convertedCAPIMachineSet.Status.Conditions = existingCAPIMachineSet.Status.Conditions

// Merge the v1beta2 conditions.
if convertedCAPIMachineSet.Status.V1Beta2 != nil {
if existingCAPIMachineSet.Status.V1Beta2 == nil {
existingCAPIMachineSet.Status.V1Beta2 = &clusterv1.MachineSetV1Beta2Status{}
}

for i := range convertedCAPIMachineSet.Status.V1Beta2.Conditions {
conditionsv1beta2.Set(existingCAPIMachineSet, convertedCAPIMachineSet.Status.V1Beta2.Conditions[i])
for _, condition := range convertedCAPIMachineSet.Status.V1Beta2.Conditions {
conditionsv1beta2.Set(existingCAPIMachineSet, condition)
}

// Copy them back to the convertedCAPIMachine.
convertedCAPIMachineSet.Status.V1Beta2.Conditions = existingCAPIMachineSet.Status.V1Beta2.Conditions
}

// Finally overwrite the entire existingCAPIMachine status with the convertedCAPIMachine status.
existingCAPIMachineSet.Status = convertedCAPIMachineSet.Status
}
Comment on lines 1000 to 1026
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

🧩 Analysis chain

Merge-copy-overwrite pattern correctly handles condition timestamps.

The refactored approach properly preserves LastTransitionTime for conditions:

  1. Merge conditions into existing (lines 1003-1005, 1016-1018)
  2. Copy merged conditions back to converted (lines 1008, 1021)
  3. Overwrite entire status (line 1025)

The addition of v1beta2 condition handling (lines 1011-1022) appears to be the fix mentioned in the PR title.

Please verify that v1beta2 condition merging has test coverage to ensure LastTransitionTime is correctly preserved during status updates and upgrades.

🏁 Script executed:

#!/bin/bash
# Search for tests covering v1beta2 condition handling in MachineSet sync
rg -n -C3 -g '**/*test.go' 'V1Beta2.*Condition|v1beta2.*condition' --type=go

Length of output: 38085

Add unit tests for v1beta2 condition merging in MachineSet sync controller

No existing tests under pkg/controllers/machinesetsync cover the new v1beta2 merge logic. Add unit tests for setChangedCAPIMachineSetStatusFields (and its counterpart) to verify that v1beta2 Conditions are merged correctly and LastTransitionTime is preserved.

🤖 Prompt for AI Agents 
In pkg/controllers/machinesetsync/machineset_sync_controller.go around lines
1000 to 1026, add unit tests that exercise setChangedCAPIMachineSetStatusFields'
v1beta2 merging path: create test cases where
existingCAPIMachineSet.Status.V1Beta2 is nil and convertedCAPIMachineSet has one
or more v1beta2 Conditions, including Conditions with non-zero
LastTransitionTime, then call the function and assert that
existingCAPIMachineSet.Status.V1Beta2 is initialized, Conditions are
copied/merged from convertedCAPIMachineSet, and LastTransitionTime values are
preserved; also add complementary tests for the counterpart function that copies
back to converted objects to ensure symmetric behavior and edge cases (empty
conditions, overlapping condition types) are handled.


// updateMAPIMachineSet updates a MAPI machine set if is out of date.
Expand Down Expand Up @@ -1062,6 +1060,7 @@ func (r *MachineSetSyncReconciler) updateMAPIMachineSet(ctx context.Context, exi
}

// setChangedMAPIMachineSetStatusFields sets the updated fields in the MAPI machine set status.
// Note: ObservedGeneration is handled after calling this function.
func setChangedMAPIMachineSetStatusFields(existingMAPIMachineSet, convertedMAPIMachineSet *mapiv1beta1.MachineSet) {
// convertedMAPIMachineSet holds the computed and desired status changes, so apply them to the existing existingMAPIMachineSet.
existingMAPIMachineSet.Status.Replicas = convertedMAPIMachineSet.Status.Replicas
Expand All @@ -1071,9 +1070,20 @@ func setChangedMAPIMachineSetStatusFields(existingMAPIMachineSet, convertedMAPIM
existingMAPIMachineSet.Status.ErrorReason = convertedMAPIMachineSet.Status.ErrorReason
existingMAPIMachineSet.Status.ErrorMessage = convertedMAPIMachineSet.Status.ErrorMessage

// Merge the v1beta1 conditions.
for i := range convertedMAPIMachineSet.Status.Conditions {
existingMAPIMachineSet.Status.Conditions = util.SetMAPICondition(existingMAPIMachineSet.Status.Conditions, &convertedMAPIMachineSet.Status.Conditions[i])
}

// Copy them back to the convertedMAPIMachineSet.
convertedMAPIMachineSet.Status.Conditions = existingMAPIMachineSet.Status.Conditions

// Keep the current SynchronizedGeneration and AuthorativeAPI. They get handled separately in `applySynchronizedConditionWithPatch`
convertedMAPIMachineSet.Status.SynchronizedGeneration = existingMAPIMachineSet.Status.SynchronizedGeneration
convertedMAPIMachineSet.Status.AuthoritativeAPI = existingMAPIMachineSet.Status.AuthoritativeAPI

// Finally overwrite the entire existingMAPIMachineSet status with the convertedMAPIMachineSet status.
existingMAPIMachineSet.Status = convertedMAPIMachineSet.Status
}

// ensureSyncFinalizer ensures the sync finalizer is present across mapi and capi machine sets.
Expand Down