Skip to content

OCPBUGS-92013: fix NodePool stuck in UpdatingVersion/UpdatingConfig due to stale conversion-data annotation #8821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-bot merged 2 commits into from
Jun 25, 2026
+86 −105
Merged

OCPBUGS-92013: fix NodePool stuck in UpdatingVersion/UpdatingConfig due to stale conversion-data annotation #8821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ae09a3
fix(nodepool): use Status.V1Beta2 instead of conversion-data annotati…
csrwng Jun 24, 2026
d2cf0ff
fix: check nil v1beta2 replica fields explicitly instead of defaultin…
csrwng Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 71 additions & 73 deletions hypershift-operator/controllers/nodepool/capi_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import (
capiazure "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
capikubevirt "sigs.k8s.io/cluster-api-provider-kubevirt/api/v1alpha1"
capiv1 "sigs.k8s.io/cluster-api/api/core/v1beta1"
"sigs.k8s.io/cluster-api/util/conversion"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
Expand Down Expand Up @@ -3284,22 +3283,6 @@ func TestMachineDeploymentComplete(t *testing.T) {
two := int32(2)
three := int32(3)

conversionData := func(replicas, upToDate, available int32, observedGen int64) string {
data := map[string]any{
"apiVersion": "cluster.x-k8s.io/v1beta2",
"kind": "MachineDeployment",
"spec": map[string]any{"replicas": replicas},
"status": map[string]any{
"observedGeneration": observedGen,
"replicas": replicas,
"upToDateReplicas": upToDate,
"availableReplicas": available,
},
}
raw, _ := json.Marshal(data)
return string(raw)
}

testCases := []struct {
name string
md *capiv1.MachineDeployment
Expand All @@ -3308,109 +3291,124 @@ func TestMachineDeploymentComplete(t *testing.T) {
{
name: "When all v1beta1 and v1beta2 fields agree it should return true",
md: &capiv1.MachineDeployment{
ObjectMeta: metav1.ObjectMeta{
Generation: 2,
Annotations: map[string]string{
conversion.DataAnnotation: conversionData(2, 2, 2, 2),
ObjectMeta: metav1.ObjectMeta{Generation: 2},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{
Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 2,
V1Beta2: &capiv1.MachineDeploymentV1Beta2Status{
UpToDateReplicas: ptr.To(int32(2)),
AvailableReplicas: ptr.To(int32(2)),
},
},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 2},
},
expected: true,
},
{
name: "When v1beta1 looks complete but v1beta2 upToDateReplicas disagrees it should return false",
md: &capiv1.MachineDeployment{
ObjectMeta: metav1.ObjectMeta{
Generation: 2,
Annotations: map[string]string{
conversion.DataAnnotation: conversionData(2, 0, 2, 2),
ObjectMeta: metav1.ObjectMeta{Generation: 2},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{
Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 2,
V1Beta2: &capiv1.MachineDeploymentV1Beta2Status{
UpToDateReplicas: ptr.To(int32(0)),
AvailableReplicas: ptr.To(int32(2)),
},
},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 2},
},
expected: false,
},
{
name: "When v1beta1 looks complete but v1beta2 replicas shows surge it should return false",
name: "When v1beta1 looks complete but v1beta2 availableReplicas disagrees it should return false",
md: &capiv1.MachineDeployment{
ObjectMeta: metav1.ObjectMeta{
Generation: 2,
Annotations: map[string]string{
conversion.DataAnnotation: conversionData(3, 2, 2, 2),
ObjectMeta: metav1.ObjectMeta{Generation: 2},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{
Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 2,
V1Beta2: &capiv1.MachineDeploymentV1Beta2Status{
UpToDateReplicas: ptr.To(int32(2)),
AvailableReplicas: ptr.To(int32(1)),
},
},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 2},
},
expected: false,
},
{
name: "When v1beta1 looks complete but v1beta2 observedGeneration is stale it should return false",
name: "When v1beta1 is not complete it should return false without checking v1beta2",
md: &capiv1.MachineDeployment{
ObjectMeta: metav1.ObjectMeta{
Generation: 2,
Annotations: map[string]string{
conversion.DataAnnotation: conversionData(2, 2, 2, 1),
},
},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 2},
ObjectMeta: metav1.ObjectMeta{Generation: 2},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{Replicas: 3, UpdatedReplicas: 1, AvailableReplicas: 2, ObservedGeneration: 2},
},
expected: false,
},
{
name: "When v1beta1 is not complete it should return false without checking conversion data",
name: "When v1beta2 status is nil it should fall back to v1beta1 only",
md: &capiv1.MachineDeployment{
ObjectMeta: metav1.ObjectMeta{
Generation: 2,
},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{Replicas: 3, UpdatedReplicas: 1, AvailableReplicas: 2, ObservedGeneration: 2},
ObjectMeta: metav1.ObjectMeta{Generation: 1},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 1},
},
expected: false,
expected: true,
},
{
name: "When no conversion-data annotation exists it should fall back to v1beta1 only",
name: "When v1beta1 replicas does not match spec it should return false",
md: &capiv1.MachineDeployment{
ObjectMeta: metav1.ObjectMeta{
Generation: 1,
ObjectMeta: metav1.ObjectMeta{Generation: 2},
Spec: capiv1.MachineDeploymentSpec{Replicas: &three},
Status: capiv1.MachineDeploymentStatus{
Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 2,
V1Beta2: &capiv1.MachineDeploymentV1Beta2Status{
UpToDateReplicas: ptr.To(int32(2)),
AvailableReplicas: ptr.To(int32(2)),
},
},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 1},
},
expected: true,
expected: false,
},
{
name: "When conversion-data annotation is malformed it should fall back to v1beta1 result",
name: "When v1beta2 upToDateReplicas is nil it should return false",
md: &capiv1.MachineDeployment{
ObjectMeta: metav1.ObjectMeta{
Generation: 1,
Annotations: map[string]string{
conversion.DataAnnotation: "not-valid-json",
ObjectMeta: metav1.ObjectMeta{Generation: 2},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{
Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 2,
V1Beta2: &capiv1.MachineDeploymentV1Beta2Status{
AvailableReplicas: ptr.To(int32(2)),
},
},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 1},
},
expected: true,
expected: false,
},
{
name: "When v1beta1 replicas does not match spec it should return false",
name: "When v1beta2 availableReplicas is nil it should return false",
md: &capiv1.MachineDeployment{
ObjectMeta: metav1.ObjectMeta{
Generation: 2,
Annotations: map[string]string{
conversion.DataAnnotation: conversionData(3, 2, 2, 2),
ObjectMeta: metav1.ObjectMeta{Generation: 2},
Spec: capiv1.MachineDeploymentSpec{Replicas: &two},
Status: capiv1.MachineDeploymentStatus{
Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 2,
V1Beta2: &capiv1.MachineDeploymentV1Beta2Status{
UpToDateReplicas: ptr.To(int32(2)),
},
},
Spec: capiv1.MachineDeploymentSpec{Replicas: &three},
Status: capiv1.MachineDeploymentStatus{Replicas: 2, UpdatedReplicas: 2, AvailableReplicas: 2, ObservedGeneration: 2},
},
expected: false,
},
{
name: "When desired replicas is zero and v1beta2 fields are nil it should return false",
md: func() *capiv1.MachineDeployment {
zero := int32(0)
return &capiv1.MachineDeployment{
ObjectMeta: metav1.ObjectMeta{Generation: 2},
Spec: capiv1.MachineDeploymentSpec{Replicas: &zero},
Status: capiv1.MachineDeploymentStatus{
Replicas: 0, UpdatedReplicas: 0, AvailableReplicas: 0, ObservedGeneration: 2,
V1Beta2: &capiv1.MachineDeploymentV1Beta2Status{},
},
}
}(),
expected: false,
},
}

for _, tc := range testCases {
Expand Down
47 changes: 15 additions & 32 deletions hypershift-operator/controllers/nodepool/nodepool_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package nodepool

import (
"context"
"encoding/json"
coreerrors "errors"
"fmt"
"os"
Expand Down Expand Up @@ -44,7 +43,6 @@ import (
capiopenstackv1beta1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1"
capiv1 "sigs.k8s.io/cluster-api/api/core/v1beta1"
"sigs.k8s.io/cluster-api/util"
"sigs.k8s.io/cluster-api/util/conversion"
"sigs.k8s.io/cluster-api/util/patch"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
Expand Down Expand Up @@ -792,8 +790,8 @@ func defaultNodePoolGCPImage(specifiedArch string, releaseImage *releaseinfo.Rel
// fields come from conversion. The converted v1beta1 fields (especially UpdatedReplicas,
// which maps from deprecated.v1beta1.updatedReplicas rather than the native upToDateReplicas)
// can transiently disagree with the v1beta2 native fields. To guard against this, when the
// v1beta1 fields indicate completion we cross-check against the authoritative v1beta2 status
// stored in the conversion-data annotation.
// v1beta1 fields indicate completion we cross-check against the v1beta2 status stored in the
// Status.V1Beta2 field, which is kept current on every status-subresource write.
func MachineDeploymentComplete(deployment *capiv1.MachineDeployment) bool {
newStatus := &deployment.Status
v1beta1Complete := newStatus.UpdatedReplicas == *(deployment.Spec.Replicas) &&
Expand All @@ -803,39 +801,24 @@ func MachineDeploymentComplete(deployment *capiv1.MachineDeployment) bool {
if !v1beta1Complete {
return false
}
return machineDeploymentCompleteFromConversionData(deployment)
return machineDeploymentCompleteFromV1Beta2Status(deployment)
}

// machineDeploymentCompleteFromConversionData parses the v1beta2 conversion-data annotation
// and verifies that the native v1beta2 status also indicates completion. If the annotation
// is absent (e.g. CAPI < v1.11), returns true to preserve backwards compatibility.
func machineDeploymentCompleteFromConversionData(deployment *capiv1.MachineDeployment) bool {
raw, ok := deployment.Annotations[conversion.DataAnnotation]
if !ok {
// machineDeploymentCompleteFromV1Beta2Status verifies that the native v1beta2 status fields
// also indicate completion. The v1beta1 Status.V1Beta2 field is populated by the v1beta2-to-v1beta1
// conversion on every status-subresource write, so it is always current.
// If V1Beta2 is nil (e.g. CAPI < v1.11), returns true to preserve backwards compatibility.
func machineDeploymentCompleteFromV1Beta2Status(deployment *capiv1.MachineDeployment) bool {
v1beta2 := deployment.Status.V1Beta2
if v1beta2 == nil {
return true
}

var v1beta2MD struct {
Spec struct {
Replicas *int32 `json:"replicas"`
} `json:"spec"`
Status struct {
ObservedGeneration int64 `json:"observedGeneration"`
Replicas *int32 `json:"replicas"`
AvailableReplicas *int32 `json:"availableReplicas"`
UpToDateReplicas *int32 `json:"upToDateReplicas"`
} `json:"status"`
}
if err := json.Unmarshal([]byte(raw), &v1beta2MD); err != nil {
ctrl.Log.WithName("nodepool").Error(err, "Failed to unmarshal conversion-data annotation, falling back to v1beta1 status")
return true
if v1beta2.UpToDateReplicas == nil || v1beta2.AvailableReplicas == nil {
return false
}

desired := ptr.Deref(v1beta2MD.Spec.Replicas, 0)
return ptr.Deref(v1beta2MD.Status.UpToDateReplicas, 0) == desired &&
ptr.Deref(v1beta2MD.Status.Replicas, 0) == desired &&
ptr.Deref(v1beta2MD.Status.AvailableReplicas, 0) == desired &&
v1beta2MD.Status.ObservedGeneration >= deployment.Generation
desired := ptr.Deref(deployment.Spec.Replicas, 0)
return *v1beta2.UpToDateReplicas == desired &&
*v1beta2.AvailableReplicas == desired
}

// GetHostedClusterByName finds and return a HostedCluster object using the specified params.
Expand Down