Add support for user defined tolerances to modules #940

tsprasannaa · 2024-11-23T15:51:44Z

There is a requirement to run selected pods on a cordoned node. One such example is device driver upgrade. The procedure involves the node to be cordoned. At the same time, there should be a way to run house keeping pods that carry out the driver upgrades. These pods should be schedulable.
This PR adds support to carry the tolerances for the pods that carry out the house keeping operations. These user defined tolerances will match the taint that gets added to the nodes.

ModuleSpec will be used to carry the tolerance to the pods which will be used during pod creation

linux-foundation-easycla · 2024-11-23T15:51:48Z

The committers listed above are authorized under a signed CLA.

✅ login: tsprasannaa / name: Prasannaa TS (a3966c9)

k8s-ci-robot · 2024-11-23T15:51:53Z

Welcome @tsprasannaa!

It looks like this is your first PR to kubernetes-sigs/kernel-module-management 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kernel-module-management has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-11-23T15:51:54Z

Hi @tsprasannaa. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

netlify · 2024-11-23T15:52:04Z

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Name	Link
🔨 Latest commit	`a3966c9`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kmm/deploys/67470894d288480008c1d65d
😎 Deploy Preview	https://deploy-preview-940--kubernetes-sigs-kmm.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

ybettan · 2024-11-25T09:42:08Z

/ok-to-test

ybettan · 2024-11-25T09:42:42Z

/cc @yevgeny-shnaidman

codecov-commenter · 2024-11-25T09:44:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.72%. Comparing base (fa23a9b) to head (a3966c9).
Report is 151 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #940      +/-   ##
==========================================
- Coverage   79.09%   72.72%   -6.38%     
==========================================
  Files          51       65      +14     
  Lines        5109     5770     +661     
==========================================
+ Hits         4041     4196     +155     
- Misses        882     1395     +513     
+ Partials      186      179       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ybettan · 2024-11-25T10:00:29Z

@tsprasannaa

Thank you for this great PR. Looks like you have thought this one through. I don't have any special comments about it.
(You do have some CI errors though)
/approve

@yevgeny-shnaidman can LGTM when he is happy with it as well.

k8s-ci-robot · 2024-11-25T10:00:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tsprasannaa, ybettan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ybettan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yevgeny-shnaidman · 2024-11-25T12:18:54Z

internal/build/pod/maker.go

@@ -134,6 +134,7 @@ func (m *maker) podSpec(mld *api.ModuleLoaderData, containerImage string, pushIm
 		RestartPolicy: v1.RestartPolicyNever,
 		Volumes:       volumes(mld.ImageRepoSecret, buildConfig),
 		NodeSelector:  selector,
+		Tolerations:   mld.Tolerations,


Do we need toleration for builder pod? In principal we should not care on what node the builder pod is running. @ybettan wdyt?

I was thinking about it, but I think we do need it.

For example:

If it needs to be built on a specific arch and we set the selector of the build pod to that node (which is currently tainted)

In the extreme case in which all nodes are tainted simultaneously (there is no reason for us to block this option).

tainting all nodes simultaneously does not make sense, it will cause a lot of other issues within the cluster. I agree regarding the different arch scenario

Thanks @yevgeny-shnaidman @ybettan . As discussed in this thread, as the tolerance addition is controlled by other controllers/users, the ideal scenario would be adding taints to nodes in an orderly fashion to get the upgrades done.

yevgeny-shnaidman · 2024-11-25T23:26:59Z

internal/node/node.go

@@ -30,7 +30,14 @@ func NewNode(client client.Client) Node {
 	}
 }

-func (n *node) IsNodeSchedulable(node *v1.Node) bool {
+func (n *node) IsNodeSchedulable(node *v1.Node, tolerations []v1.Toleration) bool {
+	for _, toleration := range tolerations {


If the node has 2 taints, but we only added one toleration to the KMM Module, then the node will be deemed schedulable, although it should be so, no?

I agree, we are only satisfied (in the current PR) with one fitting taint<-->toleration while what we need is to make sure that all the taints are tolerated.

@yevgeny-shnaidman @ybettan Thanks for this comment. Agreed. I will refactor the code to check pod tolerance for all the taints on that node.

@yevgeny-shnaidman I have updated the matching criteria. Please review. Thanks

yevgeny-shnaidman · 2024-11-26T14:19:26Z

internal/controllers/nmc_reconciler.go

@@ -155,6 +151,14 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r
 		delete(statusMap, moduleNameKey)
 	}

+	// removing label of loaded kmods
+	if !r.nodeAPI.IsNodeSchedulable(&node, nil) {


This is a problematic conditions: it means that we ignore the tolerations and the cordoned node will be always unschedulable, and that means that the code never reaches line 165, which will handle unloading of the kernel module as needed during upgrade

During module upgrade, in the first Reconcile, ProcessModuleSpec unloads the module (kernel version being same). Subsequent Reconcile, loads the new module. So the driver upgrade goes through.

Would it help if I move RemoveNodeReadyLabels call to execute just before GarbageCollectInUseLabels?

That depends on how upgrade is managed. We support 2 flows:

the label on the node is changed to a new version, and in that case the unloading will be done in the ProcessModuleSpec

the label on the node is removed, and will be later changed (this will allows user to execute whatever maintenance on the node that is needed). In that case the spec will be removed from NMC, and this scenario is handled in the ProcessUnconfiguredModuleStatus

Thanks @yevgeny-shnaidman. In that case, let me move the API just before GarbageCollectInUseLabels(). That should help both the scenarios. Let me know if this works

@yevgeny-shnaidman Pls review this change as well

ybettan · 2024-11-27T10:09:55Z

internal/node/node.go

+TAINTLOOP:
 	for _, taint := range node.Spec.Taints {
+		for _, toleration := range tolerations {
+			if taint.Key == toleration.Key && taint.Value == toleration.Value && taint.Effect == toleration.Effect {
+				continue TAINTLOOP
+			}
+		}
 		if taint.Effect == v1.TaintEffectNoSchedule {
 			return false
 		}
 	}
 	return true


Suggested change

TAINTLOOP:

for _, taint := range node.Spec.Taints {

for _, toleration := range tolerations {

if taint.Key == toleration.Key && taint.Value == toleration.Value && taint.Effect == toleration.Effect {

continue TAINTLOOP

}

}

if taint.Effect == v1.TaintEffectNoSchedule {

return false

}

}

return true

var toleratedTaints int

for _, taint := range node.Spec.Taints {

for _, toleration := range tolerations {

if toleration.ToleratesTaint(taint) {

// taint tolerated, move to the next taint

toleratedTaints++

break

}

}

}

return toleratedTaints == len(node.Spec.Taints)

WDYT?

@yevgeny-shnaidman this is changing the original behavior but I believe this is a more "complete" solution.

@ybettan your code looks good, but you are missing some corner cases:
if there is no toleration exists for the taint, but taints' effect is missing or it's effect is PreferNoSchedule, then the function should return true.
@tsprasannaa a couple of minor comments for your code: i would prefer not to use labels (TAINTLOOP) if not really needed. In this case it can be solved by a variable in the loop. Also, you should check if taint effect NoExecute, alongside with noSchedule

@ybettan @yevgeny-shnaidman Comments taken care. Please take a look. Thanks

There is a requirement to run selected pods on a cordoned node. One such example is device driver upgrade. The procedure involves the node to be cordoned. At the same time, there should be a way to run house keeping pods that carry out the driver upgrades. These pods should be schedulable. This PR adds support to carry the tolerances for the pods that carry out the house keeping operations. These user defined tolerances will match the taint that gets added to the nodes. ModuleSpec will be used to carry the tolerance to the pods which will be used during pod creation

yevgeny-shnaidman · 2024-12-02T09:15:40Z

internal/controllers/nmc_reconciler.go

@@ -169,6 +165,14 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r
 		}
 	}

+	// removing label of loaded kmods
+	if !r.nodeAPI.IsNodeSchedulable(&node, nil) {


We actually need to re-work the removal of the ready labels, since we cannot remove them for all the Modules, only for the ones without tolerations. But it is better if we do it in a different PR. I suggest we push this one , and then we will update the flow of the removing the ready labels. @TomerNewman will you have time to take care of this?

Absolutely, after merging it I will open a new PR

yevgeny-shnaidman · 2024-12-02T09:21:02Z

/lgtm

This change addresses kubernetes-sigs#940. Kmod labels are now deleted only if the kernel module does not have the appropriate tolerations for the taints on the node.

This change addresses #940. Kmod labels are now deleted only if the kernel module does not have the appropriate tolerations for the taints on the node.

k8s-ci-robot requested review from mresvanis and ybettan November 23, 2024 15:51

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 23, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 23, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 23, 2024

tsprasannaa force-pushed the main branch from be7e024 to 7dc0b56 Compare November 25, 2024 04:50

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 25, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 25, 2024

k8s-ci-robot requested a review from yevgeny-shnaidman November 25, 2024 09:42

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 25, 2024

tsprasannaa force-pushed the main branch from 7dc0b56 to a4bd602 Compare November 25, 2024 10:33

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 25, 2024

yevgeny-shnaidman reviewed Nov 25, 2024

View reviewed changes

tsprasannaa force-pushed the main branch from a4bd602 to 8cfa4c7 Compare November 26, 2024 13:09

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2024

tsprasannaa force-pushed the main branch from 8cfa4c7 to aafb256 Compare November 26, 2024 13:45

yevgeny-shnaidman reviewed Nov 26, 2024

View reviewed changes

tsprasannaa force-pushed the main branch 2 times, most recently from 56f8cf5 to 172e600 Compare November 26, 2024 15:24

tsprasannaa force-pushed the main branch from 172e600 to 9354f6a Compare November 27, 2024 09:46

ybettan reviewed Nov 27, 2024

View reviewed changes

tsprasannaa force-pushed the main branch from 9354f6a to a3966c9 Compare November 27, 2024 11:54

yevgeny-shnaidman reviewed Dec 2, 2024

View reviewed changes

k8s-ci-robot assigned yevgeny-shnaidman Dec 2, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 2, 2024

k8s-ci-robot merged commit 29b6433 into kubernetes-sigs:main Dec 2, 2024
22 checks passed

This was referenced Dec 3, 2024

Removing kmod labels without appropriate tolerations #944

Merged

Add support for user defined tolerances to modules rh-ecosystem-edge/kernel-module-management#1257

Merged

k8s-ci-robot pushed a commit that referenced this pull request Dec 9, 2024

Removing labels without tolerations

e63298a

This change addresses #940. Kmod labels are now deleted only if the kernel module does not have the appropriate tolerations for the taints on the node.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for user defined tolerances to modules #940

Add support for user defined tolerances to modules #940

tsprasannaa commented Nov 23, 2024

linux-foundation-easycla bot commented Nov 23, 2024 •

edited

Loading

k8s-ci-robot commented Nov 23, 2024

k8s-ci-robot commented Nov 23, 2024

netlify bot commented Nov 23, 2024 •

edited

Loading

ybettan commented Nov 25, 2024

ybettan commented Nov 25, 2024

codecov-commenter commented Nov 25, 2024 •

edited

Loading

ybettan commented Nov 25, 2024

k8s-ci-robot commented Nov 25, 2024

yevgeny-shnaidman Nov 25, 2024

ybettan Nov 26, 2024

yevgeny-shnaidman Nov 26, 2024

tsprasannaa Nov 26, 2024

yevgeny-shnaidman Nov 25, 2024

ybettan Nov 26, 2024

tsprasannaa Nov 26, 2024

tsprasannaa Nov 26, 2024

yevgeny-shnaidman Nov 26, 2024

tsprasannaa Nov 26, 2024

yevgeny-shnaidman Nov 26, 2024

tsprasannaa Nov 26, 2024 •

edited

Loading

tsprasannaa Nov 27, 2024

ybettan Nov 27, 2024 •

edited

Loading

yevgeny-shnaidman Nov 27, 2024

tsprasannaa Nov 27, 2024

yevgeny-shnaidman Dec 2, 2024

TomerNewman Dec 2, 2024

yevgeny-shnaidman commented Dec 2, 2024

Add support for user defined tolerances to modules #940

Add support for user defined tolerances to modules #940

Conversation

tsprasannaa commented Nov 23, 2024

linux-foundation-easycla bot commented Nov 23, 2024 • edited Loading

k8s-ci-robot commented Nov 23, 2024

k8s-ci-robot commented Nov 23, 2024

netlify bot commented Nov 23, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kmm ready!

ybettan commented Nov 25, 2024

ybettan commented Nov 25, 2024

codecov-commenter commented Nov 25, 2024 • edited Loading

Codecov Report

ybettan commented Nov 25, 2024

k8s-ci-robot commented Nov 25, 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

tsprasannaa Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ybettan Nov 27, 2024 • 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

yevgeny-shnaidman commented Dec 2, 2024

linux-foundation-easycla bot commented Nov 23, 2024 •

edited

Loading

netlify bot commented Nov 23, 2024 •

edited

Loading

codecov-commenter commented Nov 25, 2024 •

edited

Loading

tsprasannaa Nov 26, 2024 •

edited

Loading

ybettan Nov 27, 2024 •

edited

Loading