Skip to content

[FR-126] Rollback #274

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
eniko-dif wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[FR-126] Rollback #274

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8459413
Bump the Temporal API version to query LastCurrentTime
eniko-dif Apr 9, 2026
01d39f9
Implement Rollback
eniko-dif Apr 14, 2026
e990101
Implement max version age
eniko-dif Apr 20, 2026
9f5ba18
Add warning message if rollback is set slower than rollout
eniko-dif Apr 20, 2026
0ff8504
Update the docs
eniko-dif Apr 20, 2026
371578c
Address Jacob's PR comments
eniko-dif Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 75 additions & 24 deletions api/v1alpha1/temporalworker_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ func (s *TemporalWorkerDeploymentSpec) Default(ctx context.Context) error {
s.SunsetStrategy.DeleteDelay = &v1.Duration{Duration: defaults.DeleteDelay}
}

if s.RollbackStrategy == nil {
s.RollbackStrategy = &RollbackStrategy{Strategy: RollbackAllAtOnce}
Comment thread
jlegrone marked this conversation as resolved.
} else if s.RollbackStrategy.Strategy == "" {
s.RollbackStrategy.Strategy = RollbackAllAtOnce
}
Comment on lines +60 to +64
Copy link
Copy Markdown
Collaborator

@jlegrone jlegrone May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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
}

if s.RollbackStrategy.MaxVersionAge == nil {
s.RollbackStrategy.MaxVersionAge = &v1.Duration{Duration: defaults.RollbackMaxVersionAge}
}
return nil
}

Expand Down Expand Up @@ -94,41 +102,20 @@ func validateForUpdateOrCreate(old, new *TemporalWorkerDeployment) (admission.Wa
}

allErrs = append(allErrs, validateRolloutStrategy(new.Spec.RolloutStrategy)...)
allErrs = append(allErrs, validateRollbackStrategy(*new.Spec.RollbackStrategy)...)
Copy link
Copy Markdown
Collaborator

@jlegrone jlegrone May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could new.Spec.RollbackStrategy ever be nil? Should probably add a nil check here or update the type signature for validateRollbackStrategy instead of dereferencing.


if len(allErrs) > 0 {
return nil, newInvalidErr(new, allErrs)
}

return nil, nil
return warnRollbackSlowerThanRollout(new.Spec.RolloutStrategy, *new.Spec.RollbackStrategy), nil
}

func validateRolloutStrategy(s RolloutStrategy) []*field.Error {
var allErrs []*field.Error

if s.Strategy == UpdateProgressive {
rolloutSteps := s.Steps
if len(rolloutSteps) == 0 {
allErrs = append(allErrs,
field.Invalid(field.NewPath("spec.rollout.steps"), rolloutSteps, "steps are required for Progressive rollout"),
)
}
var lastRamp int
for i, s := range rolloutSteps {
// Check duration >= 30s
if s.PauseDuration.Duration < 30*time.Second {
allErrs = append(allErrs,
field.Invalid(field.NewPath(fmt.Sprintf("spec.rollout.steps[%d].pauseDuration", i)), s.PauseDuration.Duration.String(), "pause duration must be at least 30s"),
)
}

// Check ramp value greater than last
if s.RampPercentage <= lastRamp {
allErrs = append(allErrs,
field.Invalid(field.NewPath(fmt.Sprintf("spec.rollout.steps[%d].rampPercentage", i)), s.RampPercentage, "rampPercentage must increase between each step"),
)
}
lastRamp = s.RampPercentage
}
allErrs = append(allErrs, validateProgressiveStrategySteps("spec.rollout.steps", s.Steps)...)
}

// Validate gate input fields
Expand All @@ -155,6 +142,70 @@ func validateRolloutStrategy(s RolloutStrategy) []*field.Error {
return allErrs
}

func validateRollbackStrategy(s RollbackStrategy) []*field.Error {
var allErrs []*field.Error
if s.Strategy == RollbackProgressive {
allErrs = append(allErrs, validateProgressiveStrategySteps("spec.rollback.steps", s.Steps)...)
}
return allErrs
}

func warnRollbackSlowerThanRollout(rollout RolloutStrategy, rollback RollbackStrategy) admission.Warnings {
switch rollout.Strategy {
case UpdateAllAtOnce:
if rollback.Strategy != RollbackAllAtOnce {
return admission.Warnings{"rollback strategy is slower than rollout: rollout is AllAtOnce, but rollback is Progressive — is that intended?"}
}
case UpdateProgressive:
if rollback.Strategy == RollbackProgressive {
var rolloutTotal, rollbackTotal time.Duration
for _, s := range rollout.Steps {
rolloutTotal += s.PauseDuration.Duration
}
for _, s := range rollback.Steps {
rollbackTotal += s.PauseDuration.Duration
}
if rollbackTotal > rolloutTotal {
return admission.Warnings{fmt.Sprintf(
"rollback strategy is slower than rollout: progressive rollback total duration (%s) exceeds progressive rollout total duration (%s) — is that intended?",
rollbackTotal, rolloutTotal,
)}
}
}
}
return nil
}

func validateProgressiveStrategySteps(specName string, steps []RolloutStep) []*field.Error {
var allErrs []*field.Error

if len(steps) == 0 {
allErrs = append(allErrs,
field.Invalid(field.NewPath(specName), steps, "steps are required for Progressive strategy"),
)
}

var lastRamp int
for i, step := range steps {
// Check duration >= 30s
if step.PauseDuration.Duration < 30*time.Second {
allErrs = append(allErrs,
field.Invalid(field.NewPath(fmt.Sprintf("%s[%d].pauseDuration", specName, i)), step.PauseDuration.Duration.String(), "pause duration must be at least 30s"),
)
}

// Check ramp value greater than last
if step.RampPercentage <= lastRamp {
allErrs = append(allErrs,
field.Invalid(field.NewPath(fmt.Sprintf("%s[%d].rampPercentage", specName, i)), step.RampPercentage, "rampPercentage must increase between each step"),
)
}
lastRamp = step.RampPercentage
}

return allErrs
}

func newInvalidErr(dep *TemporalWorkerDeployment, errs field.ErrorList) *apierrors.StatusError {
return apierrors.NewInvalid(dep.GroupVersionKind().GroupKind(), dep.GetName(), errs)
}
178 changes: 171 additions & 7 deletions api/v1alpha1/temporalworker_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
temporaliov1alpha1 "github.com/temporalio/temporal-worker-controller/api/v1alpha1"
"github.com/temporalio/temporal-worker-controller/internal/defaults"
"github.com/temporalio/temporal-worker-controller/internal/testhelpers"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -23,8 +24,9 @@ func TestTemporalWorkerDeployment_ValidateCreate(t *testing.T) {
tests := map[string]struct {
obj runtime.Object
errorMsg string
warnMsg string
Copy link
Copy Markdown
Collaborator

@jlegrone jlegrone May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}{
"valid temporal worker deployment": {
"valid default temporal worker deployment": {
obj: testhelpers.MakeTWDWithName("valid-worker", ""),
},
"temporal worker deployment with name too long": {
Expand All @@ -39,15 +41,15 @@ func TestTemporalWorkerDeployment_ValidateCreate(t *testing.T) {
},
errorMsg: "expected a TemporalWorkerDeployment",
},
"missing rollout steps": {
"rollout strategy - invalid Progressive without steps": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("prog-rollout-missing-steps", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RolloutStrategy.Strategy = temporaliov1alpha1.UpdateProgressive
obj.Spec.RolloutStrategy.Steps = nil
return obj
}),
errorMsg: "spec.rollout.steps: Invalid value: null: steps are required for Progressive rollout",
errorMsg: "spec.rollout.steps: Invalid value: null: steps are required for Progressive strategy",
},
"ramp value for step <= previous step": {
"rollout strategy - invalid Progressive with non-increasing ramp": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("prog-rollout-decreasing-ramps", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RolloutStrategy.Strategy = temporaliov1alpha1.UpdateProgressive
obj.Spec.RolloutStrategy.Steps = []temporaliov1alpha1.RolloutStep{
Expand All @@ -62,7 +64,7 @@ func TestTemporalWorkerDeployment_ValidateCreate(t *testing.T) {
}),
errorMsg: "[spec.rollout.steps[2].rampPercentage: Invalid value: 9: rampPercentage must increase between each step, spec.rollout.steps[4].rampPercentage: Invalid value: 50: rampPercentage must increase between each step]",
},
"pause duration < 30s": {
"rollout strategy - invalid Progressive pause duration < 30s": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("prog-rollout-decreasing-ramps", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RolloutStrategy.Strategy = temporaliov1alpha1.UpdateProgressive
obj.Spec.RolloutStrategy.Steps = []temporaliov1alpha1.RolloutStep{
Expand All @@ -74,6 +76,107 @@ func TestTemporalWorkerDeployment_ValidateCreate(t *testing.T) {
}),
errorMsg: `spec.rollout.steps[1].pauseDuration: Invalid value: "10s": pause duration must be at least 30s`,
},
"rollback strategy - Progressive rollback with AllAtOnce rollout warns": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("rollback-progressive", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RollbackStrategy = &temporaliov1alpha1.RollbackStrategy{
Strategy: temporaliov1alpha1.RollbackProgressive,
Steps: []temporaliov1alpha1.RolloutStep{
{50, metav1.Duration{Duration: 30 * time.Second}},
},
}
return obj
}),
warnMsg: "rollback strategy is slower than rollout",
},
"rollback strategy - AllAtOnce rollback with Progressive rollout is valid": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("rollback-allat-once-prog-rollout", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RolloutStrategy = temporaliov1alpha1.RolloutStrategy{
Strategy: temporaliov1alpha1.UpdateProgressive,
Steps: []temporaliov1alpha1.RolloutStep{
{50, metav1.Duration{Duration: time.Minute}},
{75, metav1.Duration{Duration: time.Minute}},
},
}
obj.Spec.RollbackStrategy = &temporaliov1alpha1.RollbackStrategy{
Strategy: temporaliov1alpha1.RollbackAllAtOnce,
}
return obj
}),
},
"rollback strategy - Progressive rollback faster than Progressive rollout is valid": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("rollback-prog-faster", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RolloutStrategy = temporaliov1alpha1.RolloutStrategy{
Strategy: temporaliov1alpha1.UpdateProgressive,
Steps: []temporaliov1alpha1.RolloutStep{
{50, metav1.Duration{Duration: 2 * time.Minute}},
{75, metav1.Duration{Duration: 2 * time.Minute}},
},
}
obj.Spec.RollbackStrategy = &temporaliov1alpha1.RollbackStrategy{
Strategy: temporaliov1alpha1.RollbackProgressive,
Steps: []temporaliov1alpha1.RolloutStep{
{50, metav1.Duration{Duration: time.Minute}},
{75, metav1.Duration{Duration: time.Minute}},
},
}
return obj
}),
},
"rollback strategy - Progressive rollback slower than Progressive rollout warns": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("rollback-prog-slower", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RolloutStrategy = temporaliov1alpha1.RolloutStrategy{
Strategy: temporaliov1alpha1.UpdateProgressive,
Steps: []temporaliov1alpha1.RolloutStep{
{50, metav1.Duration{Duration: time.Minute}},
{75, metav1.Duration{Duration: time.Minute}},
},
}
obj.Spec.RollbackStrategy = &temporaliov1alpha1.RollbackStrategy{
Strategy: temporaliov1alpha1.RollbackProgressive,
Steps: []temporaliov1alpha1.RolloutStep{
{50, metav1.Duration{Duration: 2 * time.Minute}},
{75, metav1.Duration{Duration: 2 * time.Minute}},
},
}
return obj
}),
warnMsg: "rollback strategy is slower than rollout",
},
"rollback strategy - invalid Progressive without steps": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("rollback-progressive-no-steps", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RollbackStrategy = &temporaliov1alpha1.RollbackStrategy{
Strategy: temporaliov1alpha1.RollbackProgressive,
Steps: nil,
}
return obj
}),
errorMsg: "steps are required for Progressive strategy",
},
"rollback strategy - invalid Progressive pause duration < 30s": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("rollback-progressive-invalid", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RollbackStrategy = &temporaliov1alpha1.RollbackStrategy{
Strategy: temporaliov1alpha1.RollbackProgressive,
Steps: []temporaliov1alpha1.RolloutStep{
{50, metav1.Duration{Duration: 10 * time.Second}},
},
}
return obj
}),
errorMsg: "pause duration must be at least 30s",
},
"rollback strategy - invalid Progressive with non-increasing ramp": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("rollback-progressive-decreasing", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RollbackStrategy = &temporaliov1alpha1.RollbackStrategy{
Strategy: temporaliov1alpha1.RollbackProgressive,
Steps: []temporaliov1alpha1.RolloutStep{
{50, metav1.Duration{Duration: time.Minute}},
{25, metav1.Duration{Duration: time.Minute}},
},
}
return obj
}),
errorMsg: "rampPercentage must increase between each step",
},
}

for name, tc := range tests {
Expand All @@ -89,8 +192,12 @@ func TestTemporalWorkerDeployment_ValidateCreate(t *testing.T) {
require.NoError(t, err)
}

// Warnings should always be nil for this implementation
assert.Nil(t, warnings)
if tc.warnMsg != "" {
require.NotEmpty(t, warnings)
assert.Contains(t, warnings[0], tc.warnMsg)
} else {
assert.Empty(t, warnings)
}
}

// Verify that create and update enforce the same rules
Expand Down Expand Up @@ -168,6 +275,63 @@ func TestTemporalWorkerDeployment_Default(t *testing.T) {
assert.Equal(t, 24*time.Hour, obj.Spec.SunsetStrategy.DeleteDelay.Duration)
},
},
"rollback strategy initialized when nil": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("default-rollback-nil", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RollbackStrategy = nil
return obj
}),
expected: func(t *testing.T, obj *temporaliov1alpha1.TemporalWorkerDeployment) {
require.NotNil(t, obj.Spec.RollbackStrategy, "expected RollbackStrategy to be initialized by webhook")
assert.Equal(t, temporaliov1alpha1.RollbackAllAtOnce, obj.Spec.RollbackStrategy.Strategy, "expected RollbackStrategy.Strategy to default to AllAtOnce")
require.NotNil(t, obj.Spec.RollbackStrategy.MaxVersionAge)
assert.Equal(t, defaults.RollbackMaxVersionAge, obj.Spec.RollbackStrategy.MaxVersionAge.Duration)
},
},
"rollback strategy defaults empty strategy field to AllAtOnce": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("default-rollback-empty", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RollbackStrategy = &temporaliov1alpha1.RollbackStrategy{
Strategy: "",
}
return obj
}),
expected: func(t *testing.T, obj *temporaliov1alpha1.TemporalWorkerDeployment) {
require.NotNil(t, obj.Spec.RollbackStrategy)
assert.Equal(t, temporaliov1alpha1.RollbackAllAtOnce, obj.Spec.RollbackStrategy.Strategy, "expected RollbackStrategy.Strategy to default to AllAtOnce")
require.NotNil(t, obj.Spec.RollbackStrategy.MaxVersionAge)
assert.Equal(t, defaults.RollbackMaxVersionAge, obj.Spec.RollbackStrategy.MaxVersionAge.Duration)
},
},
"rollback strategy preserves explicit strategy": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("explicit-rollback-progressive", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RollbackStrategy = &temporaliov1alpha1.RollbackStrategy{
Strategy: temporaliov1alpha1.RollbackProgressive,
Steps: []temporaliov1alpha1.RolloutStep{
{50, metav1.Duration{Duration: 30 * time.Second}},
},
}
return obj
}),
expected: func(t *testing.T, obj *temporaliov1alpha1.TemporalWorkerDeployment) {
require.NotNil(t, obj.Spec.RollbackStrategy)
assert.Equal(t, temporaliov1alpha1.RollbackProgressive, obj.Spec.RollbackStrategy.Strategy, "expected RollbackStrategy.Strategy to remain Progressive")
require.NotNil(t, obj.Spec.RollbackStrategy.MaxVersionAge)
assert.Equal(t, defaults.RollbackMaxVersionAge, obj.Spec.RollbackStrategy.MaxVersionAge.Duration)
},
},
"rollback strategy preserves explicit MaxVersionAge": {
obj: testhelpers.ModifyObj(testhelpers.MakeTWDWithName("explicit-rollback-max-version-age", ""), func(obj *temporaliov1alpha1.TemporalWorkerDeployment) *temporaliov1alpha1.TemporalWorkerDeployment {
obj.Spec.RollbackStrategy = &temporaliov1alpha1.RollbackStrategy{
Strategy: temporaliov1alpha1.RollbackAllAtOnce,
MaxVersionAge: &metav1.Duration{Duration: 30 * time.Minute},
}
return obj
}),
expected: func(t *testing.T, obj *temporaliov1alpha1.TemporalWorkerDeployment) {
require.NotNil(t, obj.Spec.RollbackStrategy)
require.NotNil(t, obj.Spec.RollbackStrategy.MaxVersionAge)
assert.Equal(t, 30*time.Minute, obj.Spec.RollbackStrategy.MaxVersionAge.Duration, "expected explicit MaxVersionAge to be preserved")
},
},
}

for name, tc := range tests {
Expand Down
Loading