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

add decision group type #250

Closed

Conversation

haoqing0110
Copy link
Member

Today the filed DecisionGroup and ClustersPerDecisionGroup is on the same level, and the name "ClustersPerDecisionGroup" makes user easy to misunderstand that it's used to define the cluster numbers of "DecisionGroup".

To avoid misunderstanding, this PR wants to add a decision group type static and dynamic, to clarify that DecisionGroup and ClustersPerDecisionGroup are working separately.

Signed-off-by: haoqing0110 <[email protected]>
@openshift-ci openshift-ci bot requested review from mdelder and qiujian16 July 11, 2023 03:38
@haoqing0110
Copy link
Member Author

/assign @qiujian16 @serngawy

@serngawy
Copy link
Member

Not sure, where is a confusion come from ? The user can only set the ClustersPerDecisionGroup to divide the selected clusters to groups

Copy link
Member

@serngawy serngawy left a comment

Choose a reason for hiding this comment

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

can you elaborate more on the confusion.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 17, 2023

[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 ask for approval from qiujian16. For more information see the Kubernetes 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

@haoqing0110
Copy link
Member Author

For example as below, the "DecisionGroups" "prod-canary-west" has 120 clusters, and it actually won't be divide by "numberOfClustersPerDecisionGroup". "numberOfClustersPerDecisionGroup" only works on the rest of "DecisionGroups" "prod-canary-west".

  DecisionStrategy:
    numberOfClustersPerDecisionGroup: 50
    DecisionGroups:
    - label: prod-canary-west
      clusterSelector:
        matchExpressions:
          - key: prod-canary-west
            operator: Exist
….
status:
  numberOfSelectedClusters: 200
  decisionGroups:
  - decisionGroupIndex: 0
    decisionGroupLabel: prod-canary-west
    placementDecisions:
    - ztp-placement-decision-0 // cluster1 to cluster 100
    - ztp-placement-decision-1 // cluster101 to cluster120
    clusterCount: 120
  - decisionGroupIndex: 1
    placementDecisions:
    - ztp-placement-decision-2 // cluster121 to cluster 170
    clusterCount: 50 //
  - decisionGroupIndex: 2
    placementDecisions:
    - ztp-placement-decision-4  // cluster171 to cluster 200
    clusterCount: 30

@serngawy
Copy link
Member

For example as below, the "DecisionGroups" "prod-canary-west" has 120 clusters, and it actually won't be divide by "numberOfClustersPerDecisionGroup". "numberOfClustersPerDecisionGroup" only works on the rest of "DecisionGroups" "prod-canary-west".

  DecisionStrategy:
    numberOfClustersPerDecisionGroup: 50
    DecisionGroups:
    - label: prod-canary-west
      clusterSelector:
        matchExpressions:
          - key: prod-canary-west
            operator: Exist
….
status:
  numberOfSelectedClusters: 200
  decisionGroups:
  - decisionGroupIndex: 0
    decisionGroupLabel: prod-canary-west
    placementDecisions:
    - ztp-placement-decision-0 // cluster1 to cluster 100
    - ztp-placement-decision-1 // cluster101 to cluster120
    clusterCount: 120
  - decisionGroupIndex: 1
    placementDecisions:
    - ztp-placement-decision-2 // cluster121 to cluster 170
    clusterCount: 50 //
  - decisionGroupIndex: 2
    placementDecisions:
    - ztp-placement-decision-4  // cluster171 to cluster 200
    clusterCount: 30

Well, looks kind of strange case; How is the num of canary clusters is more than the rest of running clusters ? theoretically this could happen but practically I don't think this right.
In other hand; if that the case better to apply the numberOfClustersPerDecisionGroup to the decisionGroup so we could have many decision group with same GroupNameLabel and different GroupIndexLabel to satisfy the numberOfClustersPerDecisionGroup.

@haoqing0110
Copy link
Member Author

haoqing0110 commented Jul 18, 2023

This example is not a real-world one, it is to explain why I think the name is confusing. The defined "DecisionGroups" is actually not divided by "numberOfClustersPerDecisionGroup".

  1. If we don't expect the canary clusters to be more than the rest of the running clusters and don't expect the canary clusters to be divided, the current API name is confusing, that's why I create this PR.
  2. If we think once canary clusters is larger than the "numberOfClustersPerDecisionGroup" and need to be divided, I'm fine to keep the current API, just change the API explanation.

@serngawy
Copy link
Member

This example is not a real-world one, it is to explain why I think the name is confusing. The defined "DecisionGroups" is actually not divided by "numberOfClustersPerDecisionGroup".

  1. If we don't expect the canary clusters to be more than the rest of the running clusters and don't expect the canary clusters to be divided, the current API name is confusing, that's why I create this PR.
  2. If we think once canary clusters is larger than the "numberOfClustersPerDecisionGroup" and need to be divided, I'm fine to keep the current API, just change the API explanation.

Okay, that is fine then let's stay with option 2.

@haoqing0110
Copy link
Member Author

haoqing0110 commented Jul 18, 2023

option 2 sounds make sense, close this PR. #253

@haoqing0110 haoqing0110 deleted the br_placement branch July 18, 2023 09:54
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.

3 participants