KEP-20, Part 1: Extend admission webhook and implement `Instance.PlanExecution` #1188

zen-dog · 2019-12-16T15:38:56Z

This is part one of the KEP-20 implementation.
tl;dr:

added new Instance.Spec.PlanExecution field that is populated (holding plan name) by the admission webhook either:
- directly by the user through a manual plan trigger or
- indirectly by the instance admission webhook on parameter updates or upgrades
PlanExecution field is ignored by the instance controller (yet), the later still has the old logic to decide what to do next. This will be part of part two of the implementation.
admission webhook (instance_admission.go) was extended with a set of rules guarding conflicting direct and indirectly triggered plans in combination with upgrades. See this beautiful (!) table and this test for more details. If an instance update is invalid, an error (status code and error message) will be returned to the user
KEP-20 was extended to reflect current implementation approach

Co-authored-by: alenkacz avarkockova@mesosphere.com

pkg/apis/kudo/v1beta1/instance_types.go

Committed to start a discussion on the PE API.

All tests are green and the rules implemented. However, the implementation became complex and brittle.

- added a table explaining major compatibility rules - refactored `validateUpdate` method to be more straightforward and readable

zen-dog · 2020-01-20T14:18:21Z

pkg/apis/kudo/v1beta1/instance_types_helpers.go

@@ -90,3 +94,47 @@ func wasRunAfter(p1 PlanStatus, p2 PlanStatus) bool {
 	}
 	return p1.LastFinishedRun.Time.After(p2.LastFinishedRun.Time)
 }
+
+// GetParamDefinitions retrieves parameter metadata from OperatorVersion CRD
+func GetParamDefinitions(params map[string]string, ov *OperatorVersion) []Parameter {


These were simply moved from the instance_controller.go since they're now used in two places.

zen-dog · 2020-01-20T14:20:46Z

pkg/apis/kudo/v1beta1/instance_admission_test.go

+	upgraded := idle.DeepCopy()
+	upgraded.Spec.OperatorVersion = v1.ObjectReference{Name: "foo-operator-2.0"}
+
+	tests := []struct {


These tests cover all the rules defined in the instance_admission.go webhook.

nice tests 👏

zen-dog · 2020-01-20T14:22:00Z

pkg/apis/kudo/v1beta1/instance_admission.go

@@ -0,0 +1,239 @@
+package v1beta1


You're better off switch to the Unified diff view for this file.

alenkacz

Okay, done with the first pass :) This looks good, love the documentation and tests. I have couple of questions, but I think this is very close to being merge-able :) 👏

config/crds/kudo.dev_instances.yaml

pkg/apis/kudo/v1beta1/instance_admission.go

alenkacz · 2020-01-21T09:10:32Z

pkg/apis/kudo/v1beta1/instance_admission.go

+}
+
+// resetInstanceStatus clears Instance.Status to effectively restart existing plan
+func resetInstanceStatus(instance *Instance) error {


hmm does this really belong to webhook? 🤔 I kind of thought obnly controller modifies status

It certainly does not. It was meant as a place-holder for now - I wasn't even sure if to implement it in this PR or in the next one.

pkg/apis/kudo/v1beta1/instance_admission.go

alenkacz · 2020-01-21T09:32:40Z

pkg/apis/kudo/v1beta1/instance_admission.go

+	case isParameterUpdate:
+		// if the same plan is triggered by the update, we clean the Instance.Status to effectively restart the plan
+		if *triggeredPlan == oldPlan {
+			if err := resetInstanceStatus(old); err != nil {


I wonder if one even can update status in the hook, I hope yes. I mean in controller it's two requests... I hope that API server does not ignore those updates. I guess we could try it out :)

alenkacz · 2020-01-21T09:34:25Z

pkg/apis/kudo/v1beta1/instance_admission_test.go

+	upgraded := idle.DeepCopy()
+	upgraded.Spec.OperatorVersion = v1.ObjectReference{Name: "foo-operator-2.0"}
+
+	tests := []struct {


nice tests 👏

pkg/apis/kudo/v1beta1/instance_admission.go

alenkacz

I think this looks great, left just couple of nits.

Just a general note - if we merge this right now, is master still releasable with this? I was at one point thinking it isn't because it's using fields controller is not interpreting but now I am actually starting to feel like it IS releasable since controller will still do the right thing even with this logic so 👍

cmd/manager/main.go

config/crds/kudo.dev_instances.yaml

pkg/webhook/instance_admission.go

alenkacz · 2020-01-28T09:59:38Z

pkg/webhook/instance_admission.go

+// - <nil> when there is no change to an existing scheduled plan
+// - '' empty string when an existing plan should be canceled (not implemented yet)
+// - 'newPlan' some new plan that should be triggered
+func validateUpdate(old, new *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion) (*string, error) {


I just realized few things about this PR while working on something in controller-runtime :)

this should not be called validate as we're calling reset status in between

also if you want to reset status, you need to transform the webhook from validating to mutating in the webhook config

perhaps this PR should go in without the reset and the rest should be in another PR?

I don't mind, resetInstanceStatus still returns nil here.

…tating Instance

…Execution` (#1188) This is part one of the KEP-20 implementation. **tl;dr**: - added new `Instance.Spec.PlanExecution` field that is populated (holding plan name) by the admission webhook either: - directly by the user through a manual plan trigger or - indirectly by the instance admission webhook on parameter updates or upgrades - `PlanExecution` field is ignored by the instance controller (yet), the later still has the old logic to decide what to do next. This will be part of part two of the implementation. - admission webhook (`instance_admission.go`) was extended with a set of rules guarding conflicting direct and indirectly triggered plans in combination with upgrades. See this beautiful (!) [table](https://github.com/kudobuilder/kudo/pull/1188/files#diff-3ee7e3fb450b5a364e3d9139fb154ad2R78) and this [test](https://github.com/kudobuilder/kudo/pull/1188/files#diff-c3a36ee52a0157a756e794446d2f4e6bR67) for more details. If an instance update is invalid, an error (status code and error message) will be returned to the user - KEP-20 was extended to reflect current implementation approach Co-authored-by: alenkacz <avarkockova@mesosphere.com> Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

…Execution` (#1188) This is part one of the KEP-20 implementation. **tl;dr**: - added new `Instance.Spec.PlanExecution` field that is populated (holding plan name) by the admission webhook either: - directly by the user through a manual plan trigger or - indirectly by the instance admission webhook on parameter updates or upgrades - `PlanExecution` field is ignored by the instance controller (yet), the later still has the old logic to decide what to do next. This will be part of part two of the implementation. - admission webhook (`instance_admission.go`) was extended with a set of rules guarding conflicting direct and indirectly triggered plans in combination with upgrades. See this beautiful (!) [table](https://github.com/kudobuilder/kudo/pull/1188/files#diff-3ee7e3fb450b5a364e3d9139fb154ad2R78) and this [test](https://github.com/kudobuilder/kudo/pull/1188/files#diff-c3a36ee52a0157a756e794446d2f4e6bR67) for more details. If an instance update is invalid, an error (status code and error message) will be returned to the user - KEP-20 was extended to reflect current implementation approach Co-authored-by: alenkacz <avarkockova@mesosphere.com> Signed-off-by: Thomas Runyon <runyontr@gmail.com>

zen-dog requested a review from alenkacz December 16, 2019 15:38

zen-dog changed the title ~~First PlanExecution draft.~~ KEP-20: Manually triggered plan executions Dec 16, 2019

alenkacz reviewed Dec 16, 2019

View reviewed changes

pkg/apis/kudo/v1beta1/instance_types.go Show resolved Hide resolved

zen-dog force-pushed the fb/kep-20 branch 2 times, most recently from 6f03d74 to 9e6e6a3 Compare December 20, 2019 10:16

zen-dog and others added 8 commits January 7, 2020 13:38

First PlanExecution draft.

4afe110

Committed to start a discussion on the PE API.

Got rid of gomega in the test

ae14bc0

Pairing

d02533c

Fixing missing return

d805e59

Wip new validation rules

3307ce8

Push todo

187eab3

Wip instance admission part 2

0b482e2

Wip instance admission webhook pt. 3

0715c84

zen-dog force-pushed the fb/kep-20 branch from 722a1aa to 0715c84 Compare January 7, 2020 17:14

zen-dog added 3 commits January 9, 2020 15:12

Wip instance admission webhook pt. 4

fc08e53

All tests are green and the rules implemented. However, the implementation became complex and brittle.

Admission webhook final version

442a495

- added a table explaining major compatibility rules - refactored `validateUpdate` method to be more straightforward and readable

Updated generated files

44fe754

zen-dog force-pushed the fb/kep-20 branch from f803c9a to 44fe754 Compare January 13, 2020 16:38

zen-dog added 3 commits January 13, 2020 18:00

Merge branch 'master' into fb/kep-20

9a8f86d

Minor fixes after merged master

3443865

Extended KEP-20 to reflect current implementation

5580071

zen-dog commented Jan 20, 2020

View reviewed changes

zen-dog added 3 commits January 20, 2020 15:22

Updated generated files

638325e

Fixed test golden files

b34a1a2

Merge branch 'master' into fb/kep-20

b5949e2

zen-dog marked this pull request as ready for review January 20, 2020 15:14

zen-dog requested review from gerred and kensipe as code owners January 20, 2020 15:14

zen-dog requested a review from nfnt as a code owner January 20, 2020 15:14

zen-dog changed the title ~~KEP-20: Manually triggered plan executions~~ KEP-20, Part 1: Extend admission webhook and implement Instance.PlanExecution Jan 20, 2020

zen-dog requested a review from ANeumann82 January 20, 2020 15:59

alenkacz reviewed Jan 21, 2020

View reviewed changes

ANeumann82 reviewed Jan 21, 2020

View reviewed changes

pkg/apis/kudo/v1beta1/instance_admission.go Outdated Show resolved Hide resolved

pkg/apis/kudo/v1beta1/instance_admission.go Outdated Show resolved Hide resolved

zen-dog added 3 commits January 21, 2020 14:03

Moved instance admission to /pkg/webhook

ddaf372

Instance controller and webhook use the same GetOperatorVersion method

b5702d4

Decline Instance creation if the OV is missing the 'deploy' plan

c9dc8ac

alenkacz approved these changes Jan 22, 2020

View reviewed changes

cmd/manager/main.go Outdated Show resolved Hide resolved

config/crds/kudo.dev_instances.yaml Show resolved Hide resolved

pkg/webhook/instance_admission.go Show resolved Hide resolved

Added a ResetPlanStatus method (not yet used)

a0a7efd

alenkacz reviewed Jan 28, 2020

View reviewed changes

zen-dog added 4 commits January 28, 2020 13:13

Minor test improvements

863b92b

Renamed validateUpdate to admitUpdate since the method is also mu…

f6fc56d

…tating Instance

Merge branch 'master' into fb/kep-20

8c2f2c5

Making linter happy

b4dea0f

zen-dog merged commit 0ed9632 into master Jan 28, 2020

zen-dog deleted the fb/kep-20 branch January 28, 2020 14:03

ANeumann82 mentioned this pull request Feb 7, 2020

Explicitly specifying nonexistent trigger plan is silently treated as deploy #1268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-20, Part 1: Extend admission webhook and implement `Instance.PlanExecution` #1188

KEP-20, Part 1: Extend admission webhook and implement `Instance.PlanExecution` #1188

zen-dog commented Dec 16, 2019 •

edited

Loading

zen-dog Jan 20, 2020

zen-dog Jan 20, 2020

alenkacz Jan 21, 2020

zen-dog Jan 21, 2020

zen-dog Jan 20, 2020

alenkacz left a comment

alenkacz Jan 21, 2020

zen-dog Jan 21, 2020

alenkacz Jan 21, 2020

alenkacz Jan 21, 2020

alenkacz left a comment

alenkacz Jan 28, 2020

zen-dog Jan 28, 2020

KEP-20, Part 1: Extend admission webhook and implement Instance.PlanExecution #1188

KEP-20, Part 1: Extend admission webhook and implement Instance.PlanExecution #1188

Conversation

zen-dog commented Dec 16, 2019 • edited Loading

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

alenkacz 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

alenkacz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KEP-20, Part 1: Extend admission webhook and implement `Instance.PlanExecution` #1188

KEP-20, Part 1: Extend admission webhook and implement `Instance.PlanExecution` #1188

zen-dog commented Dec 16, 2019 •

edited

Loading