Skip to content

feat: add ability to export features for static node #2183

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vsoch
Copy link

@vsoch vsoch commented Jun 25, 2025

Problem: We want to statically export NFD features for HPC.
Solution: Add an nfd export features mode that allows for generation and export of static feature labels.

For the HPC use case, we want to be able to export static features, either to the screen (for inspection) or a file (for saving and use with a scheduler, or other compatibility checking algorithm). Adding support for this is fairly easy - we need to just add the subcommand. The usage is simple and intuitive:

# Export raw features to the terminal for inspection
nfd export features

# Save to a file path
nfd export features --path features.json

# or the same for labels
nfd export labels
nfd export labels --path labels.json

The flag is also useful for cases beyond HPC when a quick inspection is desired. There are ways this could be extended (requesting specific groups or filters) but I think it's better to start simple.

This is a follow up of #2170. If you'd like tests added, I did an attempt and it looks like the features aren't exposed in the test environment - we would need a mock. Please point me to an example for how to do that. Thanks!

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 25, 2025
@k8s-ci-robot 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 Jun 25, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @vsoch. 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.

Copy link

netlify bot commented Jun 25, 2025

Deploy Preview for kubernetes-sigs-nfd ready!

Name Link
🔨 Latest commit 0323fdf
🔍 Latest deploy log https://app.netlify.com/projects/kubernetes-sigs-nfd/deploys/688a535f5d47ab00085a103e
😎 Deploy Preview https://deploy-preview-2183--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile
Toggle QR Code...

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 k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 25, 2025
@mfranczy
Copy link
Contributor

That's a nice functionality, not only for non-k8s use cases, but also for taking a snapshot of the node's state from the perspective of enabled features.

@vsoch vsoch force-pushed the add-cli-export branch from fa2eb19 to 131ad16 Compare July 1, 2025 23:45

var ExportCmd = &cobra.Command{
Use: "export",
Short: "Export features commands",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Export features or labels commands

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! What do you think of just "Export commands" to be generic for the export group? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course much more friendly for extention :)

@vsoch vsoch force-pushed the add-cli-export branch from 131ad16 to 341bdb6 Compare July 2, 2025 02:32
Copy link
Contributor

@mfranczy mfranczy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vsoch vsoch force-pushed the add-cli-export branch 2 times, most recently from 5cf1950 to 4a3fa78 Compare July 14, 2025 04:30
@ChaoyiHuang
Copy link
Contributor

ChaoyiHuang commented Jul 18, 2025

/lgtm

@vsoch
Copy link
Author

vsoch commented Jul 23, 2025

Ping @adrianchiris @zvonkok @ArangoGutierrez @marquiz we've gone through several rounds of review and have LGTM from the compatibility working group. Can any of you review this?

@ArangoGutierrez
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jul 24, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 24, 2025
@ArangoGutierrez ArangoGutierrez self-assigned this Jul 24, 2025
)

var (
outputPath string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Var is global for the pkg export, why have it as an entry value for writeToFile?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is global (and private) because it's a shared flag, and I usually see those defined in this way. For the latter, I think it's better practice to not have package functions rely on global variables.

@vsoch vsoch force-pushed the add-cli-export branch 4 times, most recently from 43fd3b0 to 97d9001 Compare July 24, 2025 16:41
@vsoch vsoch requested a review from ArangoGutierrez July 24, 2025 16:45

### Features

**Feature export is in the experimental `v1alpha1` version.**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it OK to refer to this as v1alpha1 even if isn't an API but a command line option?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just say it is an experimental version. Would that be better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably would be better, not to give a wrong impression to the readers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do! I’m running to the bakery to be there when it opens but will change this when I get back. Thanks for the review!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

For the HPC use case, we want to be able to export
static features, raw or as labels, to the terminal
or a JSON path.

Signed-off-by: vsoch <[email protected]>
Copy link
Member

@fmuyassarov fmuyassarov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. LGTM

@fmuyassarov
Copy link
Member

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

Copy link
Contributor

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PING @marquiz for lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, mfranczy, vsoch

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants