add parameter to configure max restarts for canary pause #51

celenechang · 2020-10-20T13:46:19Z

What does this PR do?

Currently the Canary autopause feature pauses the canary deployment if any of the canary pod containers restart more than 0 times. We have found that this is too sensitive, so this PR makes this a configurable parameter with a default value of 2. The maxRestarts parameter means that this is the number of restarts of a single container that is considered tolerable before we autopause.

It also adds the ability to disable the feature entirely via the spec.

Motivation

Better user experience

Additional Notes

A subsequent PR with a new parameter to define a stability (no restarts) duration requirement is planned.

Describe your test plan

Use an image such as bash:latest in the EDS configuration that causes the container to enter CLB. Increase the canary duration to be sufficiently large to give time for restarts.

Check if canary is automatically paused when the container restarts 3 times (default) without adding the autoPause.maxRestarts setting to the EDS spec.

Then try setting autoPause.maxRestarts to a larger number and see if the canary pauses after the corresponding number of restarts (see examples/foo-eds_v2.yaml for an example configuration).

Lastly, try setting autoPause.enabled to false and autoPause.maxRestarts to 0 and make sure that a restart does not pause the canary.

github-actions

This pull request does not contain a valid label. Please add one of the following labels: bug, enhancement, documentation

github-actions

This pull request contains a valid label.

pkg/controller/utils/pod/pod.go

clamoriniere

Look good.

Could you add also a documentation about how configure and use the pause feature:

configuration example
kubectl plugin command example

clamoriniere · 2020-10-26T09:40:20Z

api/v1alpha1/extendeddaemonset_types.go

@@ -75,6 +75,7 @@ type ExtendedDaemonSetSpecStrategyRollingUpdate struct {
 type ExtendedDaemonSetSpecStrategyCanary struct {
 	Replicas     *intstr.IntOrString   `json:"replicas,omitempty"`
 	Duration     *metav1.Duration      `json:"duration,omitempty"`
+	MaxRestarts  *intstr.IntOrString   `json:"maxRestarts,omitempty"`


I'm wondering if it is necessary to have a *intstr.IntOrString . I feel like *int should be enough.

pkg/controller/utils/pod/pod.go

…led param

github-actions

This pull request contains a valid label.

github-actions

This pull request contains a valid label.

github-actions

This pull request contains a valid label.

github-actions

This pull request contains a valid label.

ahmed-mez · 2020-10-28T08:36:16Z

controllers/extendeddaemonsetreplicaset/strategy/canary.go

+	if daemonset.Spec.Strategy.Canary == nil || daemonset.Spec.Strategy.Canary.AutoPause == nil {
+		autoPauseEnabled = defaultAutoPauseEnabled
+		maxRestarts = defaultMaxRestarts
+	} else {
+		if daemonset.Spec.Strategy.Canary.AutoPause.Enabled == nil {
+			autoPauseEnabled = defaultAutoPauseEnabled
+		} else {
+			autoPauseEnabled = *daemonset.Spec.Strategy.Canary.AutoPause.Enabled
+		}
+		if daemonset.Spec.Strategy.Canary.AutoPause.MaxRestarts == nil {
+			maxRestarts = defaultMaxRestarts
+		} else {
+			maxRestarts = int(*daemonset.Spec.Strategy.Canary.AutoPause.MaxRestarts)
+		}
+	}


please correct me if I'm wrong, but I think at this point the defaulting should be already done via DefaultExtendedDaemonSetSpecStrategyCanaryAutoPause ?

to be clearer, the question is: why do we default the autopause config twice?

Thanks for bringing this up, as I didn't have a complete grasp on this.

It seems we default the ERS (in extendeddaemonset_default.go) if the minimum required settings are not defined. In this case we could set the default AutoPause settings with DefaultExtendedDaemonSetSpecStrategyCanaryAutoPause(), but it's not strictly necessary since we also have this defaulting code here in canary.go. If you think it's confusing to include the change in extendeddaemonset_default.go I can remove it.

thanks for clarifying this
tbh I would expect the defaulting to be done in one place (in extendeddaemonset_default.go), the fact that the same defaulting logic and the same constants (defaultMaxRestarts defaultAutoPauseEnabled defaultCanaryAutoPauseEnabled defaultCanaryAutoPauseMaxRestarts) are defined in different packages doesn't seem right 🤔

I think if we do it as @ahmed-mez said in extendeddaemonset_default.go, the defaulting is not needed in ManageCanaryDeployment because we always call the default method.
To achieve it, the function IsDefaultedExtendedDaemonSet() link, should be updated, to return false if the canary configuration is missing default values.

I tested it like this, with only setting the defaults in extendeddaemonset_default.go, and it led to undesired behaviors. I may have done something wrong; I will try again.

OK got this fixed, and confirmed it works whether autoPause, autoPause.Enabled, or autoPause.MaxRestarts is missing. Sorry for the confusion

ahmed-mez · 2020-10-28T08:39:06Z

examples/kind-cluster-configuration.yaml

@@ -0,0 +1,6 @@
+kind: Cluster


nit: we should probably move this under hack ? as examples should probably contain CR examples only

I think we propose to use this file in the documentation for testing the examples.
hack folder is more for contributor.

ahmed-mez · 2020-10-28T08:41:24Z

api/v1alpha1/extendeddaemonset_types.go

+	Enabled     *bool  `json:"enabled,omitempty"`
+	MaxRestarts *int32 `json:"maxRestarts,omitempty"`


could we please document these fields? especially MaxRestarts, we should make it clear that it corresponds to the max number of restarts per canary pod, not the sum of restarts of all canary pods

github-actions

This pull request contains a valid label.

controllers/extendeddaemonset/controller.go

github-actions

This pull request contains a valid label.

github-actions

This pull request contains a valid label.

codecov-io · 2020-10-29T21:20:25Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d72a1bc). Click here to learn what that means.
The diff coverage is 46.66%.

@@            Coverage Diff            @@
##             master      #51   +/-   ##
=========================================
  Coverage          ?   36.87%           
=========================================
  Files             ?       29           
  Lines             ?     1589           
  Branches          ?        0           
=========================================
  Hits              ?      586           
  Misses            ?      929           
  Partials          ?       74

Flag	Coverage Δ
unittests	`36.87% <46.66%> (?)`

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

Impacted Files	Coverage Δ
...ers/extendeddaemonsetreplicaset/strategy/canary.go	`0.00% <0.00%> (ø)`
controllers/extendeddaemonset/controller.go	`68.75% <42.85%> (ø)`
pkg/controller/utils/pod/pod.go	`16.43% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d72a1bc...76d421b. Read the comment docs.

ahmed-mez

looks great! just few comments

ahmed-mez · 2020-10-30T13:13:39Z

README.md

+- `replicas`: The number of replica pods to participate in the Canary deployment
+- `duration`: The duration of the Canary deployment, after which the Canary deployment will end and the active ExtendedReplicaSet will update
+- `autoPause.enabled`: Activation of the Canary deployment auto pausing feature (default is `true`)
+- `autoPause.maxRestarts`: The maximum number of restarts tolerable before the Canary deployment is automatically paused (default is `2`)


let's clarify that it corresponds to the max number of restarts per pod

ahmed-mez · 2020-10-30T13:15:14Z

README.md

+#### Validate Canary deployment
+
+As an alternative to waiting for the Canary duration to end, the deployment can be manually validated.
+
+`kubectl-eds canary validate <ExtendedDaemonSet name>`
+
+#### Pause Canary deployment
+
+The Canary deployment can be paused to investigate an issue.
+
+`kubectl-eds canary pause <ExtendedDaemonSet name>`
+
+#### Unpause Canary deployment
+
+The Canary deployment can be unpaused, and the Canary duration will continue.
+
+`kubectl-eds canary unpause <ExtendedDaemonSet name>`
+
+#### Fail Canary deployment
+
+The Canary deployment can be manually failed. This command will restore the currently active ExtendedReplicaSet on the Canary pods.
+
+`kubectl-eds canary fail <ExtendedDaemonSet name>`
+
+#### Reset Canary deployment
+
+Following failure of the Canary deployment, the `fail` annotation should be reset with this command.
+
+`kubectl-eds canary reset <ExtendedDaemonSet name>`


thanks that's super useful

controllers/extendeddaemonsetreplicaset/strategy/canary.go

pkg/controller/utils/pod/pod.go

github-actions

This pull request contains a valid label.

github-actions

This pull request contains a valid label.

github-actions

This pull request contains a valid label.

add parameter to configure max restarts for canary pause

10a7a4c

celenechang added this to the v0.4 milestone Oct 20, 2020

celenechang requested a review from a team as a code owner October 20, 2020 13:46

github-actions bot requested changes Oct 20, 2020

View reviewed changes

celenechang added the enhancement New feature or request label Oct 20, 2020

github-actions bot approved these changes Oct 20, 2020

View reviewed changes

ahmed-mez reviewed Oct 20, 2020

View reviewed changes

pkg/controller/utils/pod/pod.go Show resolved Hide resolved

celenechang marked this pull request as draft October 20, 2020 14:17

clamoriniere reviewed Oct 26, 2020

View reviewed changes

add new ExtendedDaemonSetSpecStrategyCanaryAutoPause struct with enab…

9ce973e

…led param

github-actions bot approved these changes Oct 27, 2020

View reviewed changes

celenechang marked this pull request as ready for review October 27, 2020 03:31

update README

509a49c

github-actions bot approved these changes Oct 27, 2020

View reviewed changes

update generated files

fa47e7e

github-actions bot approved these changes Oct 27, 2020

View reviewed changes

readme typo, try updating generated files again

17aade8

github-actions bot approved these changes Oct 27, 2020

View reviewed changes

ahmed-mez reviewed Oct 28, 2020

View reviewed changes

fix bug, update defaults, add docstring

1d2a24b

github-actions bot approved these changes Oct 29, 2020

View reviewed changes

celenechang commented Oct 29, 2020

View reviewed changes

controllers/extendeddaemonset/controller.go Show resolved Hide resolved

fix defaults

bb88c74

github-actions bot approved these changes Oct 29, 2020

View reviewed changes

resolve test issue

39afb7a

github-actions bot approved these changes Oct 29, 2020

View reviewed changes

ahmed-mez reviewed Oct 30, 2020

View reviewed changes

address review, add test

520f4dc

github-actions bot approved these changes Nov 2, 2020

View reviewed changes

fix lint issue

76d421b

github-actions bot approved these changes Nov 2, 2020

View reviewed changes

reorder test list

44c516c

github-actions bot approved these changes Nov 2, 2020

View reviewed changes

ahmed-mez approved these changes Nov 2, 2020

View reviewed changes

celenechang merged commit 738e5f9 into master Nov 2, 2020

celenechang deleted the celene/configure_pause_max_restarts branch November 2, 2020 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add parameter to configure max restarts for canary pause #51

add parameter to configure max restarts for canary pause #51

celenechang commented Oct 20, 2020 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

clamoriniere left a comment

clamoriniere Oct 26, 2020

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

ahmed-mez Oct 28, 2020

ahmed-mez Oct 28, 2020

celenechang Oct 29, 2020

ahmed-mez Oct 29, 2020

clamoriniere Oct 29, 2020

celenechang Oct 29, 2020 •

edited

Loading

celenechang Oct 29, 2020

ahmed-mez Oct 28, 2020

clamoriniere Oct 28, 2020 •

edited

Loading

ahmed-mez Oct 28, 2020

ahmed-mez Oct 28, 2020

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

codecov-io commented Oct 29, 2020 •

edited

Loading

ahmed-mez left a comment

ahmed-mez Oct 30, 2020

ahmed-mez Oct 30, 2020

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

		Enabled *bool `json:"enabled,omitempty"`
		MaxRestarts *int32 `json:"maxRestarts,omitempty"`

add parameter to configure max restarts for canary pause #51

add parameter to configure max restarts for canary pause #51

Conversation

celenechang commented Oct 20, 2020 • edited Loading

What does this PR do?

Motivation

Additional Notes

Describe your test plan

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

clamoriniere left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a 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

celenechang Oct 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamoriniere Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 29, 2020 • edited Loading

Codecov Report

ahmed-mez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

celenechang commented Oct 20, 2020 •

edited

Loading

celenechang Oct 29, 2020 •

edited

Loading

clamoriniere Oct 28, 2020 •

edited

Loading

codecov-io commented Oct 29, 2020 •

edited

Loading