OCPCLOUD-3104: Fix v1beta2 condition setting #385
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@chrischdi: This pull request references OCPCLOUD-3104 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughMerge-based refactor of MachineSet status updates: Conditions from converted CAPI/MAPI MachineSets are merged into existing objects, merged Conditions are copied back to converted objects, then the existing Status is overwritten with the converted Status; ObservedGeneration/SynchronizedGeneration/AuthoritativeAPI are handled separately. Also removed webhook-removal helpers from manifests generation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Reconciler
participant Existing as Existing MachineSet (CAPI/MAPI)
participant Converted as Converted MachineSet (v1beta1/v1beta2)
participant API as Kubernetes API
Reconciler->>Converted: Build converted.Status (includes Conditions)
Reconciler->>Existing: Read existing.Status
Note over Reconciler,Existing: For each Condition in Converted, merge into Existing.Status.Conditions
Reconciler->>Converted: Copy merged Conditions back to Converted.Status
Reconciler->>Existing: Overwrite Existing.Status = Converted.Status
Reconciler->>API: PATCH/UPDATE existing Status subresource
API-->>Reconciler: Status update response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@chrischdi: This pull request references OCPCLOUD-3104 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
1000-1024: Consider adding inline comments to clarify the condition merging pattern.The logic of merging conditions into
existingCAPIMachineSet, copying them back toconvertedCAPIMachineSet, and then overwriting the entire status is correct but non-obvious. This pattern ensuresLastTransitionTimeis properly managed while updating all status fields atomically.Add inline comments explaining why conditions are copied back before the final overwrite:
func setChangedCAPIMachineSetStatusFields(existingCAPIMachineSet, convertedCAPIMachineSet *clusterv1.MachineSet) { - // 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. + // Strategy: Merge conditions into existing (to preserve LastTransitionTime), + // copy them back to converted, then overwrite entire status with converted. + + // Merge v1beta1 conditions: conditions.Set() updates LastTransitionTime only when status changes. for _, condition := range convertedCAPIMachineSet.Status.Conditions { conditions.Set(existingCAPIMachineSet, &condition) } - // Copy them back to the convertedCAPIMachine. + // Copy merged conditions back so the final overwrite includes them. convertedCAPIMachineSet.Status.Conditions = existingCAPIMachineSet.Status.Conditions - // Merge the v1beta2 conditions. + // Repeat for v1beta2 conditions if present. if convertedCAPIMachineSet.Status.V1Beta2 != nil { if existingCAPIMachineSet.Status.V1Beta2 == nil { existingCAPIMachineSet.Status.V1Beta2 = &clusterv1.MachineSetV1Beta2Status{} } for _, condition := range convertedCAPIMachineSet.Status.V1Beta2.Conditions { conditionsv1beta2.Set(existingCAPIMachineSet, condition) } - // Copy them back to the convertedCAPIMachine. + // Copy merged v1beta2 conditions back. convertedCAPIMachineSet.Status.V1Beta2.Conditions = existingCAPIMachineSet.Status.V1Beta2.Conditions } - // Finally overwrite the entire existingCAPIMachine status with the convertedCAPIMachine status. + // Overwrite entire status: merged conditions plus all other fields from conversion. existingCAPIMachineSet.Status = convertedCAPIMachineSet.Status }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
pkg/controllers/machinesetsync/machineset_sync_controller.go(1 hunks)
🔇 Additional comments (2)
pkg/controllers/machinesetsync/machineset_sync_controller.go (2)
1062-1074: MAPI status handling inconsistent with AI-generated summary.The AI summary states that "MAPI MachineSet status update logic was adjusted to align with the CAPIM changes" and "overwrites the entire status similarly to CAPIMachineSet." However,
setChangedMAPIMachineSetStatusFieldsremains unchanged (no~marks) and still uses direct field assignments (lines 1064-1069) rather than the "merge conditions, copy back, overwrite entire status" pattern used in the CAPI version.This inconsistency raises questions:
- Was the MAPI function intentionally left unchanged?
- Does MAPI not have v1beta2 conditions, making the simpler approach sufficient?
- Should MAPI adopt the same pattern as CAPI for consistency?
Clarify whether the different handling is intentional or if further alignment is needed.
1000-1024: Ensure v1beta2 status isn’t dropped when nil
IfconvertedCAPIMachineSet.Status.V1Beta2can ever be nil, the final assignment at line 1024 will clear any existing V1Beta2 status. Confirm that conversion always initializesStatus.V1Beta2, or add logic to preserve the existing V1Beta2 status when the converted one is nil.
|
/assign @damdo |
|
@chrischdi: This pull request references OCPCLOUD-3104 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/controllers/machinesetsync/machineset_sync_controller.go (1)
1066-1072: Consider removing redundant field assignments.Lines 1066-1072 set fields on
existingMAPIMachineSetthat are immediately overwritten by line 1086's full status assignment. SinceconvertedMAPIMachineSetalready contains these fields, the direct assignments are redundant.Consider removing these lines for clarity:
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 - existingMAPIMachineSet.Status.ReadyReplicas = convertedMAPIMachineSet.Status.ReadyReplicas - existingMAPIMachineSet.Status.AvailableReplicas = convertedMAPIMachineSet.Status.AvailableReplicas - existingMAPIMachineSet.Status.FullyLabeledReplicas = convertedMAPIMachineSet.Status.FullyLabeledReplicas - existingMAPIMachineSet.Status.ErrorReason = convertedMAPIMachineSet.Status.ErrorReason - existingMAPIMachineSet.Status.ErrorMessage = convertedMAPIMachineSet.Status.ErrorMessage - // Merge the v1beta1 conditions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
pkg/controllers/machinesetsync/machineset_sync_controller.go(3 hunks)
| 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 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Merge-copy-overwrite pattern correctly handles condition timestamps.
The refactored approach properly preserves LastTransitionTime for conditions:
- Merge conditions into existing (lines 1003-1005, 1016-1018)
- Copy merged conditions back to converted (lines 1008, 1021)
- 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=goLength 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.
|
@chrischdi let me know when we are good with a review on this :) TY |
|
@damdo Should be good for a review 👍 I think this even covers a bug, the selector was not copied over. |
openshift-ci
bot
added
do-not-merge/hold
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damdo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/test e2e-azure-ovn-techpreview looks like a flake |
|
e2e-openstack-ovn-techpreview seems to be permanently red. |
It is perma-failing yes. We'll need to look into it. For now: /override ci/prow/e2e-openstack-ovn-techpreview |
|
@damdo: Overrode contexts on behalf of damdo: ci/prow/e2e-openstack-ovn-techpreview In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/testwith openshift/cluster-api/master/e2e-aws-capi-techpreview openshift/cluster-api#243 |
|
/testwith openshift/cluster-api/master/e2e-aws-capi-techpreview openshift/cluster-api#243 #385 |
|
/testwith openshift/cluster-capi-operator/main/e2e-aws-capi-techpreview openshift/cluster-api#243 |
|
@chrischdi: This pull request references OCPCLOUD-3104 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
chrischdi
force-pushed
the
pr-fix-conditions-set-lasttransitiontime
branch
from
3a2f255 to
ae094eb
Compare
|
Let's see how far we get with conversion CAPI v1.11.2 and webhooks enabled. (Not a requirement for merging) /testwith openshift/cluster-capi-operator/main/e2e-aws-capi-techpreview openshift/cluster-api/pull/248 |
|
/testwith abort (abort because I did fail to push on openshift/cluster-api/pull/248 |
|
Let's see how far we get with conversion CAPI v1.11.2 and webhooks enabled. (Not a requirement for merging) /testwith openshift/cluster-capi-operator/main/e2e-aws-capi-techpreview openshift/cluster-api#248 |
|
/testwith openshift/cluster-capi-operator/main/e2e-aws-capi-techpreview openshift/cluster-api#248 |
|
/lgtm |
|
Scheduling tests matching the |
|
/retest (we can ignore the multi-pr ci's) |
|
/retest |
|
/override ci/prow/e2e-openstack-ovn-techpreview as it is perma-failing |
|
/skip |
|
@damdo: Overrode contexts on behalf of damdo: ci/prow/e2e-openstack-ovn-techpreview In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/verified bypass Verified via https://prow.ci.openshift.org/view/gs/test-platform-results/logs/multi-pr-openshift-cluster-capi-operator-385-openshift-cluster-api-248-e2e-aws-capi-techpreview/1978118575797309440 which did not have the test failures related to |
|
/hold cancel |
openshift-ci-robot
added
the
verified
|
@chrischdi: The In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-merge-bot
bot
merged commit 372b2a5
into
openshift:main
|
@chrischdi: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Part of OCPCLOUD-3104 and required to merge:
Summary by CodeRabbit
Bug Fixes
Refactor
Chores