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.

Sign up for GitHub

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

Bugfixes for image compatibility feature #2010

Merged
merged 4 commits into from
Jan 10, 2025
+52 −3
Merged

Bugfixes for image compatibility feature #2010

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
60b8a21
Allow for rule processing in case of a missing feature
mfranczy Jan 8, 2025
75ed142
Fix image compatibility processing panic in case of a nil pointer
mfranczy Jan 8, 2025
241c886
Sort the list of compatibility artifacts in desc order
mfranczy Jan 8, 2025
8db03fe
Add unit tests for invalid feature in the compatibility spec
mfranczy Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions pkg/apis/nfd/nodefeaturerule/rule.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,11 @@ func evaluateFeatureMatcher(m *nfdv1alpha1.FeatureMatcher, features *nfdv1alpha1
fA, okA := features.Attributes[featureName]
fI, okI := features.Instances[featureName]
if !okF && !okA && !okI {
klog.V(2).InfoS("feature not available", "featureName", featureName)
return false, nil, nil
if failFast {
klog.V(2).InfoS("feature not available", "featureName", featureName)
marquiz marked this conversation as resolved.
Show resolved Hide resolved
return false, nil, nil
}
continue
}

if term.MatchExpressions != nil {
Expand Down
14 changes: 14 additions & 0 deletions pkg/client-nfd/compat/artifact-client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (
"context"
"encoding/json"
"fmt"
"slices"
"time"

ocispec "github.com/opencontainers/image-spec/specs-go/v1"
oras "oras.land/oras-go/v2"
Expand All @@ -34,6 +36,10 @@ import (
compatv1alpha1 "sigs.k8s.io/node-feature-discovery/api/image-compatibility/v1alpha1"
)

const (
ArtifactCreationTimestampKey = "org.opencontainers.image.created"
)

// ArtifactClient interface contain set of functions to manipulate compatibility artfact.
type ArtifactClient interface {
// FetchCompatibilitySpec downloads the compatibility specifcation associated with the image.
Expand Down Expand Up @@ -90,6 +96,14 @@ func (c *Client) FetchCompatibilitySpec(ctx context.Context) (*compatv1alpha1.Sp
} else if len(descs) < 1 {
return nil, fmt.Errorf("compatibility artifact not found")
}

// Sort the artifacts in desc order.
// If the artifact does not have creation timestamp it will be moved to the top of the slice.
slices.SortFunc(descs, func(i, j ocispec.Descriptor) int {
it, _ := time.Parse(time.RFC3339, i.Annotations[ArtifactCreationTimestampKey])
jt, _ := time.Parse(time.RFC3339, j.Annotations[ArtifactCreationTimestampKey])
return it.Compare(jt)
})
artifactDesc := descs[len(descs)-1]

_, content, err := oras.FetchBytes(ctx, repo.Manifests(), artifactDesc.Digest.String(), oras.DefaultFetchBytesOptions)
Expand Down
11 changes: 9 additions & 2 deletions pkg/client-nfd/compat/node-validator/node-validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ func (nv *nodeValidator) Execute(ctx context.Context) ([]*CompatibilityStatus, e
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

TODO notes sounds good

var matchedFeatureTerms nfdv1alpha1.FeatureMatcher
out := ProcessedRuleStatus{Name: rule.Name, IsMatch: matchStatus.IsMatch}

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

@ChaoyiHuang ChaoyiHuang Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mfranczy mfranczy Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@marquiz marquiz Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Expand Down Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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.).

matchedFeatureTerms = matchStatus.MatchFeatureStatus.MatchedFeaturesTerms
}
out.MatchedExpressions = evaluateFeatureMatcher(matchFeatures, matchedFeatureTerms)
}

for i, matchAnyElem := range rule.MatchAny {
matchedExpressions := evaluateFeatureMatcher(matchAnyElem.MatchFeatures, matchStatus.MatchAny[i].MatchedFeaturesTerms)
if matchStatus.MatchAny[i].MatchedFeaturesTerms != nil {
matchedFeatureTerms = matchStatus.MatchAny[i].MatchedFeaturesTerms
}
matchedExpressions := evaluateFeatureMatcher(matchAnyElem.MatchFeatures, matchedFeatureTerms)
out.MatchedAny = append(out.MatchedAny, MatchAnyElem{MatchedExpressions: matchedExpressions})
}

Expand Down