Address comments

- drop no-op MCP manager - style changes Signed-off-by: adrianc <[email protected]>
Mellanox · Jan 22, 2025 · afcbe66 · afcbe66
1 parent a6e78a6
commit afcbe66
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 76 deletions.
diff --git a/cmd/maintenance-manager/main.go b/cmd/maintenance-manager/main.go
@@ -163,14 +163,20 @@ func main() {
 			"isOpenshift", ocpUtils.IsOpenshift(), "isHypershift", ocpUtils.IsHypershift())
 	}
 
-	if err = (&controller.NodeMaintenanceReconciler{
+	nmReconciler := &controller.NodeMaintenanceReconciler{
 		Client:                   mgrClient,
 		Scheme:                   mgr.GetScheme(),
 		CordonHandler:            cordon.NewCordonHandler(mgrClient, k8sInterface),
 		WaitPodCompletionHandler: podcompletion.NewPodCompletionHandler(mgrClient),
 		DrainManager:             drain.NewManager(ctrl.Log.WithName("DrainManager"), ctx, k8sInterface),
-		MCPManager:               openshift.NewMCPManager(ocpUtils, mgrClient),
-	}).SetupWithManager(ctx, mgr, ctrl.Log.WithName("NodeMaintenanceReconciler")); err != nil {
+		MCPManager:               nil,
+	}
+
+	if ocpUtils.IsOpenshift() && !ocpUtils.IsHypershift() {
+		nmReconciler.MCPManager = openshift.NewMCPManager(mgrClient)
+	}
+
+	if err = nmReconciler.SetupWithManager(ctx, mgr, ctrl.Log.WithName("NodeMaintenanceReconciler")); err != nil {
 		setupLog.Error(err, "unable to create controller", "controller", "NodeMaintenance")
 		os.Exit(1)
 	}

diff --git a/internal/controller/nodemaintenance_controller.go b/internal/controller/nodemaintenance_controller.go
@@ -209,49 +209,50 @@ func (r *NodeMaintenanceReconciler) handleUninitiaizedState(ctx context.Context,
 func (r *NodeMaintenanceReconciler) handleScheduledState(ctx context.Context, reqLog logr.Logger, nm *maintenancev1.NodeMaintenance, node *corev1.Node) (ctrl.Result, error) {
 	reqLog.Info("Handle Scheduled NodeMaintenance")
 	var err error
-	var res ctrl.Result
 
 	if nm.GetDeletionTimestamp().IsZero() {
 		// conditionally add finalizer
 		err = k8sutils.AddFinalizer(ctx, r.Client, nm, maintenancev1.MaintenanceFinalizerName)
 		if err != nil {
 			reqLog.Error(err, "failed to set finalizer for NodeMaintenance")
-			return res, err
+			return ctrl.Result{}, err
 		}
 	} else {
 		// object is being deleted, remove finalizer if exists and return
 		reqLog.Info("NodeMaintenance object is deleting")
 
-		err = r.MCPManager.UnpauseMCP(ctx, node, nm)
-		if err != nil {
-			return res, err
+		if r.MCPManager != nil {
+			if err = r.MCPManager.UnpauseMCP(ctx, node, nm); err != nil {
+				return ctrl.Result{}, err
+			}
 		}
 
-		return res, r.handleFinalizerRemoval(ctx, reqLog, nm)
+		return ctrl.Result{}, r.handleFinalizerRemoval(ctx, reqLog, nm)
 	}
 
-	err = r.MCPManager.PauseMCP(ctx, node, nm)
-	if err != nil {
-		if errors.Is(err, openshift.ErrMachineConfigBusy) {
-			reqLog.Info("machine config pool is busy, requeue", "error", err)
-			return ctrl.Result{Requeue: true, RequeueAfter: pauseMCPRequeueTime}, nil
+	if r.MCPManager != nil {
+		if err = r.MCPManager.PauseMCP(ctx, node, nm); err != nil {
+			if errors.Is(err, openshift.ErrMachineConfigBusy) {
+				reqLog.Info("machine config pool is busy, requeue", "error", err)
+				return ctrl.Result{Requeue: true, RequeueAfter: pauseMCPRequeueTime}, nil
+			}
+			reqLog.Error(err, "failed to pause MachineConfigPool")
+			return ctrl.Result{}, fmt.Errorf("failed to pause MachineConfigPool. %w", err)
 		}
-		reqLog.Error(err, "failed to pause MachineConfigPool")
-		return res, fmt.Errorf("failed to pause MachineConfigPool. %w", err)
 	}
 
 	// Set Ready condition to ConditionReasonCordon and update object
 	err = k8sutils.SetReadyConditionReason(ctx, r.Client, nm, maintenancev1.ConditionReasonCordon)
 	if err != nil {
 		reqLog.Error(err, "failed to update status for NodeMaintenance object")
-		return res, err
+		return ctrl.Result{}, err
 	}
 
 	// emit state change event
 	r.EventRecorder.Event(
 		nm, corev1.EventTypeNormal, maintenancev1.ConditionChangedEventType, maintenancev1.ConditionReasonCordon)
 
-	return res, nil
+	return ctrl.Result{}, nil
 }
 
 func (r *NodeMaintenanceReconciler) handleCordonState(ctx context.Context, reqLog logr.Logger, nm *maintenancev1.NodeMaintenance, node *corev1.Node) error {
@@ -269,9 +270,10 @@ func (r *NodeMaintenanceReconciler) handleCordonState(ctx context.Context, reqLo
 			}
 		}
 
-		err = r.MCPManager.UnpauseMCP(ctx, node, nm)
-		if err != nil {
-			return err
+		if r.MCPManager != nil {
+			if err = r.MCPManager.UnpauseMCP(ctx, node, nm); err != nil {
+				return err
+			}
 		}
 
 		return r.handleFinalizerRemoval(ctx, reqLog, nm)
@@ -314,9 +316,10 @@ func (r *NodeMaintenanceReconciler) handleWaitPodCompletionState(ctx context.Con
 			}
 		}
 
-		err = r.MCPManager.UnpauseMCP(ctx, node, nm)
-		if err != nil {
-			return res, err
+		if r.MCPManager != nil {
+			if err = r.MCPManager.UnpauseMCP(ctx, node, nm); err != nil {
+				return res, err
+			}
 		}
 
 		err = r.handleFinalizerRemoval(ctx, reqLog, nm)
@@ -456,9 +459,10 @@ func (r *NodeMaintenanceReconciler) handleDrainStateDelete(ctx context.Context,
 		}
 	}
 
-	err = r.MCPManager.UnpauseMCP(ctx, node, nm)
-	if err != nil {
-		return res, err
+	if r.MCPManager != nil {
+		if err = r.MCPManager.UnpauseMCP(ctx, node, nm); err != nil {
+			return res, err
+		}
 	}
 
 	// remove finalizer if exists and return
@@ -528,9 +532,10 @@ func (r *NodeMaintenanceReconciler) handleTerminalState(ctx context.Context, req
 			}
 		}
 
-		err = r.MCPManager.UnpauseMCP(ctx, node, nm)
-		if err != nil {
-			return res, err
+		if r.MCPManager != nil {
+			if err = r.MCPManager.UnpauseMCP(ctx, node, nm); err != nil {
+				return res, err
+			}
 		}
 
 		// remove finalizer if exists and return

diff --git a/internal/controller/nodemaintenance_controller_test.go b/internal/controller/nodemaintenance_controller_test.go
@@ -38,7 +38,6 @@ import (
 	"github.com/Mellanox/maintenance-operator/internal/cordon"
 	"github.com/Mellanox/maintenance-operator/internal/drain"
 	"github.com/Mellanox/maintenance-operator/internal/k8sutils"
-	"github.com/Mellanox/maintenance-operator/internal/openshift"
 	"github.com/Mellanox/maintenance-operator/internal/podcompletion"
 	"github.com/Mellanox/maintenance-operator/internal/testutils"
 )
@@ -82,7 +81,7 @@ var _ = Describe("NodeMaintenance Controller", func() {
 				WaitPodCompletionHandler: podcompletion.NewPodCompletionHandler(k8sClient),
 				DrainManager: drain.NewManager(ctrllog.Log.WithName("DrainManager"),
 					testCtx, k8sInterface),
-				MCPManager: openshift.NewNoOpMcpManager(),
+				MCPManager: nil,
 			}
 
 			// setup reconciler with manager

diff --git a/internal/openshift/mcp.go b/internal/openshift/mcp.go
@@ -59,42 +59,19 @@ type MCPManager interface {
 	UnpauseMCP(ctx context.Context, node *corev1.Node, nm *maintenancev1.NodeMaintenance) error
 }
 
-// NewMCPManager returns a new MCPManager based on the cluster type
-func NewMCPManager(ocpUtils OpenshiftUtils, client client.Client) MCPManager {
-	if ocpUtils.IsOpenshift() && !ocpUtils.IsHypershift() {
-		return NewOpenshiftMcpManager(client)
-	}
-	return NewNoOpMcpManager()
-}
-
-// noOpMcpManager implements MCPManager with no-op methods
-type noOpMcpManager struct{}
-
-func (n *noOpMcpManager) PauseMCP(ctx context.Context, node *corev1.Node, nm *maintenancev1.NodeMaintenance) error {
-	return nil
-}
-
-func (n *noOpMcpManager) UnpauseMCP(ctx context.Context, node *corev1.Node, nm *maintenancev1.NodeMaintenance) error {
-	return nil
-}
-
-// NewNoOpMcpManager returns a no-op MCPManager. used for non-openshift clusters
-func NewNoOpMcpManager() MCPManager {
-	return &noOpMcpManager{}
+// NewMCPManager returns a new MCPManager
+func NewMCPManager(client client.Client) MCPManager {
+	return &mcpManager{client: client, mu: sync.Mutex{}}
 }
 
-// openshiftMcpManager implements MCPManager for openshift clusters
-type openshiftMcpManager struct {
+// mcpManager implements MCPManager for openshift clusters
+type mcpManager struct {
 	client client.Client
 	mu     sync.Mutex
 }
 
-// NewOpenshiftMcpManager returns a new MCPManager. used for openshift clusters
-func NewOpenshiftMcpManager(client client.Client) MCPManager {
-	return &openshiftMcpManager{client: client, mu: sync.Mutex{}}
-}
-
-func (o *openshiftMcpManager) PauseMCP(ctx context.Context, node *corev1.Node, nm *maintenancev1.NodeMaintenance) error {
+// PauseMCP pauses the MachineConfigPool on the given node
+func (o *mcpManager) PauseMCP(ctx context.Context, node *corev1.Node, nm *maintenancev1.NodeMaintenance) error {
 	o.mu.Lock()
 	defer o.mu.Unlock()
 
@@ -176,7 +153,8 @@ func (o *openshiftMcpManager) PauseMCP(ctx context.Context, node *corev1.Node, n
 	return nil
 }
 
-func (o *openshiftMcpManager) UnpauseMCP(ctx context.Context, node *corev1.Node, nm *maintenancev1.NodeMaintenance) error {
+// UnpauseMCP unpauses the MachineConfigPool on the given node
+func (o *mcpManager) UnpauseMCP(ctx context.Context, node *corev1.Node, nm *maintenancev1.NodeMaintenance) error {
 	o.mu.Lock()
 	defer o.mu.Unlock()
 
@@ -224,7 +202,7 @@ func (o *openshiftMcpManager) UnpauseMCP(ctx context.Context, node *corev1.Node,
 }
 
 // getMCPName returns the MachineConfigPool name for the given node
-func (o *openshiftMcpManager) getMCPName(ctx context.Context, node *corev1.Node) (string, error) {
+func (o *mcpManager) getMCPName(ctx context.Context, node *corev1.Node) (string, error) {
 	// To get the MachineConfigPool for the node, we do the following:
 	// 1. get the desired MachineConfig from the node annotation
 	// 2. find the owning MachineConfigPool for the MachineConfig we found in step 1
@@ -247,7 +225,7 @@ func (o *openshiftMcpManager) getMCPName(ctx context.Context, node *corev1.Node)
 }
 
 // changeMachineConfigPoolPause pauses/unpauses the MachineConfigPool
-func (o *openshiftMcpManager) changeMachineConfigPoolPause(ctx context.Context, mcp *mcv1.MachineConfigPool, pause bool) error {
+func (o *mcpManager) changeMachineConfigPoolPause(ctx context.Context, mcp *mcv1.MachineConfigPool, pause bool) error {
 	// create merge patch to pause/unpause and annotate/un-annotate the machine config pool
 	var patch []byte
 	if pause {
@@ -265,7 +243,7 @@ func (o *openshiftMcpManager) changeMachineConfigPoolPause(ctx context.Context,
 }
 
 // mcpUsedByOtherNodeMaintenance return true if the MCP is used by other Nodes which have NodeMaintenance associated with them that already paused MCP
-func (o *openshiftMcpManager) mcpUsedByOtherNodeMaintenance(ctx context.Context, mcp *mcv1.MachineConfigPool, nm *maintenancev1.NodeMaintenance) (bool, error) {
+func (o *mcpManager) mcpUsedByOtherNodeMaintenance(ctx context.Context, mcp *mcv1.MachineConfigPool, nm *maintenancev1.NodeMaintenance) (bool, error) {
 	// get all nodes that match the pool
 	nodesInPool := &corev1.NodeList{}
 	selector, err := metav1.LabelSelectorAsSelector(mcp.Spec.NodeSelector)

diff --git a/internal/openshift/mcp_test.go b/internal/openshift/mcp_test.go
@@ -41,15 +41,6 @@ const (
 )
 
 var _ = Describe("mcp manager tests", func() {
-	Context("mcp no-op tests", func() {
-		It("should return correct values", func() {
-			testCtx := context.TODO()
-			noOpMcpManager := openshift.NewNoOpMcpManager()
-			Expect(noOpMcpManager.PauseMCP(testCtx, nil, nil)).To(Succeed())
-			Expect(noOpMcpManager.UnpauseMCP(testCtx, nil, nil)).To(Succeed())
-		})
-	})
-
 	Context("mcp manager tests", func() {
 		var testClient client.Client
 		var testCtx context.Context
@@ -75,7 +66,7 @@ var _ = Describe("mcp manager tests", func() {
 				WithObjects(node, mc, mcp, nm).
 				WithStatusSubresource(&mcv1.MachineConfigPool{}).
 				Build()
-			mcpManager = openshift.NewOpenshiftMcpManager(testClient)
+			mcpManager = openshift.NewMCPManager(testClient)
 		})
 
 		Context("PauseMCP", func() {