Support draining DaemonSet pods using sriov devices #840

SchSeba · 2025-02-09T18:53:29Z

with this commit we also take care of removing DaemonSet owned pods using sriov devices.

we only do it when drain is requested we don't do it for reboot requests

github-actions · 2025-02-09T18:53:42Z

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 · 2025-02-09T19:00:23Z

Pull Request Test Coverage Report for Build 13370907002

Details

20 of 54 (37.04%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.05%) to 47.344%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/drain/drainer.go	20	54	37.04%

Files with Coverage Reduction	New Missed Lines	%
pkg/drain/drainer.go	2	59.33%

Totals
Change from base Build 13109399258:	0.05%
Covered Lines:	7282
Relevant Lines:	15381

💛 - Coveralls

bn222 · 2025-02-10T09:07:37Z

pkg/drain/drainer.go

+		// remove pods that are owned by a DaemonSet and use SR-IOV devices
+		dsPodsList := getDsPodsToRemove(podList)
+		for _, pod := range dsPodsList {
+			err = d.kubeClient.CoreV1().Pods(pod.Namespace).Delete(ctx, pod.Name, metav1.DeleteOptions{})


Shouldn't we block? Waiting to ensure that the pod is fully removed before continuing? A pod that's slow to delete might cause a race condition

yes I switch to use the drain from the kubernetes drain helper it does that :)

pkg/drain/drainer.go

adrianchiris · 2025-02-18T11:59:19Z

pkg/drain/drainer.go

+
+		// on full drain there is no need to try and remove pods that are owned by DaemonSets
+		// as we are going to reboot the node in any case.
+		if fullNodeDrain {


do we care ? (just thinking how to simplify) why not always remove DS pods from node IF they have sriov resources ?

I removed this one as requested :)

ykulazhenkov · 2025-02-20T13:52:31Z

test/conformance/tests/test_sriov_operator.go

@@ -2376,6 +2538,18 @@ func waitForPodRunning(p *corev1.Pod) *corev1.Pod {
 	return ret
 }

+func waitForDaemonReady(d *appsv1.DaemonSet) *appsv1.DaemonSet {


nit: waitForDaemonSetReady

ykulazhenkov · 2025-02-20T13:56:05Z

pkg/drain/drainer.go

-		reqLogger.Info("drainNode(): Draining failed, retrying", "error", err)
-		return false, nil
+
+		err = d.removeDaemonSetsFromNode(ctx, node.Name)


I'm not entirely clear on how this works. I understand the process of selecting and removing DS pods, but I'm unsure how we prevent the DS controller from restarting pods on the node we intend to drain. Could you clarify?

So the idea is that if the daemon use vfs the new pod will stuck on pending so after we finish the configuration the daemon will be able to start

Why the DS-managed Pod will be in Pending state?
Is it because of the cordon/taint? I think that the fact that the node is cordoned will not prevent DS pods to restart on it.
Or Is it because SRIOV resources are not exposed on the node? If so, Are we sure that SRIOV resources will not be exposed on the node at the moment when we try to kill the DS-managed pod? How we ensure this?

I don’t have a complete picture of how the operator’s drain logic works, so I might be missing something obvious—but I think it’s worth asking :)

in case of reconfiguration ds pod may get back to running state if there are still sriov resources on the worker IMO.

indeed cordon does not affect daemonset pods (tested in kind cluster).
adding a custom taint with effect NoSchedule will prevent DS pod from getting rescheduled.

Adding a commit from the community meeting.

instead if using taint that can be harmful for the cluster (if some daemonset get deleted they will stuck until the sriov operator finish the work)

we agree to implement in the drain controller a removal of the device plugin label from the node + the removal of the running device plugin pod

@adrianchiris @ykulazhenkov @zeeke let me know what do you think before I start coding it please :)

we roll more responsibilites to the drain controller.

maybe it should be the role of config daemon to ensure device plugin no longer runs on node before requesting drain ?
then put node label back after config completed.

just a different apporach to consider. im not against the current one.

with this commit we also take care of removing daemonset owned pods using sriov devices. we only do it when drain is requested we don't do it for reboot requests Signed-off-by: Sebastian Sch <[email protected]>

ykulazhenkov · 2025-02-28T08:28:13Z

I checked the code and it looks good. I have one question about the logic. I want to make sure that we are not missing anything.

bn222 reviewed Feb 10, 2025

View reviewed changes

SchSeba force-pushed the drain_daemon branch 2 times, most recently from 2140f89 to c20fba0 Compare February 17, 2025 13:03

adrianchiris reviewed Feb 18, 2025

View reviewed changes

pkg/drain/drainer.go Show resolved Hide resolved

adrianchiris reviewed Feb 18, 2025

View reviewed changes

SchSeba force-pushed the drain_daemon branch from c20fba0 to cd2fb78 Compare February 19, 2025 12:15

ykulazhenkov reviewed Feb 20, 2025

View reviewed changes

SchSeba force-pushed the drain_daemon branch from cd2fb78 to 2067ad5 Compare February 24, 2025 10:18

Support draining daemonset pods that use sriov devices

68754e7

with this commit we also take care of removing daemonset owned pods using sriov devices. we only do it when drain is requested we don't do it for reboot requests Signed-off-by: Sebastian Sch <[email protected]>

SchSeba force-pushed the drain_daemon branch from 2067ad5 to 68754e7 Compare February 24, 2025 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support draining DaemonSet pods using sriov devices #840

Support draining DaemonSet pods using sriov devices #840

SchSeba commented Feb 9, 2025

github-actions bot commented Feb 9, 2025

coveralls commented Feb 9, 2025 •

edited

Loading

bn222 Feb 10, 2025

SchSeba Feb 17, 2025 •

edited

Loading

adrianchiris Feb 18, 2025 •

edited

Loading

SchSeba Feb 24, 2025

ykulazhenkov Feb 20, 2025

SchSeba Feb 24, 2025

ykulazhenkov Feb 20, 2025

SchSeba Feb 24, 2025

ykulazhenkov Feb 28, 2025

adrianchiris Mar 3, 2025

SchSeba Mar 5, 2025

ykulazhenkov Mar 5, 2025

adrianchiris Mar 6, 2025

ykulazhenkov commented Feb 28, 2025

Support draining DaemonSet pods using sriov devices #840

Are you sure you want to change the base?

Support draining DaemonSet pods using sriov devices #840

Conversation

SchSeba commented Feb 9, 2025

github-actions bot commented Feb 9, 2025

coveralls commented Feb 9, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13370907002

Details

💛 - Coveralls

Choose a reason for hiding this comment

SchSeba Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

adrianchiris Feb 18, 2025 • 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

ykulazhenkov commented Feb 28, 2025

coveralls commented Feb 9, 2025 •

edited

Loading

SchSeba Feb 17, 2025 •

edited

Loading

adrianchiris Feb 18, 2025 •

edited

Loading