✨ support wildcard in ManifestConfigs #703

zhiweiyin318 · 2024-11-14T08:22:32Z

Summary

Support wildcard in the name and namespace of ManifestConfigs.ResourceIdentifier for feedbackRule and updateStrategy.
if the a resource matches more than 1 options, only the first one works.

Related issue(s)

#705
Fixes #

zhiweiyin318 · 2024-11-14T08:32:48Z

/assign @qiujian16

codecov · 2024-11-14T08:33:28Z

Codecov Report

Attention: Patch coverage is 93.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 63.41%. Comparing base (369f3fb) to head (669d798).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/work/helper/helpers.go	92.85%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #703      +/-   ##
==========================================
+ Coverage   63.37%   63.41%   +0.04%     
==========================================
  Files         185      185              
  Lines       17771    17800      +29     
==========================================
+ Hits        11262    11288      +26     
- Misses       5578     5580       +2     
- Partials      931      932       +1

Flag	Coverage Δ
unit	`63.41% <93.33%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

qiujian16 · 2024-11-15T01:13:17Z

pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go

@@ -195,7 +195,7 @@ func (m *manifestworkReconciler) applyOneManifest(
 	requiredOwner := manageOwnerRef(ownedByTheWork, owner)

 	// find update strategy option.
-	option := helper.FindManifestConiguration(resMeta, workSpec.ManifestConfigs)
+	option := helper.FindManifestConfiguration(resMeta, workSpec.ManifestConfigs)


I would want updateStrategy also support wildcard match. I think we should have helper.FindManifestConfigurations(resMeta, workSpec.ManifestConfigs) returns a list. For updateStrategy, we pick the first or latest item in the list, while for feedback rules, we pick every item.

returned a list of options .

while for feedback rules, we pick every item.

@zhiweiyin318 want to confirm for the current implementation, do we only pick up the first one?

// only handle the first option if the resource matches more than one option // need avoid a resource from being matched to more options. option := options[0] if len(option.FeedbackRules) == 0 { return values, metav1.Condition{ Type: statusFeedbackConditionType, Reason: "NoStatusFeedbackSynced", Status: metav1.ConditionTrue, } }

fixed. for feedbackRule, will pick all matched options.

changed to only pick up the first one for feedbackRule and installStrategy.

qiujian16 · 2024-11-15T01:14:45Z

test/integration/work/statusfeedback_test.go

-						Namespace: commOptions.SpokeClusterName,
-						Name:      "deploy1",
+						Namespace: "*",
+						Name:      "*1",


we should have some more tests for wildcard case.

We should also have another test with multiple matched options.

qiujian16 · 2024-11-18T06:13:40Z

/approve

@elgnay or @zhujian7 please also take a look

zhiweiyin318 · 2024-11-19T01:51:35Z

added more integration tests in feedBackRule and upgradeStrategy :

a wildcard option matches multi resources 2. a resource is matched by multi options.
@elgnay @zhujian7 please take a look. cc @qiujian16

zhujian7 · 2024-11-19T06:59:20Z

pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go

+	// need avoid a resource from being matched to more options.
+	option := workapiv1.ManifestConfigOption{}
+	if len(options) > 0 {
+		option = options[0]


If we only handle the first option, can we break the iteration when the first option found at https://github.com/open-cluster-management-io/ocm/pull/703/files#diff-1eb19addc11cd8fecd6bf310d649d8799c6676d69366230d21916e497f547f77R377, and set the default update strategy there also.
this may make the statusfeedback controller simpler as well.

fixed. for feedbackRule, will pick all matched options.

I am not sure it works. Each feedback result have a unique name, and it should not be overlapped.

changed to only pick up the first one for feedbackRule and installStrategy.

zhujian7 · 2024-11-20T05:57:01Z

/lgtm
/hold for other reviewers

qiujian16

/approve
/lgtm

openshift-ci · 2024-11-20T09:06:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, zhiweiyin318

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~~ [qiujian16]

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

zhiweiyin318 · 2024-11-20T09:07:36Z

@zhujian7 @qiujian16 please take a look again.

zhujian7 · 2024-11-20T10:04:16Z

pkg/work/helper/helpers.go

+			resourceMeta.Resource == option.ResourceIdentifier.Resource &&
+			wildcardMatch(resourceMeta.Namespace, option.ResourceIdentifier.Namespace) &&
+			wildcardMatch(resourceMeta.Name, option.ResourceIdentifier.Name) {
+			return &option


If two options matched, the first configured the FeedbackRules but no UpdateStrategy, and the second configured the UpdateStrategy but no FeedbackRules, will returning the first option cause the FeedbackRules to be lost?

I am still considering if we should make the word-by-word matching take precedence over wildcard matching?

good catch.
The old logic still has this issue.
Let me split FindManifestConfiguration to 2 functions for feedbackRule and UpdateStrategy, and check if there is feedbackRule and UpdateStrategy.

If two options matched, the first configured the FeedbackRules but no UpdateStrategy, and the second configured the UpdateStrategy but no FeedbackRules, will returning the first option cause the FeedbackRules to be lost?

fixed by splitting the FindManifestConfiguration into 2 functions.

I am still considering if we should make the word-by-word matching take precedence over wildcard matching?

I think the users only use the first option to filter resources in the most cases, so word-by-word matching is the same level as wildcard matching.
@qiujian16 WDYT?

could we just build a new option finding the first available rule and the first updateStrategy in the FindManifestConfiguration?

done. changed to one FindManifestConfiguration and return an option with the first matched updateStrategy and feedbackRule.

Signed-off-by: Zhiwei Yin <[email protected]>

qiujian16

/lgtm

zhiweiyin318 · 2024-11-21T06:54:18Z

/unhold

openshift-ci bot requested review from elgnay and zhujian7 November 14, 2024 08:22

zhiweiyin318 force-pushed the work-agent-feedbackrule branch 2 times, most recently from 168baec to 56b15f7 Compare November 14, 2024 08:30

openshift-ci bot assigned qiujian16 Nov 14, 2024

zhiweiyin318 force-pushed the work-agent-feedbackrule branch 2 times, most recently from 1384011 to a85a74f Compare November 14, 2024 09:04

qiujian16 reviewed Nov 15, 2024

View reviewed changes

zhiweiyin318 force-pushed the work-agent-feedbackrule branch from a85a74f to df0c872 Compare November 15, 2024 12:38

zhiweiyin318 changed the title ~~✨ support wildcard for feedbackrule~~ ✨ support wildcard in ManifestConfigs Nov 15, 2024

zhiweiyin318 force-pushed the work-agent-feedbackrule branch 3 times, most recently from 44ef2b0 to 1979c48 Compare November 16, 2024 03:07

openshift-ci bot added the approved label Nov 18, 2024

zhiweiyin318 force-pushed the work-agent-feedbackrule branch 3 times, most recently from 7f6b264 to 1749db2 Compare November 19, 2024 00:46

zhujian7 reviewed Nov 19, 2024

View reviewed changes

zhiweiyin318 force-pushed the work-agent-feedbackrule branch 3 times, most recently from f90bc3d to cc7a77a Compare November 19, 2024 13:19

openshift-ci bot added the do-not-merge/hold label Nov 20, 2024

openshift-ci bot assigned zhujian7 Nov 20, 2024

openshift-ci bot added the lgtm label Nov 20, 2024

zhiweiyin318 force-pushed the work-agent-feedbackrule branch from cc7a77a to 6d66ad5 Compare November 20, 2024 08:21

openshift-ci bot removed the lgtm label Nov 20, 2024

zhiweiyin318 force-pushed the work-agent-feedbackrule branch 4 times, most recently from 41f5d77 to 17a6d0c Compare November 20, 2024 09:05

qiujian16 reviewed Nov 20, 2024

View reviewed changes

openshift-ci bot added the lgtm label Nov 20, 2024

zhujian7 reviewed Nov 20, 2024

View reviewed changes

zhiweiyin318 force-pushed the work-agent-feedbackrule branch from 17a6d0c to 36a5d8b Compare November 20, 2024 13:25

openshift-ci bot removed the lgtm label Nov 20, 2024

zhiweiyin318 force-pushed the work-agent-feedbackrule branch 5 times, most recently from 296487b to a34b86c Compare November 21, 2024 03:10

support wildcard in manifestConfigs

669d798

Signed-off-by: Zhiwei Yin <[email protected]>

zhiweiyin318 force-pushed the work-agent-feedbackrule branch from a34b86c to 669d798 Compare November 21, 2024 03:31

qiujian16 reviewed Nov 21, 2024

View reviewed changes

openshift-ci bot added the lgtm label Nov 21, 2024

openshift-ci bot removed the do-not-merge/hold label Nov 21, 2024

openshift-merge-bot bot merged commit fa3a30b into open-cluster-management-io:main Nov 21, 2024
16 checks passed

zhiweiyin318 deleted the work-agent-feedbackrule branch November 21, 2024 07:02

✨ support wildcard in ManifestConfigs #703

✨ support wildcard in ManifestConfigs #703

Uh oh!

Conversation

zhiweiyin318 commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related issue(s)

Uh oh!

zhiweiyin318 commented Nov 14, 2024

Uh oh!

codecov bot commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhujian7 Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiujian16 commented Nov 18, 2024

Uh oh!

zhiweiyin318 commented Nov 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhujian7 commented Nov 20, 2024

Uh oh!

qiujian16 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 20, 2024

Uh oh!

zhiweiyin318 commented Nov 20, 2024

Uh oh!

zhujian7 Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhiweiyin318 Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiujian16 left a comment

Choose a reason for hiding this comment

Uh oh!

zhiweiyin318 commented Nov 21, 2024

Uh oh!

Uh oh!

Uh oh!

zhiweiyin318 commented Nov 14, 2024 •

edited

Loading

codecov bot commented Nov 14, 2024 •

edited

Loading

zhujian7 Nov 19, 2024 •

edited

Loading

zhujian7 Nov 20, 2024 •

edited

Loading

zhiweiyin318 Nov 20, 2024 •

edited

Loading