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
reduce clusterRole to role for config-daemon pod delete option ( better security)

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 11775513002

Details

86 of 118 (72.88%) changed or added relevant lines in 4 files are covered.
9 unchanged lines in 2 files lost coverage.
Overall coverage increased (+1.4%) to 47.024%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/utils/cluster.go	45	59	76.27%
controllers/sriovnetworknodepolicy_controller.go	21	39	53.85%

Files with Coverage Reduction	New Missed Lines	%
controllers/helper.go	1	69.96%
api/v1/helper.go	8	75.75%

Totals
Change from base Build 11775378543:	1.4%
Covered Lines:	7103
Relevant Lines:	15105

💛 - 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

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.

I removed it from this PR

pkg/daemon/daemon.go

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

I just copy the same we have on all the others
https://github.com/k8snetworkplumbingwg/sriov-network-operator/pull/747/files#diff-0e683dcc8c5eeb8433c366296d8cb81d12ff06fea8a08637949e040a3aa334e3R78

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.

controllers/drain_controller_helper.go

pkg/daemon/daemon.go

zeeke

LGTM

great design work!

always deploy sriov network device plugin and use a label to enable or disable it on the nodes Signed-off-by: Sebastian Sch <[email protected]>

ykulazhenkov

Thanks for working on this. LGTM

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 Outdated Show resolved Hide resolved

controllers/drain_controller_test.go Outdated 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 3 times, most recently from 8152db1 to e2e2ad2 Compare October 10, 2024 11:38

adrianchiris reviewed Oct 10, 2024

View reviewed changes

controllers/drain_controller_helper.go Outdated Show resolved Hide resolved

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

SchSeba force-pushed the device_plugin_redesign branch from e2e2ad2 to 31c7fee Compare November 5, 2024 13:49

SchSeba force-pushed the device_plugin_redesign branch 4 times, most recently from 3d20dff to 46affb0 Compare November 7, 2024 10:07

zeeke approved these changes Nov 7, 2024

View reviewed changes

redesign device plugin

baa41c9

always deploy sriov network device plugin and use a label to enable or disable it on the nodes Signed-off-by: Sebastian Sch <[email protected]>

SchSeba force-pushed the device_plugin_redesign branch from 46affb0 to baa41c9 Compare November 11, 2024 09:12

ykulazhenkov approved these changes Nov 19, 2024

View reviewed changes

SchSeba merged commit 623c339 into k8snetworkplumbingwg:master Nov 19, 2024
14 checks passed

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 •

edited

Loading

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

SchSeba Nov 6, 2024

adrianchiris Oct 10, 2024

SchSeba Nov 6, 2024

adrianchiris Oct 10, 2024

SchSeba Nov 5, 2024

adrianchiris Oct 10, 2024

SchSeba Nov 7, 2024

adrianchiris Oct 10, 2024

SchSeba Nov 5, 2024

adrianchiris Oct 10, 2024

SchSeba Nov 6, 2024

adrianchiris commented Oct 10, 2024 •

edited

Loading

zeeke left a comment

ykulazhenkov left a comment

Redesign device plugin reset #747

Redesign device plugin reset #747

Conversation

SchSeba commented Jul 25, 2024 • edited Loading

github-actions bot commented Jul 25, 2024

coveralls commented Jul 25, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11775513002

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

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 commented Oct 10, 2024 • edited Loading

zeeke left a comment

Choose a reason for hiding this comment

ykulazhenkov left a comment

Choose a reason for hiding this comment

SchSeba commented Jul 25, 2024 •

edited

Loading

coveralls commented Jul 25, 2024 •

edited

Loading

adrianchiris commented Oct 10, 2024 •

edited

Loading