Better support for openshift single node #213

SchSeba · 2021-12-07T18:24:05Z

This PR allows to still pause the MCP when running on OCP even if it's a single node with no drain option.

Another change is to the sriovOperatorConfig to mark the DisableDrain when there is only one node in the cluster

This commit allow to still pause the MCP when running on OCP. This is needed when you have only 1 node in the cluster and the drainSkip is true Signed-off-by: Sebastian Sch <sebassch@gmail.com>

browsell · 2021-12-08T00:31:10Z

controllers/sriovoperatorconfig_controller.go

+				return reconcile.Result{}, err
+			}
+
+			disableDrain := len(nodeList.Items) == 1


We should check that the the controlPlaneTopology = SingleReplica

The machineconfiguration.openshift.io/controlPlaneTopology: SingleReplica is a MCO label. I want this feature to work also for plain k8s

zshi-redhat · 2021-12-08T02:04:57Z

controllers/sriovoperatorconfig_controller.go

@@ -78,6 +78,14 @@ func (r *SriovOperatorConfigReconciler) Reconcile(ctx context.Context, req ctrl.
 		Name: constants.DEFAULT_CONFIG_NAME, Namespace: namespace}, defaultConfig)
 	if err != nil {
 		if errors.IsNotFound(err) {
+			// Check if we only have one node


This error won't be triggered unless user delete the default SriovOperatorConfig explicitly. Is it what we want? The default SriovOperatorConfig is created by OperatorConfig controller: https://github.com/k8snetworkplumbingwg/sriov-network-operator/blob/master/main.go#L154

Do we need to consider the upgrade scenario? e.g. from lower version to 4.10 which contains this fix. If not, I think adding the disableDrain setting in the above 154 line shall be sufficient.

@zshi-redhat thanks! I miss that file!

I will like to also work on updates so I put the change in both places.

The reason I put it on both places is a was able to make a race from when I install the operator + a policy and until there is a reconcile to add the skip

e0ne · 2021-12-08T11:37:35Z

controllers/sriovoperatorconfig_controller.go

+
+	// Update the disableDrain field if needed
+	if len(nodeList.Items) == 1 && !defaultConfig.Spec.DisableDrain {
+		defaultConfig.Spec.DisableDrain = true


Please, add logging here. We need to let users know that configuration is changed

this is a one way switch, what if the cluster scales down to one then scales up to more that 1 node ?

@adrianchiris do you think we should support this case?

This function is only if the config doesn't exist this means on install time or if the user for some reason removes this file.

If the user change the cluster topology he can

update the sriov operator config

remove the config and let the operator understand the cluster topology

I will not like to make the sriovOperatorConfig reconcile to check the cluster status.
I will not like also to add a trigger for this object on a node object change it will just spam the operator on a large cluster

This function is only if the config doesn't exist this means on install time or if the user for some reason removes this file.

This is not under the if block at L80, so it will be executed every time Reconcile is called.
maybe user put some configuration, then cluster scaled down (to 1) and operator restarted. this logic would then change the user configuration. later on cluster scaled up and we ended up with drain disabled.

So Here are my thoughts:

we document that in case of a single Node cluster, the user should explicitly set DisableDrain

we do this logic only when (re)creating the default object (within if statement block in L80 or in main.go)

im fine with either 1 or 2
2. probably having a better user experience.

I also prefer 2 changing the code as requested

e0ne · 2021-12-08T11:38:29Z

main.go

@@ -232,13 +233,23 @@ func createDefaultOperatorConfig(cfg *rest.Config) error {
 	if err != nil {
 		return fmt.Errorf("Couldn't create client: %v", err)
 	}
+
+	// Check if we only have one node


Actually this commit message doesn't align with a code below

nit: i think reading L237 is pretty clear, you can drop this comment if you like.

done

done and not removed ? :)

sorry about that

e0ne · 2021-12-08T11:40:39Z

controllers/sriovoperatorconfig_controller.go

@@ -99,6 +100,25 @@ func (r *SriovOperatorConfigReconciler) Reconcile(ctx context.Context, req ctrl.
 		return reconcile.Result{}, err
 	}

+	// Check if we only have one node


The same comment a for main.go change. Do we want to move this check to pkg.utils module as 'GetNodesConut' function?

done can you please have another look?

nit: i think reading L104 is pretty clear, you can drop this comment if you like.

pkg/daemon/daemon.go

SchSeba · 2021-12-12T11:10:25Z

Hi @e0ne @pliurh @adrianchiris can you please give this another look?

adrianchiris · 2021-12-12T14:47:59Z

pkg/daemon/daemon.go

-	done := make(chan bool)
-	go dn.getDrainLock(ctx, done)
-	<-done
+	if utils.ClusterType != utils.ClusterTypeOpenshift {


should we move (and inverse) this check to daemon.go L510 ? and only call pauseMCP in openshift cluster ?

sure no problem

adrianchiris · 2021-12-12T14:50:58Z

pkg/utils/cluster.go

+	}
+
+	if len(nodeList.Items) == 1 {
+		glog.Infof("only one node found in the cluster marking disableDrain as true")


nit: i dont think this log is needed.

if you want to keep it, please update the message as disableDrain is now out of context.
(IsSingleNodeCluster doesnt really care what operations may be done due to its invocation)

I will like to leave the message makes debug easier :)

I just change the comment

adrianchiris · 2021-12-12T15:00:52Z

@SchSeba might be a dumb question, but why do we want to disable drain on a single node cluster ? :)
critical services are daemonsets (usually) and these wont go away right ?

SchSeba · 2021-12-13T09:43:51Z

Hi @adrianchiris there are cases when the user have a 1 node cluster and deploy some applications using pod disruption budget for example that is the case in openshift we have some platform pods using the PDB so the drain will never finish and the operator will just stuck trying to drain the node.

github-actions · 2021-12-13T15:23:22Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

github-actions · 2021-12-13T15:24:27Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

pliurh · 2021-12-14T06:59:27Z

/lgtm

adrianchiris · 2021-12-14T16:15:19Z

pkg/daemon/daemon.go

+
+		glog.Infof("nodeStateSyncHandler(): get drain lock for sriov daemon")
+		done := make(chan bool)
+		go dn.getDrainLock(ctx, done)


in Kubernetes cluster Type we will now always try to get the drain lock, even if disableDrain is enabled.
previously it skipped getting the lock.

not sure its a major issue, as i think disableDrain is used mainly for testing
@zshi-redhat @pliurh WDYT ?

I think it is a minor issue. If the cluster is a single node cluster, there is no other node that needs to wait for the lock. For multi-node clusters, this flag is mainly for testing. The drawback is that, as the LeaseDuration is 5s, we will have to waste 5s on each node when it requires draining.

ack thx @pliurh ! id prefer to condition on (disableDrain or Openshift ) to avoid api calls for the lease and the 5s delay per node.

@adrianchiris done please have a look I also test it internally

github-actions · 2021-12-16T17:42:08Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

Signed-off-by: Sebastian Sch <sebassch@gmail.com>

github-actions · 2021-12-16T17:43:37Z

Thanks for your PR,
To run vendors CIs use one of:

/test-all: To run all tests for all vendors.
/test-e2e-all: To run all E2E tests for all vendors.
/test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

/skip-all: To skip all tests for all vendors.
/skip-e2e-all: To skip all E2E tests for all vendors.
/skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
Best regards.

adrianchiris · 2021-12-19T07:49:55Z

/test-all

adrianchiris · 2021-12-19T08:36:22Z

Failure related to #218

zshi-redhat · 2021-12-20T03:55:17Z

/test-all

zshi-redhat · 2021-12-21T02:11:31Z

Fix proposed in #219

adrianchiris · 2021-12-21T08:44:24Z

/test-all

SchSeba · 2021-12-22T10:09:29Z

Hi @adrianchiris anything we are missing here or we can merge this PR?

adrianchiris

Thanks for the reminder @SchSeba ! LGTM

Support pause MCP on SNO

616062f

This commit allow to still pause the MCP when running on OCP. This is needed when you have only 1 node in the cluster and the drainSkip is true Signed-off-by: Sebastian Sch <sebassch@gmail.com>

SchSeba requested review from zshi-redhat and pliurh December 7, 2021 18:24

browsell reviewed Dec 8, 2021

View reviewed changes

zshi-redhat reviewed Dec 8, 2021

View reviewed changes

SchSeba force-pushed the support_pause_on_SNO branch 2 times, most recently from fb7f39c to cd10a0a Compare December 8, 2021 10:54

e0ne requested changes Dec 8, 2021

View reviewed changes

pliurh reviewed Dec 9, 2021

View reviewed changes

pkg/daemon/daemon.go Outdated Show resolved Hide resolved

SchSeba force-pushed the support_pause_on_SNO branch from cd10a0a to 9d02c76 Compare December 9, 2021 16:31

adrianchiris reviewed Dec 12, 2021

View reviewed changes

SchSeba force-pushed the support_pause_on_SNO branch from 9d02c76 to 4d40435 Compare December 13, 2021 15:23

SchSeba force-pushed the support_pause_on_SNO branch from 4d40435 to 06d2fbe Compare December 13, 2021 15:24

github-actions bot added the lgtm label Dec 14, 2021

adrianchiris reviewed Dec 14, 2021

View reviewed changes

SchSeba force-pushed the support_pause_on_SNO branch from 06d2fbe to 58827f1 Compare December 16, 2021 17:41

Add the DisableDrain when running one a single node

0e08eb7

Signed-off-by: Sebastian Sch <sebassch@gmail.com>

SchSeba force-pushed the support_pause_on_SNO branch from 58827f1 to 0e08eb7 Compare December 16, 2021 17:43

adrianchiris approved these changes Dec 22, 2021

View reviewed changes

adrianchiris merged commit de3966d into k8snetworkplumbingwg:master Dec 22, 2021

Better support for openshift single node #213

Better support for openshift single node #213

Conversation

SchSeba commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchSeba commented Dec 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris Dec 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris commented Dec 12, 2021

SchSeba commented Dec 13, 2021

github-actions bot commented Dec 13, 2021

github-actions bot commented Dec 13, 2021

pliurh commented Dec 14, 2021

adrianchiris Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 16, 2021

github-actions bot commented Dec 16, 2021

adrianchiris commented Dec 19, 2021

adrianchiris commented Dec 19, 2021

zshi-redhat commented Dec 20, 2021

zshi-redhat commented Dec 21, 2021

adrianchiris commented Dec 21, 2021

SchSeba commented Dec 22, 2021

adrianchiris left a comment

Choose a reason for hiding this comment

adrianchiris Dec 12, 2021 •

edited

Loading

adrianchiris Dec 12, 2021 •

edited

Loading

adrianchiris Dec 14, 2021 •

edited

Loading