Skip to content
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

Jump to bottom

Allow simultaneous upgrade and parameter update #1495

Merged
merged 1 commit into from
May 7, 2020
Merged

Allow simultaneous upgrade and parameter update #1495

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 13 additions & 7 deletions pkg/webhook/instance_admission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,24 +197,28 @@ func admitUpdate(old, new *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion
}

isParameterUpdate := triggeredPlan != nil
updateIncompatibleWithUpgrade := isParameterUpdate && *triggeredPlan != kudov1beta1.DeployPlanName

// --------------------------------------------------------------------------------------------------------------------------------
// ---- Instance can have two major life-cycle phases: normal and cleanup (uninstall) phase. Different rule sets apply in both. ---
// --------------------------------------------------------------------------------------------------------------------------------
// --------------------------------------------------------------------------------------------
// --- Instance can have two major life-cycle phases: normal and cleanup (uninstall) phase. ---
// --- Different rule sets apply in both. ---
// ---------------------------------------------------------------------------------------------

// ----------------------------------
// --- Instance uninstall/cleanup ---
// ----------------------------------
// Following rules apply:
// - only 'cleanup' plan (if exists) can be scheduled in this phase
// - only the instance controller (and not the webhook or the user) can schedule 'cleanup'
// - 'cleanup' overrides any existing update/upgrade plan
// - 'cleanup' itself can *NOT* be cancelled or overridden by any other plan since the instance is being deleted
if isDeleting {
Cleanup := kudov1beta1.CleanupPlanName
cleanupOverride := oldPlan == Cleanup && newPlan != oldPlan
isCleanupOverride := oldPlan == Cleanup && newPlan != oldPlan
notCleanupScheduled := newPlan != "" && newPlan != Cleanup

switch {
case cleanupOverride:
case isCleanupOverride:
return nil, fmt.Errorf("failed to update Instance %s/%s: '%s' plan can not be cancelled or overridden by another plan since the instance is being deleted", old.Namespace, old.Name, oldPlan)
case isParameterUpdate || isUpgrade:
return nil, fmt.Errorf("failed to update Instance %s/%s: parameter update and/or upgrade is not allowed when an instance is being deleted", old.Namespace, old.Name)
Expand All @@ -225,20 +229,22 @@ func admitUpdate(old, new *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion
return nil, nil
}

// ----------------------------
// ---- Normal life-cycle -----
// ----------------------------
switch {
case hadPlan && isParameterUpdate && *triggeredPlan != oldPlan:
return nil, fmt.Errorf("failed to update Instance %s/%s: plan '%s' is scheduled (or running) and an update would trigger a different plan '%s'", old.Namespace, old.Name, oldPlan, *triggeredPlan)
case isUpgrade && hadPlan:
return nil, fmt.Errorf("failed to update Instance %s/%s: upgrade to new OperatorVersion %s while a plan '%s' is scheduled (or running) is not allowed", old.Namespace, old.Name, newOvRef, oldPlan)
case isUpgrade && isNovelPlan:
return nil, fmt.Errorf("failed to update Instance %s/%s: upgrade to new OperatorVersion %s and triggering new plan '%s' is not allowed", old.Namespace, old.Name, newOvRef, newPlan)
case isUpgrade && updateIncompatibleWithUpgrade:
return nil, fmt.Errorf("failed to update Instance %s/%s: upgrade to new OperatorVersion %s together with a parameter update triggering '%s' is not allowed", old.Namespace, old.Name, newOvRef, *triggeredPlan)
case isPlanOverride:
return nil, fmt.Errorf("failed to update Instance %s/%s: overriding currently scheduled (or running) plan '%s' with '%s' is not supported", old.Namespace, old.Name, oldPlan, newPlan)
case isPlanCancellation:
return nil, fmt.Errorf("failed to update Instance %s/%s: cancelling currently scheduled (or running) plan '%s' is not supported", old.Namespace, old.Name, oldPlan)
case isParameterUpdate && isUpgrade:
return nil, fmt.Errorf("failed to update Instance %s/%s: upgrade to new OperatorVersion %s together with a parameter update triggering '%s' is not allowed", old.Namespace, old.Name, newOvRef, *triggeredPlan)
case isParameterUpdate && isNovelPlan:
return nil, fmt.Errorf("failed to update Instance %s/%s: triggering one plan '%s' directly and through parameter update '%s' is not allowed", old.Namespace, old.Name, oldPlan, newPlan)
// this case is effectively a noop because isPlanOverride is disallowed for now. However, once plan overrides are implemented, this will be needed so don't remove.
Expand Down
27 changes: 22 additions & 5 deletions pkg/webhook/instance_admission_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ func TestValidateUpdate(t *testing.T) {
deploy := v1beta1.DeployPlanName
update := v1beta1.UpdatePlanName
cleanup := v1beta1.CleanupPlanName
backup := "backup"
empty := ""

testUUID := uuid.NewUUID()
Expand All @@ -26,7 +27,7 @@ func TestValidateUpdate(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{Name: "foo-operator", Namespace: "default"},
TypeMeta: metav1.TypeMeta{Kind: "OperatorVersion", APIVersion: "kudo.dev/v1beta1"},
Spec: v1beta1.OperatorVersionSpec{
Plans: map[string]v1beta1.Plan{deploy: {}, update: {}, cleanup: {}},
Plans: map[string]v1beta1.Plan{deploy: {}, update: {}, cleanup: {}, backup: {}},
Parameters: []v1beta1.Parameter{
{
Name: "foo",
Expand All @@ -40,6 +41,10 @@ func TestValidateUpdate(t *testing.T) {
Name: "bar",
Trigger: update,
Comment on lines 41 to 42
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually allowed? I thought an update is only performed if the OV changes, and doesn't have anything to do with param changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we established these rules. It's all a bit fuzzy and determined by some old code. 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Then we should establish some rules to remove the fuzziness. We shouldn't have legacy code that drives behavior we don't fully understand in a project that's still in beta.

},
{
Name: "backup",
Trigger: backup,
},
{
Name: "invalid",
Trigger: "missing",
Expand Down Expand Up @@ -323,27 +328,39 @@ func TestValidateUpdate(t *testing.T) {
wantErr: true,
},
{
name: "parameter update together with an upgrade IS normally NOT allowed",
name: "parameter update together with an upgrade IS NOT allowed if a plan other than deploy is triggered",
old: idle,
new: func() *v1beta1.Instance {
i := upgraded.DeepCopy()
i.Spec.Parameters = map[string]string{"foo": "newFoo"}
i.Spec.Parameters = map[string]string{"backup": "back"}
return i
}(),
ov: ov,
wantErr: true,
},
{
name: "parameter update together with an upgrade IS allowed if deploy is triggered",
old: idle,
new: func() *v1beta1.Instance {
i := upgraded.DeepCopy()
i.Spec.Parameters = map[string]string{"foo": "newFoo"}
return i
}(),
ov: ov,
wantErr: false,
want: &update,
},
{
name: "parameter update together with an upgrade IS allowed if update removes a parameter that doesn't exist in the new OV",
old: idle,
new: func() *v1beta1.Instance {
i := upgraded.DeepCopy()
delete(i.Spec.Parameters, "foo") // removing from instance parameters
delete(i.Spec.Parameters, "backup") // removing from instance parameters
return i
}(),
ov: func() *v1beta1.OperatorVersion {
o := ov.DeepCopy()
o.Spec.Parameters = o.Spec.Parameters[1:len(o.Spec.Parameters)] // "foo" is the first parameter in the array
o.Spec.Parameters = o.Spec.Parameters[:len(o.Spec.Parameters)-1] // "backup" is the last parameter in the array
return o
}(),
want: &update,
Expand Down