Introduce nfd client for image compatibilty #1932

mfranczy · 2024-10-28T08:59:40Z

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 · 2024-10-28T08:59:50Z

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.

netlify · 2024-10-28T08:59:59Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`2208978`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/6763216cffb92900084a309d
😎 Deploy Preview	https://deploy-preview-1932--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 site configuration.

ArangoGutierrez · 2024-10-28T10:08:40Z

/ok-to-test

cmd/client-nfd/__debug_bin3215279485

cmd/client-nfd/main.go

api/image-compatibility/v1alpha1/spec.go

cmd/client-nfd/subcmd/compat/validate-node.go

pkg/apis/nfd/nodefeaturerule/rule.go

ArangoGutierrez · 2024-11-04T08:32:28Z

@marquiz PTAL

api/nfd/v1alpha1/types.go

api/image-compatibility/v1alpha1/spec.go

cmd/client-nfd/subcmd/root.go

marquiz · 2024-12-12T17:29:15Z

/milestone v0.17

MatchStatus provides details about successful expressions and their results, which are the matched host features. Additionally, a new flag controls rule processing behavior: it can either stop at the first error or continue processing all expressions and rules. Signed-off-by: Marcin Franczyk <[email protected]>

Signed-off-by: Marcin Franczyk <[email protected]>

marquiz

@mfranczy very good job on this one. A few minor comments/questions below.

Also, WDYT about announcing this as experimental (see #1932 (review))?

marquiz · 2024-12-18T10:40:23Z

api/nfd/v1alpha1/types.go

@@ -298,6 +300,13 @@ type MatchExpression struct {
 	Value MatchValue `json:"value,omitempty"`
 }

+func (m MatchExpression) String() string {


Hmm, this now sticks into my eye. We should at least move this into a separate file out of types.go, to keep the type definitions file clean.

Fixed. Moved to utils.go.

marquiz · 2024-12-18T12:46:55Z

cmd/nfd/subcmd/compat/validate-node.go

+		}
+
+		if readAccessToken && readPassword {
+			return fmt.Errorf("cannot use --read-access-token and --read-password at the same time")


nit: the error message does not seem to be in sync with the actual flags

Good catch! Fixed.

marquiz · 2024-12-18T12:50:02Z

cmd/nfd/subcmd/compat/validate-node.go

+	validateNodeCmd.Flags().StringVar(&username, "reg-username", "", "registry username")
+	validateNodeCmd.Flags().BoolVar(&readPassword, "reg-password-stdin", false, "read registry password from stdin")
+	validateNodeCmd.Flags().BoolVar(&readAccessToken, "reg-token-stdin", false, "read registry access token from stdin")


note/nit: I'd consider having --registry-username or just plain --username (and ditto for the other reg-* flags), not combining abbreviated and fully spelled out words in the name. WDYT?

I like the idea, I changed to the fully spelled out words.

Signed-off-by: Marcin Franczyk <[email protected]>

mfranczy · 2024-12-18T14:41:37Z

@mfranczy very good job on this one. A few minor comments/questions below.

Also, WDYT about announcing this as experimental (see #1932 (review))?

Thanks. I've added the experimental note in the nfd client and image compatibility docs.

mfranczy · 2024-12-18T14:50:22Z

/retest

marquiz · 2024-12-18T17:44:03Z

docs/reference/node-feature-client-reference.md

+
+The `--output-json` flag prints the output as a JSON object.
+
+#### --reg-username


Ach, now that the flag names were updated we need to update the documentation, too. Sorry about that 🙈

Of course. No worries, my bad. Fixed.

Signed-off-by: Marcin Franczyk <[email protected]>

ArangoGutierrez · 2024-12-18T19:40:18Z

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

marquiz

Wow @mfranczy, I'm ready to merge.

Let's hope something greater good comes out of this work 🚀

k8s-ci-robot · 2024-12-18T20:02:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, ChaoyiHuang, 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

marquiz · 2024-12-18T20:02:49Z

If @ArangoGutierrez is still around I'll let him do the last check 😄
/assign @ArangoGutierrez

mfranczy · 2024-12-19T07:53:34Z

/retest

mfranczy · 2024-12-19T09:36:33Z

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

marquiz · 2024-12-19T11:55:53Z

/retest

marquiz · 2024-12-19T16:12:00Z

/lgtm

k8s-ci-robot · 2024-12-19T16:12:07Z

LGTM label has been added.

Git tree hash: 40dfbe159aca1fdb5bdad98fe3d14e3e4ecaf376

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 requested review from kad and zvonkok October 28, 2024 08:59

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 added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 28, 2024

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 removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2024

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

ArangoGutierrez requested changes Oct 28, 2024

View reviewed changes

cmd/client-nfd/__debug_bin3215279485 Outdated Show resolved Hide resolved

cmd/client-nfd/main.go Outdated Show resolved Hide resolved

ChaoyiHuang reviewed Oct 29, 2024

View reviewed changes

api/image-compatibility/v1alpha1/spec.go Outdated Show resolved Hide resolved

mfranczy force-pushed the image-compatibility-nfr branch from 86ddc6a to e05aa41 Compare October 29, 2024 09:34

ChaoyiHuang reviewed Oct 30, 2024

View reviewed changes

cmd/client-nfd/subcmd/compat/validate-node.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2024

mfranczy commented Nov 1, 2024

View reviewed changes

pkg/apis/nfd/nodefeaturerule/rule.go Outdated Show resolved Hide resolved

mfranczy force-pushed the image-compatibility-nfr branch from 1097251 to 1d6d96a Compare November 2, 2024 10:21

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 force-pushed the image-compatibility-nfr branch 2 times, most recently from 7491e5a to 6227de7 Compare November 2, 2024 10:32

ArangoGutierrez requested a review from ChaoyiHuang November 4, 2024 08:31

ChaoyiHuang reviewed Nov 4, 2024

View reviewed changes

api/nfd/v1alpha1/types.go Outdated Show resolved Hide resolved

ArangoGutierrez requested changes Nov 4, 2024

View reviewed changes

api/image-compatibility/v1alpha1/spec.go Outdated Show resolved Hide resolved

cmd/client-nfd/subcmd/root.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2024

k8s-ci-robot added this to the v0.17 milestone Dec 12, 2024

mfranczy force-pushed the image-compatibility-nfr branch 3 times, most recently from eccc960 to f605488 Compare December 16, 2024 16:00

marquiz mentioned this pull request Dec 17, 2024

Release v0.17 #1875

Closed

25 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2024

mfranczy added 3 commits December 18, 2024 10:48

Introduce nfd client tool with a subset of image compatibility commands

efc299e

Signed-off-by: Marcin Franczyk <[email protected]>

Add image compatibility documentation

5b57312

Signed-off-by: Marcin Franczyk <[email protected]>

mfranczy force-pushed the image-compatibility-nfr branch from f605488 to 5b57312 Compare December 18, 2024 09:49

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2024

marquiz reviewed Dec 18, 2024

View reviewed changes

Add experimental note and fix subcmds flags naming

0b7661b

Signed-off-by: Marcin Franczyk <[email protected]>

marquiz reviewed Dec 18, 2024

View reviewed changes

Update the documentation

2208978

Signed-off-by: Marcin Franczyk <[email protected]>

marquiz approved these changes Dec 18, 2024

View reviewed changes

k8s-ci-robot assigned ArangoGutierrez Dec 18, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 19, 2024

k8s-ci-robot merged commit 2fbd8a8 into kubernetes-sigs:master Dec 19, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce nfd client for image compatibilty #1932

Introduce nfd client for image compatibilty #1932

mfranczy commented Oct 28, 2024 •

edited

Loading

k8s-ci-robot commented Oct 28, 2024

netlify bot commented Oct 28, 2024 •

edited

Loading

ArangoGutierrez commented Oct 28, 2024

ArangoGutierrez commented Nov 4, 2024

marquiz commented Dec 12, 2024

marquiz left a comment

marquiz Dec 18, 2024

mfranczy Dec 18, 2024

marquiz Dec 18, 2024

mfranczy Dec 18, 2024

marquiz Dec 18, 2024

mfranczy Dec 18, 2024

mfranczy commented Dec 18, 2024

mfranczy commented Dec 18, 2024

marquiz Dec 18, 2024

mfranczy Dec 18, 2024

ArangoGutierrez commented Dec 18, 2024

marquiz left a comment

k8s-ci-robot commented Dec 18, 2024

marquiz commented Dec 18, 2024

mfranczy commented Dec 19, 2024

mfranczy commented Dec 19, 2024

marquiz commented Dec 19, 2024

marquiz commented Dec 19, 2024

k8s-ci-robot commented Dec 19, 2024


		The `--output-json` flag prints the output as a JSON object.

		#### --reg-username

Introduce nfd client for image compatibilty #1932

Introduce nfd client for image compatibilty #1932

Conversation

mfranczy commented Oct 28, 2024 • edited Loading

k8s-ci-robot commented Oct 28, 2024

netlify bot commented Oct 28, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

ArangoGutierrez commented Oct 28, 2024

ArangoGutierrez commented Nov 4, 2024

marquiz commented Dec 12, 2024

marquiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfranczy commented Dec 18, 2024

mfranczy commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArangoGutierrez commented Dec 18, 2024

marquiz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 18, 2024

marquiz commented Dec 18, 2024

mfranczy commented Dec 19, 2024

mfranczy commented Dec 19, 2024

marquiz commented Dec 19, 2024

marquiz commented Dec 19, 2024

k8s-ci-robot commented Dec 19, 2024

mfranczy commented Oct 28, 2024 •

edited

Loading

netlify bot commented Oct 28, 2024 •

edited

Loading