From 7a72b87c2d971bffe843da8a381d3c6e621d4775 Mon Sep 17 00:00:00 2001 From: hmongia12 Date: Mon, 11 May 2026 21:12:14 +0530 Subject: [PATCH 1/2] RFE-9213: operator: enforce machine-config-controller minimum replicas Related: https://redhat.atlassian.net/browse/RFE-9213 Co-authored-by: Cursor --- lib/resourcemerge/apps.go | 8 ++ lib/resourcemerge/apps_test.go | 61 +++++++++++++++ .../machineconfigcontroller/deployment.yaml | 1 + pkg/operator/operator.go | 26 +++++-- pkg/operator/sync.go | 74 ++++++++++++++++++- 5 files changed, 164 insertions(+), 6 deletions(-) diff --git a/lib/resourcemerge/apps.go b/lib/resourcemerge/apps.go index 98d1fde01b..fb499f42fe 100644 --- a/lib/resourcemerge/apps.go +++ b/lib/resourcemerge/apps.go @@ -20,6 +20,14 @@ func EnsureDeployment(modified *bool, existing *appsv1.Deployment, required apps existing.Spec.Selector = required.Spec.Selector } + if required.Spec.Replicas != nil { + if existing.Spec.Replicas == nil || *existing.Spec.Replicas != *required.Spec.Replicas { + *modified = true + r := *required.Spec.Replicas + existing.Spec.Replicas = &r + } + } + ensurePodTemplateSpec(modified, &existing.Spec.Template, required.Spec.Template) } diff --git a/lib/resourcemerge/apps_test.go b/lib/resourcemerge/apps_test.go index 3a6c8477cf..253be9cd30 100644 --- a/lib/resourcemerge/apps_test.go +++ b/lib/resourcemerge/apps_test.go @@ -5,7 +5,9 @@ import ( "testing" appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/utils/ptr" ) @@ -146,3 +148,62 @@ func TestMergeDaemonSetUpdateStrategy(t *testing.T) { }) } } + +func deploymentFixture(replicas int32) appsv1.Deployment { + labels := map[string]string{"k8s-app": "mcc"} + return appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "machine-config-controller", Namespace: "openshift-machine-config-operator"}, + Spec: appsv1.DeploymentSpec{ + Replicas: ptr.To(replicas), + Selector: &metav1.LabelSelector{MatchLabels: labels}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: labels}, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "c", + Image: "test", + TerminationMessagePath: corev1.TerminationMessagePathDefault, + ImagePullPolicy: corev1.PullIfNotPresent, + }}, + }, + }, + }, + } +} + +func TestEnsureDeploymentReplicasMerge(t *testing.T) { + t.Run("updates existing replicas when required specifies a different count", func(t *testing.T) { + existing := deploymentFixture(0) + required := deploymentFixture(1) + modified := ptr.To(false) + EnsureDeployment(modified, &existing, required) + if !*modified { + t.Fatal("expected modified true when correcting replicas 0 -> 1") + } + if existing.Spec.Replicas == nil || *existing.Spec.Replicas != 1 { + t.Fatalf("expected replicas 1, got %#v", existing.Spec.Replicas) + } + }) + + t.Run("does not change replicas when required omits replicas", func(t *testing.T) { + existing := deploymentFixture(3) + required := deploymentFixture(3) + required.Spec.Replicas = nil + modified := ptr.To(false) + EnsureDeployment(modified, &existing, required) + if existing.Spec.Replicas == nil || *existing.Spec.Replicas != 3 { + t.Fatalf("expected replicas unchanged at 3, got %#v", existing.Spec.Replicas) + } + }) + + t.Run("replicas stay at 1 when required also specifies 1", func(t *testing.T) { + existing := deploymentFixture(1) + required := deploymentFixture(1) + modified := ptr.To(false) + EnsureDeployment(modified, &existing, required) + // modified may still be true if merge fills defaults on metadata/template; replicas must remain 1. + if existing.Spec.Replicas == nil || *existing.Spec.Replicas != 1 { + t.Fatalf("expected replicas 1, got %#v", existing.Spec.Replicas) + } + }) +} diff --git a/manifests/machineconfigcontroller/deployment.yaml b/manifests/machineconfigcontroller/deployment.yaml index 002b1881e6..3f2570127a 100644 --- a/manifests/machineconfigcontroller/deployment.yaml +++ b/manifests/machineconfigcontroller/deployment.yaml @@ -4,6 +4,7 @@ metadata: name: machine-config-controller namespace: {{.TargetNamespace}} spec: + replicas: 1 selector: matchLabels: k8s-app: machine-config-controller diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index d512cbd8d3..eb9327c507 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -419,6 +419,21 @@ func (optr *Operator) Run(workers int, stopCh <-chan struct{}) { defer utilruntime.HandleCrash() defer optr.queue.ShutDown() + // Record stop channel and start machine-config-controller replica repair before cache waits. + // Otherwise a slow or stuck ControllerConfig informer sync can delay Run() for a long time + // (or return early) without ever reaching the worker loop, leaving MCC scaled to 0 with no fix. + optr.stopCh = stopCh + go wait.Until(func() { + changed, err := optr.ensureMachineConfigControllerReplicaCount(1) + if err != nil { + klog.Errorf("periodic machine-config-controller replica reconcile: %v", err) + return + } + if changed { + klog.Info("periodic machine-config-controller replica reconcile: patched deployment to 1 replica") + } + }, 30*time.Second, stopCh) + apiClient := optr.apiExtClient.ApiextensionsV1() _, err := apiClient.CustomResourceDefinitions().Get(context.TODO(), "controllerconfigs.machineconfiguration.openshift.io", metav1.GetOptions{}) if err != nil { @@ -505,12 +520,15 @@ func (optr *Operator) Run(workers int, stopCh <-chan struct{}) { klog.Info("Starting MachineConfigOperator") defer klog.Info("Shutting down MachineConfigOperator") - optr.stopCh = stopCh + workQueueKey := fmt.Sprintf("%s/%s", optr.namespace, optr.name) for i := 0; i < workers; i++ { go wait.Until(optr.worker, time.Second, stopCh) } + // One immediate sync (many controllers do this); otherwise we only run when an informer fires. + optr.queue.Add(workQueueKey) + <-stopCh } @@ -609,11 +627,9 @@ func (optr *Operator) sync(key string) error { // syncFuncs is the list of sync functions that are executed in order. // any error marks sync as failure. syncFuncs := []syncFunc{ - // OSImageStream must run FIRST to provide OS image information as RenderConfig will read - // images references from OSImageStream + // OSImageStream must run before RenderConfig so RenderConfig can read OS image references from OSImageStream {"OSImageStream", optr.syncOSImageStream}, - // "RenderConfig" should be the first one to run (except OSImageStream) as it sets the renderConfig in - // the operator for the sync funcs below + // "RenderConfig" sets renderConfig on the operator for the sync funcs below {"RenderConfig", optr.syncRenderConfig}, {"MachineConfiguration", optr.syncMachineConfiguration}, {"MachineConfigNode", optr.syncMachineConfigNodes}, diff --git a/pkg/operator/sync.go b/pkg/operator/sync.go index 5eeda92e0c..37673dc3f8 100644 --- a/pkg/operator/sync.go +++ b/pkg/operator/sync.go @@ -167,6 +167,12 @@ type syncError struct { } func (optr *Operator) syncAll(syncFuncs []syncFunc) error { + // Restore machine-config-controller replicas before fetchClusterOperator or any sync func. + // If fetchClusterOperator (or clearDegradedStatus after an early step) fails, we must still + // repair a manual scale-to-0; otherwise the operator pod is up but nothing fixes MCC replicas. + if err := optr.syncMachineConfigControllerReplicasEarly(nil, nil); err != nil { + return fmt.Errorf("MachineConfigControllerReplicas (pre-sync): %w", err) + } co, err := optr.fetchClusterOperator() if err != nil { @@ -1179,6 +1185,60 @@ func (optr *Operator) syncControllerConfig(config *renderConfig) error { return optr.waitForControllerConfigToBeCompleted(cc) } +// syncMachineConfigControllerReplicasEarly restores machine-config-controller replica count when it +// was scaled to 0 (or otherwise diverged). Called from syncAll before fetchClusterOperator so replica +// repair is not skipped when that call or later sync steps fail. +func (optr *Operator) syncMachineConfigControllerReplicasEarly(_ *renderConfig, _ *configv1.ClusterOperator) error { + klog.V(4).Info("MachineConfigControllerReplicas: running early replica check") + changed, err := optr.ensureMachineConfigControllerReplicaCount(1) + if err != nil { + klog.Errorf("MachineConfigControllerReplicas: %v", err) + return err + } + if changed { + klog.Info("MachineConfigControllerReplicas: patched machine-config-controller to desired replica count") + } + return nil +} + +// ensureMachineConfigControllerReplicaCount sets deployment.spec.replicas to desired when it differs. +// Uses strategic merge patch on the Deployment (same effect as oc scale / UpdateScale, avoids Scale +// subresource metadata/resourceVersion edge cases on some API servers). +// The returned bool is true when a patch was applied. +// +// Replica enforcement is also covered by: replicas in manifests/machineconfigcontroller/deployment.yaml, +// merge in lib/resourcemerge EnsureDeployment, this function from syncMachineConfigController, pre-sync +// in syncAll, and a 30s loop in Operator.Run (so repair is not blocked by a long syncAll). +func (optr *Operator) ensureMachineConfigControllerReplicaCount(desired int32) (bool, error) { + name := ctrlcommon.ControllerConfigName + ns := ctrlcommon.MCONamespace + d, err := optr.kubeClient.AppsV1().Deployments(ns).Get(context.TODO(), name, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + return false, nil + } + return false, fmt.Errorf("could not get deployment %s: %w", name, err) + } + cur := int32(1) + if d.Spec.Replicas != nil { + cur = *d.Spec.Replicas + } + if cur == desired { + return false, nil + } + klog.Infof("MachineConfigControllerReplicas: restoring %s/%s spec.replicas from %d to %d", ns, name, cur, desired) + patch := []byte(fmt.Sprintf(`{"spec":{"replicas":%d}}`, desired)) + if retryErr := retry.OnError(retry.DefaultRetry, mcoResourceApply.IsApplyErrorRetriable, func() error { + _, err := optr.kubeClient.AppsV1().Deployments(ns).Patch( + context.TODO(), name, types.StrategicMergePatchType, patch, metav1.PatchOptions{}, + ) + return err + }); retryErr != nil { + return false, fmt.Errorf("could not patch deployment %s replicas to %d: %w", name, desired, retryErr) + } + return true, nil +} + func (optr *Operator) syncMachineConfigController(config *renderConfig, _ *configv1.ClusterOperator) error { paths := manifestPaths{ clusterRoles: []string{ @@ -1261,7 +1321,19 @@ func (optr *Operator) syncMachineConfigController(config *renderConfig, _ *confi }); retryErr != nil { return retryErr } - if updated { + + // Reconcile replica count after ApplyDeployment: the merge path updates spec.replicas from the + // rendered manifest, but a manual scale-to-0 can still diverge until this runs. + desiredReplicas := int32(1) + if mcc.Spec.Replicas != nil { + desiredReplicas = *mcc.Spec.Replicas + } + scaleUpdated, err := optr.ensureMachineConfigControllerReplicaCount(desiredReplicas) + if err != nil { + return err + } + + if updated || scaleUpdated { if err := optr.waitForDeploymentRollout(mcc); err != nil { return err } From af22dc0e4c830675ab83c99f7d39b1a516c36ac9 Mon Sep 17 00:00:00 2001 From: hmongia12 Date: Wed, 13 May 2026 10:03:54 +0530 Subject: [PATCH 2/2] RFE-9213: document MCC replica checks; centralize default replica constant - Add HACKING.md steps to verify scale-to-0 and scale-to-2 converge to 1. - Add DefaultMachineConfigControllerReplicas and use it in operator sync paths. - Clarify EnsureDeployment and manifest replica comments. Co-authored-by: Cursor --- docs/HACKING.md | 56 +++++++++++++++++++ lib/resourcemerge/apps.go | 3 + .../machineconfigcontroller/deployment.yaml | 1 + pkg/controller/common/constants.go | 4 ++ pkg/operator/operator.go | 2 +- pkg/operator/sync.go | 5 +- 6 files changed, 68 insertions(+), 3 deletions(-) diff --git a/docs/HACKING.md b/docs/HACKING.md index d21c62eb5f..b07bd74dd2 100644 --- a/docs/HACKING.md +++ b/docs/HACKING.md @@ -321,6 +321,62 @@ Then, to directly push your images, run: $ ./hack/cluster-push.sh ``` +### Verifying machine-config-controller minimum replicas (RFE-9213) + +Related tracker: [RFE-9213](https://redhat.atlassian.net/browse/RFE-9213). + +After you deploy a **custom `machine-config-operator` image** (for example built with +`Dockerfile.local` — operator binary only — and only patching +`deployment/machine-config-operator` in `openshift-machine-config-operator`): + +1. **Confirm the operator pod uses your image** + +```bash +oc project openshift-machine-config-operator +oc get deploy machine-config-operator \ + -o jsonpath='{.spec.template.spec.containers[?(@.name=="machine-config-operator")].image}{"\n"}' +oc get pod -l k8s-app=machine-config-operator \ + -o jsonpath='{range .items[*]}{.metadata.name}{"\t"}{.status.containerStatuses[?(@.name=="machine-config-operator")].image}{"\n"}{end}' +``` + +2. **Scale-to-zero should recover to one replica** + +In one terminal, watch desired replicas: + +```bash +oc get deploy machine-config-controller \ + -o jsonpath='{.spec.replicas}{"\n"}' -w +``` + +In another: + +```bash +oc scale deployment/machine-config-controller --replicas=0 +``` + +You should see `spec.replicas` go to `0` briefly, then return to `1` (often within +about one operator sync or the periodic reconcile window). + +3. **Scale-to-two should not stay at two** + +```bash +oc scale deployment/machine-config-controller --replicas=2 +oc get deploy machine-config-controller -o jsonpath='{.spec.replicas}{"\n"}' -w +``` + +You should see `2` briefly, then `1` again. + +4. **Operator logs** (optional) + +```bash +oc logs deploy/machine-config-operator -c machine-config-operator --since=5m | \ + grep -iE 'MachineConfigControllerReplicas|periodic machine-config-controller|restoring' +``` + +You may also see API warnings about `node-role.kubernetes.io/master` vs +`control-plane`, or feature-gate messages when the operator binary is newer than +the cluster's `FeatureGate` object — those are unrelated to replica enforcement. + ## Hacking on `rhel-coreos` image If you own part of the operating system (from kernel to kubelet) you diff --git a/lib/resourcemerge/apps.go b/lib/resourcemerge/apps.go index fb499f42fe..b097400556 100644 --- a/lib/resourcemerge/apps.go +++ b/lib/resourcemerge/apps.go @@ -8,6 +8,8 @@ import ( // EnsureDeployment ensures that the existing matches the required. // modified is set to true when existing had to be updated with required. +// When required.Spec.Replicas is set, it is merged into existing so manual scale +// of the live Deployment cannot diverge from the rendered manifest (RFE-9213). func EnsureDeployment(modified *bool, existing *appsv1.Deployment, required appsv1.Deployment) { resourcemerge.EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) @@ -20,6 +22,7 @@ func EnsureDeployment(modified *bool, existing *appsv1.Deployment, required apps existing.Spec.Selector = required.Spec.Selector } + // spec.replicas: keep cluster object aligned with rendered manifest when required sets it. if required.Spec.Replicas != nil { if existing.Spec.Replicas == nil || *existing.Spec.Replicas != *required.Spec.Replicas { *modified = true diff --git a/manifests/machineconfigcontroller/deployment.yaml b/manifests/machineconfigcontroller/deployment.yaml index 3f2570127a..234cf95e4b 100644 --- a/manifests/machineconfigcontroller/deployment.yaml +++ b/manifests/machineconfigcontroller/deployment.yaml @@ -4,6 +4,7 @@ metadata: name: machine-config-controller namespace: {{.TargetNamespace}} spec: + # Declared desired count; operator also enforces ctrlcommon.DefaultMachineConfigControllerReplicas (RFE-9213). replicas: 1 selector: matchLabels: diff --git a/pkg/controller/common/constants.go b/pkg/controller/common/constants.go index 80b5540f30..a673de097b 100644 --- a/pkg/controller/common/constants.go +++ b/pkg/controller/common/constants.go @@ -36,6 +36,10 @@ const ( // ControllerConfigName is the name of the ControllerConfig object that controllers use ControllerConfigName = "machine-config-controller" + // DefaultMachineConfigControllerReplicas is the default and enforced minimum replica count for + // the machine-config-controller Deployment (RFE-9213). + DefaultMachineConfigControllerReplicas int32 = 1 + // KernelTypeDefault denominates the default kernel type KernelTypeDefault = "default" diff --git a/pkg/operator/operator.go b/pkg/operator/operator.go index eb9327c507..b3a3e4e2bc 100644 --- a/pkg/operator/operator.go +++ b/pkg/operator/operator.go @@ -424,7 +424,7 @@ func (optr *Operator) Run(workers int, stopCh <-chan struct{}) { // (or return early) without ever reaching the worker loop, leaving MCC scaled to 0 with no fix. optr.stopCh = stopCh go wait.Until(func() { - changed, err := optr.ensureMachineConfigControllerReplicaCount(1) + changed, err := optr.ensureMachineConfigControllerReplicaCount(ctrlcommon.DefaultMachineConfigControllerReplicas) if err != nil { klog.Errorf("periodic machine-config-controller replica reconcile: %v", err) return diff --git a/pkg/operator/sync.go b/pkg/operator/sync.go index 37673dc3f8..9eaab42db2 100644 --- a/pkg/operator/sync.go +++ b/pkg/operator/sync.go @@ -1190,7 +1190,7 @@ func (optr *Operator) syncControllerConfig(config *renderConfig) error { // repair is not skipped when that call or later sync steps fail. func (optr *Operator) syncMachineConfigControllerReplicasEarly(_ *renderConfig, _ *configv1.ClusterOperator) error { klog.V(4).Info("MachineConfigControllerReplicas: running early replica check") - changed, err := optr.ensureMachineConfigControllerReplicaCount(1) + changed, err := optr.ensureMachineConfigControllerReplicaCount(ctrlcommon.DefaultMachineConfigControllerReplicas) if err != nil { klog.Errorf("MachineConfigControllerReplicas: %v", err) return err @@ -1209,6 +1209,7 @@ func (optr *Operator) syncMachineConfigControllerReplicasEarly(_ *renderConfig, // Replica enforcement is also covered by: replicas in manifests/machineconfigcontroller/deployment.yaml, // merge in lib/resourcemerge EnsureDeployment, this function from syncMachineConfigController, pre-sync // in syncAll, and a 30s loop in Operator.Run (so repair is not blocked by a long syncAll). +// The minimum desired count is ctrlcommon.DefaultMachineConfigControllerReplicas. func (optr *Operator) ensureMachineConfigControllerReplicaCount(desired int32) (bool, error) { name := ctrlcommon.ControllerConfigName ns := ctrlcommon.MCONamespace @@ -1324,7 +1325,7 @@ func (optr *Operator) syncMachineConfigController(config *renderConfig, _ *confi // Reconcile replica count after ApplyDeployment: the merge path updates spec.replicas from the // rendered manifest, but a manual scale-to-0 can still diverge until this runs. - desiredReplicas := int32(1) + desiredReplicas := ctrlcommon.DefaultMachineConfigControllerReplicas if mcc.Spec.Replicas != nil { desiredReplicas = *mcc.Spec.Replicas }