OCPCLOUD-3104: sync: allow Cluster API v1beta2 version for ownerrefs and set Cluster v1beta2 ControlPlaneInitialized #391
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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
WalkthroughAdds acceptance of cluster.x-k8s.io v1beta2 API versions in ownerReference checks and metadata diffing, and records a v1beta2 control-plane-initialized condition when the existing ControlPlaneInitializedCondition is marked true. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as CoreClusterController
participant Cluster as Cluster resource
participant KubeAPI as API server
Controller->>Cluster: detect ControlPlaneInitializedCondition == True
Note right of Controller: New step added
Controller->>Controller: MarkTrue(clusterv1.ControlPlaneInitializedCondition)
Controller->>Controller: Set conditionsv1beta2.ClusterControlPlaneInitializedV1Beta2Condition (Status=True, Reason)
Controller->>KubeAPI: Patch Cluster (client.MergeFrom) to persist v1beta2 condition
KubeAPI-->>Controller: Patch result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
|
Skipping CI for Draft Pull Request. |
|
/testwith openshift/cluster-capi-operator/main/e2e-aws-capi-techpreview openshift/cluster-api#248 |
|
/test lint |
|
/testwith openshift/cluster-capi-operator/main/e2e-aws-capi-techpreview openshift/cluster-api#248 To test if it was a flake |
chrischdi
changed the title
|
/testwith openshift/cluster-capi-operator/main/e2e-aws-capi-techpreview openshift/cluster-api#248 |
|
/test lint |
|
/test help |
|
@chrischdi: The specified target(s) for The following commands are available to trigger optional jobs: Use 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. |
chrischdi
force-pushed
the
pr-capi-rebase-fix-groupversion-ownerref
branch
from
b46fab0 to
2c0018a
Compare
|
/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#243 |
|
/testwith openshift/cluster-capi-operator/main/e2e-aws-capi-techpreview openshift/cluster-api#243 |
1 similar comment
|
/testwith openshift/cluster-capi-operator/main/e2e-aws-capi-techpreview openshift/cluster-api#243 |
chrischdi
force-pushed
the
pr-capi-rebase-fix-groupversion-ownerref
branch
from
16eaf56 to
5424f4d
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (3)
pkg/controllers/corecluster/corecluster_controller.go(2 hunks)pkg/controllers/machinesetsync/machineset_sync_controller.go(3 hunks)pkg/controllers/machinesync/machine_sync_controller.go(4 hunks)
🔇 Additional comments (8)
pkg/controllers/machinesetsync/machineset_sync_controller.go (2)
40-40: LGTM - Import required for v1beta2 support.The
runtime/schemaimport is necessary to construct the v1beta2 GroupVersion for owner reference validation at line 644.
644-644: LGTM - Correctly extends validation for v1beta2.The owner reference validation now accepts both v1beta1 and v1beta2 API versions, enabling interoperability during the API version transition.
pkg/controllers/corecluster/corecluster_controller.go (2)
33-33: LGTM - Import required for v1beta2 condition support.The
conditionsv1beta2import is necessary to set the v1beta2 ControlPlaneInitialized condition alongside the v1beta1 condition.
203-207: LGTM - Correctly adds v1beta2 condition support.The implementation properly sets the v1beta2 ControlPlaneInitialized condition alongside the v1beta1 condition, enabling dual-version support during the API transition.
pkg/controllers/machinesync/machine_sync_controller.go (4)
44-44: LGTM - Import required for v1beta2 support.The
runtime/schemaimport is necessary to construct the v1beta2 GroupVersion for owner reference validation at lines 889 and 920.
889-889: LGTM - Correctly extends validation for v1beta2 MachineSet owners.The owner reference validation now accepts both v1beta1 and v1beta2 API versions for MachineSet owners, consistent with the changes in
machineset_sync_controller.go.
920-920: LGTM - Correctly extends validation for v1beta2 Cluster owners.The owner reference validation now accepts both v1beta1 and v1beta2 API versions for Cluster owners, maintaining consistency with the MachineSet validation at line 889.
1241-1247: LGTM - Correctly sanitizes metadata before comparison.The implementation properly creates sanitized copies of both machines by stripping the conversion-data annotation before comparing metadata, preventing false positives during diff detection.
|
|
||
| if diffObjectMeta := util.ObjectMetaEqual(capiMachineSet1.ObjectMeta, capiMachineSet2.ObjectMeta); len(diffObjectMeta) > 0 { | ||
| diffCAPIMachineSet1 := capiMachineSet1.DeepCopy() | ||
| delete(diffCAPIMachineSet1.Annotations, "cluster.x-k8s.io/conversion-data") | ||
|
|
||
| diffCAPIMachineSet2 := capiMachineSet1.DeepCopy() | ||
| delete(diffCAPIMachineSet2.Annotations, "cluster.x-k8s.io/conversion-data") | ||
|
|
||
| if diffObjectMeta := util.ObjectMetaEqual(diffCAPIMachineSet1.ObjectMeta, diffCAPIMachineSet2.ObjectMeta); len(diffObjectMeta) > 0 { | ||
| diff[".metadata"] = diffObjectMeta |
There was a problem hiding this comment.
Fix copy-paste error in metadata diff computation.
Line 1406 copies capiMachineSet1 instead of capiMachineSet2, causing both sanitized copies to be identical. This breaks the metadata diff detection and prevents the controller from identifying differences between the two MachineSets.
Apply this diff to fix the bug:
diffCAPIMachineSet1 := capiMachineSet1.DeepCopy()
delete(diffCAPIMachineSet1.Annotations, "cluster.x-k8s.io/conversion-data")
-diffCAPIMachineSet2 := capiMachineSet1.DeepCopy()
+diffCAPIMachineSet2 := capiMachineSet2.DeepCopy()
delete(diffCAPIMachineSet2.Annotations, "cluster.x-k8s.io/conversion-data")
if diffObjectMeta := util.ObjectMetaEqual(diffCAPIMachineSet1.ObjectMeta, diffCAPIMachineSet2.ObjectMeta); len(diffObjectMeta) > 0 {🤖 Prompt for AI Agents
In pkg/controllers/machinesetsync/machineset_sync_controller.go around lines
1402 to 1410, there is a copy-paste error: the second sanitized copy uses
capiMachineSet1.DeepCopy() instead of capiMachineSet2.DeepCopy(), making both
sanitized objects identical and hiding metadata differences; fix it by changing
the second DeepCopy call to capiMachineSet2.DeepCopy() and keep the subsequent
deletion of the "cluster.x-k8s.io/conversion-data" annotation so each sanitized
copy reflects its respective MachineSet before calling util.ObjectMetaEqual.
| diffCAPIMachineSet1 := capiMachineSet1.DeepCopy() | ||
| delete(diffCAPIMachineSet1.Annotations, "cluster.x-k8s.io/conversion-data") | ||
|
|
||
| diffCAPIMachineSet2 := capiMachineSet1.DeepCopy() | ||
| delete(diffCAPIMachineSet2.Annotations, "cluster.x-k8s.io/conversion-data") |
There was a problem hiding this comment.
I'd move these into
cluster-capi-operator/pkg/util/sync.go
Lines 56 to 58 in a1a97d6
we explicitly say there that's only including the fields/structs we care about for the MAPI-CAPI conversion.
Also independently on whether we add them there or not I'd add a comment on why we are dropping these annotations :)
There was a problem hiding this comment.
My thought was: the util func is also used for non-capi objects. But thinking again about it: totally fine to add this directly to the util 👍
Updated and also added a description inside ObjectMetaEqual.
|
[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
|
@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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controllers/machinesync/machine_sync_controller.go (1)
775-775: Update line 775 to accept both v1beta1 and v1beta2 APIVersions for consistency.The
shouldMirrorCAPIMachineToMAPIMachinefunction at line 775 only validates againstclusterv1.GroupVersion.String()(v1beta1), but theconvertCAPIMachineOwnerReferencesToMAPIfunction at lines 889 and 920 explicitly validates against both v1beta1 and v1beta2.This inconsistency means CAPI Machines with v1beta2 owner references pointing to MachineSets will be skipped and not mirrored to MAPI, even though the conversion logic explicitly supports v1beta2. Line 775 should be updated to match the validation at lines 889 and 920.
🧹 Nitpick comments (1)
pkg/controllers/machinesync/machine_sync_controller.go (1)
889-889: Consider extracting the v1beta2 GroupVersion string to reduce duplication.The inline construction of the v1beta2 GroupVersion string is duplicated on lines 889 and 920. Extracting this to a package-level constant or variable would improve maintainability.
For example, at the package level:
var ( capiV1Beta2GroupVersion = schema.GroupVersion{Group: clusterv1.GroupVersion.Group, Version: "v1beta2"}.String() )Then use it in both locations:
- if capiOwnerReference.APIVersion != clusterv1.GroupVersion.String() && capiOwnerReference.APIVersion != (schema.GroupVersion{Group: clusterv1.GroupVersion.Group, Version: "v1beta2"}).String() { + if capiOwnerReference.APIVersion != clusterv1.GroupVersion.String() && capiOwnerReference.APIVersion != capiV1Beta2GroupVersion {- if capiOwnerReference.APIVersion != clusterv1.GroupVersion.String() && capiOwnerReference.APIVersion != (schema.GroupVersion{Group: clusterv1.GroupVersion.Group, Version: "v1beta2"}).String() { + if capiOwnerReference.APIVersion != clusterv1.GroupVersion.String() && capiOwnerReference.APIVersion != capiV1Beta2GroupVersion {Also applies to: 920-920
📜 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 (3)
pkg/controllers/machinesetsync/machineset_sync_controller.go(2 hunks)pkg/controllers/machinesync/machine_sync_controller.go(3 hunks)pkg/util/sync.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controllers/machinesetsync/machineset_sync_controller.go
🔇 Additional comments (1)
pkg/controllers/machinesync/machine_sync_controller.go (1)
44-44: LGTM!The import is necessary for constructing v1beta2 GroupVersion strings in owner reference validation.
| // Ignore conversion-data because this data is managed by Cluster API for down conversion. | ||
| aLabels := a.DeepCopy().GetLabels() | ||
| delete(aLabels, "cluster.x-k8s.io/conversion-data") | ||
|
|
||
| bLabels := b.DeepCopy().GetLabels() | ||
| delete(bLabels, "cluster.x-k8s.io/conversion-data") | ||
|
|
There was a problem hiding this comment.
Fix compilation error: GetLabels() method does not exist on *metav1.ObjectMeta.
The metav1.ObjectMeta type does not have a GetLabels() method. Labels should be accessed via the .Labels field directly.
Apply this diff to fix the compilation error:
- // Ignore conversion-data because this data is managed by Cluster API for down conversion.
- aLabels := a.DeepCopy().GetLabels()
- delete(aLabels, "cluster.x-k8s.io/conversion-data")
-
- bLabels := b.DeepCopy().GetLabels()
- delete(bLabels, "cluster.x-k8s.io/conversion-data")
-
- if diffLabels := deep.Equal(aLabels, bLabels); len(diffLabels) > 0 {
+ // Ignore conversion-data because this data is managed by Cluster API for down conversion.
+ aCopy := a.DeepCopy()
+ if aCopy.Labels != nil {
+ delete(aCopy.Labels, "cluster.x-k8s.io/conversion-data")
+ }
+
+ bCopy := b.DeepCopy()
+ if bCopy.Labels != nil {
+ delete(bCopy.Labels, "cluster.x-k8s.io/conversion-data")
+ }
+
+ if diffLabels := deep.Equal(aCopy.Labels, bCopy.Labels); len(diffLabels) > 0 {
objectMetaDiff[".labels"] = diffLabels
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/util/sync.go around lines 62 to 68, the code calls GetLabels() on
a.DeepCopy() and b.DeepCopy(), but metav1.ObjectMeta has no GetLabels() method;
replace those calls with direct access to the Labels field on the copied
ObjectMeta (e.g., copy := a.DeepCopy(); aLabels := copy.Labels) and then delete
the "cluster.x-k8s.io/conversion-data" key from the resulting map; ensure you
handle the case where Labels may be nil (initialize map before deleting if
necessary).
|
@chrischdi: The following test failed, say
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. |
3 participants
This hopefully is the last PR to support bumping to v1beta2
Part of OCPCLOUD-3104 and required to merge:
Summary by CodeRabbit