ctrl: nro: handle paused MCPs

The situtaion is mainly mitigated in the kubeletconfig controller,
however we have a bug in the NRO controller such that it puts the NRO CR
in progressing state because paused MCP is not in updated condition.
This commit ignores checking whether a paused MCP is up-to-date, and
introduces a new status condition to report paused MCPs if exist.

Signed-off-by: Shereen Haj <shajmakh@redhat.com>

Author: shajmakh

internal/controller/numaresourcesoperator_controller.go

@@ -20,6 +20,7 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"strings"
 	"time"
 
 	appsv1 "k8s.io/api/apps/v1"
@@ -31,6 +32,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/klog/v2"
 
@@ -141,7 +143,7 @@ func (r *NUMAResourcesOperatorReconciler) Reconcile(ctx context.Context, req ctr
 
 	initialStatus := *instance.Status.DeepCopy()
 	if len(initialStatus.Conditions) == 0 {
-		instance.Status.Conditions = status.DefaultBaseConditions(time.Now())
+		instance.Status.Conditions = status.NewNUMAResourcesOperatorConditions()
 	}
 
 	if req.Name != objectnames.DefaultNUMAResourcesOperatorCrName {
@@ -241,7 +243,7 @@ func (r *NUMAResourcesOperatorReconciler) reconcileResourceAPI(ctx context.Conte
 func (r *NUMAResourcesOperatorReconciler) reconcileResourceMachineConfig(ctx context.Context, instance *nropv1.NUMAResourcesOperator, existing *rtestate.ExistingManifests, trees []nodegroupv1.Tree) intreconcile.Step {
 	// we need to sync machine configs first and wait for the MachineConfigPool updates
 	// before checking additional components for updates
-	mcpUpdatedFunc, err := r.syncMachineConfigs(ctx, instance, existing, trees)
+	mcpUpdatedFunc, pausedMCPs, err := r.syncMachineConfigs(ctx, instance, existing, trees)
 	if err != nil {
 		r.Recorder.Eventf(instance, corev1.EventTypeWarning, "FailedMCSync", "Failed to set up machine configuration for worker nodes: %v", err)
 		err = fmt.Errorf("failed to sync machine configs: %w", err)
@@ -251,7 +253,7 @@ func (r *NUMAResourcesOperatorReconciler) reconcileResourceMachineConfig(ctx con
 
 	// MCO needs to update the SELinux context removal and other stuff, and need to trigger a reboot.
 	// It can take a while.
-	mcpStatuses, mcpNamePending := syncMachineConfigPoolsStatuses(instance.Name, trees, r.ForwardMCPConds, mcpUpdatedFunc)
+	mcpStatuses, mcpNamePending := syncMachineConfigPoolsStatuses(instance.Name, trees, r.ForwardMCPConds, mcpUpdatedFunc, pausedMCPs)
 	instance.Status.MachineConfigPools = mcpStatuses
 
 	if mcpNamePending != "" {
@@ -260,6 +262,8 @@ func (r *NUMAResourcesOperatorReconciler) reconcileResourceMachineConfig(ctx con
 	}
 	instance.Status.MachineConfigPools = syncMachineConfigPoolNodeGroupConfigStatuses(instance.Status.MachineConfigPools, trees)
 
+	updateMachineConfigPoolPausedCondition(instance.Status.Conditions, instance.Generation, pausedMCPs)
+
 	return intreconcile.StepSuccess()
 }
 
@@ -399,7 +403,7 @@ func (r *NUMAResourcesOperatorReconciler) syncNodeResourceTopologyAPI(ctx contex
 	return (updatedCount == len(objStates)), err
 }
 
-func (r *NUMAResourcesOperatorReconciler) syncMachineConfigs(ctx context.Context, instance *nropv1.NUMAResourcesOperator, existing *rtestate.ExistingManifests, trees []nodegroupv1.Tree) (rtestate.MCPWaitForUpdatedFunc, error) {
+func (r *NUMAResourcesOperatorReconciler) syncMachineConfigs(ctx context.Context, instance *nropv1.NUMAResourcesOperator, existing *rtestate.ExistingManifests, trees []nodegroupv1.Tree) (rtestate.MCPWaitForUpdatedFunc, sets.Set[string], error) {
 	klog.V(4).InfoS("Machine Config Sync start", "trees", len(trees))
 	defer klog.V(4).Info("Machine Config Sync stop")
 
@@ -409,7 +413,7 @@ func (r *NUMAResourcesOperatorReconciler) syncMachineConfigs(ctx context.Context
 	// In case of operator upgrade from 4.1X → 4.18, it's necessary to remove the old MachineConfig,
 	// unless an emergency annotation is provided which forces the operator to use custom policy
 
-	objStates, waitFunc := existing.MachineConfigsState(r.RTEManifests)
+	objStates, waitFunc, pausedMCPs := existing.MachineConfigsState(r.RTEManifests)
 	for _, objState := range objStates {
 		klog.InfoS("objState", "desired", objState.Desired, "existing", objState.Existing, "createOrUpdate", objState.IsCreateOrUpdate())
 		if objState.IsCreateOrUpdate() {
@@ -429,10 +433,10 @@ func (r *NUMAResourcesOperatorReconciler) syncMachineConfigs(ctx context.Context
 			break
 		}
 	}
-	return waitFunc, err
+	return waitFunc, pausedMCPs, err
 }
 
-func syncMachineConfigPoolsStatuses(instanceName string, trees []nodegroupv1.Tree, forwardMCPConds bool, updatedFunc rtestate.MCPWaitForUpdatedFunc) ([]nropv1.MachineConfigPool, string) {
+func syncMachineConfigPoolsStatuses(instanceName string, trees []nodegroupv1.Tree, forwardMCPConds bool, updatedFunc rtestate.MCPWaitForUpdatedFunc, pausedMCPs sets.Set[string]) ([]nropv1.MachineConfigPool, string) {
 	klog.V(4).InfoS("Machine Config Status Sync start", "trees", len(trees))
 	defer klog.V(4).Info("Machine Config Status Sync stop")
 
@@ -441,6 +445,11 @@ func syncMachineConfigPoolsStatuses(instanceName string, trees []nodegroupv1.Tre
 		for _, mcp := range tree.MachineConfigPools {
 			mcpStatuses = append(mcpStatuses, extractMCPStatus(mcp, forwardMCPConds))
 
+			if pausedMCPs.Has(mcp.Name) {
+				klog.V(5).InfoS("Paused MachineConfigPool detected", "name", mcp.Name)
+				continue
+			}
+
 			isUpdated := updatedFunc(instanceName, mcp)
 			klog.V(5).InfoS("Machine Config Pool state", "name", mcp.Name, "instance", instanceName, "updated", isUpdated)
 
@@ -813,3 +822,21 @@ func getTreesByNodeGroup(ctx context.Context, cli client.Client, nodeGroups []nr
 		return nil, fmt.Errorf("unsupported platform")
 	}
 }
+
+func updateMachineConfigPoolPausedCondition(conditions []metav1.Condition, generation int64, pausedMCPs sets.Set[string]) {
+	pausedStatus := metav1.ConditionFalse
+	message := ""
+	if pausedMCPs.Len() > 0 {
+		pausedStatus = metav1.ConditionTrue
+		message = "detected paused MCPs: " + strings.Join(pausedMCPs.UnsortedList(), ", ")
+	}
+	condition := metav1.Condition{
+		Type:               status.ConditionMachineConfigPoolPaused,
+		Status:             pausedStatus,
+		Reason:             status.ConditionMachineConfigPoolPaused,
+		Message:            message,
+		ObservedGeneration: generation,
+		LastTransitionTime: metav1.Now(),
+	}
+	status.UpdateConditionsInPlace(conditions, condition, time.Time{})
+}

internal/controller/numaresourcesoperator_controller_test.go

@@ -2195,6 +2195,105 @@ var _ = Describe("Test NUMAResourcesOperator Reconcile", func() {
 				Expect(apierrors.IsNotFound(err)).To(BeTrue(), "MachineConfig %s expected to be deleted; err=%v", mc2Key.Name, err)
 			})
 		})
+
+		Context("with status condition updates", func() {
+			var nro *nropv1.NUMAResourcesOperator
+			var mcp *machineconfigv1.MachineConfigPool
+			var reconciler *NUMAResourcesOperatorReconciler
+			var label map[string]string
+			var key client.ObjectKey
+			var err error
+
+			ctx := context.TODO()
+
+			BeforeEach(func() {
+				label = map[string]string{
+					"test1": "test1",
+				}
+
+				ng := nropv1.NodeGroup{
+					MachineConfigPoolSelector: &metav1.LabelSelector{
+						MatchLabels: label,
+					},
+				}
+				nro = testobjs.NewNUMAResourcesOperator(objectnames.DefaultNUMAResourcesOperatorCrName, ng)
+				key = client.ObjectKeyFromObject(nro)
+
+				mcp = testobjs.NewMachineConfigPool("test1", label, &metav1.LabelSelector{MatchLabels: label}, &metav1.LabelSelector{MatchLabels: label})
+
+				reconciler, err = NewFakeNUMAResourcesOperatorReconciler(platform.OpenShift, defaultOCPVersion, nro, mcp)
+				Expect(err).ToNot(HaveOccurred())
+
+				result, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: key})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(result).To(Equal(reconcile.Result{}))
+			})
+			It("should report progressing condition for un-paused MCPs but in updating state", func() {
+				Expect(reconciler.Client.Get(ctx, key, nro)).To(Succeed())
+				Expect(reconciler.Client.Get(ctx, client.ObjectKeyFromObject(mcp), mcp)).To(Succeed())
+				mcp.Status.Conditions = []machineconfigv1.MachineConfigPoolCondition{
+					{
+						Type:   machineconfigv1.MachineConfigPoolUpdated,
+						Status: corev1.ConditionFalse,
+					},
+				}
+
+				Expect(reconciler.Client.Update(ctx, mcp)).To(Succeed())
+				result, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: key})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(result).To(Equal(reconcile.Result{RequeueAfter: time.Minute}))
+
+				Expect(reconciler.Client.Get(ctx, key, nro)).To(Succeed())
+
+				progressingCondition := getConditionByType(nro.Status.Conditions, status.ConditionProgressing)
+				Expect(progressingCondition.Status).To(Equal(metav1.ConditionTrue))
+				Expect(progressingCondition.Reason).To(Equal("MachineConfigPoolIsUpdating"))
+				Expect(progressingCondition.Message).To(ContainSubstring("test1 is updating"))
+			})
+			It("should not report progressing condition for paused MCPs", func() {
+				Expect(reconciler.Client.Get(ctx, client.ObjectKeyFromObject(mcp), mcp)).To(Succeed())
+				mcp.Status.Conditions = []machineconfigv1.MachineConfigPoolCondition{
+					{
+						Type:   machineconfigv1.MachineConfigPoolUpdated,
+						Status: corev1.ConditionFalse,
+					},
+				}
+				mcp.Spec.Paused = true
+
+				Expect(reconciler.Client.Update(ctx, mcp)).To(Succeed())
+				result, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: key})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(result).To(Equal(reconcile.Result{}))
+
+				Expect(reconciler.Client.Get(ctx, key, nro)).To(Succeed())
+
+				progressingCondition := getConditionByType(nro.Status.Conditions, status.ConditionProgressing)
+				Expect(progressingCondition.Status).To(Equal(metav1.ConditionFalse), "Progressing condition should be false because the MCP is paused, got conditions: %v", nro.Status.Conditions)
+			})
+
+			It("should report MachineConfigPoolPaused condition for paused MCPs", func() {
+				Expect(reconciler.Client.Get(ctx, client.ObjectKeyFromObject(mcp), mcp)).To(Succeed())
+				mcp.Status.Conditions = []machineconfigv1.MachineConfigPoolCondition{
+					{
+						Type:   machineconfigv1.MachineConfigPoolUpdated,
+						Status: corev1.ConditionFalse,
+					},
+				}
+				mcp.Spec.Paused = true
+
+				Expect(reconciler.Client.Update(ctx, mcp)).To(Succeed())
+				result, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: key})
+				Expect(err).ToNot(HaveOccurred())
+				Expect(result).To(Equal(reconcile.Result{}))
+
+				Expect(reconciler.Client.Get(ctx, key, nro)).To(Succeed())
+
+				pausedCondition := getConditionByType(nro.Status.Conditions, status.ConditionMachineConfigPoolPaused)
+				Expect(pausedCondition.Status).To(Equal(metav1.ConditionTrue), "Paused condition should be true because the MCP is paused, got conditions: %v", nro.Status.Conditions)
+				Expect(pausedCondition.Reason).To(Equal(status.ConditionMachineConfigPoolPaused))
+				Expect(pausedCondition.Message).To(ContainSubstring("detected paused MCPs: test1"))
+			})
+		})
 	})
 })
 

internal/controller/numaresourcesscheduler_controller.go

@@ -117,7 +117,7 @@ func (r *NUMAResourcesSchedulerReconciler) Reconcile(ctx context.Context, req ct
 
 	initialStatus := *instance.Status.DeepCopy()
 	if len(initialStatus.Conditions) == 0 {
-		instance.Status.Conditions = status.NewNUMAResourcesSchedulerBaseConditions()
+		instance.Status.Conditions = status.NewNUMAResourcesSchedulerConditions()
 	}
 
 	if req.Name != objectnames.DefaultNUMAResourcesSchedulerCrName {

pkg/objectstate/rte/machineconfigpool.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog/v2"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -129,14 +130,20 @@ func MatchMachineConfigPoolCondition(conditions []machineconfigv1.MachineConfigP
 
 type MCPWaitForUpdatedFunc func(string, *machineconfigv1.MachineConfigPool) bool
 
-func (em *ExistingManifests) MachineConfigsState(mf Manifests) ([]objectstate.ObjectState, MCPWaitForUpdatedFunc) {
+func (em *ExistingManifests) MachineConfigsState(mf Manifests) ([]objectstate.ObjectState, MCPWaitForUpdatedFunc, sets.Set[string]) {
+	pausedMCPs := sets.New[string]()
 	var ret []objectstate.ObjectState
 	if mf.Core.MachineConfig == nil {
-		return ret, nullMachineConfigPoolUpdated
+		return ret, nullMachineConfigPoolUpdated, pausedMCPs
 	}
 	enabledMCCount := 0
 	for _, tree := range em.trees {
 		for _, mcp := range tree.MachineConfigPools {
+			if mcp.Spec.Paused {
+				pausedMCPs.Insert(mcp.Name)
+				continue
+			}
+
 			mcName := objectnames.GetMachineConfigName(em.instance.Name, mcp.Name)
 			if mcp.Spec.MachineConfigSelector == nil {
 				klog.Warningf("the machine config pool %q does not have machine config selector", mcp.Name)
@@ -183,10 +190,16 @@ func (em *ExistingManifests) MachineConfigsState(mf Manifests) ([]objectstate.Ob
 	}
 
 	klog.V(4).InfoS("machineConfigsState", "enabledMachineConfigs", enabledMCCount)
+	// TODO: the API design allows to configure custom annotation per nodegroup,
+	// meaning that the function that checks if an MCP is updated or not is different
+	// in each case. This should be refactored to adapt the flexible configuration or
+	// get consensus on the API design to correct this. The problem with the current
+	// approach is that nodegroups with default selinux will need to have the custom
+	// machine config, which is never met.
 	if enabledMCCount > 0 {
-		return ret, IsMachineConfigPoolUpdated
+		return ret, IsMachineConfigPoolUpdated, pausedMCPs
 	}
-	return ret, IsMachineConfigPoolUpdatedAfterDeletion
+	return ret, IsMachineConfigPoolUpdatedAfterDeletion, pausedMCPs
 }
 
 func nullMachineConfigPoolUpdated(instanceName string, mcp *machineconfigv1.MachineConfigPool) bool {

pkg/status/status.go

@@ -36,9 +36,15 @@ const (
 )
 
 const (
+	// scheduler conditions
 	ConditionDedicatedInformerActive = "DedicatedInformerActive"
 )
 
+const (
+	// operator conditions
+	ConditionMachineConfigPoolPaused = "MachineConfigPoolPaused"
+)
+
 // TODO: are we duping these?
 const (
 	ReasonAsExpected    = "AsExpected"
@@ -181,9 +187,9 @@ func DefaultBaseConditions(timestamp time.Time) []metav1.Condition {
 	}
 }
 
-// NewNUMAResourcesSchedulerBaseConditions creates specific conditions on
+// NewNUMAResourcesSchedulerConditions creates specific scheduler conditions on
 // top of NewBaseConditions.
-func NewNUMAResourcesSchedulerBaseConditions() []metav1.Condition {
+func NewNUMAResourcesSchedulerConditions() []metav1.Condition {
 	now := time.Now()
 	conds := append(DefaultBaseConditions(now), metav1.Condition{
 		Type:               ConditionDedicatedInformerActive,
@@ -194,6 +200,19 @@ func NewNUMAResourcesSchedulerBaseConditions() []metav1.Condition {
 	return conds
 }
 
+// NewNUMAResourcesOperatorConditions creates specific operator conditions on
+// top of NewBaseConditions.
+func NewNUMAResourcesOperatorConditions() []metav1.Condition {
+	now := time.Now()
+	conds := append(DefaultBaseConditions(now), metav1.Condition{
+		Type:               ConditionMachineConfigPoolPaused,
+		Status:             metav1.ConditionUnknown,
+		LastTransitionTime: metav1.Time{Time: now},
+		Reason:             ConditionMachineConfigPoolPaused,
+	})
+	return conds
+}
+
 // CloneConditions creates a deep copy of the given `conditions`.
 func CloneConditions(conditions []metav1.Condition) []metav1.Condition {
 	var c = make([]metav1.Condition, len(conditions))

pkg/status/status_test.go

@@ -331,7 +331,7 @@ func TestUpdateConditionsInPlace(t *testing.T) {
 	}{
 		{
 			name:       "first reconcile iteration - with operator condition",
-			conditions: NewNUMAResourcesSchedulerBaseConditions(),
+			conditions: NewNUMAResourcesSchedulerConditions(),
 			condition: metav1.Condition{
 				Type:               ConditionAvailable,
 				Status:             metav1.ConditionTrue,
@@ -372,7 +372,7 @@ func TestUpdateConditionsInPlace(t *testing.T) {
 		},
 		{
 			name:       "first reconcile iteration - with informer condition",
-			conditions: NewNUMAResourcesSchedulerBaseConditions(),
+			conditions: NewNUMAResourcesSchedulerConditions(),
 			condition: metav1.Condition{
 				Type:               ConditionDedicatedInformerActive,
 				Status:             metav1.ConditionTrue,