Add disabled field in rollout spec #155

Kuromesi · 2023-06-26T02:53:51Z

Ⅰ. Describe what this PR does

Added Disabled field in rollout spec.
Added a rollout phase constant.

// RolloutPhaseDisabled indicates a rollout is disabled
RolloutPhaseDisabled RolloutPhase = "Disabled"

How the Disabled spec works?

If a rollout is disabled, the rollout is not working.
When an rolling rollout is disabled, release the resources of the rollout and set its phase as Disabled.

Ⅱ. Does this pull request fix one issue?

fixes #151

Ⅲ. Special notes for reviews

kruise-bot · 2023-06-26T02:54:07Z

Welcome @Kuromesi! It looks like this is your first PR to openkruise/rollouts 🎉

sonatype-lift · 2023-06-26T02:57:47Z

pkg/controller/rollout/rollout_status.go

+			rollout.Status.Phase = v1alpha1.RolloutPhaseTerminating
+			cond := util.NewRolloutCondition(v1alpha1.RolloutConditionTerminating, corev1.ConditionTrue, v1alpha1.TerminatingReasonInTerminating, "Rollout is in terminating")
+			util.SetRolloutCondition(&rollout.Status, *cond)
+			r.reconcileRolloutTerminating(rollout, &rollout.Status)


G104: Errors unhandled.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

sonatype-lift · 2023-06-26T02:57:49Z

pkg/controller/rollout/rollout_status.go

+					rollout.Status.Phase = v1alpha1.RolloutPhaseTerminating
+					cond := util.NewRolloutCondition(v1alpha1.RolloutConditionTerminating, corev1.ConditionTrue, v1alpha1.TerminatingReasonInTerminating, "Rollout is in terminating")
+					util.SetRolloutCondition(&rollout.Status, *cond)
+					r.reconcileRolloutTerminating(rollout, &rollout.Status)


G104: Errors unhandled.

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

sonatype-lift · 2023-06-26T02:57:51Z

pkg/webhook/rollout/validating/rollout_create_update_handler.go

@@ -120,11 +120,14 @@ func (h *RolloutCreateUpdateHandler) validateRolloutUpdate(oldObj, newObj *appsv

 func (h *RolloutCreateUpdateHandler) validateRollout(rollout *appsv1alpha1.Rollout) field.ErrorList {
 	errList := validateRolloutSpec(rollout, field.NewPath("Spec"))
-	errList = append(errList, h.validateRolloutConflict(rollout, field.NewPath("Conflict Checker"))...)
+	// errList = append(errList, h.validateRolloutConflict(rollout, field.NewPath("Conflict Checker"))...)
 	return errList
 }

 func (h *RolloutCreateUpdateHandler) validateRolloutConflict(rollout *appsv1alpha1.Rollout, path *field.Path) field.ErrorList {


U1000: func (*RolloutCreateUpdateHandler).validateRolloutConflict is unused

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

sonatype-lift · 2023-06-26T02:57:54Z

pkg/controller/rollout/rollout_status.go

+
+// check if an enabled rollout conflicts with other enabled rollout
+func (r *RolloutReconciler) checkConflict(rollout *v1alpha1.Rollout) (bool, error) {
+	// if rollout is enabling or terminating then the conflict check is not neccessary


misspell: neccessary is a misspelling of necessary

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

zmberg · 2023-06-26T06:04:19Z

@Kuromesi I think that for the moment we can dispense with the "rollout conflicts with another rollout" scenario and assume that there is a rollout.

codecov-commenter · 2023-06-27T07:24:17Z

Codecov Report

Merging #155 (10b8f2a) into master (8737f33) will decrease coverage by 0.02%.
The diff coverage is 21.05%.

❗ Current head 10b8f2a differs from pull request most recent head bf8b850. Consider uploading reports for the commit bf8b850 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #155      +/-   ##
==========================================
- Coverage   43.72%   43.70%   -0.02%     
==========================================
  Files          50       50              
  Lines        5263     5297      +34     
==========================================
+ Hits         2301     2315      +14     
- Misses       2575     2588      +13     
- Partials      387      394       +7

Flag	Coverage Δ
unittests	`43.70% <21.05%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/controller/rollout/rollout_canary.go	`42.75% <ø> (ø)`
pkg/controller/rollout/rollout_controller.go	`11.26% <0.00%> (-0.33%)`	⬇️
pkg/controller/rollout/rollout_event_handler.go	`48.57% <ø> (ø)`
...bhook/workload/mutating/workload_update_handler.go	`33.21% <0.00%> (-0.38%)`	⬇️
pkg/controller/rollout/rollout_status.go	`24.52% <24.24%> (+5.29%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

zmberg · 2023-06-28T10:43:58Z

api/v1alpha1/rollout_types.go

@@ -72,6 +72,7 @@ type RolloutSpec struct {
 	// RolloutID should be changed before each workload revision publication.
 	// It is to distinguish consecutive multiple workload publications and rollout progress.
 	DeprecatedRolloutID string `json:"rolloutID,omitempty"`
+	Disabled            bool   `json:"disabled"`


add comments to describe the meaning and behaviour of disabled

zmberg · 2023-06-28T10:49:24Z

pkg/controller/rollout/rollout_canary.go

@@ -454,6 +454,11 @@ func (m *canaryReleaseManager) finalizingBatchRelease(c *RolloutContext) (bool,
 		return false, err
 	}
 	klog.Infof("rollout(%s/%s) patch batchRelease(%s) success", c.Rollout.Namespace, c.Rollout.Name, body)
+
+	// if rollout is disabled, then the batchrelease should be deleted
+	if c.Rollout.Spec.Disabled {


I think the code can be removed, because if rollout is disabled, the program will not execute here

sumengzs · 2023-06-29T02:30:26Z

@Kuromesi 我看了一下，你的代码中，似乎没有考虑到 rollout 从 Progressing 转换到 Disabled 如何处理的问题，你需要仔细阅读源代码， rollout 这个 CR 实际上是一个控制角色，实际的执行角色是，BatchRelease 这个CR，当 rollout 处于 Progressing 状态时，BatchRelease 已经被创建，它会忠实的履行 rollout 给到的指令。或许你可以思考一下如何处理这个部分。

sonatype-lift · 2023-06-29T03:14:17Z

api/v1alpha1/rollout_types.go

@@ -196,6 +197,10 @@ const (
 	ProgressingReasonCancelling   = "Cancelling"
 	ProgressingReasonPaused       = "Paused"

+	// Disabling condition
+	RolloutConditionDisabling RolloutConditionType = "Disabling"


SA9004: only the first constant in this group has an explicit type

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command Usage

@sonatype-lift ignore Leave out the above finding from this PR

@sonatype-lift ignoreall Leave out all the existing findings from this PR

@sonatype-lift exclude <file|issue|path|tool> Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

Kuromesi · 2023-06-29T06:17:39Z

@Kuromesi 我看了一下，你的代码中，似乎没有考虑到 rollout 从 Progressing 转换到 Disabled 如何处理的问题，你需要仔细阅读源代码， rollout 这个 CR 实际上是一个控制角色，实际的执行角色是，BatchRelease 这个CR，当 rollout 处于 Progressing 状态时，BatchRelease 已经被创建，它会忠实的履行 rollout 给到的指令。或许你可以思考一下如何处理这个部分。

感谢建议，在新版的代码中已经对逻辑进行了重写

zmberg · 2023-07-04T05:40:31Z

pkg/controller/rollout/rollout_event_handler.go

@@ -92,7 +92,7 @@ func (w *enqueueRequestForWorkload) getRolloutForWorkload(key types.NamespacedNa
 			continue
 		}

-		if targetRef.Kind == gvk.Kind && targetGV.Group == gvk.Group && targetRef.Name == key.Name {
+		if targetRef.Kind == gvk.Kind && targetGV.Group == gvk.Group && targetRef.Name == key.Name && rollout.Status.Phase != rolloutv1alpha1.RolloutPhaseDisabled {


Remove the judgement for now.

zmberg · 2023-07-04T05:41:46Z

pkg/controller/rollout/rollout_status.go

+		}
+	}
+
+	if newStatus.Phase == "" || newStatus.Phase == v1alpha1.RolloutPhaseDisabled {


Why newStatus.phase == newStatus.Phase == v1alpha1.RolloutPhaseDisabled, then set newStatus.Phase = v1alpha1.RolloutPhaseInitial ?

zmberg · 2023-07-04T05:44:50Z

pkg/controller/rollout/rollout_status.go

@@ -49,7 +49,23 @@ func (r *RolloutReconciler) calculateRolloutStatus(rollout *v1alpha1.Rollout) (r
 		}
 		return false, newStatus, nil
 	}
-	if newStatus.Phase == "" {
+
+	if rollout.Spec.Disabled && newStatus.Phase != v1alpha1.RolloutPhaseDisabling {


if rollout.Spec.Disabled && (newStatus.Phase != v1alpha1.RolloutPhaseDisabling || newStatus.Phase != v1alpha1.RolloutPhaseDisabled {

zmberg · 2023-07-04T05:49:01Z

pkg/controller/rollout/rollout_status.go

+			newStatus.Phase = v1alpha1.RolloutPhaseDisabling
+			newStatus.Message = "Disabling rollout, release resources"
+		} else {
+			*newStatus = v1alpha1.RolloutStatus{


Why you new struct RolloutStatus? Can you set the newStatus filed immediately?

zmberg · 2023-07-04T05:51:12Z

test/e2e/rollout_test.go

+			CreateObject(rollout1)
+			time.Sleep(1 * time.Second)
+
+			By("Create another enabled rollout")


I think that if there are multiple Rollout creations, the Webhook should reject the later rollout creations.

zmberg · 2023-07-04T05:52:11Z

api/v1alpha1/rollout_types.go

@@ -196,6 +198,11 @@ const (
 	ProgressingReasonCancelling   = "Cancelling"
 	ProgressingReasonPaused       = "Paused"

+	// Disabling condition
+	RolloutConditionDisabling RolloutConditionType = "Disabling"


I think we can remove the disabling condition currently.

zmberg · 2023-07-05T02:23:39Z

pkg/controller/rollout/rollout_canary.go

@@ -454,6 +454,11 @@ func (m *canaryReleaseManager) finalizingBatchRelease(c *RolloutContext) (bool,
 		return false, err
 	}
 	klog.Infof("rollout(%s/%s) patch batchRelease(%s) success", c.Rollout.Namespace, c.Rollout.Name, body)
+
+	// if rollout is disabling, then the batchrelease should be deleted
+	if c.NewStatus.Phase == v1alpha1.RolloutPhaseDisabling {


Remove the code, because the progressing will never run this, when rollout.spec.disabled is true.

zmberg · 2023-07-05T02:26:45Z

pkg/controller/rollout/rollout_status.go

+			newStatus.Message = "Disabling rollout, release resources"
+		} else {
+			newStatus.Phase = v1alpha1.RolloutPhaseDisabled
+			newStatus.ObservedGeneration = rollout.Generation


remove the generation code, because the above code already set this field.

zmberg · 2023-07-05T02:28:22Z

pkg/controller/rollout/rollout_status.go

@@ -58,10 +71,12 @@ func (r *RolloutReconciler) calculateRolloutStatus(rollout *v1alpha1.Rollout) (r
 		klog.Errorf("rollout(%s/%s) get workload failed: %s", rollout.Namespace, rollout.Name, err.Error())
 		return false, nil, err
 	} else if workload == nil {


} else if workload == nil && !rollout.Spec.Disabled {

zmberg · 2023-07-05T02:29:40Z

pkg/controller/rollout/rollout_status.go

@@ -122,6 +137,11 @@ func (r *RolloutReconciler) calculateRolloutStatus(rollout *v1alpha1.Rollout) (r
 			}
 			newStatus.Message = "workload deployment is completed"
 		}
+	case v1alpha1.RolloutPhaseDisabled:
+		if !rollout.Spec.Disabled {
+			newStatus.Phase = v1alpha1.RolloutPhaseHealthy


newStatus.Phase = v1alpha1.RolloutPhaseInitial, Because workload may be not found.

zmberg · 2023-07-05T02:31:15Z

pkg/controller/rollout/rollout_status.go

+	} else if done {
+		klog.Infof("rollout(%s/%s) is disabled", rollout.Namespace, rollout.Name)
+		newStatus.Phase = v1alpha1.RolloutPhaseDisabled
+		newStatus.ObservedGeneration = rollout.Generation


Remove the generation code.

zmberg · 2023-07-05T02:32:11Z

pkg/webhook/workload/mutating/workload_update_handler.go

@@ -406,7 +406,7 @@ func (h *WorkloadHandler) fetchMatchedRollout(obj client.Object) (*appsv1alpha1.
 	}
 	for i := range rolloutList.Items {
 		rollout := &rolloutList.Items[i]
-		if !rollout.DeletionTimestamp.IsZero() || rollout.Spec.ObjectRef.WorkloadRef == nil {
+		if !rollout.DeletionTimestamp.IsZero() || rollout.Spec.ObjectRef.WorkloadRef == nil || rollout.Status.Phase == appsv1alpha1.RolloutPhaseDisabled {


when rollout is disabled, add log.

zmberg · 2023-07-05T03:30:58Z

/lgtm

veophi · 2023-07-05T05:41:18Z

/lgtm

furykerry · 2023-07-05T07:06:45Z

test/e2e/rollout_test.go

@@ -5485,6 +5485,82 @@ var _ = SIGDescribe("Rollout", func() {
 		})

 	})
+
+	KruiseDescribe("Disabled rollout tests", func() {


add an ut about enable a disabled rollout

furykerry · 2023-07-05T07:08:28Z

config/crd/bases/rollouts.kruise.io_rollouts.yaml

@@ -271,6 +275,7 @@ spec:
                    type: boolean
                type: object
            required:
+            - disabled


can disabled be optional ? defaults to false?

Have already fixed the issues mentioned above

Signed-off-by: Kuromesi <blackfacepan@163.com>

furykerry · 2023-07-05T09:19:11Z

/lgtm

zmberg · 2023-07-05T10:45:06Z

/approve

kruise-bot · 2023-07-05T10:45:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zmberg

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~~ [zmberg]

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: Kuromesi <blackfacepan@163.com>

kruise-bot requested review from FillZpp and zmberg June 26, 2023 02:53

kruise-bot added the size/L label Jun 26, 2023

sonatype-lift bot reviewed Jun 26, 2023

View reviewed changes

Kuromesi force-pushed the dev branch from 184dfc9 to ed75b6d Compare June 26, 2023 09:17

kruise-bot added size/XXL needs-rebase size/L and removed size/L needs-rebase size/XXL labels Jun 26, 2023

kruise-bot added size/XL and removed size/L labels Jun 27, 2023

Kuromesi changed the title ~~Dev~~ Disabled rollouts Jun 28, 2023

kruise-bot added size/L and removed size/XL labels Jun 28, 2023

Kuromesi force-pushed the dev branch from 1b516ea to aa2c9c4 Compare June 28, 2023 09:23

zmberg reviewed Jun 28, 2023

View reviewed changes

sonatype-lift bot reviewed Jun 29, 2023

View reviewed changes

Kuromesi force-pushed the dev branch 3 times, most recently from 07da68f to e0c50cb Compare June 29, 2023 03:41

zmberg reviewed Jul 4, 2023

View reviewed changes

Kuromesi changed the title ~~Disabled rollouts~~ Add disabled field in rollout spec Jul 4, 2023

Kuromesi force-pushed the dev branch from 58f9424 to a759390 Compare July 5, 2023 01:44

zmberg reviewed Jul 5, 2023

View reviewed changes

Kuromesi force-pushed the dev branch from 30acbf5 to 5d6dddc Compare July 5, 2023 03:17

kruise-bot assigned zmberg Jul 5, 2023

kruise-bot added the lgtm label Jul 5, 2023

kruise-bot assigned veophi Jul 5, 2023

furykerry reviewed Jul 5, 2023

View reviewed changes

kruise-bot removed the lgtm label Jul 5, 2023

Kuromesi force-pushed the dev branch 2 times, most recently from 10b8f2a to bf8b850 Compare July 5, 2023 08:01

disabled rollout

4b7f488

Signed-off-by: Kuromesi <blackfacepan@163.com>

Kuromesi force-pushed the dev branch from bf8b850 to 4b7f488 Compare July 5, 2023 08:11

kruise-bot assigned furykerry Jul 5, 2023

kruise-bot added the lgtm label Jul 5, 2023

kruise-bot added the approved label Jul 5, 2023

kruise-bot merged commit 88e4bb7 into openkruise:master Jul 5, 2023

Kuromesi added a commit to Kuromesi/rollouts that referenced this pull request Jul 17, 2023

disabled rollout (openkruise#155)

94c2afe

Signed-off-by: Kuromesi <blackfacepan@163.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add disabled field in rollout spec #155

Add disabled field in rollout spec #155

Kuromesi commented Jun 26, 2023 •

edited

Loading

kruise-bot commented Jun 26, 2023

sonatype-lift bot Jun 26, 2023

sonatype-lift bot Jun 26, 2023

sonatype-lift bot Jun 26, 2023

sonatype-lift bot Jun 26, 2023

zmberg commented Jun 26, 2023

codecov-commenter commented Jun 27, 2023 •

edited

Loading

zmberg Jun 28, 2023

zmberg Jun 28, 2023

sumengzs commented Jun 29, 2023 •

edited

Loading

sonatype-lift bot Jun 29, 2023

Kuromesi commented Jun 29, 2023

zmberg Jul 4, 2023

zmberg Jul 4, 2023

zmberg Jul 4, 2023

zmberg Jul 4, 2023

zmberg Jul 4, 2023

zmberg Jul 4, 2023

zmberg Jul 5, 2023

zmberg Jul 5, 2023

zmberg Jul 5, 2023

zmberg Jul 5, 2023

zmberg Jul 5, 2023

zmberg Jul 5, 2023

zmberg commented Jul 5, 2023

veophi commented Jul 5, 2023

furykerry Jul 5, 2023

furykerry Jul 5, 2023

Kuromesi Jul 5, 2023

furykerry commented Jul 5, 2023

zmberg commented Jul 5, 2023

kruise-bot commented Jul 5, 2023

Command	Usage
`@sonatype-lift ignore`	Leave out the above finding from this PR
`@sonatype-lift ignoreall`	Leave out all the existing findings from this PR
`@sonatype-lift exclude <file\|issue\|path\|tool>`	Exclude specified `file\|issue\|path\|tool` from Lift findings by updating your config.toml file

Add disabled field in rollout spec #155

Add disabled field in rollout spec #155

Conversation

Kuromesi commented Jun 26, 2023 • edited Loading

Ⅰ. Describe what this PR does

How the Disabled spec works?

Ⅱ. Does this pull request fix one issue?

Ⅲ. Special notes for reviews

kruise-bot commented Jun 26, 2023

sonatype-lift bot Jun 26, 2023

Choose a reason for hiding this comment

sonatype-lift bot Jun 26, 2023

Choose a reason for hiding this comment

sonatype-lift bot Jun 26, 2023

Choose a reason for hiding this comment

sonatype-lift bot Jun 26, 2023

Choose a reason for hiding this comment

zmberg commented Jun 26, 2023

codecov-commenter commented Jun 27, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumengzs commented Jun 29, 2023 • edited Loading

sonatype-lift bot Jun 29, 2023

Choose a reason for hiding this comment

Kuromesi commented Jun 29, 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

Choose a reason for hiding this comment

zmberg commented Jul 5, 2023

veophi commented Jul 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furykerry commented Jul 5, 2023

zmberg commented Jul 5, 2023

kruise-bot commented Jul 5, 2023

Kuromesi commented Jun 26, 2023 •

edited

Loading

codecov-commenter commented Jun 27, 2023 •

edited

Loading

sumengzs commented Jun 29, 2023 •

edited

Loading