export swap behavior via label #2192

fmuyassarov · 2025-07-07T13:33:52Z

This patch exports a label indicating the kubelet's configured swap behavior. By default, it reads from /var/lib/kubelet/config.yaml. If a custom kubelet config path is used, it can be set via the -kubelet-config-path flag.
If the swap behavior is not specified, NoSwap is assumed (matching kubelet's default).

Note: feature.node.kubernetes.io/memory-swap.behavior (or in general kubelet's memorySwap.swapBehavior) reflects whether k8s workloads are allowed to use swap. A node may have swap enabled, but if kubelet is set to NoSwap, pods cannot use it. As such, the behavior label will be exported only if node level swap is enabled.

      feature.node.kubernetes.io/memory-swap: "true"
      feature.node.kubernetes.io/memory-swap.behavior: LimitedSwap

Fixes: #2178

netlify · 2025-07-07T13:33:57Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`a75f334`
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-nfd/deploys/68c80f165bed6900080383c8
😎 Deploy Preview	https://deploy-preview-2192--kubernetes-sigs-nfd.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 project configuration.

k8s-ci-robot · 2025-07-07T13:33:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fmuyassarov
Once this PR has been reviewed and has the lgtm label, please assign marquiz for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

deployment/components/common/worker-mounts.yaml

fmuyassarov · 2025-07-08T19:32:12Z

/test pull-node-feature-discovery-build-image-cross-generic

marquiz

@fmuyassarov thank you for the patch and working on this. Let's get this finalized.

My main comment is that in topology-updater we already read the kubelet config (read from the kubelet configz endpoint by default), helpers are in pkg/utils/kubeconf/. Could we do the same on nfd-worker?

fmuyassarov · 2025-08-04T13:34:49Z

@fmuyassarov thank you for the patch and working on this. Let's get this finalized.

My main comment is that in topology-updater we already read the kubelet config (read from the kubelet configz endpoint by default), helpers are in pkg/utils/kubeconf/. Could we do the same on nfd-worker?

I will check that.

fmuyassarov · 2025-09-08T21:09:37Z

Hi @marquiz . I've reworked the patch to utilize the configz. The current state is missing the documentation update which I thought of adding once I get a green light that this the right approach. Please take a look.

Signed-off-by: Feruzjon Muyassarov <[email protected]>

marquiz

Thanks for the update @fmuyassarov. A few quick comments below.

The bigger question to me is how should we hierarchically place the new feature. The feature is related to swap, but specifically kubernetes/kubelet config setting. I could think e.g. a new system.kubelet or system.kubernetes feature. Then kubeletconfig config opts would not be command line flags for nfd-worker but config options for the source. And we would get robustness, nfd-worker would not refuse to start if it was not able to get the kubelet config

All in all, I'm not questioning the need for the feature. Just need to think what would be the most logical way to expose and implement it. Need to sleep over this.

marquiz · 2025-09-15T12:39:57Z

deployment/base/rbac/worker-role.yaml

@@ -1,5 +1,5 @@
 apiVersion: rbac.authorization.k8s.io/v1
-kind: Role
+kind: ClusterRole


With the principle of least privileges in mind, I think we should create a clusterrole for "nodes/proxy" (only) and keep the existing role as is. WDYT?

marquiz · 2025-09-15T12:43:04Z

pkg/nfd-worker/nfd-worker.go

 		// Add pod owner reference if it exists
 		if podName != "" {
-			if selfPod, err := w.k8sClient.CoreV1().Pods(w.kubernetesNamespace).Get(context.TODO(), podName, metav1.GetOptions{}); err != nil {
+			if selfPod, err := w.k8sClient.CoreV1().Pods(podNamespace).Get(context.TODO(), podName, metav1.GetOptions{}); err != nil {


What's wrong with w.kubernetesNamespace?

If I recall correctly from my test, if kubernetesNamespace is empty it will refuse because we are not using the clusterRole. But if I follow your comment earlier, keeping the Role as is and creating the ClusterRole only for the nodes/proxy, then we can keep w.kubernetesNamespace.

So we can expect this to be reverted?

So we can expect this to be reverted?

yes

marquiz · 2025-09-15T12:46:32Z

pkg/nfd-worker/nfd-worker.go

@@ -720,3 +749,38 @@ func (c *sourcesConfig) UnmarshalJSON(data []byte) error {

 	return nil
 }
+
+func getKubeletConfigFunc(uri, apiAuthTokenFile string) (func() (*kubeletconfigv1beta1.KubeletConfiguration, error), error) {


Would it be possible to share code with nfd-topology-updater?

Yup, I can move the code around to avoid duplication.

ArangoGutierrez · 2025-09-16T10:45:33Z

/retest

Copilot

Pull Request Overview

This PR exports a label indicating the kubelet's configured swap behavior for memory swap feature discovery. The change allows NFD to report whether Kubernetes workloads are allowed to use swap memory based on kubelet configuration.

Adds functionality to read kubelet configuration and extract swap behavior settings
Implements support for both file-based and API-based kubelet config access
Updates RBAC permissions to support accessing kubelet configuration endpoints

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
source/memory/memory.go	Adds swap behavior detection and labeling functionality
pkg/nfd-worker/nfd-worker.go	Implements kubelet config reading and swap behavior initialization
deployment/components/common/env.yaml	Adds required environment variables for node address and pod namespace
deployment/base/rbac/worker-rolebinding.yaml	Changes from Role to ClusterRole binding for broader permissions
deployment/base/rbac/worker-role.yaml	Updates permissions to include nodes/proxy access for kubelet API
cmd/nfd-worker/main.go	Adds command-line flags and default kubelet config URI construction

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-16T10:47:05Z

pkg/nfd-worker/nfd-worker.go

+	nfd = &nfdWorker{
+		kubeletConfigFunc: kubeletConfigFunc,
+	}
+
+	for _, o := range opts {
+		o.apply(nfd)
 	}



The nfd variable is being reassigned to a new nfdWorker struct, which discards all the previous initialization including the stop channel and other fields set earlier in the function. This will cause the worker to lose its previous configuration.

Suggested change

nfd = &nfdWorker{

kubeletConfigFunc: kubeletConfigFunc,

}

for _, o := range opts {

o.apply(nfd)

}

nfd.kubeletConfigFunc = kubeletConfigFunc

Copilot · 2025-09-16T10:47:06Z

pkg/nfd-worker/nfd-worker.go

+	// Append a label with app=nfd
+	labels["app"] = "nfd"



This hardcoded label addition appears unrelated to the swap behavior feature and should be removed or moved to a separate change. The comment and code don't align with the PR's purpose of exporting swap behavior.

Suggested change

// Append a label with app=nfd

labels["app"] = "nfd"

Copilot · 2025-09-16T10:47:06Z

source/memory/memory.go

+		if swapBehavior == "" {
+			swap["behavior"] = defaultSwapBehavior
+		} else {


[nitpick] This logic can be simplified using a ternary-like approach or by setting a default value for swapBehavior during initialization, reducing code duplication.

Suggested change

if swapBehavior == "" {

swap["behavior"] = defaultSwapBehavior

} else {

swap["behavior"] = defaultSwapBehavior

if swapBehavior != "" {

Copilot · 2025-09-16T10:47:07Z

cmd/nfd-worker/main.go

+
+func isIPv6(addr string) bool {
+	ip := net.ParseIP(addr)
+	return ip != nil && strings.Count(ip.String(), ":") >= 2


The IPv6 detection logic is unreliable. A valid IPv4 address could contain colons in unusual formats, and this doesn't properly distinguish IPv6. Use ip.To4() == nil after successful parsing to reliably detect IPv6 addresses.

Suggested change

return ip != nil && strings.Count(ip.String(), ":") >= 2

return ip != nil && ip.To4() == nil

this just a copy paste of the same helper function from topology updater. But I can fix it in both places.

ArangoGutierrez

2 comments

ArangoGutierrez · 2025-09-16T10:47:52Z

pkg/nfd-worker/nfd-worker.go

 		// Add pod owner reference if it exists
 		if podName != "" {
-			if selfPod, err := w.k8sClient.CoreV1().Pods(w.kubernetesNamespace).Get(context.TODO(), podName, metav1.GetOptions{}); err != nil {
+			if selfPod, err := w.k8sClient.CoreV1().Pods(podNamespace).Get(context.TODO(), podName, metav1.GetOptions{}); err != nil {


So we can expect this to be reverted?

ArangoGutierrez · 2025-09-16T10:47:59Z

deployment/base/rbac/worker-role.yaml

@@ -1,5 +1,5 @@
 apiVersion: rbac.authorization.k8s.io/v1
-kind: Role
+kind: ClusterRole


k8s-ci-robot · 2025-09-16T10:57:05Z

@fmuyassarov: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-node-feature-discovery-verify-master	`a75f334`	link	true	`/test pull-node-feature-discovery-verify-master`
pull-node-feature-discovery-e2e-test-master	`a75f334`	link	true	`/test pull-node-feature-discovery-e2e-test-master`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

marquiz · 2025-09-16T12:54:24Z

@fmuyassarov independent of where we logically put the new feature, I think the detection should be outside nfd-worker core package. If we keep it in memory.swap, then probably the feature name should somehow reflect that it's kubelet behavior.

fmuyassarov · 2025-09-16T13:37:11Z

@fmuyassarov independent of where we logically put the new feature, I think the detection should be outside nfd-worker core package. If we keep it in memory.swap, then probably the feature name should somehow reflect that it's kubelet behavior.

The reason for splitting the detection between memory and worker pkgs is that, memory pkg doesn't have access to the kubeconfig & kubelet. Technically speaking, there is no need to do anything within the memory pkg. But, the reason I used memory pkg is because we are detecting the memory related feature. Sure, it is via kubelet, but the main thing is not kubelet, but the memory. Since we already had a logic of detecting the swap memory within the memory pkg, I thought of extending it for swap memory type too.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 7, 2025

k8s-ci-robot requested review from adrianchiris and PiotrProkop July 7, 2025 13:33

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 7, 2025

fmuyassarov commented Jul 7, 2025

View reviewed changes

deployment/components/common/worker-mounts.yaml Outdated Show resolved Hide resolved

fmuyassarov force-pushed the devel/swapehavior branch from 2a0f109 to 12aecfe Compare July 8, 2025 08:07

marquiz reviewed Jul 24, 2025

View reviewed changes

fmuyassarov force-pushed the devel/swapehavior branch from 12aecfe to 2d60fa9 Compare September 8, 2025 20:52

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

fmuyassarov force-pushed the devel/swapehavior branch from 2d60fa9 to e993fd8 Compare September 8, 2025 21:05

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

fmuyassarov force-pushed the devel/swapehavior branch from e993fd8 to 91a79d5 Compare September 8, 2025 21:07

fmuyassarov force-pushed the devel/swapehavior branch 2 times, most recently from 8565ce9 to 30fc56f Compare September 15, 2025 12:31

Expose swap behavior via label

a75f334

Signed-off-by: Feruzjon Muyassarov <[email protected]>

fmuyassarov force-pushed the devel/swapehavior branch from 30fc56f to a75f334 Compare September 15, 2025 13:05

marquiz reviewed Sep 15, 2025

View reviewed changes

ArangoGutierrez requested a review from Copilot September 16, 2025 10:45

Copilot AI reviewed Sep 16, 2025

View reviewed changes

ArangoGutierrez reviewed Sep 16, 2025

View reviewed changes

	return ip != nil && strings.Count(ip.String(), ":") >= 2
	return ip != nil && ip.To4() == nil

export swap behavior via label #2192

Are you sure you want to change the base?

export swap behavior via label #2192

Conversation

fmuyassarov commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Uh oh!

k8s-ci-robot commented Jul 7, 2025

Uh oh!

Uh oh!

fmuyassarov commented Jul 8, 2025

Uh oh!

marquiz left a comment

Choose a reason for hiding this comment

Uh oh!

fmuyassarov commented Aug 4, 2025

Uh oh!

fmuyassarov commented Sep 8, 2025

Uh oh!

marquiz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArangoGutierrez commented Sep 16, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 16, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArangoGutierrez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 16, 2025

Uh oh!

marquiz commented Sep 16, 2025

Uh oh!

fmuyassarov commented Sep 16, 2025

Uh oh!

Uh oh!

fmuyassarov commented Jul 7, 2025 •

edited

Loading

netlify bot commented Jul 7, 2025 •

edited

Loading