✨ Implement ManifestWorkReplicaSet RollOut strategy #259

serngawy · 2023-08-29T15:25:23Z

Summary

Related issue(s)

Fixes #

serngawy · 2023-08-29T15:25:56Z

/hold

Signed-off-by: melserngawy <melserng@redhat.com>

codecov · 2023-10-10T19:53:28Z

Codecov Report

Attention: 41 lines in your changes are missing coverage. Please review.

Comparison is base (5903140) 61.06% compared to head (c182f35) 61.61%.
Report is 9 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #259      +/-   ##
==========================================
+ Coverage   61.06%   61.61%   +0.55%     
==========================================
  Files         129      132       +3     
  Lines       13771    13956     +185     
==========================================
+ Hits         8409     8599     +190     
- Misses       4588     4591       +3     
+ Partials      774      766       -8

Flag	Coverage Δ
unit	`61.61% <75.30%> (+0.55%)`	⬆️

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

Files	Coverage Δ
...troller/manifestworkreplicaset_status_reconcile.go	`86.66% <88.23%> (+4.84%)`	⬆️
...setcontroller/manifestworkreplicaset_controller.go	`47.87% <45.45%> (-0.33%)`	⬇️
pkg/work/helper/helpers.go	`66.89% <0.00%> (-2.10%)`	⬇️
...troller/manifestworkreplicaset_deploy_reconcile.go	`77.65% <80.35%> (+8.72%)`	⬆️

... and 16 files with indirect coverage changes

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

serngawy · 2023-10-18T17:18:34Z

/unhold

serngawy · 2023-10-18T18:35:04Z

/cc @qiujian16

serngawy · 2023-10-18T18:35:58Z

/cc @haoqing0110

haoqing0110 · 2023-10-19T07:14:10Z

pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go

+
+	// Check for the expected manifestWorkReplicaSetSummary
+	mwrSetSummary := workapiv1alpha1.ManifestWorkReplicaSetSummary{
+		Total:       clsPerGroup + int(placement1.Status.NumberOfSelectedClusters),


What if there's clusters overlap in placement1 and placement2, will the cluster number be counted twice in the summary?

We use the mwrSet name to create the mw , so if a cluster is selected twice in placement1 & placement2 what will happen is;

For placement1; the cluster will get selected and the mw will be created with ref to placement 1 in its labels as here

The placement1 Summary will count the cluster

For placement2; the cluster will also get selected and the mw label ref to placement will be updated to refer to placement2 as here.

The placement2 Summary will also count the cluster (cause now it is belong to it)

So yes, it will be counted for each placement Summary (as the placement doesn't know if the cluster is selected in another placement) AND will be counted twice in the mwrSet Summary.

I'm not really sure how the logic should be, should we raise error ? should we skip it ? is it okay to count the cluster twice as it is exist in both placements?

My thought is a cluster counted twice makes the meaning of Total confusing to the user, since the actual number of ManifestWorks is less than it.

For addon, one addon only belongs to one placement (the latter one), and use that rollout strategy do rollout. It may not be the same as the mwrSet here, just for reference.

qiujian16 · 2023-10-20T13:41:09Z

sorry I am stuck by other stuff this week, will go through this PR early next week.

qiujian16 · 2023-10-23T07:02:03Z

pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_controller.go

@@ -37,6 +37,10 @@ const (
 	// TODO move this to the api repo
 	ManifestWorkReplicaSetControllerNameLabelKey = "work.open-cluster-management.io/manifestworkreplicaset"

+	// ManifestWorkReplicaSetPlacementNameLabelKey is the label key on manifestwork to ref to the Placement that select
+	// the managedCluster on the manifestWorkReplicaSet's PlacementRef.
+	ManifestWorkReplicaSetPlacementNameLabelKey = "work.open-cluster-management.io/PlacementName"


the label key should not have capital in it.

qiujian16 · 2023-10-24T07:11:40Z

.../hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go

-	}
+		for _, mw := range manifestWorks {
+			// Check if ManifestWorkTemplate changes, ManifestWork will need to be updated.
+			if !equality.Semantic.DeepEqual(mwrSet.Spec.ManifestWorkTemplate, mw.Spec) {


this check is not quite reliable, since there could diff on somefield with empty or nil values. It is better to user the work applier helper.

I think we should always apply, and check the returned updated value to know if if it is updated.

okay, I used the workApplier.ManifestWorkEqual instead.

Add a TODO here: we should have an interface in applier to check whether a mw should be applied.

qiujian16 · 2023-10-24T07:17:10Z

.../hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go

+	}
+	// applied and Progressing conditions return status as Progressing
+	if apimeta.IsStatusConditionTrue(manifestWork.Status.Conditions, workv1.WorkApplied) ||
+		apimeta.IsStatusConditionTrue(manifestWork.Status.Conditions, workv1.WorkProgressing) {


do we have a WorkProgressing status right now?

I don't think so unless you can point me. I checked the manifestWork controllers sounds like there is no WorkProgressing status defined yet. I added comment to clarify that.

qiujian16 · 2023-10-24T07:17:50Z

.../hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go

+		clsRolloutStatus.Status = clusterv1alpha1.Succeeded
+	}
+	// Degraded condition return status as Failed
+	if apimeta.IsStatusConditionTrue(manifestWork.Status.Conditions, workv1.WorkDegraded) {


There is not degraded status defined yet. We should add some note here.

Yes, same there is no degraded status defined. I Considered the Applied condition with False status as Failed, added comments to clarify that.

Signed-off-by: melserngawy <melserng@redhat.com>

qiujian16

/approve

@haoqing0110 would you take a final look on this?

openshift-ci · 2023-11-01T00:17:08Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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-11-01T03:41:28Z

LGTM
Only one concern for the overlapped clusters being counted twice in the summary message. This could be a follow-up fix when receiving more feedback.

serngawy · 2023-11-01T13:08:48Z

LGTM Only one concern for the overlapped clusters being counted twice in the summary message. This could be a follow-up fix when receiving more feedback.

Yes, we decided to keep it as it is for now (manifestWork will be associated with the latest placement) cause it can be valid for both ways. Will wait for feedback if any changes is necessary.

qiujian16 · 2023-11-02T03:05:20Z

/lgtm

openshift-ci bot requested review from zhiweiyin318 and zhujian7 August 29, 2023 15:25

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

serngawy force-pushed the mwrSetRollout branch 2 times, most recently from 2b5829a to 8e17ac0 Compare August 29, 2023 15:58

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

serngawy force-pushed the mwrSetRollout branch from 8e17ac0 to bf8bf67 Compare September 28, 2023 18:03

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

serngawy force-pushed the mwrSetRollout branch from 3de8620 to 917c5b1 Compare October 3, 2023 21:38

openshift-merge-robot added the needs-rebase label Oct 10, 2023

Implement rollout strategy

3b2cc0c

Signed-off-by: melserngawy <melserng@redhat.com>

serngawy force-pushed the mwrSetRollout branch from 917c5b1 to ab8273b Compare October 10, 2023 19:45

openshift-merge-robot removed the needs-rebase label Oct 10, 2023

serngawy force-pushed the mwrSetRollout branch 4 times, most recently from a8b92f9 to 26cd588 Compare October 18, 2023 16:13

serngawy changed the title ~~✨ (WIP) Implement ManifestWorkReplicaSet RollOut strategy~~ ✨ Implement ManifestWorkReplicaSet RollOut strategy Oct 18, 2023

openshift-ci bot removed the do-not-merge/hold label Oct 18, 2023

serngawy force-pushed the mwrSetRollout branch from 26cd588 to 66a6f4c Compare October 18, 2023 17:34

openshift-ci bot requested a review from qiujian16 October 18, 2023 18:35

openshift-ci bot requested a review from haoqing0110 October 18, 2023 18:35

haoqing0110 reviewed Oct 19, 2023

View reviewed changes

serngawy force-pushed the mwrSetRollout branch from 53b37d2 to 83f47f9 Compare October 20, 2023 19:01

qiujian16 reviewed Oct 24, 2023

View reviewed changes

serngawy force-pushed the mwrSetRollout branch from 83f47f9 to 8f69f82 Compare October 26, 2023 19:06

Update API and new logic

c182f35

Signed-off-by: melserngawy <melserng@redhat.com>

serngawy force-pushed the mwrSetRollout branch from 8f69f82 to c182f35 Compare October 31, 2023 18:53

qiujian16 reviewed Nov 1, 2023

View reviewed changes

openshift-ci bot added the approved label Nov 1, 2023

openshift-ci bot assigned qiujian16 Nov 2, 2023

openshift-ci bot added the lgtm label Nov 2, 2023

openshift-ci bot merged commit 35680c3 into open-cluster-management-io:main Nov 2, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Implement ManifestWorkReplicaSet RollOut strategy #259

✨ Implement ManifestWorkReplicaSet RollOut strategy #259

serngawy commented Aug 29, 2023

serngawy commented Aug 29, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading

serngawy commented Oct 18, 2023

serngawy commented Oct 18, 2023

serngawy commented Oct 18, 2023

haoqing0110 Oct 19, 2023

serngawy Oct 19, 2023

haoqing0110 Oct 20, 2023 •

edited

Loading

qiujian16 commented Oct 20, 2023

qiujian16 Oct 23, 2023

serngawy Oct 25, 2023

qiujian16 Oct 24, 2023

serngawy Oct 25, 2023

qiujian16 Oct 31, 2023

qiujian16 Oct 24, 2023

serngawy Oct 25, 2023

qiujian16 Oct 24, 2023

serngawy Oct 26, 2023

qiujian16 left a comment

openshift-ci bot commented Nov 1, 2023

haoqing0110 commented Nov 1, 2023

serngawy commented Nov 1, 2023

qiujian16 commented Nov 2, 2023

✨ Implement ManifestWorkReplicaSet RollOut strategy #259

✨ Implement ManifestWorkReplicaSet RollOut strategy #259

Conversation

serngawy commented Aug 29, 2023

Summary

Related issue(s)

serngawy commented Aug 29, 2023

codecov bot commented Oct 10, 2023 • edited Loading

Codecov Report

serngawy commented Oct 18, 2023

serngawy commented Oct 18, 2023

serngawy commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haoqing0110 Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

qiujian16 commented Oct 20, 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

qiujian16 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 1, 2023

haoqing0110 commented Nov 1, 2023

serngawy commented Nov 1, 2023

qiujian16 commented Nov 2, 2023

codecov bot commented Oct 10, 2023 •

edited

Loading

haoqing0110 Oct 20, 2023 •

edited

Loading