CNTRLPLANE-3603: Migrate clients from CAPI v1beta1 to v1beta2 - #8717
CNTRLPLANE-3603: Migrate clients from CAPI v1beta1 to v1beta2#8717clebs wants to merge 5 commits into
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
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: clebs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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
| // Set status.initialization.controlPlaneInitialized for CAPI 1.11 v1beta2 contract. | ||
| // CAPI reads this field from the ControlPlane provider object to determine if the | ||
| // control plane is initialized (ControlPlaneInitialized condition on the CAPI Cluster). | ||
| hostedControlPlane.Status.Initialization.ControlPlaneInitialized = ptr.To(true) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locations writing/readings ControlPlaneInitialized =="
rg -n -C3 'ControlPlaneInitialized|Status\.Initialization|Status\.Initialized' --type go
echo
echo "== Reconcile flow around assignment in hostedcontrolplane controller =="
rg -n -C6 'Status\.Initialization\.ControlPlaneInitialized|Status\.Initialized\s*=' control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
echo
echo "== CAPI/HostedCluster reconciliation references that depend on initialization contracts =="
rg -n -C4 'ControlPlaneInitialized|InfrastructureReady|initialized' hypershift-operator/controllers/hostedcluster --type goRepository: openshift/hypershift
Length of output: 49187
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Other writes to hostedControlPlane.Status.Initialized in hostedcontrolplane_controller.go =="
rg -n 'Status\.Initialized\s*=' control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
echo
echo "== Writes to hostedControlPlane.Status.Initialization.ControlPlaneInitialized =="
rg -n 'Initialization\.ControlPlaneInitialized\s*=' control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
echo
echo "== Read access patterns of HCP.Status.Initialized in hypershift-operator HostedCluster controller =="
rg -n 'Status\.Initialized' hypershift-operator/controllers/hostedcluster --type go
echo
echo "== Read/contract references of status.initialized / ControlPlaneInitialized in repo =="
rg -n 'status\.initialized|ControlPlaneInitialized' --type go --type ts --type py --type yaml
echo
echo "== Inspect surrounding code for the assignment block (hostedcontrolplane_controller.go:650-710) =="
sed -n '630,710p' control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go | cat -nRepository: openshift/hypershift
Length of output: 14059
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go"
echo "== Where reconcileAvailabilityAndReadyStatus is defined =="
rg -n 'func \(r \*.*\) reconcileAvailabilityAndReadyStatus\(' "$FILE"
echo
echo "== Body of reconcileAvailabilityAndReadyStatus (up to next func) =="
python3 - <<'PY'
import re, itertools
path="control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go"
start_pat=re.compile(r'func \(r \*.*\) reconcileAvailabilityAndReadyStatus\(')
with open(path,'r') as f:
lines=f.readlines()
start=None
for i,l in enumerate(lines):
if start_pat.search(l):
start=i
break
assert start is not None
# print next ~250 lines until next "func " at col 0
out=[]
for j in range(start, min(start+350, len(lines))):
if j>start and lines[j].startswith("func "):
break
out.append(f"{j+1}:{lines[j].rstrip()}")
print("\n".join(out))
PY
echo
echo "== Any other writes to ControlPlaneInitialized in control-plane-operator =="
rg -n 'ControlPlaneInitialized\s*=\s*ptr\.To\((true|false)\)' --type go control-plane-operator
echo
echo "== What conditions are set around KubeAPIServerAvailable / InfrastructureReady etc. in this controller =="
rg -n 'KubeAPIServerAvailable|InfrastructureReady|ControlPlaneAvailable|ControlPlaneInitialized' --type go "$FILE"Repository: openshift/hypershift
Length of output: 2946
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CAPI contract wording for ControlPlaneInitialized (v1beta2) =="
sed -n '145,190p' vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/cluster_types.go | cat -n
echo
sed -n '1360,1390p' vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/cluster_types.go | cat -n
echo
echo "== Find reconcileAvailabilityStatus definition =="
rg -n 'func reconcileAvailabilityStatus\(' control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go control-plane-operator/controllers/hostedcontrolplane -S
echo
echo "== Print reconcileAvailabilityStatus implementation =="
python3 - <<'PY'
import subprocess, re, pathlib, sys
import os, json
root="control-plane-operator/controllers/hostedcontrolplane"
# find exact file with reconcileAvailabilityStatus
res=subprocess.check_output(["bash","-lc",f"rg -n 'func reconcileAvailabilityStatus\\(' {root} -S | head -n 1"]).decode().strip()
print("match:",res)
file=res.split(":")[0]
line=int(res.split(":")[1])
print("file:",file,"line:",line)
# print 200 lines starting at line-1
start=max(1,line-1)
cmd=f"sed -n '{start},{start+220}p' {file}"
print(subprocess.check_output(["bash","-lc",cmd]).decode())
PYRepository: openshift/hypershift
Length of output: 15930
Gate CAPI controlPlaneInitialized on API-server readiness (avoid unconditional true)
hostedControlPlane.Status.Initialization.ControlPlaneInitialized = ptr.To(true) is set unconditionally, but the CAPI v1beta2 contract says controlPlaneInitialized should be “functional enough to accept requests” and is used to orchestrate provisioning that depends on a functional API server. This reconciler already computes hyperv1.HostedControlPlaneAvailable via reconcileAvailabilityStatus (which can be False when kubeconfig is missing, KubeAPIServerAvailable is False, load balancer healthz fails, or components aren’t available). Setting ControlPlaneInitialized to true even when those checks aren’t satisfied risks CAPI starting too early.
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`
around lines 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.
| Verbs: []string{"get", "list", "patch", "watch"}, | ||
| }, |
There was a problem hiding this comment.
Don't keep capi-provider on the patched CRD role.
Adding patch here means the shared binding now gives capi-provider cluster-wide write access to CRDs, while Lines 3063-3065 still assume the role is read-only. Split the provider onto its own get/list/watch role or stop binding it to this ClusterRole.
Also applies to: 3063-3070
🤖 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 `@hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go`
around lines 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).
| versions := api.Scheme.VersionsForGroupKind(schema.GroupKind{Group: ref.APIGroup, Kind: ref.Kind}) | ||
| if len(versions) == 0 { | ||
| return fmt.Errorf("no versions registered for GroupKind %s/%s", ref.APIGroup, ref.Kind) | ||
| } | ||
| apiVersion := schema.GroupVersion{Group: ref.APIGroup, Version: versions[0].Version}.String() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file under review
FILE="hypershift-operator/controllers/nodepool/capi.go"
test -f "$FILE" || { echo "Missing $FILE"; exit 1; }
echo "== Snippet around lines 230-290 =="
nl -ba "$FILE" | sed -n '230,290p'
echo "== Snippet around lines 430-480 =="
nl -ba "$FILE" | sed -n '430,480p'
echo "== Snippet around lines 920-990 =="
nl -ba "$FILE" | sed -n '920,990p'
echo "== Finding ContractVersionedObjectReference usages in this file =="
rg -n "ContractVersionedObjectReference" "$FILE" || true
echo "== Locate struct definition in module cache / repo =="
# Try to locate in repo/vendor first
if fd -t f "types.go" . >/dev/null 2>&1; then
:
fi
# Search in repository for the type definition
rg -n "type +ContractVersionedObjectReference" -S . || true
# If not found, search in Go module cache paths (best-effort; sandbox may limit)
go env GOMODCACHE >/dev/null 2>&1 || true
GOMODCACHE="$(go env GOMODCACHE 2>/dev/null || true)"
if [ -n "${GOMODCACHE:-}" ] && [ -d "$GOMODCACHE" ]; then
echo "== Searching in GOMODCACHE for ContractVersionedObjectReference =="
rg -n "type +ContractVersionedObjectReference" "$GOMODCACHE" -S --hidden --no-ignore 2>/dev/null | head -n 20 || true
fi
echo "== Search for VersionsForGroupKind usage =="
rg -n "VersionsForGroupKind" "$FILE" || trueRepository: openshift/hypershift
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="hypershift-operator/controllers/nodepool/capi.go"
test -f "$FILE" || { echo "Missing $FILE"; exit 1; }
echo "== Snippet around lines 240-275 =="
sed -n '240,275p' "$FILE"
echo "== Snippet around lines 435-465 =="
sed -n '435,465p' "$FILE"
echo "== Snippet around lines 935-970 =="
sed -n '935,970p' "$FILE"
echo "== Finding ContractVersionedObjectReference usages in this file =="
rg -n "ContractVersionedObjectReference" "$FILE" || true
echo "== Finding VersionsForGroupKind usage in this file =="
rg -n "VersionsForGroupKind" "$FILE" || true
echo "== Attempt to locate ContractVersionedObjectReference type in repo (module/vendor) =="
# Prefer repo search
rg -n "type +ContractVersionedObjectReference" . || true
# If nothing, attempt in module cache
if command -v go >/dev/null 2>&1; then
GOMODCACHE="$(go env GOMODCACHE 2>/dev/null || true)"
if [ -n "${GOMODCACHE:-}" ] && [ -d "$GOMODCACHE" ]; then
rg -n "type +ContractVersionedObjectReference" "$GOMODCACHE" -S --hidden --no-ignore 2>/dev/null | head -n 20 || true
fi
fiRepository: openshift/hypershift
Length of output: 4835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/common_types.go"
test -f "$FILE" || { echo "Missing $FILE"; exit 1; }
# Print around the struct definition
sed -n '330,430p' "$FILE"
# Also search within the vendor file for the struct fields
rg -n "type +ContractVersionedObjectReference +struct" -n "$FILE"Repository: openshift/hypershift
Length of output: 3141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="hypershift-operator/controllers/nodepool/capi.go"
test -f "$FILE" || { echo "Missing $FILE"; exit 1; }
rg -n "func \\(c \\*.*\\) cleanupMachineTemplates" "$FILE" || true
rg -n "cleanupMachineTemplates" "$FILE" || true
# Print the whole function (or as much as reasonable)
# Find start line number
START="$(rg -n "cleanupMachineTemplates" "$FILE" | head -n1 | cut -d: -f1 || true)"
if [ -n "${START:-}" ]; then
# print 120 lines from start
END=$((START+140))
sed -n "${START},${END}p" "$FILE"
fi
# Show the comment near line 248
sed -n '235,265p' "$FILE"
# Search for SetAPIVersion usage in this file
rg -n "SetAPIVersion\\(" "$FILE" || trueRepository: openshift/hypershift
Length of output: 6767
Fix MachineTemplate cleanup to delete across all template API versions (avoid VersionsForGroupKind(...)[0])
cleanupMachineTemplates reconstructs a single apiVersion from api.Scheme.VersionsForGroupKind(...)[0] and lists MachineTemplates for only that APIVersion, so any MachineTemplates stored under other versions for the same GroupKind won’t be discovered/deleted. Update it to list/delete for all versions returned by VersionsForGroupKind (instead of index 0).
The writer sites creating capiv1.ContractVersionedObjectReference don’t need to change: in vendored cluster-api v1beta2 (vendor/sigs.k8s.io/cluster-api/api/core/v1beta2/common_types.go), the type has only Kind, Name, and APIGroup (no Version field), so the version can’t be persisted there.
🤖 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 `@hypershift-operator/controllers/nodepool/capi.go` around lines 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).
openshift-ci
Bot
added
the
area/documentation
openshift-ci
Bot
added
the
needs-rebase
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
/test e2e-aws |
|
/test e2e-aws |
openshift-ci
Bot
added
the
needs-rebase
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>
MachineDeploymentComplete() still has flakiness when introducing v1beta2 due to lossy conversion and stale caches on the patch.Nelper that does the conversion. The conversion-data check added on the CAPI 1.11 bump makes this less likely because 2 separate checks take longer, going past the flakiness window. Now that v1beta2 is natively used, the conversion-data check is redundant. Introduced rate limiting for CAPI reconciliations to make this less likely to happen. Additionally, guard against it on the HO side by adding MachineSet template cross-verification to MachineDeploymentComplete(). Signed-off-by: Borja Clemente <bclement@redhat.com>
Docs navigaiton needs to be alphabetically sorted. One entry was not correctly sorted. Signed-off-by: Borja Clemente <bclement@redhat.com>
|
/test e2e-aws |
openshift-ci
Bot
removed
the
needs-rebase
|
@clebs: The following test failed, say
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. |
|
Now I have a clear picture. The v1beta2 Now I have all evidence needed. Let me construct the final report. The root cause is clear: The Test Failure Analysis:
|
| Test | Expected Condition | Timeout | Error |
|---|---|---|---|
TestAdditionalTrustBundlePropagation |
UpdatingConfig=True |
5m | got UpdatingConfig=False: AsExpected |
TestRollingUpgrade |
UpdatingPlatformMachineTemplate=True |
2m | got UpdatingPlatformMachineTemplate=False: AsExpected |
TestNTOMachineConfigGetsRolledOut |
UpdatingConfig=True |
5m | got UpdatingConfig=False: AsExpected |
TestNodePoolReplaceUpgrade |
UpdatingVersion=True |
10m | got UpdatingVersion=False: AsExpected |
Root Cause
MachineDeploymentComplete returns true prematurely after v1beta2 migration
The CAPI v1beta1→v1beta2 migration in PR #8717 rewrote MachineDeploymentComplete (in nodepool_controller.go:778-786) to use v1beta2 native status fields. This introduced a race condition where the function returns true before the CAPI MachineDeployment controller has started the rollout, causing NodePool annotations/status to be set to their target values prematurely.
Mechanism:
-
Test triggers a NodePool update (e.g., patches
Spec.Release.Imagefor version upgrade, or modifies config for config change) -
First HyperShift reconcile loop:
propagateVersionAndTemplate()detects the change, updates the MachineDeployment spec, and returns early (line 484 incapi.go).reconcileMachineDeploymentStatus()does NOT run. ✅ Correct. -
Second reconcile loop:
propagateVersionAndTemplate()sees the MachineDeployment spec is already updated → returnsfalse. Falls through toreconcileMachineDeploymentStatus()(line 487), which callsMachineDeploymentComplete(). -
MachineDeploymentCompletereturnstrueprematurely because:- The CAPI controller has observed the new generation (
ObservedGeneration >= Generation) but the old machines haven't been replaced yet - In v1beta2,
UpToDateReplicas,ReadyReplicas, andReplicasare optional*int32pointers withptr.Deref(field, 0)defaulting to 0. During the CAPI controller's processing window, these fields transiently show old values that still equaldesired(all old machines are still "ready" and counted as "up-to-date" before the CAPI controller marks them otherwise) - The old v1beta1 code had
UpdatedReplicas(which only counts machines with the new template) and a conversion-data cross-check that verified the v1beta2 native status separately — both of these safety mechanisms were removed
- The CAPI controller has observed the new generation (
-
When
MachineDeploymentCompletereturnstrue,reconcileMachineDeploymentStatus(lines 617-638 incapi.go) immediately:- Sets
nodePool.Status.Version = targetVersion - Sets
nodePool.Annotations[nodePoolAnnotationCurrentConfig] = targetConfigHash - Sets
nodePool.Annotations[nodePoolAnnotationPlatformMachineTemplate] = machineTemplateCR.GetName()
- Sets
-
These annotation/status updates make
isUpdating*returnfalse(lines 721-731 innodepool_controller.go), so theUpdatingVersion,UpdatingConfig, andUpdatingPlatformMachineTemplateconditions remainFalse: AsExpected— they never transition toTrue.
Key code change (diff from f3d7aa1dcce):
// OLD (v1beta1) — with conversion-data safety cross-check:
func MachineDeploymentComplete(deployment *capiv1.MachineDeployment) bool {
newStatus := &deployment.Status
v1beta1Complete := newStatus.UpdatedReplicas == *(deployment.Spec.Replicas) &&
newStatus.Replicas == *(deployment.Spec.Replicas) &&
newStatus.AvailableReplicas == *(deployment.Spec.Replicas) &&
newStatus.ObservedGeneration >= deployment.Generation
if !v1beta1Complete { return false }
return machineDeploymentCompleteFromConversionData(deployment) // ← safety check removed
}
// NEW (v1beta2) — no cross-check:
func MachineDeploymentComplete(deployment *capiv1.MachineDeployment) bool {
desired := ptr.Deref(deployment.Spec.Replicas, 0)
s := &deployment.Status
return ptr.Deref(s.UpToDateReplicas, 0) == desired &&
ptr.Deref(s.Replicas, 0) == desired &&
ptr.Deref(s.ReadyReplicas, 0) == desired &&
s.ObservedGeneration >= deployment.Generation
}The v1beta2 UpToDateReplicas field has different semantics from v1beta1's UpdatedReplicas. In v1beta1, UpdatedReplicas counted only machines matching the new template. In v1beta2, UpToDateReplicas reflects machines where the UpToDate condition is true — which may transiently include old machines before the CAPI controller sets the condition to false on stale machines, creating a window where completion is detected prematurely.
Recommendations
-
Add a rollout-in-progress guard to
MachineDeploymentComplete: Before returningtrue, verify that no new MachineSet has been created with 0 ready replicas (indicating a rollout is starting). Alternatively, check the MachineDeployment'sMachinesUpToDatecondition isTruein addition to the replica counts. -
Reintroduce semantic equivalence of
UpdatedReplicascheck: The v1beta1UpdatedReplicasfield (now atDeprecated.V1Beta1.UpdatedReplicas) counted only machines with the desired template spec. Consider checking this deprecated field as a fallback until CAPI v1beta2UpToDateReplicassemantics are confirmed to be equivalent. -
Verify
MachinesReadyConditionvsReadyCondition: The migration changed the condition lookup fromReadyConditiontoMachinesReadyCondition("MachinesReady") inreconcileMachineDeploymentStatus. Confirm that the CAPI version in CI actually populates theMachinesReadycondition on MachineDeployments. If not, the NodePoolReadycondition will never be set from the MachineDeployment. -
Add MachineDeployment rollout status logging: In
reconcileMachineDeploymentStatus, log the MachineDeployment'sReadyReplicas,UpToDateReplicas,Replicas,ObservedGeneration, andGenerationvalues whenMachineDeploymentCompletereturnstrue. This will make future race conditions visible.
Evidence
E2E Test Output
TestNodePoolReplaceUpgrade (10m timeout):
eventually.go:225: observed *v1beta1.NodePool e2e-clusters-2qmxw/node-pool-x25gj-test-replaceupgrade
invalid at RV 132683 after 10m0s:
incorrect condition: wanted UpdatingVersion=True, got UpdatingVersion=False: AsExpected
nodepool_upgrade_test.go:225: *v1beta1.NodePool conditions:
UpdatingConfig=False: AsExpected
UpdatingVersion=False: AsExpected ← Never becomes True
UpdatingPlatformMachineTemplate=False: AsExpected
Ready=True: Ready
TestRollingUpgrade (2m timeout):
eventually.go:225: observed *v1beta1.NodePool e2e-clusters-2qmxw/node-pool-x25gj-test-rolling-upgrade
invalid at RV 116136 after 2m0s:
incorrect condition: wanted UpdatingPlatformMachineTemplate=True,
got UpdatingPlatformMachineTemplate=False: AsExpected
TestNTOMachineConfigGetsRolledOut (5m timeout):
eventually.go:225: observed *v1beta1.NodePool e2e-clusters-2qmxw/node-pool-x25gj-test-ntomachineconfig-replace
invalid at RV 118615 after 5m0s:
incorrect condition: wanted UpdatingConfig=True, got UpdatingConfig=False: AsExpected
TestAdditionalTrustBundlePropagation (5m timeout):
eventually.go:225: observed *v1beta1.NodePool e2e-clusters-8lv27/node-pool-c5zvr-test-additional-trust-bundle-propagation
invalid at RV 131252 after 5m0s:
incorrect condition: wanted UpdatingConfig=True, got UpdatingConfig=False: AsExpected
Code Flow Analysis
isUpdatingVersion / isUpdatingConfig / isUpdatingMachineTemplate (nodepool_controller.go:721-731):
func isUpdatingVersion(nodePool *hyperv1.NodePool, targetVersion string) bool {
return targetVersion != nodePool.Status.Version // Returns false if Status.Version already matches target
}
func isUpdatingConfig(nodePool *hyperv1.NodePool, targetConfigHash string) bool {
return targetConfigHash != nodePool.GetAnnotations()[nodePoolAnnotationCurrentConfig]
}
func isUpdatingMachineTemplate(nodePool *hyperv1.NodePool, targetMachineTemplate string) bool {
return targetMachineTemplate != nodePool.GetAnnotations()[nodePoolAnnotationPlatformMachineTemplate]
}reconcileMachineDeploymentStatus premature status update (capi.go:617-638):
if MachineDeploymentComplete(machineDeployment) { // ← Returns true prematurely
nodePool.Status.Version = targetVersion // ← Causes isUpdatingVersion → false
nodePool.Annotations[nodePoolAnnotationCurrentConfig] = targetConfigHash // ← Causes isUpdatingConfig → false
nodePool.Annotations[nodePoolAnnotationPlatformMachineTemplate] = machineTemplateCR.GetName() // ← Causes isUpdatingMachineTemplate → false
}PR Commits
f3d7aa1dcce—build(capi): migrate CAPI core types from v1beta1 to v1beta2bc0db69d5b0—test(capi): update tests for CAPI v1beta2 type migration17b8504cc38—build(vendor): Update vendor
Files Changed
| File | Key Change |
|---|---|
nodepool_controller.go:778-786 |
MachineDeploymentComplete rewritten: UpdatedReplicas → UpToDateReplicas, AvailableReplicas → ReadyReplicas, conversion-data cross-check removed |
capi.go:617-638 |
reconcileMachineDeploymentStatus unchanged but affected by MachineDeploymentComplete returning true too early |
capi.go:641-660 |
ReadyCondition → MachinesReadyCondition, AvailableReplicas → ptr.Deref(AvailableReplicas, 0) |
conditions.go |
corev1.ConditionStatus → metav1.ConditionStatus, findMachineStatusCondition updated for []metav1.Condition |
capi.go:446-460 |
InfrastructureRef changed from corev1.ObjectReference to capiv1.ContractVersionedObjectReference |
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
lifecycle/stale
|
PR needs rebase. 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. |
|
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 |
3 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:
Checklist:
Summary by CodeRabbit
New Features
Refactor
Behavior