KEP-23: Enabling and disabling features in operators #1317

zmalik · 2020-01-29T09:10:25Z

What this PR does / why we need it:
This KEP provides a way for operator developers to enable and disable features in their KUDO operators.

Fixes #1147

Signed-off-by: Zain Malik zmalikshxil@gmail.com

ANeumann82 · 2020-01-29T10:22:55Z

I like it so far. Given the stated limitations it should accomplish the defined goal.

I see a lot of potential for improvement though, especially

Opposing Features:
- Param A == true enables Feature X and disables Feature Y
- Param A == false disables Feature Y and enables Feature X
Multiple Parameters for evaluation of the enabled/disabled status would be great, that would allow nested features to work.

I think it will be possible to extend this KEP in the future to allow the above mentioned things.

ANeumann82

Regarding the Alternatives:

Feature enablement field in the Instance doesn't really look appealing to me, for the stated reasons.
Reconcile the World/Drift detection: This would actually be my preferred way, as its a lot closer to "define an expected state and let KUDO handle the rest", but as we have multiple plans with different outcomes that may create/modify/delete overlapping resources, I think reconciliation would be a lot harder to define and implement.

alenkacz

I've left couple of comments to the KEP in general. Overall this is in line with what we discussed end of last year. I still like the FeatureTask as it fits well into the domain of operators (wrapping applications). But it's quite error prone to set this up right which is the main downside of this (you have to have param, linked to plan, that linked to task, it has to have certain values...)... I'll keep thinking about it :)

alenkacz · 2020-01-29T11:25:29Z

keps/0023-enable-disable-features.md

+
+## Summary 
+
+KUDO enables Operator developers to create Operators and develop features for them. 


maybe it's rather exposing features of the underlying applications?

SGTM
I just didn't want to use words like extend operators which would diverge from the word feature

keps/0023-enable-disable-features.md

alenkacz · 2020-01-29T11:32:49Z

keps/0023-enable-disable-features.md

+apiVersion: kudo.dev/v1beta1
+parameters:
+  - name: MIRROR_MAKER_ENABLED
+    default: "false"


as part of this we should add a linting rule that this could be only true/false? Too bad we don't have types here, because then anyone can come and change the value of this param to "whatever" and I am not sure what we do then...

linting would help with default values maybe, but when somebody comes and update with a random value that cannot be directly mapped to a boolean value, we should fail the plan.

for when we have types, if we decide to do that, it would be easier as the error could be direct to parameter type related. And not a plan failing.

change the value of this param to "whatever" and I am not sure what we do then...

that would be a FATAL_ERROR 🤷‍♀

@zmalik what do you think about having an implicit param for a toggle task? why force a number of tasks on operator dev that "must" be followed in a specific pattern?

we could have an implicit param based on the task name for mirror-maker-configuration... which automatically follows the "rules" which also doesn't need to be linted because it is correct by convention.

I propose that mirror-maker-configuration or mirror-maker-configuration-enabled based on the task name. To enable or disable is to pass -p mirror-maker-configuration=false to install/update with a default being true.

regarding default perhaps the default is defined in the Toggle task. Default enabled | disabled or Enabled: true|false

I'd prefer it to be explicit.

It keeps everything you can define with -p in the params.yaml

It gives the operator developer more freedom with regards to naming their parameters. (Which might not be a good thing ;)

It allows one to use one param on multiple toggle-tasks.

If we go with implicits, there are a couple questions:

Is this implicit variable allowed to be used somewhere else in templates?

We probably would need validation to prevent a param with the same name to be defined in params.yaml

alenkacz · 2020-01-29T11:34:02Z

keps/0023-enable-disable-features.md

+
+In case the parameter value is **false**:
+
+If there are objects to be deleted the Task will be marked as successful, once those are marked for Deletion. 


so it basically does client.Delete for all resources specified in the resources of that task? Maybe that's a better description? WDYT?

zen-dog

I think the proposed way of encapsulating the logic within a new task kind is a lesser evil 👍 At least we could deprecate it later and the impact would be limited. I'm in favor of calling it kind: Toggle as it conveys the apply/delete (on/off) semantic clearly. The fact that it is toggling an app feature is less important IMHO.

keps/0023-enable-disable-features.md

zen-dog · 2020-01-29T14:22:02Z

keps/0023-enable-disable-features.md

+apiVersion: kudo.dev/v1beta1
+parameters:
+  - name: MIRROR_MAKER_ENABLED
+    default: "false"


change the value of this param to "whatever" and I am not sure what we do then...

that would be a FATAL_ERROR 🤷‍♀

keps/0023-enable-disable-features.md

zen-dog · 2020-01-29T14:28:18Z

keps/0023-enable-disable-features.md

+
+There are two alternatives to this approach
+
+- Add feature enablement field in Instance and drop using parameters for enabling features in operators. 


I read this sentence twice but I still don't understand how it is meant. Could you please elaborate?

let me re-write with some example there, but basically its to use something with clear intention to enable a feature like Instance.spec.enabledFeatures to get a list of enabled features and drop using Instance.spec.parameters

zen-dog · 2020-01-29T14:31:58Z

keps/0023-enable-disable-features.md

+That would mean redesigning the instance updates architecture. As right now, this is the only way to updates the KUDO Instances. 
+But the impact of introducing the new field would be quite limiting as it won't provide much more than what we already have with parameters 
+
+- Reconcile the world. KUDO creates a state of an instance based on `Instance` and `OperatorVersion` objects. And on


If that were always possible, why bother with serial phases/steps/tasks at all? If you could always define the outcome state and ignore individual steps and its order then you wouldn't need KUDO.

state can include phases/steps/tasks. And reconciling a state doesn't mean to ignore the order to reach that state.

zen-dog · 2020-01-29T14:47:42Z

keps/0023-enable-disable-features.md

+
+## Alternatives
+
+There are two alternatives to this approach


Here is another alternative:
The current suggestion encapsulates the if-else logic in a new task type. However, both branches (true and false) are already covered by the Apply and Delete tasks so we might as well put the if-else logic directly into the plan like:

plans: mirror-maker-plan: strategy: serial phases: - name: mirror-maker-phase strategy: serial steps: - name: apply-mirror-maker tasks: - apply-mirror-maker-configuration - apply-mirror-maker when: (MIRROR_MAKER_ENABLED == "true") - name: delete-mirror-maker tasks: - delete-mirror-maker-configuration - delete-mirror-maker when: (MIRROR_MAKER_ENABLED == "false")

Here, e.g. apply-mirror-maker is a simple Apply and delete-mirror-maker a Delete kind of task (omited here for brevity).

Pros:

we don't need a new task type

more conditions flexibility: it's easy to extend the when to whatever we need it to be

Cons:

YAMLy programming language 🤮

hard to debug

P.S. I'm not advocating for it. I think it is awful 😂 But It is semantically the same and makes it clear that we're not in the "declarative world" anymore.

Good point for an alternative. And it sounds tempting, because it's kind of powerful, but as you said, hard to debug, and if we want programming, it shouldn't be in yaml.

added the alternative!

zen-dog · 2020-01-29T16:00:23Z

keps/0023-enable-disable-features.md

@@ -92,7 +92,7 @@ plans:
              - mirror-maker
 ```

-In the tasks is where we can use the `Feature` kind. Which will apply the resources rendered by templates `mirror-maker.yaml` and `mirror-maker-cm.yaml` in case the value of parameter `MIRROR_MAKER_ENABLED` is true. 
+If the `spec.parameter` is `"true"`, feature task acts like `Apply` task, applies the provided resources and waits for them to become healthy. If the `spec.parameter` is `"false"` it acts like a `Delete` task, deleting the resources.
 In case, the value is `false` it will delete the resources rendered by `mirror-maker.yaml` and `mirror-maker-cm.yaml`. 


wdyt:

Suggested change

In case, the value is `false` it will delete the resources rendered by `mirror-maker.yaml` and `mirror-maker-cm.yaml`.

So in the above example, if the parameter is `"false"` it will delete the resources rendered by `mirror-maker.yaml` and `mirror-maker-cm.yaml`.

Signed-off-by: Zain Malik <zmalikshxil@gmail.com>

Co-Authored-By: Aleksey Dukhovniy <adukhovniy@mesosphere.io>

Signed-off-by: Zain Malik <zmalikshxil@gmail.com>

zen-dog

Lgtm!

kensipe

I would like to discuss the value of an implicit before we merge this... this otherwise /lgtm

kensipe · 2020-01-30T15:38:01Z

keps/0023-enable-disable-features.md

+apiVersion: kudo.dev/v1beta1
+parameters:
+  - name: MIRROR_MAKER_ENABLED
+    default: "false"


@zmalik what do you think about having an implicit param for a toggle task? why force a number of tasks on operator dev that "must" be followed in a specific pattern?

we could have an implicit param based on the task name for mirror-maker-configuration... which automatically follows the "rules" which also doesn't need to be linted because it is correct by convention.

I propose that mirror-maker-configuration or mirror-maker-configuration-enabled based on the task name. To enable or disable is to pass -p mirror-maker-configuration=false to install/update with a default being true.

kensipe

Looking to have my proposal considered and added as an alternative... but this looks good as a provisional.

Nice work!!!

zmalik · 2020-01-30T16:20:20Z

@kensipe thanks for the review
I used the example of

Given a feature, `A` is composed of Kubernetes object `x`, `y` and `z`

This came down to clarify the scope of this KEP, but a feature A right now can also be kubernetes objects x, y and z and a small snippet a that belongs to object C

right now this other small snippet part is handled well by KUDO and we just add

{{ if eq .Params.MIRROR_MAKER_ENABLED "true" }}
<snippet-to-add>
{{ end }}

if its implicit it would result into something like this:

{{ if eq .Params.mirror-maker-configuration-enabled "true" }}
<snippet-to-add>
{{ end }}

As long as we support both ways this should be fine, because being implicit also implies we are supposing some default value of implicit parameter. Which will be false I guess and if somebody wants it to be default true they can just add explicit parameter
for example in case of some monitoring feature, it would make sense by default to be true

kensipe · 2020-01-30T16:26:07Z

@zmalik I do like the idea of using task or step implicits as params within the yaml... although I don't want to "stop" it... I just don't want to promote it. I think the implicits should be for a task (which is a collection of resources) or steps which is a collection of tasks. The above example is necessary based on limitations today... I believe that restructuring would eliminate the need for this. AND if needed in some cases would still be possible.

* kep: kep-23 enabling and disablig features in operators * Apply suggestions from code review * rename task feature to toggle * add the conditional expression in plan alternative * add feature definition in Motivation section Signed-off-by: Zain Malik <zmalikshxil@gmail.com> Co-authored-by: Aleksey Dukhovniy <adukhovniy@mesosphere.io> Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

* kep: kep-23 enabling and disablig features in operators * Apply suggestions from code review * rename task feature to toggle * add the conditional expression in plan alternative * add feature definition in Motivation section Signed-off-by: Zain Malik <zmalikshxil@gmail.com> Co-authored-by: Aleksey Dukhovniy <adukhovniy@mesosphere.io> Signed-off-by: Thomas Runyon <runyontr@gmail.com>

zmalik requested review from kensipe, alenkacz, gerred, ANeumann82, nfnt and zen-dog January 29, 2020 09:10

zmalik self-assigned this Jan 29, 2020

zmalik force-pushed the zain/kep-22 branch 2 times, most recently from 4f3759f to 49186c6 Compare January 29, 2020 09:44

ANeumann82 approved these changes Jan 29, 2020

View reviewed changes

alenkacz reviewed Jan 29, 2020

View reviewed changes

runyontr changed the title ~~kep: kep-23 enabling and disablig features in operators~~ kep: kep-23 enabling and disabling features in operators Jan 29, 2020

zen-dog changed the title ~~kep: kep-23 enabling and disabling features in operators~~ KEP-23: Enabling and disabling features in operators Jan 29, 2020

zen-dog added the kep/23 label Jan 29, 2020

zen-dog added this to In Progress in KUDO Global Jan 29, 2020

zen-dog reviewed Jan 29, 2020

View reviewed changes

zmalik force-pushed the zain/kep-22 branch from 6465ee9 to f2f372d Compare January 30, 2020 09:14

zmalik and others added 5 commits January 30, 2020 10:23

kep: kep-23 enabling and disablig features in operators

4fa7858

Signed-off-by: Zain Malik <zmalikshxil@gmail.com>

Apply suggestions from code review

8e4001f

Co-Authored-By: Aleksey Dukhovniy <adukhovniy@mesosphere.io>

rename task feature to toggle

10aa710

Signed-off-by: Zain Malik <zmalikshxil@gmail.com>

add the conditional expression in plan alternative

a68d36c

Signed-off-by: Zain Malik <zmalikshxil@gmail.com>

add feature definition in Motivation section

3e04789

Signed-off-by: Zain Malik <zmalikshxil@gmail.com>

zmalik force-pushed the zain/kep-22 branch from 5ba7542 to 3e04789 Compare January 30, 2020 09:23

zen-dog approved these changes Jan 30, 2020

View reviewed changes

kensipe requested changes Jan 30, 2020

View reviewed changes

kensipe approved these changes Jan 30, 2020

View reviewed changes

zmalik merged commit 21d3b6b into kudobuilder:master Jan 30, 2020

KUDO Global automation moved this from In Progress to Done Jan 30, 2020

zmalik deleted the zain/kep-22 branch January 30, 2020 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-23: Enabling and disabling features in operators #1317

KEP-23: Enabling and disabling features in operators #1317

zmalik commented Jan 29, 2020 •

edited by zen-dog

Loading

ANeumann82 commented Jan 29, 2020

ANeumann82 left a comment

alenkacz left a comment

alenkacz Jan 29, 2020

zmalik Jan 29, 2020

alenkacz Jan 29, 2020

zmalik Jan 29, 2020

zen-dog Jan 29, 2020

kensipe Jan 30, 2020

kensipe Jan 30, 2020

ANeumann82 Jan 30, 2020

alenkacz Jan 29, 2020

zen-dog left a comment •

edited

Loading

zen-dog Jan 29, 2020

zen-dog Jan 29, 2020

zmalik Jan 30, 2020

zen-dog Jan 29, 2020

zmalik Jan 30, 2020

zen-dog Jan 29, 2020

ANeumann82 Jan 30, 2020

zmalik Jan 30, 2020

zen-dog Jan 29, 2020

zen-dog left a comment

kensipe left a comment

kensipe Jan 30, 2020

kensipe left a comment

zmalik commented Jan 30, 2020

kensipe commented Jan 30, 2020


		## Summary

		KUDO enables Operator developers to create Operators and develop features for them.


		In case the parameter value is false:

		If there are objects to be deleted the Task will be marked as successful, once those are marked for Deletion.


		There are two alternatives to this approach

		- Add feature enablement field in Instance and drop using parameters for enabling features in operators.

	In case, the value is `false` it will delete the resources rendered by `mirror-maker.yaml` and `mirror-maker-cm.yaml`.
	So in the above example, if the parameter is `"false"` it will delete the resources rendered by `mirror-maker.yaml` and `mirror-maker-cm.yaml`.

KEP-23: Enabling and disabling features in operators #1317

KEP-23: Enabling and disabling features in operators #1317

Conversation

zmalik commented Jan 29, 2020 • edited by zen-dog Loading

ANeumann82 commented Jan 29, 2020

ANeumann82 left a 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

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

zen-dog left a comment • 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

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

zen-dog left a comment

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kensipe left a comment

Choose a reason for hiding this comment

zmalik commented Jan 30, 2020

kensipe commented Jan 30, 2020

zmalik commented Jan 29, 2020 •

edited by zen-dog

Loading

zen-dog left a comment •

edited

Loading