🌱 addon consume rollout helpers #225

haoqing0110 · 2023-07-20T04:11:13Z

Summary

Addon consuming rollout helper functions of open-cluster-management-io/api#254. This PR includes the rollout type All, Progressive and PreogresivePerGroup, timeout is not included.

Related issue(s)

#114

haoqing0110 · 2023-07-20T04:12:11Z

cc @qiujian16

codecov · 2023-07-28T12:48:47Z

Codecov Report

Patch coverage: 79.50% and project coverage change: +0.05% 🎉

Comparison is base (bd4982f) 61.03% compared to head (2463035) 61.09%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #225      +/-   ##
==========================================
+ Coverage   61.03%   61.09%   +0.05%     
==========================================
  Files         128      129       +1     
  Lines       13719    13752      +33     
==========================================
+ Hits         8374     8402      +28     
  Misses       4579     4579              
- Partials      766      771       +5

Flag	Coverage Δ
unit	`61.09% <79.50%> (+0.05%)`	⬆️

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

Files Changed	Coverage Δ
pkg/work/helper/helpers.go	`68.98% <ø> (+1.64%)`	⬆️
...addon/controllers/addonconfiguration/controller.go	`16.12% <9.09%> (-16.63%)`	⬇️
pkg/addon/controllers/addonconfiguration/graph.go	`85.55% <82.70%> (-1.95%)`	⬇️
...donconfiguration/addon_configuration_reconciler.go	`87.17% <100.00%> (ø)`
...configuration/mgmt_addon_progressing_reconciler.go	`90.90% <100.00%> (+0.13%)`	⬆️
pkg/common/helpers/clusters.go	`100.00% <100.00%> (ø)`
...troller/manifestworkreplicaset_deploy_reconcile.go	`68.93% <100.00%> (ø)`

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

haoqing0110 · 2023-08-16T09:27:37Z

/hold

qiujian16 · 2023-09-12T06:59:58Z

pkg/addon/controllers/addonconfiguration/controller.go

@@ -121,6 +122,13 @@ func (c *addonConfigurationController) sync(ctx context.Context, syncCtx factory
 		return err
 	}

+	// snapshot the rollout result before calling reconcile()
+	// so that all the reconcilers are using the same rollout result
+	err = graph.getRolloutResult()


it this is snapshot, just call it snapshot.

rename to generateRolloutResult()

qiujian16 · 2023-09-12T07:01:59Z

pkg/addon/controllers/addonconfiguration/controller.go


+			// get placement
+			placement, err := c.getPlacement(installProgression.PlacementRef.Name, installProgression.PlacementRef.Namespace)


It seems to me this part should be done within adding placementNode

qiujian16 · 2023-09-12T07:03:16Z

pkg/addon/controllers/addonconfiguration/graph.go

@@ -341,3 +383,11 @@ func desiredConfigsEqual(a, b addonConfigMap) bool {

 	return true
 }
+
+func initClusters(pdTracker *clusterv1beta1.PlacementDecisionClustersTracker) (sets.Set[string], error) {


I am not quite sure why we need this func

The input pdTracker does not include any clusters, so want to call Get() to fill in the existingScheduledClusterGroups and return the existing clusters. Might need to enhance the PlacementDecisionClustersTracker functions rather than add initClusters() here.

remove this func

qiujian16 · 2023-09-12T07:03:48Z

pkg/addon/controllers/addonconfiguration/graph.go

 	placementRef := installProgression.PlacementRef
 	installConfigReference := installProgression.ConfigReferences

+	// init pdTracker clusters


enhance the comment here, it is not quite clear the intention

remove initClusters func and add comments // refresh and get existing decision clusters

qiujian16 · 2023-09-12T07:10:00Z

pkg/addon/controllers/addonconfiguration/graph.go

-	for _, addon := range n.children {
-		if desiredConfigsEqual(addon.desiredConfigs, n.desiredConfigs) && addon.mcaUpgradeStatus == upgraded {
-			count += 1
+func (n *installStrategyNode) getRolloutResult() error {


call it getXXX is confusing, since it does not return a result. Commonly such get func should return a result and not change the internal state. You might want to call if generateRolloutResult

rename to generateRolloutResult()

qiujian16 · 2023-09-19T01:52:01Z

/approve

would you also check the ut coverage?

haoqing0110 · 2023-09-19T02:57:11Z

@qiujian16 yes, will add more UT, and another 2 PRs need to merge before merging this one. open-cluster-management-io/api#260 open-cluster-management-io/addon-framework#208

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

qiujian16 · 2023-09-20T02:09:09Z

/approve
/lgtm

openshift-ci · 2023-09-20T02:09:16Z

[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:

~~OWNERS~~ [qiujian16]

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

haoqing0110 · 2023-09-20T02:12:41Z

/unhold

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

openshift-ci bot requested review from zhiweiyin318 and zhujian7 July 20, 2023 04:11

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

haoqing0110 force-pushed the addon-rollout branch from c2b4cda to b8ad457 Compare July 27, 2023 08:01

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

haoqing0110 force-pushed the addon-rollout branch from b8ad457 to fa37d3c Compare July 28, 2023 12:41

openshift-merge-robot added the needs-rebase label Aug 5, 2023

haoqing0110 force-pushed the addon-rollout branch from fa37d3c to 332756d Compare August 8, 2023 08:15

openshift-merge-robot removed the needs-rebase label Aug 8, 2023

haoqing0110 force-pushed the addon-rollout branch 2 times, most recently from 36e2ffa to 6dc54ef Compare August 9, 2023 09:16

haoqing0110 force-pushed the addon-rollout branch from 6dc54ef to aa39283 Compare August 16, 2023 09:24

haoqing0110 changed the title ~~[wip] addon consume rollout helpers~~ 🌱 addon consume rollout helpers Aug 16, 2023

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

openshift-ci bot added the do-not-merge/hold label Aug 16, 2023

haoqing0110 force-pushed the addon-rollout branch 2 times, most recently from d87ef70 to aefc37f Compare August 23, 2023 06:27

openshift-merge-robot added the needs-rebase label Aug 23, 2023

haoqing0110 force-pushed the addon-rollout branch 2 times, most recently from 76e70d7 to 1f4e6a8 Compare September 7, 2023 08:16

openshift-merge-robot added needs-rebase and removed needs-rebase labels Sep 7, 2023

haoqing0110 force-pushed the addon-rollout branch from 1f4e6a8 to 40a001c Compare September 8, 2023 03:34

openshift-merge-robot removed the needs-rebase label Sep 8, 2023

haoqing0110 force-pushed the addon-rollout branch 2 times, most recently from 023fd5c to 08ce150 Compare September 11, 2023 03:18

haoqing0110 force-pushed the addon-rollout branch from 08ce150 to 3776d87 Compare September 11, 2023 08:04

qiujian16 reviewed Sep 12, 2023

View reviewed changes

openshift-merge-robot added the needs-rebase label Sep 13, 2023

haoqing0110 force-pushed the addon-rollout branch from e8a812b to f40a82e Compare September 13, 2023 06:10

openshift-merge-robot removed the needs-rebase label Sep 13, 2023

haoqing0110 force-pushed the addon-rollout branch 2 times, most recently from bed2c7e to a33b4bd Compare September 14, 2023 08:09

haoqing0110 mentioned this pull request Sep 18, 2023

sync addonconfiguration controller code from ocm open-cluster-management-io/addon-framework#208

Merged

haoqing0110 force-pushed the addon-rollout branch 2 times, most recently from 4cd980a to 98db014 Compare September 18, 2023 09:26

openshift-ci bot added the approved label Sep 19, 2023

haoqing0110 force-pushed the addon-rollout branch 3 times, most recently from f2027dc to d1803d5 Compare September 19, 2023 10:47

addon consume rollout helpers

2463035

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

haoqing0110 force-pushed the addon-rollout branch from d1803d5 to 2463035 Compare September 20, 2023 01:10

openshift-ci bot assigned qiujian16 Sep 20, 2023

openshift-ci bot added the lgtm label Sep 20, 2023

openshift-ci bot removed the do-not-merge/hold label Sep 20, 2023

openshift-merge-robot merged commit c8410bf into open-cluster-management-io:main Sep 20, 2023
13 checks passed

haoqing0110 deleted the addon-rollout branch September 20, 2023 02:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 addon consume rollout helpers #225

🌱 addon consume rollout helpers #225

haoqing0110 commented Jul 20, 2023 •

edited

Loading

haoqing0110 commented Jul 20, 2023

codecov bot commented Jul 28, 2023 •

edited

Loading

haoqing0110 commented Aug 16, 2023

qiujian16 Sep 12, 2023

haoqing0110 Sep 13, 2023

qiujian16 Sep 12, 2023

haoqing0110 Sep 13, 2023

qiujian16 Sep 12, 2023

haoqing0110 Sep 12, 2023

haoqing0110 Sep 13, 2023

qiujian16 Sep 12, 2023

haoqing0110 Sep 13, 2023

qiujian16 Sep 12, 2023

haoqing0110 Sep 13, 2023

qiujian16 commented Sep 19, 2023

haoqing0110 commented Sep 19, 2023

qiujian16 commented Sep 20, 2023

openshift-ci bot commented Sep 20, 2023

haoqing0110 commented Sep 20, 2023


		// get placement
		placement, err := c.getPlacement(installProgression.PlacementRef.Name, installProgression.PlacementRef.Namespace)

🌱 addon consume rollout helpers #225

🌱 addon consume rollout helpers #225

Conversation

haoqing0110 commented Jul 20, 2023 • edited Loading

Summary

Related issue(s)

haoqing0110 commented Jul 20, 2023

codecov bot commented Jul 28, 2023 • edited Loading

Codecov Report

haoqing0110 commented Aug 16, 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

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 Sep 19, 2023

haoqing0110 commented Sep 19, 2023

qiujian16 commented Sep 20, 2023

openshift-ci bot commented Sep 20, 2023

haoqing0110 commented Sep 20, 2023

haoqing0110 commented Jul 20, 2023 •

edited

Loading

codecov bot commented Jul 28, 2023 •

edited

Loading