Redesign device plugin reset #747

SchSeba · 2024-07-25T10:30:24Z

This commit introduces a new redesign on how the operator resets the device plugin

use a general nodeSelector to avoid updating the daemonset yaml
remove the config-daemon removing pod (better security)
make the operator in charge of resetting the device plugin via annotations
mark the node as cordon BEFORE we remove the device plugin (without drain) to avoid scheduling new pods until the device plugin is backed up

github-actions · 2024-07-25T10:30:35Z

Thanks for your PR,
To run vendors CIs, Maintainers can 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, Maintainers can 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 · 2024-07-25T10:36:53Z

Pull Request Test Coverage Report for Build 11273255954

Details

273 of 435 (62.76%) changed or added relevant lines in 8 files are covered.
18 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.2%) to 45.18%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/drain/drainer.go	12	14	85.71%
pkg/daemon/daemon.go	11	16	68.75%
pkg/utils/cluster.go	27	32	84.38%
pkg/platforms/openshift/openshift.go	0	9	0.0%
controllers/drain_controller_helper.go	196	337	58.16%

Files with Coverage Reduction	New Missed Lines	%
controllers/helper.go	1	70.37%
pkg/daemon/daemon.go	2	46.9%
pkg/utils/cluster.go	3	38.58%
controllers/drain_controller.go	4	75.0%
api/v1/helper.go	8	75.07%

Totals
Change from base Build 11258053806:	0.2%
Covered Lines:	6721
Relevant Lines:	14876

💛 - Coveralls

controllers/drain_controller.go

SchSeba · 2024-08-19T11:33:39Z

@zeeke @adrianchiris @ykulazhenkov when you have time please take a look on this :)

zeeke

Left a few comments

controllers/drain_controller.go

pkg/utils/cluster.go

controllers/drain_controller.go

zeeke

Left a few comments. Overall design looks good to me but we must be super sure about all of these annotation states. I'm pretty scared of having a deadlock in some production cluster :)

controllers/helper.go

controllers/drain_controller_helper.go

controllers/drain_controller_test.go

controllers/drain_controller_helper.go

pkg/consts/constants.go

…device plugin * use a general nodeSelector to avoid updating the daemonset yaml * remove the config-daemon removing pod (better security) * make the operator in charge of resetting the device plugin via annotations * mark the node as cordon BEFORE we remove the device plugin (without drain) to avoid scheduling new pods until the device plugin is backed up Signed-off-by: Sebastian Sch <sebassch@gmail.com>

adrianchiris

partial review.

left with drain controller and its helper.

its hard to review added logic since a bunch of code was moved while new code added in existing functions in the same commit :\

adrianchiris · 2024-10-10T13:21:57Z

pkg/utils/cluster.go

+	}
+
+	if newObj.GetLabels()[key] != value {
+		log.Log.V(2).Info("LabelObject(): Annotate object",


nit: Annotate -> Label

adrianchiris · 2024-10-10T13:22:20Z

pkg/utils/cluster.go

+		err := c.Patch(ctx,
+			newObj, patch)
+		if err != nil {
+			log.Log.Error(err, "annotateObject(): Failed to patch object")


nit: fix func name

adrianchiris · 2024-10-10T13:25:53Z

pkg/utils/cluster.go

@@ -161,3 +162,40 @@ func AnnotateNode(ctx context.Context, nodeName string, key, value string, c cli

 	return AnnotateObject(ctx, node, key, value, c)
 }
+
+// LabelObject adds label to a kubernetes object
+func LabelObject(ctx context.Context, obj client.Object, key, value string, c client.Client) error {


nit: keep it internal to the package for now ? since you only use LabelNode in controller.

adrianchiris · 2024-10-10T13:28:06Z

pkg/platforms/openshift/openshift.go

@@ -228,6 +228,18 @@ func (c *openshiftContext) OpenshiftAfterCompleteDrainNode(ctx context.Context,
 		return false, err
 	}

+	value, exist := mcp.Annotations[consts.MachineConfigPoolPausedAnnotation]


is this change related to this PR ?

if not, do you prefer to submit a separate PR for it.

adrianchiris · 2024-10-10T13:34:28Z

pkg/daemon/daemon.go

+		consts.NodeStateDrainAnnotationCurrent,
+		consts.DrainIdle) {
+		log.Log.Info("nodeStateSyncHandler(): apply 'Device_Plugin_Reset_Required' annotation for node")
+		err := utils.AnnotateNode(context.Background(), vars.NodeName, consts.NodeDrainAnnotation, consts.DevicePluginResetRequired, dn.client)


do we still need to annotate both node and nodestate ?

why are we requesting device plugin restart in a different place than the original?

adrianchiris · 2024-10-10T13:38:11Z

pkg/consts/constants.go

@@ -67,12 +67,17 @@ const (
 	MachineConfigPoolPausedAnnotationIdle   = "Idle"
 	MachineConfigPoolPausedAnnotationPaused = "Paused"

+	SriovDevicePluginLabel         = "sriovnetwork.openshift.io/device-plugin"
+	SriovDevicePluginLabelEnabled  = "Enabled"


should we use lower case for label values ?

no special preference on my end. but i seldom see label values that are CamelCase

adrianchiris · 2024-10-10T13:56:28Z

controllers/helper.go

@@ -185,42 +173,17 @@ func syncPluginDaemonObjs(ctx context.Context,
 	data.Data["ReleaseVersion"] = os.Getenv("RELEASEVERSION")
 	data.Data["ResourcePrefix"] = vars.ResourcePrefix
 	data.Data["ImagePullSecrets"] = GetImagePullSecrets()
-	data.Data["NodeSelectorField"] = GetDefaultNodeSelector()
+	data.Data["NodeSelectorField"] = GetDefaultNodeSelectorForDevicePlugin()


should we merge this with the labels we set on sriov network config daemon ?

that way we have sriov device plugin deployed wherever sriov network config daemon is deployed AND has the device-plugin enabled label.

just in case we have leftovers OR we changed the selector for config daemon.

also should we clean this label (and possibly some annot while at it) from node obj if config daemon is not targeting them ?

adrianchiris · 2024-10-10T14:09:53Z

General comment:

to me it seems (much ?) simpler that the config daemon will be in charge of updating its own node label to evict DP pod.
in this case it just requests cordon from the drain controller.

the drain controller will then handle only what it was supposed to do originally (handle cordon/drain related operations)
which is easier to comprehend IMO.

we should discuss this.

adrianchiris · 2024-10-10T14:21:09Z

controllers/drain_controller_helper.go

+	}
+
+	// if we manage to cordon we label the node state with drain completed and finish
+	err = utils.AnnotateObject(ctx, nodeNetworkState, constants.NodeStateDrainAnnotationCurrent, constants.DrainComplete, dr.Client)


a bit confusing for the device plugin reset flow where we did not really drain.

adrianchiris · 2024-10-10T14:27:19Z

pkg/daemon/daemon.go

+		consts.NodeStateDrainAnnotationCurrent,
+		consts.DrainIdle) {
+		log.Log.Info("nodeStateSyncHandler(): apply 'Device_Plugin_Reset_Required' annotation for node")
+		err := utils.AnnotateNode(context.Background(), vars.NodeName, consts.NodeDrainAnnotation, consts.DevicePluginResetRequired, dn.client)


why are we requesting device plugin restart in a different place than the original?

bn222 reviewed Jul 25, 2024

View reviewed changes

controllers/drain_controller.go Outdated Show resolved Hide resolved

controllers/drain_controller.go Outdated Show resolved Hide resolved

controllers/drain_controller.go Outdated Show resolved Hide resolved

SchSeba changed the title ~~This commit introduces a new redesign on how the operator resets the …~~ Redesign device plugin reset Jul 25, 2024

SchSeba force-pushed the device_plugin_redesign branch 5 times, most recently from a3e8a58 to ba8d6ef Compare July 30, 2024 09:05

SchSeba mentioned this pull request Jul 30, 2024

Change logic to prevent device-plugin daemonset spec change #740

Closed

SchSeba force-pushed the device_plugin_redesign branch from ba8d6ef to ef25135 Compare August 5, 2024 13:20

zeeke reviewed Aug 19, 2024

View reviewed changes

controllers/drain_controller.go Outdated Show resolved Hide resolved

pkg/utils/cluster.go Outdated Show resolved Hide resolved

controllers/drain_controller.go Outdated Show resolved Hide resolved

SchSeba force-pushed the device_plugin_redesign branch 4 times, most recently from d719a8c to 0981cec Compare September 3, 2024 13:04

zeeke reviewed Sep 3, 2024

View reviewed changes

controllers/helper.go Outdated Show resolved Hide resolved

controllers/drain_controller_helper.go Show resolved Hide resolved

controllers/drain_controller_test.go Show resolved Hide resolved

controllers/drain_controller_helper.go Outdated Show resolved Hide resolved

SchSeba force-pushed the device_plugin_redesign branch from 0981cec to a71ec00 Compare October 1, 2024 10:37

zeeke reviewed Oct 1, 2024

View reviewed changes

pkg/consts/constants.go Outdated Show resolved Hide resolved

SchSeba force-pushed the device_plugin_redesign branch 3 times, most recently from 146b706 to 95e9d84 Compare October 7, 2024 13:01

SchSeba force-pushed the device_plugin_redesign branch 2 times, most recently from 324dfb1 to 8152db1 Compare October 10, 2024 10:58

SchSeba force-pushed the device_plugin_redesign branch from 8152db1 to e2e2ad2 Compare October 10, 2024 11:38

adrianchiris reviewed Oct 10, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign device plugin reset #747

Redesign device plugin reset #747

SchSeba commented Jul 25, 2024

github-actions bot commented Jul 25, 2024

coveralls commented Jul 25, 2024 •

edited

Loading

SchSeba commented Aug 19, 2024

zeeke left a comment

zeeke left a comment

adrianchiris left a comment

adrianchiris Oct 10, 2024

adrianchiris Oct 10, 2024

adrianchiris Oct 10, 2024

adrianchiris Oct 10, 2024

adrianchiris Oct 10, 2024

adrianchiris Oct 10, 2024 •

edited

Loading

adrianchiris Oct 10, 2024

adrianchiris Oct 10, 2024

adrianchiris commented Oct 10, 2024 •

edited

Loading

adrianchiris Oct 10, 2024

adrianchiris Oct 10, 2024 •

edited

Loading

Redesign device plugin reset #747

Are you sure you want to change the base?

Redesign device plugin reset #747

Conversation

SchSeba commented Jul 25, 2024

github-actions bot commented Jul 25, 2024

coveralls commented Jul 25, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11273255954

Details

💛 - Coveralls

SchSeba commented Aug 19, 2024

zeeke left a comment

Choose a reason for hiding this comment

zeeke left a comment

Choose a reason for hiding this comment

adrianchiris left a 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 Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianchiris commented Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

adrianchiris Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

coveralls commented Jul 25, 2024 •

edited

Loading

adrianchiris Oct 10, 2024 •

edited

Loading

adrianchiris commented Oct 10, 2024 •

edited

Loading

adrianchiris Oct 10, 2024 •

edited

Loading