Removing labels when node NotReady #933

TomerNewman · 2024-11-17T15:02:03Z

Until now, nodes retained their kmod labels even when they became NotReady for any reason (e.g., a reboot).
Today, we are removing these labels when a node is NotReady to address a potential race condition.

Besides updating the unit tests, I also tested this manually with simple-kmod, and it works as expected (label was deleted when Node became NotReady).

Note

I am also checking whether to remove the label using IsDeprecatedKernelModuleReadyNodeLabel. If this is not relevant, please let me know, and I will update it accordingly.

/cc @yevgeny-shnaidman @ybettan

k8s-ci-robot · 2024-11-17T15:02:13Z

Hi @TomerNewman. 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-17T15:02:20Z

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Name	Link
🔨 Latest commit	`a2a4f0b`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kmm/deploys/67445780397e5e000880667c
😎 Deploy Preview	https://deploy-preview-933--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.

yevgeny-shnaidman · 2024-11-18T08:46:22Z

/ok-to-test

ybettan · 2024-11-18T15:33:52Z

internal/controllers/nmc_reconciler.go

+		modifiedNode := node.DeepCopy()
+		patchFrom := client.MergeFrom(modifiedNode)
+		for label := range modifiedNode.GetLabels() {
+			if ok, _, _ := utils.IsKernelModuleReadyNodeLabel(label); ok ||
+				utils.IsDeprecatedKernelModuleReadyNodeLabel(label) {
+				delete(node.ObjectMeta.Labels, label)
+			}
+		}
+		if err := r.client.Patch(ctx, &node, patchFrom); err != nil {
+			return ctrl.Result{}, fmt.Errorf("could not patch node %s: %v", node.Name, err)


How about we use r.nodeAPI.UpdateLabels instead?

How about we use r.nodeAPI.UpdateLabels instead?

Good idea

yevgeny-shnaidman · 2024-11-19T08:20:57Z

internal/controllers/nmc_reconciler.go

 	if !r.nodeAPI.IsNodeSchedulable(&node) {
+		var labelsToRemove []string


Tomer, wdyt about moving this whole if section to a nodeAPI? This code does not require any data from nmc, it just looks at the node.

ybettan · 2024-11-19T14:26:19Z

internal/node/node.go

 	NodeBecomeReadyAfter(node *v1.Node, checkTime metav1.Time) bool
+	RemoveAllKmodLabels(ctx context.Context, node *v1.Node) error


Suggested change

RemoveAllKmodLabels(ctx context.Context, node *v1.Node) error

RemoveKmodReadyLabels(ctx context.Context, node *v1.Node) error

i would suggest renaming it "RemoveNodeReadyLabels", since it is not related to a specific kmod, but to any kmod on the node

i would suggest renaming it "RemoveNodeReadyLabels", since it is not related to a specific kmod, but to any kmod on the node

RemoveNodeReadyLabels is not to general? I think for example a node can have a label for his Ready status.

ok, then RemoveNodeKmodReadyLabels. @ybettan wdyt?

I would go with RemoveNodeReadyLabels.

There is no "ready" label on the node by default:

$ kubectl get node/minikube -o yaml | yq '.metadata.labels' beta.kubernetes.io/arch: amd64 beta.kubernetes.io/os: linux kubernetes.io/arch: amd64 kubernetes.io/hostname: minikube kubernetes.io/os: linux minikube.k8s.io/commit: 5883c09216182566a63dff4c326a6fc9ed2982ff minikube.k8s.io/name: minikube minikube.k8s.io/primary: "true" minikube.k8s.io/updated_at: 2024_11_25T11_06_54_0700 minikube.k8s.io/version: v1.33.1 node-role.kubernetes.io/control-plane: "" node.kubernetes.io/exclude-from-external-load-balancers: ""

The "ready" on the node is a condition and not a label:

$ kubectl get node/minikube -o yaml | yq '.status.conditions[] | select (.type == "Ready")' lastHeartbeatTime: "2024-11-25T09:07:08Z" lastTransitionTime: "2024-11-25T09:07:08Z" message: kubelet is posting ready status reason: KubeletReady status: "True" type: Ready

You are right, I changed it to RemoveNodeReadyLabels then.

codecov-commenter · 2024-11-25T09:21:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.65%. Comparing base (fa23a9b) to head (a2a4f0b).
Report is 147 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #933      +/-   ##
==========================================
- Coverage   79.09%   72.65%   -6.45%     
==========================================
  Files          51       65      +14     
  Lines        5109     5756     +647     
==========================================
+ Hits         4041     4182     +141     
- Misses        882     1395     +513     
+ Partials      186      179       -7

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

TomerNewman · 2024-11-25T10:07:24Z

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes missing coverage. Please review.

Project coverage is 72.60%. Comparing base (fa23a9b) to head (3098b4b).
Report is 146 commits behind head on main.

Files with missing lines Patch % Lines
internal/controllers/nmc_reconciler.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files

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

🚨 Try these New Features:
* [Flaky Tests Detection](https://docs.codecov.com/docs/test-result-ingestion-beta) - Detect and resolve failed and flaky tests

Noticed, added another unit-test to catch the failing scenario.

Until now nodes kept their kmod labels when became not ready. Today we are deleting them when they are not ready due to potential race condition.

ybettan · 2024-11-25T10:58:10Z

/approve
/assign @yevgeny-shnaidman

k8s-ci-robot · 2024-11-25T10:58:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: TomerNewman, 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-25T11:39:15Z

/lgtm

k8s-ci-robot requested review from ybettan and yevgeny-shnaidman November 17, 2024 15:02

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 17, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 17, 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 18, 2024

ybettan reviewed Nov 18, 2024

View reviewed changes

TomerNewman force-pushed the bugfix/deleting-label-when-node-notready branch from e668a21 to bb7d3dd Compare November 18, 2024 17:08

yevgeny-shnaidman reviewed Nov 19, 2024

View reviewed changes

TomerNewman force-pushed the bugfix/deleting-label-when-node-notready branch from bb7d3dd to 3098b4b Compare November 19, 2024 10:49

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 19, 2024

ybettan reviewed Nov 19, 2024

View reviewed changes

TomerNewman force-pushed the bugfix/deleting-label-when-node-notready branch from 3098b4b to bb35bc3 Compare November 25, 2024 10:05

Removing labels when node not ready

a2a4f0b

Until now nodes kept their kmod labels when became not ready. Today we are deleting them when they are not ready due to potential race condition.

TomerNewman force-pushed the bugfix/deleting-label-when-node-notready branch from bb35bc3 to a2a4f0b Compare November 25, 2024 10:54

k8s-ci-robot assigned yevgeny-shnaidman Nov 25, 2024

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

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

k8s-ci-robot merged commit 3596563 into kubernetes-sigs:main Nov 25, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing labels when node NotReady #933

Removing labels when node NotReady #933

TomerNewman commented Nov 17, 2024 •

edited

Loading

k8s-ci-robot commented Nov 17, 2024

netlify bot commented Nov 17, 2024 •

edited

Loading

yevgeny-shnaidman commented Nov 18, 2024

ybettan Nov 18, 2024

TomerNewman Nov 18, 2024

yevgeny-shnaidman Nov 19, 2024

ybettan Nov 19, 2024

TomerNewman Nov 19, 2024

ybettan Nov 19, 2024

yevgeny-shnaidman Nov 19, 2024

TomerNewman Nov 19, 2024 •

edited

Loading

yevgeny-shnaidman Nov 19, 2024

TomerNewman Nov 24, 2024

ybettan Nov 25, 2024

TomerNewman Nov 25, 2024

codecov-commenter commented Nov 25, 2024 •

edited

Loading

TomerNewman commented Nov 25, 2024

Codecov Report

ybettan commented Nov 25, 2024

k8s-ci-robot commented Nov 25, 2024

yevgeny-shnaidman commented Nov 25, 2024

		if !r.nodeAPI.IsNodeSchedulable(&node) {
		var labelsToRemove []string

		NodeBecomeReadyAfter(node *v1.Node, checkTime metav1.Time) bool
		RemoveAllKmodLabels(ctx context.Context, node *v1.Node) error

	RemoveAllKmodLabels(ctx context.Context, node *v1.Node) error
	RemoveKmodReadyLabels(ctx context.Context, node *v1.Node) error

Removing labels when node NotReady #933

Removing labels when node NotReady #933

Conversation

TomerNewman commented Nov 17, 2024 • edited Loading

Note

k8s-ci-robot commented Nov 17, 2024

netlify bot commented Nov 17, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kmm ready!

yevgeny-shnaidman commented Nov 18, 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

TomerNewman Nov 19, 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

codecov-commenter commented Nov 25, 2024 • edited Loading

Codecov Report

TomerNewman commented Nov 25, 2024

Codecov Report

ybettan commented Nov 25, 2024

k8s-ci-robot commented Nov 25, 2024

yevgeny-shnaidman commented Nov 25, 2024

TomerNewman commented Nov 17, 2024 •

edited

Loading

netlify bot commented Nov 17, 2024 •

edited

Loading

TomerNewman Nov 19, 2024 •

edited

Loading

codecov-commenter commented Nov 25, 2024 •

edited

Loading