RFE-9213: enforce machine-config-controller minimum replicas

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

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,15 @@ 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
+			r := *required.Spec.Replicas
+			existing.Spec.Replicas = &r
+		}
+	}
+
 	ensurePodTemplateSpec(modified, &existing.Spec.Template, required.Spec.Template)
 }
 

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)
+		}
+	})
+}

manifests/machineconfigcontroller/deployment.yaml

@@ -4,6 +4,8 @@ metadata:
   name: machine-config-controller
   namespace: {{.TargetNamespace}}
 spec:
+  # Declared desired count; operator also enforces ctrlcommon.DefaultMachineConfigControllerReplicas (RFE-9213).
+  replicas: 1
   selector:
     matchLabels:
       k8s-app: machine-config-controller

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"
 

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(ctrlcommon.DefaultMachineConfigControllerReplicas)
+		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},

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,61 @@ 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(ctrlcommon.DefaultMachineConfigControllerReplicas)
+	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).
+// The minimum desired count is ctrlcommon.DefaultMachineConfigControllerReplicas.
+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 +1322,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 := ctrlcommon.DefaultMachineConfigControllerReplicas
+	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
 		}