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.

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

select clusters with CEL expressions #137

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

haoqing0110
Copy link
Member

ref: #136

@openshift-ci openshift-ci bot requested review from deads2k and qiujian16 December 18, 2024 11:07
Copy link

openshift-ci bot commented Dec 18, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: haoqing0110
Once this PR has been reviewed and has the lgtm label, please assign deads2k 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

- Variable `managedCluster` will be added.
The input types may either be instances of `proto.Message` or `ref.Type`. Since OCM API does not implement `proto.Message`, we will add a function called ConvertManagedCluster() to convert ManagedCluster to a Map as an easier way to get started.

- Function `score` will be added.
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to enhance AddonPlacementScore to add original score value. And aeed a more appropriate name than score.

- requiredClusterSelector:
celSelector:
celExpressions:
- managedCluster.metadata.labels["version"].versionIsGreaterThan("1.30.0")
Copy link
Member

Choose a reason for hiding this comment

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

what kind of version does it support? only semVersion? What if the value is "v0.13.1"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Should support both Semantic Versioning and v as a prefix, will update the detail in the proposal.

- requiredClusterSelector:
celSelector:
celExpressions:
- managedCluster.score("default, "cpuAvailable") < 2
Copy link
Member

Choose a reason for hiding this comment

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

could we do like managedCluster.score.default.cpuAvailable? or we have to use a function here?

Copy link
Member Author

Choose a reason for hiding this comment

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

managedCluster does not have a score field so if we pass managedCluster.score.default.cpuAvailable to the cel for evaluation, it can not recognize it.

Maybe we can use something like managedCluster.score(default).cpuAvailable. function score("crname") will return a map of [scorename]scorevalue, and user can use standard cel expression to select it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants