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

NFD image compatibility proposal #1845

Merged

Conversation

mfranczy
Copy link
Contributor

Proposal to describe image requirements by using NFD labels and OCI artifact.

@k8s-ci-robot k8s-ci-robot requested review from kad and marquiz August 19, 2024 12:11
@k8s-ci-robot 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 Aug 19, 2024
@k8s-ci-robot
Copy link
Contributor

Welcome @mfranczy!

It looks like this is your first PR to kubernetes-sigs/node-feature-discovery 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/node-feature-discovery has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 19, 2024
Copy link

netlify bot commented Aug 19, 2024

Deploy Preview for kubernetes-sigs-nfd ready!

Name Link
🔨 Latest commit f435c1b
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/670f7e1ff04172000812bbd2
😎 Deploy Preview https://deploy-preview-1845--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 site configuration.

@mfranczy
Copy link
Contributor Author

/cc @ArangoGutierrez

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.

I like where this is going, as with every KEP, let's iterate fast, and release often. Let's try to avoid a long running discussion on this PR

@marquiz

cc @vsoch

enhancements/1845-nfd-image-compatibility/README.md Outdated Show resolved Hide resolved
Copy link

@vsoch vsoch left a comment

Choose a reason for hiding this comment

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

I recognize all of this work! Thanks for opening this up @mfranczy - looks good to me. 👍


- `validate` - validate a NodeFeatureRule object (implemented in kubectl plugin).
- `test` - test a NodeFeatureRule object against a node (implemented in kubectl plugin).
- `dryrun` - DryRun a NodeFeatureRule object against a NodeFeature file (implemented in kubectl plugin).
Copy link

Choose a reason for hiding this comment

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

Can you better explain "dryrun" ? I'm used to seeing it as --dryrun to supplement a more clearly obvious action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am also used to see it as you. I kept it that way to be compatibile with the kubectl dryrun command.

@mfranczy
Copy link
Contributor Author

mfranczy commented Sep 6, 2024

I am in the process of building nfd client prototype. I reduced the scope with commands. ORAS SDK provides a lot of useful functions so there is no need to introduce much for artifact manipulation (thanks ORAS folks!).

@marquiz
Copy link
Contributor

marquiz commented Sep 18, 2024

Thanks @mfranczy for the proposal. Just thinking aloud below, not proposing anything or having any answers.

It looks like we need to rely on node labels to be able to integrate this with the kubernetes components (scheduler etc). If we want to do without changes to the kubernetes API or extend the kubernetes components with some plugins looking at NFD CRDs (or similar) or something exotic like that.

What we need to think is the node label management on NFD side. We have a bunch of built-in labels (that get populated by just deploying NFD) but a lot of stuff is only visible in NodeFeatures only. For automatizing the creation NodeFeatureRules (a future, step, yes) we need to understand what we get automatically, from the built-in-labels and what we need custom labels for. We could use namespacing e.g. prefix all image compatibility labels with oci.feature.node.kubernetes.io/ (or opencontainers.org/ or similar).

Regarding the expressions, would CEL (or parts of it) be useful there?
https://github.com/google/cel-spec

@mfranczy
Copy link
Contributor Author

@marquiz thanks for looking into it.

It looks like we need to rely on node labels to be able to integrate this with the kubernetes components (scheduler etc). If we want to do without changes to the kubernetes API or extend the kubernetes components with some plugins looking at NFD CRDs (or similar) or something exotic like that.

I think looking at NFD CRDs would be a good choice for start.

What we need to think is the node label management on NFD side. We have a bunch of built-in labels (that get populated by just deploying NFD) but a lot of stuff is only visible in NodeFeatures only. For automatizing the creation NodeFeatureRules (a future, step, yes) we need to understand what we get automatically, from the built-in-labels and what we need custom labels for. We could use namespacing e.g. prefix all image compatibility labels with oci.feature.node.kubernetes.io/ (or opencontainers.org/ or similar).

Right, a few missing pieces I already identified, like discovery of runtime configuration (/proc/sys) that could be a built-in source only visible to NodeFeatures. Basically for image compatibility there is a lot of things already covered by the project (like kernel modules, config, version, usb and pci devices etc.). Maybe we don't need to introduce a new namespacing for the image compatibility purpose. From my perspective we could extend the built-in functionality and keep the original NFD labels (or features - if the label is not populated to a node) to describe the compatibility. What do you think?

Regarding the expressions, would CEL (or parts of it) be useful there?
https://github.com/google/cel-spec

Looks very promising, I think we could use it. That's a good idea.

@ChaoyiHuang
Copy link
Contributor

Thanks @mfranczy for the proposal. Just thinking aloud below, not proposing anything or having any answers.

It looks like we need to rely on node labels to be able to integrate this with the kubernetes components (scheduler etc). If we want to do without changes to the kubernetes API or extend the kubernetes components with some plugins looking at NFD CRDs (or similar) or something exotic like that.

What we need to think is the node label management on NFD side. We have a bunch of built-in labels (that get populated by just deploying NFD) but a lot of stuff is only visible in NodeFeatures only.

A node don't know what images(not only one, usually many) will run on it until the image is scheduled to the node, and each image's compatibility may require different set of node features, it is difficult to set node feature rules to label node automatically.

Considering the danymic situation, extending scheduler with plugin to validate the image compatibility and node features seems to be more viable.

@vsoch
Copy link

vsoch commented Sep 25, 2024

Considering the danymic situation, extending scheduler with plugin to validate the image compatibility and node features seems to be more viable.

In that context agree - the scheduler would determine compatibility, but then the container runtime doing the pull would do so based on more fine-grained selection for optional performance. It would be scoped to what the scheduler scoped it to (architecture, GPU and devices, etc).

@mfranczy
Copy link
Contributor Author

Regarding the expressions, would CEL (or parts of it) be useful there?
https://github.com/google/cel-spec

Looks very promising, I think we could use it. That's a good idea.

@marquiz I will try to implement this in the prototype and will inform if there is anything that stops us to use CEL.

@mfranczy
Copy link
Contributor Author

mfranczy commented Oct 8, 2024

@marquiz there is nothing that prevents to add extra fields for CEL expressions in the spec.

However, @marquiz @ArangoGutierrez after looking deeply into NodeFeatureRules, I believe we could just use the rules object (or part of it) to describe image requirements in the artifact. It provides everything what we need to describe image requirements, it already allows to use expressions, it's possible to set a custom hierarchy, it operates directly on features and most important we don't need to create any complex translator from the new spec to NFR for the deployment in k8s cluster.

If you're ok with the idea then I would update the proposal to include NFR rules object into compatibility artifact.

@marquiz
Copy link
Contributor

marquiz commented Oct 9, 2024

If you're ok with the idea then I would update the proposal to include NFR rules object into compatibility artifact.

In principle, I like this idea. That just means that the Kubernetes needs to be made aware of this stuff which complicates matters in the short term. With this, I also agree with @ChaoyiHuang @vsoch that integrating this in the kubernetes control plane (with new functionality, say a scheduler plugin) is a more viable and maintainable solution for the longer term

@mfranczy mfranczy force-pushed the nfd-image-compatibility branch from a13303b to 6316d50 Compare October 15, 2024 10:45
@mfranczy
Copy link
Contributor Author

@marquiz @ArangoGutierrez I have updated the proposal based on our discussion and consider it complete from my side.

Signed-off-by: Marcin Franczyk <marcin0franczyk@gmail.com>
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.

@marquiz I think we can get this KEP in and start working on code right?

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 4, 2024
@marquiz
Copy link
Contributor

marquiz commented Dec 12, 2024

@mfranczy this would be good to get in, does it need to be updated wrt to the latest code PR`?

@mfranczy
Copy link
Contributor Author

It doesn't have to be updated. The proposed API is still the same and the proposal doesn't include any deep implementation suggestions. It can go as it is.

Copy link
Contributor

@marquiz marquiz 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 @mfranczy for writing the proposal. Let's get this in before the impl.
/ok-to-test
/lgtm

@k8s-ci-robot 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 Dec 12, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 12, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 2dd871deaebb2b00214a87ab11943b930311e73e

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [ArangoGutierrez,marquiz]

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 merged commit 6bc66ed into kubernetes-sigs:master Dec 12, 2024
6 checks passed
@marquiz marquiz mentioned this pull request Dec 12, 2024
24 tasks
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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