Skip to content

✨ Workload conditions #910

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

Jump to bottom
Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

✨ Workload conditions #910

wants to merge 25 commits into from
+1,802 −176

Conversation

bhperry
Copy link
Contributor

@bhperry bhperry commented Mar 26, 2025

Summary

Support for evaluating manifestwork condition rules

Related PR: open-cluster-management-io/api#362

Related issue(s)

open-cluster-management-io/enhancements#139
#842

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Mar 26, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

bhperry
bhperry commented Mar 26, 2025
View reviewed changes
@bhperry bhperry force-pushed the workload-conditions branch from 236902c to 0877a70 Compare March 27, 2025 19:02
@bhperry bhperry force-pushed the workload-conditions branch 2 times, most recently from 0a6137c to feed50a Compare March 27, 2025 19:07
@bhperry bhperry mentioned this pull request Mar 28, 2025
Closed
5 tasks
@bhperry bhperry force-pushed the workload-conditions branch from a82b159 to cc0e15a Compare April 1, 2025 20:39
@codecov Codecov
Copy link

codecov bot commented Apr 1, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 62.16216% with 84 lines in your changes missing coverage. Please review.

Project coverage is 64.26%. Comparing base (9844373) to head (f5fae30).

Files with missing lines Patch % Lines
pkg/work/spoke/conditions/reader.go 62.10% 60 Missing and 12 partials ⚠️
...lers/manifestcontroller/manifestwork_controller.go 0.00% 5 Missing ⚠️
pkg/work/helper/helpers.go 25.00% 2 Missing and 1 partial ⚠️
pkg/work/spoke/spokeagent.go 40.00% 2 Missing and 1 partial ⚠️
pkg/work/spoke/conditions/rules/rule.go 91.66% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #910      +/-   ##
==========================================
- Coverage   64.27%   64.26%   -0.02%     
==========================================
  Files         195      197       +2     
  Lines       19118    19335     +217     
==========================================
+ Hits        12288    12425     +137     
- Misses       5821     5887      +66     
- Partials     1009     1023      +14
Flag Coverage Δ
unit 64.26% <62.16%> (-0.02%) ⬇️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@bhperry bhperry force-pushed the workload-conditions branch from cc0e15a to 95902d6 Compare April 23, 2025 19:17
@bhperry bhperry force-pushed the workload-conditions branch 7 times, most recently from 0722403 to a9e9245 Compare April 30, 2025 14:44
@bhperry bhperry marked this pull request as ready for review April 30, 2025 14:45
@openshift-ci openshift-ci bot requested review from elgnay and zhiweiyin318 April 30, 2025 14:45
@bhperry bhperry requested a review from haoqing0110 April 30, 2025 14:46
@bhperry bhperry changed the title ✨ WIP: Workload conditions ✨ Workload conditions Apr 30, 2025
@bhperry bhperry force-pushed the workload-conditions branch from a9e9245 to 01af4ef Compare April 30, 2025 15:01
@bhperry bhperry mentioned this pull request May 21, 2025
Merged
bhperry added 24 commits June 2, 2025 13:14
@bhperry
Import OCM API changes for workload conditions
09b0e26 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Implement condition rule evaluator
9b83c37 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Evaluate manifest condition rules after apply
994c474 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
note to self
5bcbdb4 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Cleanup
1be8674 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Return config option if rules are set
5724226 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
update api
c1be6ca 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Always return an error to inform user about the state of their condit…
4cfd3c5 
…ion rule

Signed-off-by: Ben Perry <[email protected]>
@bhperry
Condition rule errors should not result in retrying apply
04f8995 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Test condition rule reconciliation
4642ecf 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Return condition status Unknown when an internal CEL error occurs
6d71fb2 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Update api
c451a57 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Switch to common CEL lib
fa952c4 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Update to simplified celExpressions format
e2474b4 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Formatting
41c8daa 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
tidy
b5ae50d 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Update ocm api
14a1196 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Update sdk-go
6dd2a23 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Switch to sdk-go ConditionLib
e038f3a 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Update API
12dc08c 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Switch to WellKnownConditions with required Condition field
c95a7ea 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Support CEL evaluation budget
0ac0928 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Update sdk-go
5e859c2 
Signed-off-by: Ben Perry <[email protected]>
@bhperry
Update API
50d9629 
Signed-off-by: Ben Perry <[email protected]>
@bhperry bhperry force-pushed the workload-conditions branch from aec6449 to 50d9629 Compare June 2, 2025 18:27
@bhperry
lint
f5fae30 
Signed-off-by: Ben Perry <[email protected]>
qiujian16
qiujian16 reviewed Jun 3, 2025
View reviewed changes
Copy link
Member

@qiujian16 qiujian16 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could you add some integration test also? The PR looks good in general.

pkg/work/helper/helpers.go
@@ -387,7 +390,7 @@ func FindManifestConfiguration(resourceMeta workapiv1.ManifestResourceMeta,
}
}

if len(rstOption.FeedbackRules) == 0 && rstOption.UpdateStrategy == nil {
if len(rstOption.FeedbackRules) == 0 && len(rstOption.ConditionRules) == 0 && rstOption.UpdateStrategy == nil {
Copy link
Member

Choose a reason for hiding this comment

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

you might want to add some ut.

pkg/work/spoke/conditions/reader.go
"object": obj.Object,
})
if err != nil {
return metav1.ConditionFalse, workapiv1.ConditionRuleExpressionError, remainingBudget, err
Copy link
Member

Choose a reason for hiding this comment

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

could you add some comment on why here is false and line156 return unknown?

pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go
@@ -77,6 +80,9 @@ func (m *manifestworkReconciler) reconcile(
// Add applied status condition
manifestCondition.Conditions = append(manifestCondition.Conditions, buildAppliedStatusCondition(result))

// Add result conditions
manifestCondition.Conditions = append(manifestCondition.Conditions, result.Conditions...)
Copy link
Member

Choose a reason for hiding this comment

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

I think this reader should be used in status controller, the thing is status controller will return faster and periodically, and you can use status update interval to control how frequent the resource will be read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm ok I'll take a look at that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiujian16 qiujian16 qiujian16 left review comments

@haoqing0110 haoqing0110 haoqing0110 left review comments

@elgnay elgnay Awaiting requested review from elgnay

@zhiweiyin318 zhiweiyin318 Awaiting requested review from zhiweiyin318

Assignees

@haoqing0110 haoqing0110

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bhperry @qiujian16 @haoqing0110 @openshift-merge-robot