Parallel drain #555

SchSeba · 2023-12-05T13:47:12Z

The PR moves the draining from the daemon to the main controller it's based on the work from @e0ne and #552.

Also, the PR introduced a new feature to allow multiple nodes to be drained in parallel

base on #479

github-actions · 2023-12-05T13:47:25Z

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.

SchSeba · 2023-12-05T13:47:26Z

please first review #553

pkg/host/store.go

github-actions · 2023-12-05T16:01:47Z

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.

coveralls · 2023-12-05T16:07:52Z

Pull Request Test Coverage Report for Build 8249687295

Details

459 of 853 (53.81%) changed or added relevant lines in 14 files are covered.
16 unchanged lines in 4 files lost coverage.
Overall coverage increased (+1.5%) to 37.422%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/webhook/validate.go	15	19	78.95%
pkg/utils/cluster.go	29	35	82.86%
cmd/sriov-network-config-daemon/start.go	0	7	0.0%
pkg/webhook/webhook.go	0	13	0.0%
api/v1/helper.go	12	28	42.86%
pkg/platforms/mock/mock_platforms.go	22	39	56.41%
controllers/helper.go	31	51	60.78%
pkg/daemon/daemon.go	46	74	62.16%
pkg/drain/drainer.go	70	108	64.81%
controllers/drain_controller.go	216	307	70.36%

Files with Coverage Reduction	New Missed Lines	%
pkg/platforms/mock/mock_platforms.go	1	68.18%
pkg/platforms/openshift/openshift.go	1	0.0%
controllers/sriovnetworkpoolconfig_controller.go	3	63.7%
pkg/daemon/daemon.go	11	48.01%

Totals
Change from base Build 8234792093:	1.5%
Covered Lines:	4799
Relevant Lines:	12824

💛 - Coveralls

github-actions · 2023-12-05T20:48:43Z

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 · 2023-12-06T16:50:47Z

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 · 2023-12-07T16:32:06Z

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 · 2023-12-08T10:39:14Z

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 · 2023-12-08T18:07:51Z

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 · 2023-12-11T20:55:48Z

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 · 2023-12-17T18:16:51Z

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 · 2023-12-18T06:33:52Z

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 · 2023-12-18T06:36:02Z

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 · 2023-12-18T07:52: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.

github-actions · 2023-12-18T08:19:45Z

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 · 2024-03-05T15:41:17Z

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.

SchSeba · 2024-03-05T15:42:51Z

I think I answered all the comments. please take another look when you have time

ykulazhenkov · 2024-03-06T09:05:21Z

controllers/drain_controller.go

+	// configure logs
+	reqLogger := log.FromContext(ctx)
+	reqLogger.Info("Reconciling Drain")
+	reqLogger.V(2).Info("node locked for drain controller", "nodeName", req.Name)


I think we can drop this log message.

ykulazhenkov · 2024-03-06T09:06:43Z

controllers/drain_controller.go

+}
+
+func (dr *DrainReconcile) getObject(ctx context.Context, req ctrl.Request, object client.Object) error {
+	// configure logs


I think it is fine to remove this comment :)

ykulazhenkov · 2024-03-06T09:06:58Z

controllers/drain_controller.go

+func (dr *DrainReconcile) getObject(ctx context.Context, req ctrl.Request, object client.Object) error {
+	// configure logs
+	reqLogger := log.FromContext(ctx)
+	reqLogger.Info("checkForNodeDrain():")


wrong function name in the log message

ykulazhenkov · 2024-03-06T09:10:03Z

deploy/clusterrole.yaml

- apiGroups: [""]
-  resources: ["pods/eviction"]
-  verbs: ["create"]
- apiGroups: ["machineconfiguration.openshift.io"]


Do we really need to remove "machineconfiguration.openshift.io" from the role?

for nodes yes please don't need it anymore as it's part of the operator now

ykulazhenkov · 2024-03-06T09:24:24Z

pkg/utils/cluster.go

@@ -110,3 +111,53 @@ func openshiftControlPlaneTopologyStatus(c client.Client) (configv1.TopologyMode
 	}
 	return infra.Status.ControlPlaneTopology, nil
 }
+
+func ObjectHasAnnotationKey(obj metav1.Object, annoKey string) bool {
+	// Check if node already contains annotation


Can you move this comment to the docstring?

nit: the function can be simplified

func ObjectHasAnnotationKey(obj metav1.Object, annoKey string) bool { _, hasKey := obj.GetAnnotations()[annoKey] return hasKey }

ykulazhenkov · 2024-03-06T09:25:18Z

pkg/utils/cluster.go

+}
+
+func ObjectHasAnnotation(obj metav1.Object, annoKey string, value string) bool {
+	// Check if node already contains annotation


Can you move this to the docstring?

ykulazhenkov · 2024-03-06T09:49:51Z

pkg/daemon/daemon.go

-	}
-
-	if dn.nodeState.GetGeneration() == latest {
+	if dn.currentNodeState.GetGeneration() == latest && !utils.ObjectHasAnnotation(dn.desiredNodeState, consts.NodeStateDrainAnnotationCurrent, consts.DrainComplete) {


Can you move !utils.ObjectHasAnnotation(dn.desiredNodeState, consts.NodeStateDrainAnnotationCurrent, consts.DrainComplete) part to the helper function with a meaningful name, e.g. isDrainInProgress()

ykulazhenkov · 2024-03-06T10:37:36Z

pkg/daemon/daemon.go

-	if !dn.disableDrain {
-		if err := drain.RunCordonOrUncordon(dn.drainer, dn.node, false); err != nil {
+	if reqReboot {
+		log.Log.Info("handleDrain(): apply 'Reboot_Required' label for node")


ykulazhenkov · 2024-03-06T10:37:47Z

pkg/daemon/daemon.go

-		pausePatch := []byte("{\"spec\":{\"paused\":false}}")
-		if _, err := dn.platformHelpers.GetMcClient().MachineconfigurationV1().MachineConfigPools().Patch(context.Background(), dn.mcpName, types.MergePatchType, pausePatch, metav1.PatchOptions{}); err != nil {
-			log.Log.Error(err, "completeDrain(): failed to resume MCP", "mcp-name", dn.mcpName)
+		log.Log.Info("handleDrain(): apply 'Reboot_Required' label for nodeState")


ykulazhenkov · 2024-03-06T10:38:07Z

pkg/daemon/daemon.go

+		log.Log.Info("handleDrain(): apply 'Reboot_Required' label for node")
+		err := utils.AnnotateNode(vars.NodeName, consts.NodeDrainAnnotation, consts.RebootRequired, dn.client)
+		if err != nil {
+			log.Log.Error(err, "applyDrainRequired(): Failed to annotate node")


wrong func name in the log message

ykulazhenkov · 2024-03-06T10:38:22Z

pkg/daemon/daemon.go

 	}

-	if err := dn.annotateNode(vars.NodeName, annoIdle); err != nil {
-		log.Log.Error(err, "completeDrain(): failed to annotate node")
+	log.Log.Info("handleDrain(): apply 'Drain_Required' label for nodeState")


ykulazhenkov · 2024-03-06T13:06:05Z

controllers/drain_controller.go

+	return value, nil
+}
+
+func (dr *DrainReconcile) checkForNodeDrain(ctx context.Context, node *corev1.Node) (*reconcile.Result, error) {


The function name is a bit confusing for me. Rename to tryDrainNode?

ykulazhenkov · 2024-03-06T13:11:07Z

controllers/drain_controller.go

+
+		return selectedNpcl[0], nodeList.Items, nil
+	} else {
+		logger.V(2).Info("using \"default\" sriovNetworkPool with MaxUnavailable of one", "pool", *defaultNpcl)


Do we want to mention default pool in the log message? Maybe we should change the message to say something like: node doesn't belong to any pool, using default drain configuration

github-actions · 2024-03-07T18:53:40Z

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.

SchSeba · 2024-03-07T18:54:25Z

Done @ykulazhenkov with the comments thanks!

be aware that in the daemon I use the background context as we don't have context there.
that will change on my next PR where we change the daemon to use controller-runtime manager

github-actions · 2024-03-07T20:09:36Z

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.

ykulazhenkov · 2024-03-08T07:31:42Z

Done @ykulazhenkov with the comments thanks!

be aware that in the daemon I use the background context as we don't have context there. that will change on my next PR where we change the daemon to use controller-runtime manager

Thanks @SchSeba for addressing my comments.
Please, check the last one #555 (comment)

github-actions · 2024-03-10T08:30:13Z

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.

SchSeba · 2024-03-10T09:41:52Z

Hi @ykulazhenkov done thanks!

github-actions · 2024-03-10T13:51:10Z

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.

SchSeba · 2024-03-11T11:50:20Z

Hi @zeeke @e0ne @ykulazhenkov when you can please take another look on the PR I will like to try and get this one merge soon.

Thanks!

ykulazhenkov · 2024-03-11T12:05:30Z

Thanks, @SchSeba.
I think it is important to have agreement about this comment #555 (comment) before we merge.
Can you comment?

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

github-actions · 2024-03-12T13:29:50Z

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.

SchSeba · 2024-03-12T14:41:34Z

Hi @ykulazhenkov I did the requested change for the empty node selector let me know if that works for you :)

ykulazhenkov

Thanks @SchSeba,
LGTM

e0ne

LGTM

SchSeba · 2024-03-15T14:28:56Z

Thanks for all the help reviewing this complicated feature!

merging after 3 approvals

SchSeba mentioned this pull request Dec 5, 2023

Drain controller #552

Closed

github-advanced-security bot found potential problems Dec 5, 2023

View reviewed changes

pkg/host/store.go Fixed Show fixed Hide fixed

SchSeba force-pushed the parallel_drain branch from 7a7cf01 to db8e79a Compare December 5, 2023 16:01

SchSeba force-pushed the parallel_drain branch from db8e79a to 72587d9 Compare December 5, 2023 20:48

SchSeba force-pushed the parallel_drain branch from c3dc047 to 054ead2 Compare December 7, 2023 16:31

SchSeba force-pushed the parallel_drain branch from 054ead2 to a9fc289 Compare December 8, 2023 10:39

SchSeba force-pushed the parallel_drain branch from a9fc289 to 5d1a68f Compare December 8, 2023 18:07

SchSeba force-pushed the parallel_drain branch from 5d1a68f to 1f01bce Compare December 11, 2023 20:55

SchSeba force-pushed the parallel_drain branch from c4bb38f to 726a60f Compare December 18, 2023 06:33

SchSeba force-pushed the parallel_drain branch from 726a60f to 6d02042 Compare December 18, 2023 06:35

SchSeba force-pushed the parallel_drain branch from 6d02042 to 7c8cfbb Compare December 18, 2023 07:52

SchSeba force-pushed the parallel_drain branch from 7c8cfbb to 4527d48 Compare December 18, 2023 08:19

This was referenced Dec 21, 2023

Drain controller #488

Closed

Release SRIOV Network Operator V1.3.0 #450

Closed

If the pod expulsion or removal fails during the nodedrain operation, the entire node will be schdulingDisabled #429

Closed

SchSeba added 2 commits March 5, 2024 17:40

update operator and daemon package to use the new drain method

bc7383a

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

add webhook validation for pool

fb36c70

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

SchSeba force-pushed the parallel_drain branch from 05a6158 to 8d80f3e Compare March 5, 2024 15:41

ykulazhenkov reviewed Mar 6, 2024

View reviewed changes

SchSeba force-pushed the parallel_drain branch from 8d80f3e to 1681015 Compare March 7, 2024 18:53

SchSeba force-pushed the parallel_drain branch from 1681015 to b883018 Compare March 7, 2024 20:09

SchSeba force-pushed the parallel_drain branch from b883018 to 58f941f Compare March 10, 2024 08:30

SchSeba force-pushed the parallel_drain branch from 58f941f to c4dd503 Compare March 10, 2024 13:50

Fix comments and rebase

8b98f50

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

SchSeba force-pushed the parallel_drain branch from c4dd503 to 8b98f50 Compare March 12, 2024 13:29

ykulazhenkov approved these changes Mar 13, 2024

View reviewed changes

e0ne approved these changes Mar 13, 2024

View reviewed changes

SchSeba merged commit 1fac942 into k8snetworkplumbingwg:master Mar 15, 2024
12 checks passed

Parallel drain #555

Parallel drain #555

Conversation

SchSeba commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

SchSeba commented Dec 5, 2023

github-actions bot commented Dec 5, 2023

coveralls commented Dec 5, 2023 • edited Loading

Pull Request Test Coverage Report for Build 8249687295

Details

💛 - Coveralls

github-actions bot commented Dec 5, 2023

github-actions bot commented Dec 6, 2023

github-actions bot commented Dec 7, 2023

github-actions bot commented Dec 8, 2023

github-actions bot commented Dec 8, 2023

github-actions bot commented Dec 11, 2023

github-actions bot commented Dec 17, 2023

github-actions bot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

github-actions bot commented Dec 18, 2023

github-actions bot commented Mar 5, 2024

SchSeba commented Mar 5, 2024

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

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 Mar 7, 2024

SchSeba commented Mar 7, 2024

github-actions bot commented Mar 7, 2024

ykulazhenkov commented Mar 8, 2024

github-actions bot commented Mar 10, 2024

SchSeba commented Mar 10, 2024

github-actions bot commented Mar 10, 2024

SchSeba commented Mar 11, 2024

ykulazhenkov commented Mar 11, 2024

github-actions bot commented Mar 12, 2024

SchSeba commented Mar 12, 2024

ykulazhenkov left a comment

Choose a reason for hiding this comment

e0ne left a comment

Choose a reason for hiding this comment

SchSeba commented Mar 15, 2024

coveralls commented Dec 5, 2023 •

edited

Loading