Support Pause/Unpause MCP in Openshift #61
Conversation
adrianchiris
changed the title
Pull Request Test Coverage Report for Build 12904981600Details
💛 - Coveralls |
|
code is complete, i need to test on ocp setup. LMK if you prefer i split to multiple commits |
Implement MCPManager which pauses/unpauses MCP in openshift environments. - Add Openshift detection logic - Implement MCPManager - Integrate MCPManager in nodemaintenance controller Signed-off-by: adrianc <[email protected]>
adrianchiris
force-pushed
the
mcp-support
branch
2 times, most recently
from
8def208 to
a6e78a6
Compare
adrianchiris
changed the title
| reqLog.Info("Handle Scheduled NodeMaintenance") | ||
| var err error | ||
| var res ctrl.Result |
There was a problem hiding this comment.
Why do we need the res? It always the same, no?
There was a problem hiding this comment.
nit: my preference is to explicitly return a separate instance of ctrl.Result in each return statement.
There was a problem hiding this comment.
Why do we need the res? It always the same, no?
we need to return a non empty result in case of L237
removed the res and returning ctrl.Result
| @@ -248,7 +269,11 @@ func (r *NodeMaintenanceReconciler) handleCordonState(ctx context.Context, reqLo | |||
| } | |||
| } | |||
|
|
|||
| // TODO(adrianc): unpause MCP in OCP when support is added. | |||
| err = r.MCPManager.UnpauseMCP(ctx, node, nm) | |||
There was a problem hiding this comment.
nit:
if err := r.MCPManager.UnpauseMCP(ctx, node, nm); err != nil {
return err
}
There was a problem hiding this comment.
aligned for mcp manager, though the rest of the code does not follow this style.
| @@ -289,7 +314,11 @@ func (r *NodeMaintenanceReconciler) handleWaitPodCompletionState(ctx context.Con | |||
| } | |||
| } | |||
|
|
|||
| // TODO(adrianc): unpause MCP in OCP when support is added. | |||
| err = r.MCPManager.UnpauseMCP(ctx, node, nm) | |||
There was a problem hiding this comment.
nit: use the single line if err :=
cmd/maintenance-manager/main.go
Outdated
| if err = (&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), |
There was a problem hiding this comment.
nit: just an opinion
IMO, It would be better to explicitly select the manager in main.go and skip initializing MCPManager for non-OpenShift clusters. This helps avoid confusion in the Reconciler code, where it's not immediately clear that certain logic applies only to OpenShift.
There was a problem hiding this comment.
so, you prefer to have in the controller code :
if r.MCPManager != nil {
// do stuff with MCPManager
}
?
i was kinda on the fence between the two approaches. lemme rework this then.
There was a problem hiding this comment.
I prefer to have an "if openshift" check everywhere to decide if we need to make a call to MCPManager or not. This means we will call openshift-specific logic very explicitly.
The current implementation is also ok, but for me it is a bit harder to follow. It is ok to keep everything as is if you prefer so.
| reqLog.Info("Handle Scheduled NodeMaintenance") | ||
| var err error | ||
| var res ctrl.Result |
There was a problem hiding this comment.
nit: my preference is to explicitly return a separate instance of ctrl.Result in each return statement.
- drop no-op MCP manager - style changes Signed-off-by: adrianc <[email protected]>
adrianchiris
force-pushed
the
mcp-support
branch
from
7047d78 to
afcbe66
Compare
Implement MCPManager which pauses/unpauses MCP
in openshift environments.