Install NRT by default with topology-updater

[fmuyassarov]

Since topology-updater relies on the presence of the NRT (NodeResourceTopology) CRDs to function correctly, it should be enabled by default. This prevents accidental pod failures due to a missing but required component. Instead of requiring users to explicitly enable it, we now require them to explicitly disable it if needed.

[k8s-ci-robot]

[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 arangogutierrez 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

[fmuyassarov]

/assign @marquiz

[netlify]

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	0064da6
🔍 Latest deploy log	https://app.netlify.com/projects/kubernetes-sigs-nfd/deploys/686fb26d9b24a7000843318a
😎 Deploy Preview	https://deploy-preview-2194--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.

[marquiz]

Hmm, I think this might be a bad idea because NFD does not "own" the CRD and it's used also elsewhere.

On a quick thought I see two possible main problem scenarios:

1. If NodeResourceTopology is already present (installed via some other means) NFD deployment would fail, AFAIU
2. CRDs will be deleted on NFD uninstall which could break some other users of the CRD

Thoughts?

/cc @PiotrProkop @adrianchiris

[fmuyassarov]

Hmm, I think this might be a bad idea because NFD does not "own" the CRD and it's used also elsewhere.

On a quick thought I see two possible main problem scenarios:

1. If NodeResourceTopology is already present (installed via some other means) NFD deployment would fail, AFAIU

I'm afraid it won't fail and rather skip the installation if CRD is already present on the cluster.

2. CRDs will be deleted on NFD uninstall which could break some other users of the CRD

That's true and actually weird. Because, those CRDs that are "owned" by NFD won't be uninstalled after helm uninstall while the ones installed conditionally (like NRT) will be deleted. I would normally expect the opposite but perhaps I'm missing something in terms of how Helm decides which resources it should delete.

I agree that it's problematic to delete a resource that might be used by other components in the cluster. One way to prevent the deletion of the NRT CRD is to place it under the crds/ directory, similar to how other resources are handled. This approach ensures that the CRD is not removed during a helm uninstall, and more importantly, it gets installed by default during a helm install, reducing the risk of missing the CRD installation.

Moving the CRD to crds/ means that NRT will be installed even if the topology updater isn't deployed. However, I don't believe that's an issue, since we already install other NFD resources that may not be used during the NFD lifecycle.

[marquiz]

I'm afraid it won't fail and rather skip the installation if CRD is already present on the cluster.

Just tested, it actually does fail:

$ helm -n nfd install nfd nfd/node-feature-discovery  --set topologyUpdater.enable=true --set topologyUpdater.createCRDs=true
Error: INSTALLATION FAILED: Unable to continue with install: CustomResourceDefinition "noderesourcetopologies.topology.node.k8s.io" in namespace "" exists and cannot be imported into the current release: invalid ownership metadata; label validation error: missing key "app.kubernetes.io/managed-by": must be set to "Helm"; annotation validation error: missing key "meta.helm.sh/release-name": must be set to "nfd"; annotation validation error: missing key "meta.helm.sh/release-namespace": must be set to "nfd"

[ffromani]

the problem here is nobody really owns this CRD indeed. My take on this is we should be careful and deliberate:

1. if the user wants nfd-topology-updater then we should also install the CRD but
2. there should be a way to opt out and don't install the CRD, but only nfd-topology-updater

this is more complex, but it seems to me it's the way which most conciles userfriendliness and good ecosystem citizenship. I'm open to discuss :)