ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP

Having a paused MCP should prevent updating/creating the corresponding
config map for the specified node group. So far, the code wasn't
considering the case of paused MCPs, which lead to creating/updating the config map
a thing that caused a mismatch between the configuration in the config
map and the one reflected on the NRTs.
In this commit, we modify the kubeletconfig controller to skip on
updating RTE config maps that belong to paused MCPs.

Signed-off-by: Shereen Haj <[email protected]>

Author: shajmakh

internal/controller/kubeletconfig_controller.go

@@ -49,6 +49,7 @@ import (
 
 	nropv1 "github.com/openshift-kni/numaresources-operator/api/v1"
 	"github.com/openshift-kni/numaresources-operator/internal/machineconfigpools"
+	intreconcile "github.com/openshift-kni/numaresources-operator/internal/reconcile"
 	"github.com/openshift-kni/numaresources-operator/pkg/apply"
 	"github.com/openshift-kni/numaresources-operator/pkg/kubeletconfig"
 	"github.com/openshift-kni/numaresources-operator/pkg/objectnames"
@@ -57,7 +58,8 @@ import (
 )
 
 const (
-	kubeletConfigRetryPeriod = 30 * time.Second
+	kubeletConfigRetryPeriod           = 30 * time.Second
+	MachineConfigPoolPausedRetryPeriod = 2 * time.Minute
 )
 
 const (
@@ -116,22 +118,20 @@ func (r *KubeletConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 
 	// KubeletConfig changes are expected to be sporadic, yet are important enough
 	// to be made visible at kubernetes level. So we generate events to handle them
-	cm, err := r.reconcileConfigMap(ctx, instance, req.NamespacedName)
-	if err != nil {
-		var klErr *InvalidKubeletConfig
-		if errors.As(err, &klErr) {
-			r.Recorder.Event(instance, "Normal", "ProcessSkip", "ignored kubelet config "+klErr.ObjectName)
-			return ctrl.Result{}, nil
-		}
-
-		klog.ErrorS(err, "failed to reconcile configmap", "controller", "kubeletconfig")
+	cm, step := r.reconcileConfigMap(ctx, instance, req.NamespacedName)
+	if step.Error != nil && step.ConditionInfo.Reason != intreconcile.EventProcessSkip {
+		klog.ErrorS(step.Error, "failed to reconcile configmap", "controller", "kubeletconfig")
+		r.Recorder.Event(instance, step.ConditionInfo.Type, step.ConditionInfo.Reason, step.ConditionInfo.Message)
+		return step.Result, step.Error
+	}
 
-		r.Recorder.Event(instance, "Warning", "ProcessFailed", "Failed to update RTE config from kubelet config "+req.NamespacedName.String())
-		return ctrl.Result{}, err
+	if step.ConditionInfo.Reason == intreconcile.EventProcessSuccess {
+		step = step.WithMessage(fmt.Sprintf("Updated RTE config %s/%s from kubelet config %s", cm.Namespace, cm.Name, req.NamespacedName.String()))
 	}
 
-	r.Recorder.Event(instance, "Normal", "ProcessOK", fmt.Sprintf("Updated RTE config %s/%s from kubelet config %s", cm.Namespace, cm.Name, req.NamespacedName.String()))
-	return ctrl.Result{}, nil
+	r.Recorder.Event(instance, step.ConditionInfo.Type, step.ConditionInfo.Reason, step.ConditionInfo.Message)
+
+	return step.Result, nil
 }
 
 func (r *KubeletConfigReconciler) SetupWithManager(mgr ctrl.Manager) error {
@@ -197,25 +197,29 @@ func (e *InvalidKubeletConfig) Unwrap() error {
 	return e.Err
 }
 
-func (r *KubeletConfigReconciler) reconcileConfigMap(ctx context.Context, instance *nropv1.NUMAResourcesOperator, kcKey client.ObjectKey) (*corev1.ConfigMap, error) {
+func (r *KubeletConfigReconciler) reconcileConfigMap(ctx context.Context, instance *nropv1.NUMAResourcesOperator, kcKey client.ObjectKey) (*corev1.ConfigMap, intreconcile.Step) {
 	// first check if the ConfigMap should be deleted
 	// to save all the additional work related for create/update
 	cm, deleted, err := r.deleteConfigMap(ctx, instance, kcKey)
 	if deleted {
-		return cm, err
+		return cm, intreconcile.StepWarning(fmt.Errorf("Failed to update RTE config from kubelet config %s: %v", kcKey.Name, err))
 	}
 
-	kcHandler, err := r.makeKCHandlerForPlatform(ctx, instance, kcKey)
-	if err != nil {
-		return nil, err
+	kcHandler, step := r.makeKCHandlerForPlatform(ctx, instance, kcKey)
+	if step.Error != nil {
+		return nil, step
 	}
+
 	kubeletConfig, err := kubeletconfig.MCOKubeletConfToKubeletConf(kcHandler.mcoKc)
 	if err != nil {
 		klog.ErrorS(err, "cannot extract KubeletConfiguration from MCO KubeletConfig", "name", kcKey.Name)
-		return nil, err
+		return nil, FailedConfigMapUpdateStep(kcKey.Name, err)
 	}
-
-	return r.syncConfigMap(ctx, kubeletConfig, instance, kcHandler)
+	cm, err = r.syncConfigMap(ctx, kubeletConfig, instance, kcHandler)
+	if err != nil {
+		return cm, FailedConfigMapUpdateStep(kcKey.Name, err)
+	}
+	return cm, intreconcile.StepNormalSucess("")
 }
 
 func (r *KubeletConfigReconciler) syncConfigMap(ctx context.Context, kubeletConfig *kubeletconfigv1beta1.KubeletConfiguration, instance *nropv1.NUMAResourcesOperator, kcHandler *kubeletConfigHandler) (*corev1.ConfigMap, error) {
@@ -244,63 +248,68 @@ func (r *KubeletConfigReconciler) syncConfigMap(ctx context.Context, kubeletConf
 	return rendered, nil
 }
 
-func (r *KubeletConfigReconciler) makeKCHandlerForPlatform(ctx context.Context, instance *nropv1.NUMAResourcesOperator, kcKey client.ObjectKey) (*kubeletConfigHandler, error) {
+func (r *KubeletConfigReconciler) makeKCHandlerForPlatform(ctx context.Context, instance *nropv1.NUMAResourcesOperator, kcKey client.ObjectKey) (*kubeletConfigHandler, intreconcile.Step) {
 	switch r.Platform {
 	case platform.OpenShift:
 		mcoKc := &mcov1.KubeletConfig{}
 		if err := r.Client.Get(ctx, kcKey, mcoKc); err != nil {
-			return nil, err
+			return nil, FailedConfigMapUpdateStep(kcKey.Name, err)
 		}
 
 		mcps, err := machineconfigpools.GetListByNodeGroupsV1(ctx, r.Client, instance.Spec.NodeGroups)
 		if err != nil {
-			return nil, err
+			return nil, FailedConfigMapUpdateStep(kcKey.Name, err)
 		}
 
 		mcp, err := machineconfigpools.FindBySelector(mcps, mcoKc.Spec.MachineConfigPoolSelector)
 		if err != nil {
 			klog.ErrorS(err, "cannot find a matching mcp for MCO KubeletConfig", "name", kcKey.Name)
 			var notFound *machineconfigpools.NotFound
 			if errors.As(err, &notFound) {
-				return nil, &InvalidKubeletConfig{
-					ObjectName: kcKey.Name,
-					Err:        notFound,
-				}
+				return nil, intreconcile.StepNormalSkip(fmt.Errorf("%s: %v", kcKey, notFound))
 			}
-			return nil, err
+			return nil, FailedConfigMapUpdateStep(kcKey.Name, err)
 		}
 
 		klog.V(3).InfoS("matched MCP to MCO KubeletConfig", "kubeletconfig name", kcKey.Name, "MCP name", mcp.Name)
 
 		// nothing we care about, and we can't do much anyway
 		if mcoKc.Spec.KubeletConfig == nil {
 			klog.InfoS("detected KubeletConfig with empty payload, ignoring", "name", kcKey.Name)
-			return nil, &InvalidKubeletConfig{ObjectName: kcKey.Name}
+			return nil, intreconcile.StepNormalSkip(fmt.Errorf("Invalid KubeletConfig %s", kcKey.Name))
 		}
+
+		if mcp.Spec.Paused {
+			klog.InfoS("detected paused MCP", "name", mcp.Name)
+			step := intreconcile.StepNormalSkip(fmt.Errorf("MachineConfigPool of KubeletConfig %s is paused", kcKey.Name))
+			step.Result = ctrl.Result{RequeueAfter: MachineConfigPoolPausedRetryPeriod}
+			return nil, step
+		}
+
 		return &kubeletConfigHandler{
 			ownerObject: mcoKc,
 			mcoKc:       mcoKc,
 			poolName:    mcp.Name,
 			setCtrlRef:  controllerutil.SetControllerReference,
-		}, nil
+		}, intreconcile.StepNormalSucess("")
 
 	case platform.HyperShift:
 		cmKc := &corev1.ConfigMap{}
 		if err := r.Client.Get(ctx, kcKey, cmKc); err != nil {
-			return nil, err
+			return nil, FailedConfigMapUpdateStep(kcKey.Name, err)
 		}
 
 		nodePoolName := cmKc.Labels[HyperShiftNodePoolLabel]
 		kcData := cmKc.Data[HyperShiftConfigMapConfigKey]
 		mcoKc, err := kubeletconfig.DecodeFromData([]byte(kcData), r.Scheme)
 		if err != nil {
-			return nil, err
+			return nil, FailedConfigMapUpdateStep(kcKey.Name, err)
 		}
 
 		// nothing we care about, and we can't do much anyway
 		if mcoKc.Spec.KubeletConfig == nil {
 			klog.InfoS("detected KubeletConfig with empty payload, ignoring", "name", kcKey.Name)
-			return nil, &InvalidKubeletConfig{ObjectName: kcKey.Name}
+			return nil, intreconcile.StepNormalSkip(fmt.Errorf("Invalid KubeletConfig %s", kcKey.Name))
 		}
 		return &kubeletConfigHandler{
 			ownerObject: cmKc,
@@ -312,9 +321,12 @@ func (r *KubeletConfigReconciler) makeKCHandlerForPlatform(ctx context.Context,
 			setCtrlRef: func(owner, controlled metav1.Object, scheme *runtime.Scheme, opts ...controllerutil.OwnerReferenceOption) error {
 				return nil
 			},
-		}, nil
+		}, intreconcile.StepNormalSucess("")
 	}
-	return nil, fmt.Errorf("unsupported platform: %s", r.Platform)
+	return nil, FailedConfigMapUpdateStep(kcKey.Name, fmt.Errorf("unsupported platform: %s", r.Platform))
+}
+func FailedConfigMapUpdateStep(objName string, err error) intreconcile.Step {
+	return intreconcile.StepWarning(fmt.Errorf("Failed to update RTE config from kubelet config %s: %v", objName, err))
 }
 
 func (r *KubeletConfigReconciler) deleteConfigMap(ctx context.Context, instance *nropv1.NUMAResourcesOperator, kcKey client.ObjectKey) (*corev1.ConfigMap, bool, error) {

internal/controller/kubeletconfig_controller_test.go

@@ -272,4 +272,90 @@ var _ = Describe("Test KubeletConfig Reconcile", func() {
 		Entry("OpenShift Platform", NewFakeKubeletConfigReconciler, platform.OpenShift),
 		Entry("HyperShift Platform", NewFakeKubeletConfigReconcilerForHyperShift, platform.HyperShift),
 	)
+
+	Context("kubeletconfig updates with paused MCPs", func() {
+		var nro *nropv1.NUMAResourcesOperator
+		var mcp1, mcpPaused *machineconfigv1.MachineConfigPool
+		var mcoKC1, mcoKCPaused *machineconfigv1.KubeletConfig
+		var label1 map[string]string
+		var kc1Key, kc2Key client.ObjectKey
+		var poolName1, poolName2 string
+		cmKc1 := &corev1.ConfigMap{}
+		var reconciler *KubeletConfigReconciler
+		var err error
+
+		BeforeEach(func() {
+			label1 = map[string]string{
+				"test1": "test1",
+			}
+			mcp1 = testobjs.NewMachineConfigPool("test1", label1, &metav1.LabelSelector{MatchLabels: label1}, &metav1.LabelSelector{MatchLabels: label1})
+			poolName1 = mcp1.Name
+			kubeletConfig := &kubeletconfigv1beta1.KubeletConfiguration{}
+			mcoKC1 = testobjs.NewKubeletConfig(poolName1, label1, mcp1.Spec.MachineConfigSelector, kubeletConfig)
+			kc1Key = client.ObjectKeyFromObject(mcoKC1)
+
+			label2 := map[string]string{
+				"test2": "test2",
+			}
+			mcpPaused = testobjs.NewMachineConfigPool("test2", label2, &metav1.LabelSelector{MatchLabels: label2}, &metav1.LabelSelector{MatchLabels: label2})
+			mcpPaused.Spec.Paused = true
+			poolName2 = mcpPaused.Name
+			kubeletConfigPaused := &kubeletconfigv1beta1.KubeletConfiguration{}
+			mcoKCPaused = testobjs.NewKubeletConfig(poolName2, label2, mcpPaused.Spec.MachineConfigSelector, kubeletConfigPaused)
+			kc2Key = client.ObjectKeyFromObject(mcoKCPaused)
+
+			ng1 := nropv1.NodeGroup{
+				PoolName: &poolName1,
+			}
+			ng2 := nropv1.NodeGroup{
+				PoolName: &poolName2,
+			}
+			nro = testobjs.NewNUMAResourcesOperator(objectnames.DefaultNUMAResourcesOperatorCrName, ng1, ng2)
+
+			reconciler, err = NewFakeKubeletConfigReconciler(nro, mcp1, mcoKC1, cmKc1, mcpPaused, mcoKCPaused) //TODO remove cmKc1
+			Expect(err).ToNot(HaveOccurred())
+		})
+		It("should create configmap for active MCP", func() {
+			result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: kc1Key})
+			Expect(err).ToNot(HaveOccurred())
+			Expect(result).To(Equal(reconcile.Result{}))
+			cm := &corev1.ConfigMap{}
+			key := client.ObjectKey{
+				Namespace: testNamespace,
+				Name:      objectnames.GetComponentName(nro.Name, poolName1),
+			}
+			Expect(reconciler.Client.Get(context.TODO(), key, cm)).To(Succeed())
+		})
+
+		It("should not create configmap for paused MCP, then create it when un-paused", func() {
+			result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: kc2Key})
+			Expect(err).ToNot(HaveOccurred())
+
+			fakeRecorder, ok := reconciler.Recorder.(*record.FakeRecorder)
+			Expect(ok).To(BeTrue())
+			event := <-fakeRecorder.Events
+			Expect(event).To(ContainSubstring("ProcessSkip"))
+			Expect(event).To(ContainSubstring("is paused"))
+			Expect(result).To(Equal(reconcile.Result{RequeueAfter: MachineConfigPoolPausedRetryPeriod}))
+
+			cm := &corev1.ConfigMap{}
+			key := client.ObjectKey{
+				Namespace: testNamespace,
+				Name:      objectnames.GetComponentName(nro.Name, poolName2),
+			}
+			Expect(reconciler.Client.Get(context.TODO(), key, cm)).To(HaveOccurred())
+
+			mcpPaused.Spec.Paused = false
+			Expect(reconciler.Client.Update(context.TODO(), mcpPaused)).To(Succeed())
+
+			result, err = reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: kc2Key})
+			Expect(err).ToNot(HaveOccurred())
+			Expect(result).To(Equal(reconcile.Result{}))
+			Expect(reconciler.Client.Get(context.TODO(), key, cm)).To(Succeed())
+			event = <-fakeRecorder.Events
+			Expect(event).To(ContainSubstring("ProcessOK"))
+			Expect(event).To(ContainSubstring(mcoKCPaused.Name))
+		})
+	})
+
 })

internal/reconcile/event.go

@@ -0,0 +1 @@
+package reconcile

internal/reconcile/step.go

@@ -25,6 +25,15 @@ import (
 	"github.com/openshift-kni/numaresources-operator/pkg/status/conditioninfo"
 )
 
+const (
+	EventTypeNormal  = "Normal"
+	EventTypeWarning = "Warning"
+
+	EventProcessSuccess = "ProcessOK"
+	EventProcessSkip    = "ProcessSkip"
+	EventProcessFailed  = "ProcessFailed"
+)
+
 type Step struct {
 	Result        ctrl.Result
 	ConditionInfo conditioninfo.ConditionInfo
@@ -92,3 +101,35 @@ func StepFailed(err error) Step {
 		Error:         err,
 	}
 }
+
+func StepWarning(err error) Step {
+	return Step{
+		ConditionInfo: conditioninfo.ConditionInfo{
+			Type:    EventTypeWarning,
+			Message: status.MessageFromError(err),
+			Reason:  EventProcessFailed,
+		},
+		Error: err,
+	}
+}
+
+func StepNormalSkip(err error) Step {
+	return Step{
+		ConditionInfo: conditioninfo.ConditionInfo{
+			Type:    EventTypeNormal,
+			Message: status.MessageFromError(err),
+			Reason:  EventProcessSkip,
+		},
+		Error: err,
+	}
+}
+
+func StepNormalSucess(msg string) Step {
+	return Step{
+		ConditionInfo: conditioninfo.ConditionInfo{
+			Type:    EventTypeNormal,
+			Message: msg,
+			Reason:  EventProcessSuccess,
+		},
+	}
+}