KEP-20 part 2: trigger plans manually #1352

zen-dog · 2020-02-19T18:02:29Z

Summary:

current ValidationWebhookConfiguration is replaced by MutatingWebhookConfiguration. Instance admission webhook is now setting the Spec.PlanExecution.PlanName on updates/creates
kudo-manager-instance-validation-webhook-config webhook (when active) is enforcing all rules around triggering plans directly (manually) and indirectly (through Instance updates), making sure that no conflicting plans are running
new cli command $ kubectl kudo plan trigger --name deploy --instance dummy-instance is available to trigger a plan manually
manually triggering plans is only possible when webhooks are enabled (kudo init --webhook=InstanceValidation ...)

Note:

change: Instance CRD got updated
heavily change: ValidationWebhookConfiguration from previous KUDO version (in case webhooks were used) has to be removed manually

What this PR does / why we need it:

Summary: - current `ValidationWebhookConfiguration` is replaced by `MutatingWebhookConfiguration` to allow instance admission webhook to set the `Spec.PlanExecution.PlanName` - `Spec.PlanExecution.PlanName` is now not required anymore Note: - Instance CRD got updated - breaking change: ValidationWebhookConfiguration from previous KUDO version (in case webhooks were used) has to be removed manually Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

so that when the same plan is re-triggered a new UID is generated and the controller can restart the plan. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

…instance additionally, instance controller now properly handles the cases where the webhooks are enabled and where they are disabled. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog · 2020-02-28T17:04:03Z

pkg/apis/kudo/v1beta1/instance_types_helpers.go

@@ -116,6 +128,85 @@ func (i *Instance) PlanStatus(plan string) *PlanStatus {
 	return nil
 }

+// annotateSnapshot stores the current spec of Instance into the snapshot annotation


Moved here from the instance_controller as they're needed in the webhook too.

new command `kudoctl plan trigger --name=<planNam> --instance=<instanceName>` is now available Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

ANeumann82

First pass, more to come

pkg/controller/instance/instance_controller.go

pkg/kudoctl/cmd/plan.go

pkg/kudoctl/kudoinit/prereq/webhook.go

pkg/webhook/instance_admission.go

pkg/kudoctl/util/kudo/kudo.go

ANeumann82 · 2020-03-03T17:22:30Z

pkg/controller/instance/instance_controller.go

+	// 2. Cleanup plan exists in the operator version and has *never run* before
+	// 3. Cleanup hasn't been scheduled yet (first time the deletion is being reconciled)
+	// we set the Spec.PlanExecution.PlanName = 'cleanup'
+	hasToScheduleCleanupAfterDeletion := func() bool {


Is there a specific reason that these are inline funcs? I'd rather have them outside as actual funcs or just plain code in this method.

Since they're only used once inside this method, they would otherwise "pollute" the controller namespace. I prefer to inline them to give a "speaking name" to otherwise complicated check.

Yeah, I get the "pollution" argument, although I blame go for it ;)

Still, I wouldn't mind having just the simple booleans inside this func instead of full functions that are only called once, but it's not a blocker for me. Just looks weird.

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

ANeumann82

I'm not sure if we should keep the webhooks optional now, but... This looks ok so far.

ANeumann82 · 2020-03-04T15:26:09Z

pkg/controller/instance/instance_controller.go

+	// 2. Cleanup plan exists in the operator version and has *never run* before
+	// 3. Cleanup hasn't been scheduled yet (first time the deletion is being reconciled)
+	// we set the Spec.PlanExecution.PlanName = 'cleanup'
+	hasToScheduleCleanupAfterDeletion := func() bool {


Yeah, I get the "pollution" argument, although I blame go for it ;)

Still, I wouldn't mind having just the simple booleans inside this func instead of full functions that are only called once, but it's not a blocker for me. Just looks weird.

ANeumann82 · 2020-03-04T15:34:00Z

I'd like to see at least one more review on this though. It's a big PR ;)

zen-dog · 2020-03-04T16:08:50Z

I'd like to see at least one more review on this though. It's a big PR ;)

absolutely! 💯

alenkacz

I was not able to finish the review, will come back to it tomorrow, sorry

cmd/manager/main.go

pkg/apis/kudo/v1beta1/instance_types_helpers.go

pkg/kudoctl/cmd/testdata/deploy-kudo-webhook.yaml.golden

pkg/kudoctl/kudoinit/prereq/webhook.go

alenkacz

Did my first full pass :) I need to go over the webhook logic one more.

Most comments are nits, there are two bigger things I think we should take a look at (controller logic + that namespace default)

Overall nice work 👏

cmd/manager/main.go

pkg/controller/instance/instance_controller.go

pkg/kudoctl/cmd/plan/plan_trigger.go

pkg/kudoctl/util/kudo/kudo.go

pkg/webhook/instance_admission.go

Proper namespace handling for CREATEd instances, robust detection of instances deletion life-cycle and logging improvements Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

alenkacz

👏 nice work, the logic is very complex but you did a great job documenting it!

My only bigger concern is - you cannot trigger plan when running kudo without webhooks. I was always thinkig about the whole thing in this way:

kudo without webhooks is only suitable for newcomers who want to quickly start something locally and run some very basic operator like cowsay and see what kudo is doing and play around with its features OR for developers of operators who run local and know what they are doing since no webhooks are not giving you consistency guarantees
with webhooks for ALL OTHER CASES

I think it will be misleading that you can trigger plans only for kudo running in cluster and with webhooks.

cmd/manager/main.go

pkg/kudoctl/cmd/plan.go

pkg/kudoctl/cmd/testdata/deploy-kudo-webhook.yaml.golden

pkg/kudoctl/util/kudo/kudo.go

pkg/kudoctl/util/kudo/kudo_test.go

pkg/webhook/instance_admission.go

porridge

Mostly nitpicks here and there, but the thing I'm concerned about is that we're adding a very complex component without integration tests.

cmd/manager/main.go

pkg/kudoctl/kudoinit/prereq/webhook.go

cmd/manager/main.go

pkg/apis/kudo/v1beta1/instance_types_helpers.go

pkg/kudoctl/cmd/testdata/deploy-kudo-webhook.yaml.golden

pkg/kudoctl/cmd/plan/plan_trigger.go

pkg/kudoctl/cmd/init_integration_test.go

zen-dog · 2020-03-09T14:15:29Z

I think it will be misleading that you can trigger plans only for kudo running in cluster and with webhooks.

The bottom line is: KUDO API is edge-based which suits a lot of real-world practical scenarios really good. However, the only way KUDO operations stay consistent (and e.g. avoid data loss) is by guarding them with the webhook. While it was possible to run into inconsistent behavior without manually triggering plans, the later definitely exaggerates it. I'm strongly against allowing it without webhooks.

alenkacz · 2020-03-09T14:59:38Z

I think it will be misleading that you can trigger plans only for kudo running in cluster and with webhooks.

The bottom line is: KUDO API is edge-based which suits a lot of real-world practical scenarios really good. However, the only way KUDO operations stay consistent (and e.g. avoid data loss) is by guarding them with the webhook. While it was possible to run into inconsistent behavior without manually triggering plans, the later definitely exaggerates it. I'm strongly against allowing it without webhooks.

Then please at least make it obvious that it's not allowed from CLI, not just silently ignore it. Also document it with the reasoning 🙏 thanks

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

I addressed all smaller issues and the bigger one (e2e test) will be a followup PR

Summary: - current `ValidationWebhookConfiguration` is replaced by `MutatingWebhookConfiguration`. Instance admission webhook is now setting the `Spec.PlanExecution.PlanName` on updates/creates - `kudo-manager-instance-validation-webhook-config` webhook (when active) is enforcing all rules around triggering plans directly (manually) and indirectly (through Instance updates), making sure that no conflicting plans are running - new CLI command `$ kubectl kudo plan trigger --name deploy --instance dummy-instance` is available to trigger a plan manually - manually triggering plans is only possible when webhooks are enabled (`kudo init --webhook=InstanceValidation ...`) **Notes**: - **change**: Instance CRD got updated - **heavy change**: `ValidationWebhookConfiguration` from previous KUDO version (in case webhooks were used) has to be removed manually  **What this PR does / why we need it**:  Fixes #649 #741 Signed-off-by: Thomas Runyon <runyontr@gmail.com>

zen-dog force-pushed the ad/kep-20 branch 2 times, most recently from bff7f29 to a441ee6 Compare February 21, 2020 14:46

zen-dog added 3 commits February 25, 2020 14:23

Added new Instance.Spec.PlanExecution.UID field

f66a280

so that when the same plan is re-triggered a new UID is generated and the controller can restart the plan. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Implemented webhook rules around the deletion life-cycle phase of an …

78f68b8

…instance additionally, instance controller now properly handles the cases where the webhooks are enabled and where they are disabled. Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog force-pushed the ad/kep-20 branch from a441ee6 to 78f68b8 Compare February 27, 2020 17:15

Minor fixes around cleanup life-cycle and plan termination

f5e074c

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog commented Feb 28, 2020

View reviewed changes

zen-dog added 3 commits March 2, 2020 14:13

Merge branch 'master' into ad/kep-20

0a03bc7

Added cli support for triggering plans manually

0d8b0d3

new command `kudoctl plan trigger --name=<planNam> --instance=<instanceName>` is now available Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Fixing failing cli-install-uninstall test

b33f385

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog marked this pull request as ready for review March 3, 2020 12:30

zen-dog requested review from alenkacz, gerred, kensipe and nfnt as code owners March 3, 2020 12:30

zen-dog requested review from ANeumann82 and porridge March 3, 2020 12:31

zen-dog added release-highlight release/breaking-change This PR contains breaking changes and is marked in the release notes labels Mar 3, 2020

ANeumann82 requested changes Mar 3, 2020

View reviewed changes

ANeumann82 reviewed Mar 3, 2020

View reviewed changes

pkg/kudoctl/util/kudo/kudo.go Show resolved Hide resolved

ANeumann82 reviewed Mar 3, 2020

View reviewed changes

ANeumann82 added release/highlight This PR is a highlight for the next release and removed release-highlight labels Mar 4, 2020

New tests and review feedback round one

86421a9

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

zen-dog mentioned this pull request Mar 4, 2020

Simplify instance update code #1378

Merged

Merge branch 'master' into ad/kep-20

9981637

ANeumann82 approved these changes Mar 4, 2020

View reviewed changes

alenkacz reviewed Mar 4, 2020

View reviewed changes

alenkacz reviewed Mar 5, 2020

View reviewed changes

zen-dog added 2 commits March 5, 2020 15:43

More PR review feedback

dac7bed

Proper namespace handling for CREATEd instances, robust detection of instances deletion life-cycle and logging improvements Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Merge branch 'master' into ad/kep-20

df37308

alenkacz approved these changes Mar 6, 2020

View reviewed changes

porridge previously requested changes Mar 6, 2020

View reviewed changes

zen-dog added 2 commits March 9, 2020 16:28

Better comments and webhook documentation in test/README

4be346b

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>

Merge branch 'master' into ad/kep-20

5e04f06

zen-dog merged commit c6155ca into master Mar 9, 2020

zen-dog deleted the ad/kep-20 branch March 9, 2020 19:40

porridge mentioned this pull request Mar 10, 2020

test/manitests/kudo.yaml is not regenerated from source #1406

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-20 part 2: trigger plans manually #1352

KEP-20 part 2: trigger plans manually #1352

zen-dog commented Feb 19, 2020 •

edited

Loading

zen-dog Feb 28, 2020 •

edited

Loading

ANeumann82 left a comment

ANeumann82 Mar 3, 2020

zen-dog Mar 4, 2020

ANeumann82 Mar 4, 2020

ANeumann82 left a comment

ANeumann82 Mar 4, 2020

ANeumann82 commented Mar 4, 2020

zen-dog commented Mar 4, 2020

alenkacz left a comment

alenkacz left a comment

alenkacz left a comment

porridge left a comment

zen-dog commented Mar 9, 2020

alenkacz commented Mar 9, 2020

KEP-20 part 2: trigger plans manually #1352

KEP-20 part 2: trigger plans manually #1352

Conversation

zen-dog commented Feb 19, 2020 • edited Loading

zen-dog Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

ANeumann82 left a comment

Choose a reason for hiding this comment

ANeumann82 Mar 3, 2020

Choose a reason for hiding this comment

zen-dog Mar 4, 2020

Choose a reason for hiding this comment

ANeumann82 Mar 4, 2020

Choose a reason for hiding this comment

ANeumann82 left a comment

Choose a reason for hiding this comment

ANeumann82 Mar 4, 2020

Choose a reason for hiding this comment

ANeumann82 commented Mar 4, 2020

zen-dog commented Mar 4, 2020

alenkacz left a comment

Choose a reason for hiding this comment

alenkacz left a comment

Choose a reason for hiding this comment

alenkacz left a comment

Choose a reason for hiding this comment

porridge left a comment

Choose a reason for hiding this comment

zen-dog commented Mar 9, 2020

alenkacz commented Mar 9, 2020

zen-dog commented Feb 19, 2020 •

edited

Loading

zen-dog Feb 28, 2020 •

edited

Loading