🌱 placement support decision groups #200

haoqing0110 · 2023-06-28T07:08:06Z

Summary

Related issue(s)

#236

codecov · 2023-06-28T07:28:49Z

Codecov Report

Patch coverage: 84.61% and project coverage change: +0.23 🎉

Comparison is base (c8ec750) 59.91% compared to head (37c85a6) 60.14%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #200      +/-   ##
==========================================
+ Coverage   59.91%   60.14%   +0.23%     
==========================================
  Files         128      128              
  Lines       13241    13376     +135     
==========================================
+ Hits         7933     8045     +112     
- Misses       4568     4585      +17     
- Partials      740      746       +6

Flag	Coverage Δ
unit	`60.14% <84.61%> (+0.23%)`	⬆️

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

Impacted Files	Coverage Δ
...nt/controllers/scheduling/scheduling_controller.go	`65.84% <84.37%> (+6.51%)`	⬆️
pkg/placement/controllers/scheduling/schedule.go	`68.69% <100.00%> (-0.86%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

…nagement-io#200) Signed-off-by: Wei Liu <liuweixa@redhat.com>

haoqing0110 · 2023-07-03T03:50:58Z

/assign @qiujian16 @serngawy

qiujian16 · 2023-07-03T07:20:17Z

pkg/placement/controllers/scheduling/schedule.go

 // ScheduleResult is the result for a certain schedule.
 type scheduleResult struct {
 	feasibleClusters     []*clusterapiv1.ManagedCluster
-	scheduledDecisions   []clusterapiv1beta1.ClusterDecision


I actually think the scheduledDecisions here should be []*clusterapiv1.ManagedCluster,

And you should have a process like

schedule

generateDecision

bind

Change to []*clusterapiv1.ManagedCluster and update the placement decision generate and update flow.

qiujian16 · 2023-07-03T07:25:38Z

pkg/placement/controllers/scheduling/scheduling_controller.go

+		})
+
+		// generate placement decisions, group index starts from 0, decision name index starts from 1 for readability.
+		pds, groupStatus := c.generatePlacementDecisionsAndStatus(placement, decisionGroup, decisionGroupIndex, placementDecisionIndex+1)


this is weird. the two should at least start with the same index

change to start from 0.

qiujian16 · 2023-07-03T07:29:15Z

test/integration/placement/placement_test.go

+			assertNumberOfDecisions(placementName, namespace, 0, 1)
+		})
+
+		ginkgo.It("Should update the decision group once clusters belong to labelselector clusterset are added/deleted", func() {


I think we also have a static group field? should we also test it?

Updated the case.

pkg/placement/controllers/scheduling/scheduling_controller.go

qiujian16 · 2023-07-07T01:18:06Z

pkg/placement/controllers/scheduling/scheduling_controller.go

+	// The group length depends on ClustersPerDecisionGroup.
+	if len(clusterNames) > 0 {
+		matched := []clusterapiv1beta1.ClusterDecision{}
+		for _, cluster := range clusters {


why we do not directly iterate the clusterNames, but still iterate cluster?
can we do something like

for len(clusterNames) > 0 { clusterList := clusterNames.List() resultNumber := len(clusterList) if length < resultNumber { resultNumber = length } matched := []clusterapiv1beta1.ClusterDecision{} for i:=0; i<resultNumber; i++ { matched = append(matched, clusterapiv1beta1.ClusterDecision{ ClusterName: clusterList[i], }) delete(clusterNames, clusterList[i]) } decisionGroup := clusterDecisionGroup{ decisionGroupName: "", clusterDecisions: matched, } groups = append(groups, decisionGroup) }

why we do not directly iterate the clusterNames, but still iterate cluster?

This is to ensure the dynamic group clusters are always in the same order. The above suggestion looks good.

One question is, when the group length is 0, do we expect it to be the same as 100%, or expect no group (all the clusters not selected by the decision group will not be shown in the placement decision)? or we expect the length to be at least 1?

should be 100% I think

actual I do not think it could be 0, we should validate on API.

Update code, and will update the api in another PR.

qiujian16 · 2023-07-10T08:14:45Z

/approve

openshift-ci · 2023-07-10T08:14:51Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/placement/OWNERS~~ [haoqing0110,qiujian16]
~~test/OWNERS~~ [qiujian16]

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

Signed-off-by: haoqing0110 <qhao@redhat.com>

qiujian16 · 2023-07-10T13:41:35Z

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress label Jun 28, 2023

openshift-ci bot requested review from elgnay and qiujian16 June 28, 2023 07:08

haoqing0110 force-pushed the br_placement-strategy branch from 89a6de6 to 59e5c68 Compare June 28, 2023 07:21

haoqing0110 force-pushed the br_placement-strategy branch from 59e5c68 to d721dd5 Compare June 28, 2023 10:32

openshift-merge-robot added the needs-rebase label Jun 29, 2023

elgnay pushed a commit to elgnay/ocm that referenced this pull request Jun 29, 2023

increse controller workers for available and cleanup (open-cluster-ma…

10037a4

…nagement-io#200) Signed-off-by: Wei Liu <liuweixa@redhat.com>

haoqing0110 force-pushed the br_placement-strategy branch from d721dd5 to 05c277b Compare June 30, 2023 07:06

openshift-merge-robot removed the needs-rebase label Jun 30, 2023

haoqing0110 force-pushed the br_placement-strategy branch from 05c277b to 21892ee Compare June 30, 2023 14:54

haoqing0110 changed the title ~~[wip] placement support decision groups~~ 🌱 [wip] placement support decision groups Jun 30, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Jun 30, 2023

haoqing0110 changed the title ~~🌱 [wip] placement support decision groups~~ [wip] 🌱 placement support decision groups Jun 30, 2023

openshift-ci bot added the do-not-merge/work-in-progress label Jun 30, 2023

openshift-ci bot assigned qiujian16 and serngawy Jul 3, 2023

haoqing0110 changed the title ~~[wip] 🌱 placement support decision groups~~ 🌱 placement support decision groups Jul 3, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Jul 3, 2023

qiujian16 reviewed Jul 3, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label Jul 4, 2023

haoqing0110 force-pushed the br_placement-strategy branch 2 times, most recently from a754dca to d5af93d Compare July 5, 2023 03:25

openshift-merge-robot removed the needs-rebase label Jul 5, 2023

haoqing0110 force-pushed the br_placement-strategy branch from d5af93d to dd41ad5 Compare July 5, 2023 07:22

qiujian16 reviewed Jul 6, 2023

View reviewed changes

pkg/placement/controllers/scheduling/scheduling_controller.go Outdated Show resolved Hide resolved

pkg/placement/controllers/scheduling/scheduling_controller.go Outdated Show resolved Hide resolved

haoqing0110 force-pushed the br_placement-strategy branch 2 times, most recently from 9b14b19 to 37c85a6 Compare July 6, 2023 08:52

qiujian16 reviewed Jul 7, 2023

View reviewed changes

haoqing0110 force-pushed the br_placement-strategy branch from 37c85a6 to 24e0ec5 Compare July 10, 2023 06:19

openshift-ci bot added the approved label Jul 10, 2023

haoqing0110 added 6 commits July 10, 2023 08:46

placement support decision groups

c9158d5

Signed-off-by: haoqing0110 <qhao@redhat.com>

update decision generation flow

5c6b55f

Signed-off-by: haoqing0110 <qhao@redhat.com>

fix label update issue

91c58cc

Signed-off-by: haoqing0110 <qhao@redhat.com>

update decision group logic

36a2382

Signed-off-by: haoqing0110 <qhao@redhat.com>

update placement and decision with patcher

d41c3c2

Signed-off-by: haoqing0110 <qhao@redhat.com>

modify code based on review comments

38a2716

Signed-off-by: haoqing0110 <qhao@redhat.com>

haoqing0110 force-pushed the br_placement-strategy branch from 24e0ec5 to 38a2716 Compare July 10, 2023 08:47

openshift-ci bot added the lgtm label Jul 10, 2023

openshift-merge-robot merged commit 6649834 into open-cluster-management-io:main Jul 10, 2023

haoqing0110 deleted the br_placement-strategy branch July 10, 2023 13:48

haoqing0110 mentioned this pull request Jul 26, 2023

Placement Strategy enhancement #236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 placement support decision groups #200

🌱 placement support decision groups #200

haoqing0110 commented Jun 28, 2023 •

edited

Loading

codecov bot commented Jun 28, 2023 •

edited

Loading

haoqing0110 commented Jul 3, 2023

qiujian16 Jul 3, 2023

haoqing0110 Jul 5, 2023

qiujian16 Jul 3, 2023

haoqing0110 Jul 5, 2023

qiujian16 Jul 3, 2023

haoqing0110 Jul 5, 2023

qiujian16 Jul 7, 2023

haoqing0110 Jul 7, 2023 •

edited

Loading

qiujian16 Jul 7, 2023

qiujian16 Jul 7, 2023

haoqing0110 Jul 10, 2023

qiujian16 commented Jul 10, 2023

openshift-ci bot commented Jul 10, 2023

qiujian16 commented Jul 10, 2023

🌱 placement support decision groups #200

🌱 placement support decision groups #200

Conversation

haoqing0110 commented Jun 28, 2023 • edited Loading

Summary

Related issue(s)

codecov bot commented Jun 28, 2023 • edited Loading

Codecov Report

haoqing0110 commented Jul 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haoqing0110 Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiujian16 commented Jul 10, 2023

openshift-ci bot commented Jul 10, 2023

qiujian16 commented Jul 10, 2023

haoqing0110 commented Jun 28, 2023 •

edited

Loading

codecov bot commented Jun 28, 2023 •

edited

Loading

haoqing0110 Jul 7, 2023 •

edited

Loading