CNTRLPLANE-3603: Migrate clients from CAPI v1beta1 to v1beta2 - #8717
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@clebs: This pull request references CNTRLPLANE-3603 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 story to target the "5.0.0" version, but no target version was set. DetailsIn 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
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR migrates code and tests from Cluster API v1beta1 to v1beta2 across APIs, controllers, platform integrations, schemes, manifests, and utilities. It adds HostedControlPlane.Status.Initialization and HostedControlPlaneInitializationStatus, sets Initialization.ControlPlaneInitialized in HostedControlPlane reconciliation, patches CAPI Cluster Status.Initialization.InfrastructureProvisioned, replaces several CAPI spec/status shapes (ContractVersionedObjectReference, pointer Spec.Paused, seconds-based timeout fields), removes CAPI conversion webhook plumbing, and updates golangci lint exclusions. Sequence Diagram(s)sequenceDiagram
participant HostedControlPlane
participant HostedControlPlaneController
participant HostedClusterController
participant ClusterAPI
participant Tests
HostedControlPlane->>HostedControlPlaneController: expose Status.Initialization.ControlPlaneInitialized
HostedControlPlaneController->>HostedClusterController: reconciliation observes InfrastructureReady
HostedClusterController->>ClusterAPI: patch status.initialization.infrastructureProvisioned=true
ClusterAPI->>Tests: tests updated to use v1beta2 types and condition shapes
Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
api/hypershift/v1beta1/hosted_controlplane.go (1)
440-440: ⚡ Quick winUse consistent marker syntax for default value.
This file uses
+kubebuilder:default=on lines 322, 331, and 338. For consistency, change+default=falseto+kubebuilder:default=false.📝 Suggested change
- // +default=false + // +kubebuilder:default=false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/hypershift/v1beta1/hosted_controlplane.go` at line 440, Change the marker annotation for the boolean default to use the same kubebuilder syntax as the other fields: replace the comment line `+default=false` with `+kubebuilder:default=false` on the corresponding boolean field in the HostedControlPlane spec (the same block that contains the other `+kubebuilder:default=` annotations around lines where the spec fields are defined), ensuring consistency with the markers used on the other fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Around line 681-685: Don't set
hostedControlPlane.Status.Initialization.ControlPlaneInitialized
unconditionally; instead gate it on the availability checks already computed by
reconcileAvailabilityStatus. Replace the unconditional assignment of
hostedControlPlane.Status.Initialization.ControlPlaneInitialized = ptr.To(true)
with logic that sets it to true only when the reconciler determines
hyperv1.HostedControlPlaneAvailable is true (and specifically
KubeAPIServerAvailable is true / kubeconfig exists and LB health/component
checks passed); otherwise leave it false or nil. Locate the assignment and use
the results/conditions produced by reconcileAvailabilityStatus (or the same
booleans it computes) to decide the value.
In
`@control-plane-operator/hostedclusterconfigoperator/controllers/machine/machine_test.go`:
- Around line 467-468: The test case title "With Failing machine with internal
addresses and passthrow service should mark endpointslices as not ready/not
serving" does not match the setup which uses
pairOfDualStackMachines(capiv1.MachinePhaseRunning,
capiv1.MachinePhaseDeleting); either rename the test title to refer to a
"Deleting machine" to match the current setup, or change the second machine
phase to capiv1.MachinePhaseFailed so the scenario truly models a failing
machine; update the test case entry (the name string and/or the machines call)
accordingly, keeping the rest of the assertions intact.
In `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`:
- Around line 3043-3044: The ClusterRole that includes Verbs:
[]string{"get","list","patch","watch"} is granting the capi-provider subject
cluster-wide patch (write) access to CRDs which contradicts the later code that
assumes a read-only binding for capi-provider; either remove "patch" from that
ClusterRole's Verbs so capi-provider only gets get/list/watch, or create a
separate ClusterRole (e.g., "capi-provider-crd-reader") with Verbs:
[]string{"get","list","watch"} and bind capi-provider to that instead, leaving
the original ClusterRole with "patch" bound only to trusted controller
subjects—update the RoleBinding/ClusterRoleBinding creation for the
capi-provider subject accordingly so the access model matches the assumptions in
the subsequent code paths (lines around where capi-provider is referenced).
In `@hypershift-operator/controllers/nodepool/capi.go`:
- Around line 249-253: The cleanupMachineTemplates function currently uses
api.Scheme.VersionsForGroupKind(...)[0] to build a single apiVersion and only
lists MachineTemplates for that version; change it to iterate over all versions
returned by api.Scheme.VersionsForGroupKind(schema.GroupKind{Group:
ref.APIGroup, Kind: ref.Kind}) and for each version construct
schema.GroupVersion{Group: ref.APIGroup, Version: ver.Version}.String(), then
list and delete MachineTemplates for each apiVersion (rather than only
versions[0]). Keep using the existing capiv1.ContractVersionedObjectReference
writers (no change needed to the reference type).
---
Nitpick comments:
In `@api/hypershift/v1beta1/hosted_controlplane.go`:
- Line 440: Change the marker annotation for the boolean default to use the same
kubebuilder syntax as the other fields: replace the comment line
`+default=false` with `+kubebuilder:default=false` on the corresponding boolean
field in the HostedControlPlane spec (the same block that contains the other
`+kubebuilder:default=` annotations around lines where the spec fields are
defined), ensuring consistency with the markers used on the other fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5cdbc23e-aa7c-4f55-a404-d1a9a463f044
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hosted_controlplane.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (63)
api/.golangci.ymlapi/hypershift/v1beta1/hosted_controlplane.gocmd/cluster/core/dump.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/kas/kubeconfig.gocontrol-plane-operator/hostedclusterconfigoperator/api/scheme.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/globalps/globalps_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/inplaceupgrader/inplaceupgrader.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/inplaceupgrader/inplaceupgrader_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/inplaceupgrader/setup.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/machine/machine.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/machine/machine_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/machine/setup.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/node/node.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/node/node_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/spotremediation/spotremediation.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/spotremediation/spotremediation_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_webhook.gohypershift-operator/controllers/hostedcluster/internal/platform/agent/agent.gohypershift-operator/controllers/hostedcluster/internal/platform/agent/agent_test.gohypershift-operator/controllers/hostedcluster/internal/platform/aws/aws.gohypershift-operator/controllers/hostedcluster/internal/platform/azure/azure.gohypershift-operator/controllers/hostedcluster/internal/platform/gcp/gcp.gohypershift-operator/controllers/hostedcluster/internal/platform/ibmcloud/ibmcloud.gohypershift-operator/controllers/hostedcluster/internal/platform/ibmcloud/ibmcloud_test.gohypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt.gohypershift-operator/controllers/hostedcluster/internal/platform/kubevirt/kubevirt_test.gohypershift-operator/controllers/hostedcluster/internal/platform/openstack/openstack.gohypershift-operator/controllers/hostedcluster/internal/platform/openstack/openstack_test.gohypershift-operator/controllers/hostedcluster/internal/platform/powervs/powervs.gohypershift-operator/controllers/manifests/controlplaneoperator/manifests.gohypershift-operator/controllers/nodepool/aws.gohypershift-operator/controllers/nodepool/aws_test.gohypershift-operator/controllers/nodepool/azure_test.gohypershift-operator/controllers/nodepool/capi.gohypershift-operator/controllers/nodepool/capi_test.gohypershift-operator/controllers/nodepool/conditions.gohypershift-operator/controllers/nodepool/conditions_test.gohypershift-operator/controllers/nodepool/gcp.gohypershift-operator/controllers/nodepool/metrics/metrics.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/nodepool/nodepool_controller_test.gohypershift-operator/controllers/nodepool/powervs.gohypershift-operator/controllers/nodepool/scale_from_zero_test.gohypershift-operator/controllers/nodepool/version.gohypershift-operator/controllers/nodepool/version_test.gokarpenter-operator/controllers/karpenter/karpenter_controller.gokarpenter-operator/controllers/karpenter/karpenter_controller_test.gosupport/api/capi_types.gosupport/api/scheme.gosupport/k8sutil/resources.gosupport/upsert/upsert.gotest/e2e/autoscaling_test.gotest/e2e/nodepool_day2_tags_test.gotest/e2e/nodepool_kv_advanced_multinet_test.gotest/e2e/nodepool_osp_advanced_test.gotest/e2e/nodepool_rolling_upgrade_test.gotest/e2e/nodepool_spot_termination_handler_test.gotest/e2e/upgrade_hypershift_operator_test.gotest/e2e/util/util.gotest/e2e/v2/backuprestore/cleanup.go
💤 Files with no reviewable changes (2)
- hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go
- support/api/scheme.go
openshift-ci
Bot
added
the
area/documentation
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
added
the
needs-rebase
|
Stale PRs rot after 14d of inactivity. Mark the PR as fresh by commenting If this PR is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
|
/remove-lifecycle rotten |
|
/test e2e-aws |
3 similar comments
|
/test e2e-aws |
|
/test e2e-aws |
|
/test e2e-aws |
|
/test e2e-aks |
|
/test e2e-aws |
|
/assign @csrwng |
|
@clebs: This pull request references CNTRLPLANE-3603 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 story to target the "5.1.0" version, but no target version was set. DetailsIn 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: clebs The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-aws |
|
/test images |
| // +kubebuilder:validation:MinProperties=1 | ||
| type HostedControlPlaneInitializationStatus struct { | ||
| // controlPlaneInitialized is true when the control plane is functional enough to accept requests. | ||
| // Once this condition is marked true, its value is never changed. See the Ready condition for an |
There was a problem hiding this comment.
What enforces that this value is never changed? What would happen if this value does get changed somehow?
There was a problem hiding this comment.
HI @everettraven!
that is a good question. There is nothing mechanically enforcing this (other than the controller setting it and making sure it stays that way), it is a property that mirrors what CAPI sets, which has the same comment: https://github.com/kubernetes-sigs/cluster-api/blob/75072e39a4977fec5cbb0223869d26c89b609653/api/core/v1beta2/cluster_types.go#L182
It was working the same way before. The old status.initialized was also unconditionally set to true at the same point, with the same "once true, never reverted" convention and no mechanical enforcement.
The new status.initialization.controlPlaneInitialized is just the v1beta2 contract equivalent of the existing status.initialized field, placed at the exact same location with the same semantics.
There was a problem hiding this comment.
Should we add some validation to enforce this semantic and prevent accidental changes to this field once it has been set to true?
What would happen if this was set to true and somehow got modified?
Bump Cluster API core imports from v1beta1 to v1beta2 across all
consumers. Provider imports (Azure, GCP, OpenStack, AWS, IBM Cloud)
remain on v1beta1 as their ControlPlaneEndpoint field still references
core v1beta1.APIEndpoint — no provider has migrated this yet.
Key type changes:
- corev1.ObjectReference → capiv1.ContractVersionedObjectReference
- Machine.Status.NodeRef: pointer → MachineNodeReference (use .IsDefined())
- Cluster.Spec.Paused: bool → *bool
- Version/FailureDomain: *string → string
- Status replica fields: int32 → *int32 (access via ptr.Deref)
- Strategy → Rollout.Strategy
- NodeDrainTimeout → Deletion.NodeDrainTimeoutSeconds
- MHC: UnhealthyConditions → Checks.UnhealthyNodeConditions,
MaxUnhealthy → Remediation.TriggerIf.UnhealthyLessThanOrEqualTo
- Conditions: capiv1.Condition → metav1.Condition
- ReadyCondition → MachinesReadyCondition (on MachineDeployment/MachineSet)
- machineConditionResult.Status: corev1.ConditionStatus → metav1.ConditionStatus
- MachineDeploymentComplete: use UpToDateReplicas + ReadyReplicas
- Constants renamed with V1Beta1 suffix (e.g. WaitingForNodeRefReason)
Control plane changes:
- Add HCP Status.Initialization.ControlPlaneInitialized
- Add patchInfrastructureInitializationProvisioned for CAPI v1beta2 contract
- Remove conversion webhook (no longer needed with single API version)
- Remove v1beta1 scheme registration (core, addons, ipam)
Signed-off-by: Borja Clemente <bclement@redhat.com>
- ContractVersionedObjectReference (APIGroup instead of APIVersion, no Namespace) - FailureDomain: *string nil → empty string - Version: *string → string - Status replica fields: int32 → *int32 via ptr.To - Conditions: []capiv1.Condition → []metav1.Condition - MHC: restructured Checks/Remediation fields - MachineDeploymentComplete rewritten for v1beta2 native fields - MachinePhaseFailed (deprecated) → MachinePhaseDeleting - machineConditionResult expectations: corev1 → metav1 ConditionStatus - Constants renamed with V1Beta1 suffix - Fix bare v1beta2 import and misleading alias in e2e tests Signed-off-by: Borja Clemente <bclement@redhat.com>
Update vendored dependencies after updating clients. Signed-off-by: Borja Clemente <bclement@redhat.com>
The MD complete check was wrongly assuming that when ObservedGeneration is up to date, replica counters are always correct, which is not true, as is confirmed in kubernetes-sigs/cluster-api#13738. To guard against old Machines producing false positives a check is added to make sure that old MachineSets have no replicas. Signed-off-by: Borja Clemente <bclement@redhat.com>
|
/lgtm |
|
Scheduling tests matching the |
|
/retest |
|
/verified by @clebs and e2e tests. All Cluster, Machine and Node creation/upgrade behaviors unchanged. |
|
@clebs: This PR has been marked as verified by DetailsIn 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. |
|
/retest |
|
@clebs: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
This branch was successfully deployed
6 participants
What this PR does / why we need it:
As a the next step after bumping CAPI to 1.11 in CNTRLPLANE-2207, we are updating all clients from
v1beta1tov1beta2.Provider specific code can not be updated yet since all providers are still using
v1beta1in their types.Which issue(s) this PR fixes:
Fixes CNTRLPLANE-3603
Special notes for your reviewer:
The fix for MachineDeployment completeness relates to the upstream issue kubernetes-sigs/cluster-api#13738
Checklist:
Summary by CodeRabbit
New Features
Refactor
Behavior