Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unprefixed keys are not allowed #2074

Open
ademariag opened this issue Mar 7, 2025 · 7 comments · May be fixed by #2114
Open

unprefixed keys are not allowed #2074

ademariag opened this issue Mar 7, 2025 · 7 comments · May be fixed by #2114
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@ademariag
Copy link

ademariag commented Mar 7, 2025

What would you like to be added: The ability to add no namespace

Why is this needed:
I am using GKE, and in order to use the Nvidia GPU Operator correctly I need to disable the installation of the device plugin by adding this label

gke-no-default-nvidia-gpu-device-plugin=true

see https://cloud.google.com/kubernetes-engine/docs/how-to/gpu-operator#setup-node-pool

I am using

apiVersion: nfd.k8s-sigs.io/v1alpha1
kind: NodeFeatureRule
metadata:
  name: gke-gpu-labeler
spec:
  rules:
    - name: label-gke-gpu-nodes
      labels:
        gke-no-default-nvidia-gpu-device-plugin: "true"
      matchFeatures:
        - feature: pci.device
          matchExpressions:
            vendor:
              op: In
              value:
                - 10de

which unfortunately produces feature.node.kubernetes.io/gke-no-default-nvidia-gpu-device-plugin=true

Unfortunately NFD offers no ways that I can see to remove the prefix namespace.
I understand that it is against best practices, but we're all adults :)

@ademariag ademariag added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 7, 2025
@ademariag
Copy link
Author

ademariag commented Mar 7, 2025

nevermind! https://kubernetes-sigs.github.io/node-feature-discovery/v0.15/reference/master-configuration-reference.html#autodefaultns

even with this flag disabled, I get

E0307 01:06:34.233420       1 nfd-master.go:642] "ignoring label" err="unprefixed keys are not allowed" labelKey="gke-no-default-nvidia-gpu-device-plugin"

@ademariag ademariag reopened this Mar 7, 2025
@ademariag ademariag changed the title Allow for no namespace unprefixed keys are not allowed Mar 7, 2025
@fmuyassarov
Copy link
Member

/assign

@fmuyassarov
Copy link
Member

I believe this is a bug rather than a feature since I expected DisableAutoPrefix to prevent the namespace from being added, but it currently does not. However, we can fix this, for example, within the filterFeatureLabels(). @marquiz any thoughts, are we okay to allow skipping namespace addition?

@fmuyassarov
Copy link
Member

fmuyassarov commented Mar 13, 2025

I've tested it and even when the DisableAutoPrefix feature gate is enabled, custom labels don't get set.

 args:
    - "-feature-gates=DisableAutoPrefix=true"

@marquiz
Copy link
Contributor

marquiz commented Mar 13, 2025

I've tested it and even when the DisableAutoPrefix feature gate is enabled, custom labels don't get set.

Yes, this is by design, unprefixed labels are being blocked. I think we could allow those in case DisableAutoPrefix is enabled. Would kinda make sense.

@marquiz
Copy link
Contributor

marquiz commented Mar 13, 2025

I think we should also consider graduating DisableAutoPrefix to beta with on-by-default. Otherwise we'll never get any feedback for that...

@fmuyassarov
Copy link
Member

I've tested it and even when the DisableAutoPrefix feature gate is enabled, custom labels don't get set.

Yes, this is by design, unprefixed labels are being blocked. I think we could allow those in case DisableAutoPrefix is enabled. Would kinda make sense.

I will send a patch for review then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants