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

Introduce nfd client for image compatibilty #1932

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mfranczy
Copy link

@mfranczy mfranczy commented Oct 28, 2024

The PR provides an initial implementation of #1845. Image compatibility is defined by Node Feature Rules, so a change was also necessary there. A run strategy pattern has been implemented to control rule execution, allowing either all rules to be executed or stopping at the first failure.

The work is still in progress but is ready for the first review round.
Remaining tasks:
- [x] write unit tests
- [x] write documentation and add appropriate code comments
- [x] refine the commands

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 28, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mfranczy
Once this PR has been reviewed and has the lgtm label, please assign arangogutierrez for approval. For more information see the Kubernetes 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:

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 28, 2024
@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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 28, 2024
Copy link

netlify bot commented Oct 28, 2024

Deploy Preview for kubernetes-sigs-nfd ready!

Name Link
🔨 Latest commit cf7b0ba
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/673c709936d6ec0008c5ee13
😎 Deploy Preview https://deploy-preview-1932--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 mfranczy force-pushed the image-compatibility-nfr branch 2 times, most recently from b269c77 to 86ddc6a Compare October 28, 2024 09:05
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2024
@ArangoGutierrez
Copy link
Contributor

/ok-to-test

@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 Oct 28, 2024
cmd/client-nfd/__debug_bin3215279485 Outdated Show resolved Hide resolved
cmd/client-nfd/main.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2024
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2024
@mfranczy mfranczy force-pushed the image-compatibility-nfr branch 2 times, most recently from 7491e5a to 6227de7 Compare November 2, 2024 10:32
@ArangoGutierrez
Copy link
Contributor

@marquiz PTAL

api/image-compatibility/v1alpha1/spec.go Outdated Show resolved Hide resolved
cmd/client-nfd/subcmd/root.go Outdated Show resolved Hide resolved
@mfranczy mfranczy force-pushed the image-compatibility-nfr branch 2 times, most recently from d91a178 to 125f36c Compare November 9, 2024 17:16
@mfranczy mfranczy changed the title [WIP] Introduce nfd client for image compatibilty Introduce nfd client for image compatibilty Nov 9, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2024
@mfranczy
Copy link
Author

mfranczy commented Nov 9, 2024

/retest

@ChaoyiHuang
Copy link
Contributor

ChaoyiHuang commented Nov 11, 2024

Additionally, I’m including a sample output from the nfd compat validate-node.. cmd image

I belive this first implementation is good enough to be included in v0.17.0 release.

if matched features can be returned and output correctly, then all other unmatched feature should be failure in feature matching.

so maybe no need to introduce runstrategy, just in the output, for the features not in the matched feature list(but in the compatibility yaml), tag it as failed.

then we may get same report as that with runstrategy

@mfranczy
Copy link
Author

mfranczy commented Nov 11, 2024

Additionally, I’m including a sample output from the nfd compat validate-node.. cmd image
I belive this first implementation is good enough to be included in v0.17.0 release.

if matched features can be returned and output correctly, then all other unmatched feature should be failure in feature matching.

The current rules implementation doesn’t easily match executed features (whether failed or succeeded) with expressions in the spec, as it was not designed for that. Without changes, validate-node would need overly complex algorithms. Starting a full refactor now risks delaying inclusion in v0.17, so it’s better to start small. Additionally, I would like to discuss the planned changes with @marquiz first.

so maybe no need to introduce runstrategy, just in the output, for the features not in the matched feature list(but in the compatibility yaml), tag it as failed.
then we may get same report as that with runstrategy

A run strategy implementation is required because, by default, rule processing stops at the first failure. validate-node must process all the rules.

@mfranczy
Copy link
Author

mfranczy commented Nov 14, 2024

The current rules implementation doesn’t easily match executed features (whether failed or succeeded) with expressions in the spec, as it was not designed for that. Without changes, validate-node would need overly complex algorithms. Starting a full refactor now risks delaying inclusion in v0.17, so it’s better to start small. Additionally, I would like to discuss the planned changes with @marquiz first.

After discussions with end users, it became clear that the minimal functionality wasn't sufficient, so I revisited the problem from a new perspective.

Previously, I attempted to handle expression execution status similar to MatchedFeatures, which introduced significant complexity and required extensive changes to the NFR code. I found a better approach by extending the expression struct with an isMatch field, allowing the expression status to update directly during processing.

@ArangoGutierrez @marquiz apologies for the multiple updates, but this is the final version. The PR is ready for review, and I consider it complete. Please review whenever you have a moment. This commit shows the changes to NFR code.

Example output from the validate-node command:
image

@mfranczy
Copy link
Author

/retest

@mfranczy mfranczy force-pushed the image-compatibility-nfr branch 2 times, most recently from 3346daf to 502e89e Compare November 14, 2024 17:35
@mfranczy
Copy link
Author

/retest

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2024
The strategy pattern allows for either executing all rules or
failing fast. For image compatibility, it’s necessary to always
execute all rules to identify which ones fail.

Signed-off-by: Marcin Franczyk <marcin0franczyk@gmail.com>
Signed-off-by: Marcin Franczyk <marcin0franczyk@gmail.com>
Signed-off-by: Marcin Franczyk <marcin0franczyk@gmail.com>
Signed-off-by: Marcin Franczyk <marcin0franczyk@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2024
@mfranczy
Copy link
Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants