Add ERS canary NoRestartDuration and AutoFail configuration options to complement AutoPause behavior #66
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Codecov Report
@@ Coverage Diff @@
## master #66 +/- ##
==========================================
- Coverage 36.75% 34.71% -2.04%
==========================================
Files 29 29
Lines 1570 1688 +118
==========================================
+ Hits 577 586 +9
- Misses 919 1028 +109
Partials 74 74
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
…is exceeded TODO - CR validation: - Check that autoFail.enabled = true if canary.noRestartsDuration != nil - Check that autoFail.maxRestarts > autoPause.maxRestarts
xornivore
changed the title
xornivore
force-pushed
the
xornivore/no-restart-duration
branch
from
fb07462
to
b5c4838
Compare
xornivore
force-pushed
the
xornivore/no-restart-duration
branch
from
b5c4838
to
a18451c
Compare
- Extract `manageCanaryPodRestarts` to be separate from the rest of the pod counting logic. - Unit tests - Clarify comment for `MaxRestarts` that it is taken per pod. - Validate `AutoFail.MaxRestarts`
xornivore
force-pushed
the
xornivore/no-restart-duration
branch
from
a18451c
to
f852e00
Compare
| result.NewStatus.Desired = desiredPods | ||
| result.NewStatus.Ready = readyPods | ||
| result.NewStatus.Available = availablePods | ||
| result.NewStatus.Current = currentPods | ||
|
|
There was a problem hiding this comment.
I think we don't update the result.NewStatus.Reason with the info: Paused, Failed
There was a problem hiding this comment.
So while there is currently no result.NewStatus.Reason there is result.NewStatus.Status which is updated to:
canaryfor active and paused canaries.canary-failedfor failed canaries.
type ExtendedDaemonSetReplicaSetStatus struct {
Status string `json:"status"`
Desired int32 `json:"desired"`
Current int32 `json:"current"`
Ready int32 `json:"ready"`
Available int32 `json:"available"`
IgnoredUnresponsiveNodes int32 `json:"ignoredUnresponsiveNodes"`
// Conditions Represents the latest available observations of a DaemonSet's current state.
// +listType=map
// +listMapKey=type
Conditions []ExtendedDaemonSetReplicaSetCondition `json:"conditions,omitempty"`
}
We interpret status as an enumerable field today (as opposed to a freeform description). Example where it is being taken into consideration as such: https://github.com/DataDog/extendeddaemonset/blob/master/controllers/extendeddaemonsetreplicaset/controller.go#L183
clamoriniere
approved these changes
Nov 30, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds new features to handle restarts in the canary phase:
NoRestartsDurationfor canary preventing end of the canary phase until there are no restarts within this time.AutoFailbehaving similarly to pause but causing canary to be failed either onMaxRestartsor on exceedingMaxRestartsDurationfor pod restarts.Additional Notes
Status.Conditionsare used to record the last restart of an ERS pod and progressively track the difference between initial transition and update times.A future TODO - adding more pod termination statuses recorded as reason for canary pause or fail statuses, currently only
CrashLoopBackOffandOOMKilledare captured, the rest is mapped toUnknown.There's quite a bit of refactoring of existing canary management logic to make it more testable with unit tests. Hoping reviewers can critique the changes and advise on possible improvements.
Describe your test plan
For
NoRestartsDuration- configure a canary with a pod that is causing restarts within no restart duration, make sure that canary phase is not ended until this duration elapses.For
AutoFail- configure it with eitherMaxRestartsorMaxRestartsDurationand make sure the transition tocanary-failedstate takes place.