[FR-126] Rollback#274
Conversation
|
|
| } | ||
|
|
||
| if config.RollbackStrategy.MaxVersionAge != nil && time.Since(*targetVersionInfo.LastCurrentTime) > config.RollbackStrategy.MaxVersionAge.Duration { | ||
| l.Info("Skipping rollback: the version's last current time exceeds MaxVersionAge", |
There was a problem hiding this comment.
| l.Info("Skipping rollback: the version's last current time exceeds MaxVersionAge", | |
| l.Debug("Skipping rollback: the version's last current time exceeds MaxVersionAge", |
There was a problem hiding this comment.
The logger has no Debug level, only Info and Error. (If I remember correctly, it was related to K8s events?)
| } | ||
|
|
||
| allErrs = append(allErrs, validateRolloutStrategy(new.Spec.RolloutStrategy)...) | ||
| allErrs = append(allErrs, validateRollbackStrategy(*new.Spec.RollbackStrategy)...) |
There was a problem hiding this comment.
Could new.Spec.RollbackStrategy ever be nil? Should probably add a nil check here or update the type signature for validateRollbackStrategy instead of dereferencing.
|
|
||
| // MaxVersionAge limits which versions are eligible as rollback targets. | ||
| // A version is only considered a rollback target if it was last current within this duration. | ||
| // If nil, there is no age limit. |
There was a problem hiding this comment.
The default applied in the webhook is 1h though, so this comment doesn't match current behavior. We should also document the meaning of setting to 0.
Right now I don't think there is a way to achieve the "no age limit" behavior, beyond setting a very high value? (I think this is fine, but just clarifying since it seems like you can either specify a limit or disable rollback entirely by setting to 0).
| if s.RollbackStrategy == nil { | ||
| s.RollbackStrategy = &RollbackStrategy{Strategy: RollbackAllAtOnce} | ||
| } else if s.RollbackStrategy.Strategy == "" { | ||
| s.RollbackStrategy.Strategy = RollbackAllAtOnce | ||
| } |
There was a problem hiding this comment.
Nit: this is entirely a matter of taste but I think two if statements is easier to read than the if/else flow (more clear that the strategy is always all at once if left unset).
| if s.RollbackStrategy == nil { | |
| s.RollbackStrategy = &RollbackStrategy{Strategy: RollbackAllAtOnce} | |
| } else if s.RollbackStrategy.Strategy == "" { | |
| s.RollbackStrategy.Strategy = RollbackAllAtOnce | |
| } | |
| if s.RollbackStrategy == nil { | |
| s.RollbackStrategy = &RollbackStrategy{} | |
| } | |
| if s.RollbackStrategy.Strategy == "" { | |
| s.RollbackStrategy.Strategy = RollbackAllAtOnce | |
| } |
| default: | ||
| strategy = temporaliov1alpha1.UpdateAllAtOnce |
There was a problem hiding this comment.
🤔 I think this ought to be an error condition; if the controller sees an unknown strategy it should fall back to manual mode and stop attempting updates to the default version.
| }, | ||
| } | ||
|
|
||
| testCases := []struct { |
There was a problem hiding this comment.
I think there should be a test cases for
1.RollbackStrategy == nil (even though the webhook should set a default, it's still possible to run the controller without the webhook enabled). When nil I would assert that the behavior matches the default rollback strategy configured by the webhook.
2. RollbackStrategy.MaxVersionAge == nil (assert default max version age applied, again to match the default webhook behavior)
3. RollbackStrategy.MaxVersionAge == 0 (assert false, even when the rest of the current status would normally have resulted in a rollback)
| @@ -23,8 +24,9 @@ func TestTemporalWorkerDeployment_ValidateCreate(t *testing.T) { | |||
| tests := map[string]struct { | |||
| obj runtime.Object | |||
| errorMsg string | |||
| warnMsg string | |||
There was a problem hiding this comment.
Should there be a warning emitted for a combination of Manual rollout + Progressive rollback? That seems in the same vein as rollout steps being faster than rollback steps. Please add a test case for that if so.
eniko-dif
force-pushed
the
eniko/feature-rollback
branch
from
371578c to
a5d1e33
Compare
3 participants
What was changed
Add a separate rollback config to the deployment specification that can be used to have different rollback behavior from the rollout one. It supports AllAtOnce or Progressive and does not have a gate. Uses the LastCurrentTime of a deployment to decide if the target version supposed to be rolled out or rolled back.
Why?
Open feature issue: #126
Checklist
Closes [Feature Request] Rollback mode #126
How was this tested:
Unit and integration tests (ongoing actual test with local setup).
Yes, but needs to be decided where.