Bugfixes for image compatibility feature #2010

mfranczy · 2025-01-08T13:04:24Z

Bugfixes:

Always pull the latest compatibility artifact
Handle missing compatibility matches to avoid panic
Continue procesing the rule if the feature is missing

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

k8s-ci-robot · 2025-01-08T13:04:34Z

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 · 2025-01-08T13:04:42Z

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	`8db03fe`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/677f940414a5730008764506
😎 Deploy Preview	https://deploy-preview-2010--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.

mfranczy · 2025-01-08T13:04:53Z

/cc @marquiz @ArangoGutierrez @ChaoyiHuang

k8s-ci-robot · 2025-01-08T13:04:58Z

@mfranczy: GitHub didn't allow me to request PR reviews from the following users: ChaoyiHuang.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @marquiz @ArangoGutierrez @ChaoyiHuang

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.

pkg/apis/nfd/nodefeaturerule/rule.go

marquiz · 2025-01-08T13:11:54Z

/cherry-pick release-0.17

k8s-infra-cherrypick-robot · 2025-01-08T13:11:56Z

@marquiz: once the present PR merges, I will cherry-pick it on top of release-0.17 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.17

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.

marquiz · 2025-01-08T13:12:12Z

/ok-to-test

mfranczy · 2025-01-08T15:15:10Z

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

ChaoyiHuang · 2025-01-09T03:14:34Z

pkg/client-nfd/compat/node-validator/node-validator.go

@@ -97,6 +97,7 @@ func (nv *nodeValidator) Execute(ctx context.Context) ([]*CompatibilityStatus, e
 }

 func evaluateRuleStatus(rule *nfdv1alpha1.Rule, matchStatus *nodefeaturerule.MatchStatus) ProcessedRuleStatus {


could this function be a function of (nv *nodeValidator). and the function refactory will enhance the readbility

We could add the refactoring stuff as TODO notes and/or create a GitHub issue for tracking and concentrate solely on bugfixes in this PR. WDYT?

I agree. I plan to do refactoring but in a different PR, in this specific one I would prefer only to concentrate on the bugfixes I provided.

TODO notes sounds good

ChaoyiHuang · 2025-01-09T03:15:33Z

pkg/client-nfd/compat/node-validator/node-validator.go

@@ -97,6 +97,7 @@ func (nv *nodeValidator) Execute(ctx context.Context) ([]*CompatibilityStatus, e
 }

 func evaluateRuleStatus(rule *nfdv1alpha1.Rule, matchStatus *nodefeaturerule.MatchStatus) ProcessedRuleStatus {
+	var matchedFeatureTerms nfdv1alpha1.FeatureMatcher
 	out := ProcessedRuleStatus{Name: rule.Name, IsMatch: matchStatus.IsMatch}

 	evaluateFeatureMatcher := func(featureMatcher, matchedFeatureTerms nfdv1alpha1.FeatureMatcher) []MatchedExpression {


nested evaluateFeatureMatcher function make it become a little long for evaluateRuleStatus. could the evaluateFeatureMatcher become a function of (nv *nodeValidator)

and in the evaluateFeatureMatcher nested function, processedTerm := range matchedFeatureTerms where matchedFeatureTerms comes from the function parameter in expression condition, and processedTerm := range matchStatus.MatchedFeaturesTerms where the processedTerm comes from the parent function parameter.

the evaluateFeatureMatcher will process two condition, it can be divided into two function, which handle expression or name separately, or one function with same parameters, but processing according to evaluation on expression or name.

obviously that the processedTerm have multiple sources, so we can handle it two function layer:

one is to handle various sources, the other layer to handle proecssedTerm

@ChaoyiHuang I will provide the refactored code but in a different PR. I would prefer that it doesn't grow too much that we can merge the fixes fast.

Also, doing refactoring in a separate PR makes backporting to release branch cleaner and less risky

EDIT: meaning if possible we only backport the bugfixes but not the refactoring

ChaoyiHuang · 2025-01-09T03:32:21Z

pkg/client-nfd/compat/node-validator/node-validator.go

@@ -163,11 +164,17 @@ func evaluateRuleStatus(rule *nfdv1alpha1.Rule, matchStatus *nodefeaturerule.Mat
 	}

 	if matchFeatures := rule.MatchFeatures; matchFeatures != nil {
-		out.MatchedExpressions = evaluateFeatureMatcher(matchFeatures, matchStatus.MatchedFeaturesTerms)
+		if matchStatus.MatchFeatureStatus != nil {


non supported features in nfd could be added as some test cases. the tool should deal with unrecognized feature name or expression as well. in the future may check whether non-supported features are in image-compatibility artifact

I have added a unit test to check the behaviour. Currently, it's allowed to add any feature to the spec, if it doesn't exist the rule will fail. The user will get a log about it (in debug mode). In the future, we can think of additional behaviours (like stopping the execution if the feature doesn't exist etc.).

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

marquiz

Looks good to go from my pov.

@ChaoyiHuang WDYT?

k8s-ci-robot · 2025-01-09T09:28:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [marquiz]

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

ChaoyiHuang · 2025-01-10T00:44:13Z

Looks good to go from my pov.

@ChaoyiHuang WDYT?

LGTM

marquiz · 2025-01-10T07:37:12Z

/lgtm

k8s-ci-robot · 2025-01-10T07:37:19Z

LGTM label has been added.

Git tree hash: 1bca56f2209f55853fae1b45f9d8dff338bc0019

k8s-infra-cherrypick-robot · 2025-01-10T07:59:10Z

@marquiz: new pull request created: #2016

In response to this:

/cherry-pick release-0.17

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.

mfranczy added 3 commits January 8, 2025 13:58

Allow for rule processing in case of a missing feature

60b8a21

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

Fix image compatibility processing panic in case of a nil pointer

75ed142

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

Sort the list of compatibility artifacts in desc order

241c886

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 8, 2025

k8s-ci-robot requested review from jjacobelli and TessaIO January 8, 2025 13:04

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 8, 2025

k8s-ci-robot requested review from marquiz and ArangoGutierrez January 8, 2025 13:04

marquiz reviewed Jan 8, 2025

View reviewed changes

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

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 Jan 8, 2025

ChaoyiHuang reviewed Jan 9, 2025

View reviewed changes

Add unit tests for invalid feature in the compatibility spec

8db03fe

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

marquiz approved these changes Jan 9, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2025

marquiz mentioned this pull request Jan 9, 2025

Release v0.17.1 #2008

Open

16 tasks

k8s-ci-robot assigned marquiz Jan 10, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2025

k8s-ci-robot merged commit feea0e3 into kubernetes-sigs:master Jan 10, 2025
10 checks passed

k8s-infra-cherrypick-robot mentioned this pull request Jan 10, 2025

[release-0.17] Bugfixes for image compatibility feature #2016

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfixes for image compatibility feature #2010

Bugfixes for image compatibility feature #2010

mfranczy commented Jan 8, 2025

k8s-ci-robot commented Jan 8, 2025

netlify bot commented Jan 8, 2025 •

edited

Loading

mfranczy commented Jan 8, 2025

k8s-ci-robot commented Jan 8, 2025

marquiz commented Jan 8, 2025

k8s-infra-cherrypick-robot commented Jan 8, 2025

marquiz commented Jan 8, 2025

mfranczy commented Jan 8, 2025

ChaoyiHuang Jan 9, 2025

marquiz Jan 9, 2025

mfranczy Jan 9, 2025

ChaoyiHuang Jan 10, 2025

ChaoyiHuang Jan 9, 2025 •

edited

Loading

mfranczy Jan 9, 2025 •

edited

Loading

marquiz Jan 9, 2025 •

edited

Loading

ChaoyiHuang Jan 9, 2025

mfranczy Jan 9, 2025

marquiz left a comment

k8s-ci-robot commented Jan 9, 2025

ChaoyiHuang commented Jan 10, 2025

marquiz commented Jan 10, 2025

k8s-ci-robot commented Jan 10, 2025

k8s-infra-cherrypick-robot commented Jan 10, 2025

		@@ -97,6 +97,7 @@ func (nv nodeValidator) Execute(ctx context.Context) ([]CompatibilityStatus, e
		}

		func evaluateRuleStatus(rule nfdv1alpha1.Rule, matchStatus nodefeaturerule.MatchStatus) ProcessedRuleStatus {

Bugfixes for image compatibility feature #2010

Bugfixes for image compatibility feature #2010

Conversation

mfranczy commented Jan 8, 2025

k8s-ci-robot commented Jan 8, 2025

netlify bot commented Jan 8, 2025 • edited Loading

✅ Deploy Preview for kubernetes-sigs-nfd ready!

mfranczy commented Jan 8, 2025

k8s-ci-robot commented Jan 8, 2025

marquiz commented Jan 8, 2025

k8s-infra-cherrypick-robot commented Jan 8, 2025

marquiz commented Jan 8, 2025

mfranczy commented Jan 8, 2025

ChaoyiHuang Jan 9, 2025

Choose a reason for hiding this comment

marquiz Jan 9, 2025

Choose a reason for hiding this comment

mfranczy Jan 9, 2025

Choose a reason for hiding this comment

ChaoyiHuang Jan 10, 2025

Choose a reason for hiding this comment

ChaoyiHuang Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

mfranczy Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

marquiz Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

ChaoyiHuang Jan 9, 2025

Choose a reason for hiding this comment

mfranczy Jan 9, 2025

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 9, 2025

ChaoyiHuang commented Jan 10, 2025

marquiz commented Jan 10, 2025

k8s-ci-robot commented Jan 10, 2025

k8s-infra-cherrypick-robot commented Jan 10, 2025

netlify bot commented Jan 8, 2025 •

edited

Loading

ChaoyiHuang Jan 9, 2025 •

edited

Loading

mfranczy Jan 9, 2025 •

edited

Loading

marquiz Jan 9, 2025 •

edited

Loading