KEP for adding toggle behaviour to KudoOperator #1794

asekretenko · 2021-05-05T16:16:23Z

Signed-off-by: Andrei Sekretenko [email protected]

Signed-off-by: Andrei Sekretenko <[email protected]>

akirillov

thanks, @asekretenko. The KEP looks good and functionality-wise covers the initial request for the toggle behavior. Left a question regarding the uninstall. It would be great if someone from the committers can review this KEP.

akirillov · 2021-05-13T16:43:00Z

keps/0036-kudo-operator-toggle-behavior.md

+task will be handled as a normal, unconditional `KudoOperator`. When the
+parameter equals to `false`, execution of the task will ensure that
+the corresponding operator instance is not installed (uninstalling it if
+necessary).


will the uninstall step trigger the clanup plan of the child operator?

@akirillov Yes, it will. Normally, removing an instance triggers the cleanup plan (given that the plan exists, hasn't been triggered yet, and so on). In this KEP, there is no plan to treat child instance removal as a special case.

kensipe

Thanks for creating the KEP. We have a fairly low bar to merge a provisional KEP... expecting that it will take several rounds of thinking, and several rounds of PR merges to refine to the point of "implementable". I would suggest that focus more initially on the edge cases and what will and will not be supported prior to refining a proposal for implementation.

Thanks again! IMO this is the hard work... designing and understand the impact of a feature across a large number of other features and expectations. Great job getting this ball rolling!

kensipe · 2021-05-14T15:17:38Z

keps/0036-kudo-operator-toggle-behavior.md

+authors:
+  - "@asekretenko"
+owners:
+  - "@asekretenko"


Historically owners have been committers and I don't know @asekretenko. Are we expecting him to be a project committer? There is usually a voting process by owners in the CNCF process for this. That said there is a lot of flux in the committers and owners which is unfortunate.

Please review the KEP process. In particular: https://github.com/kudobuilder/kudo/blame/main/keps/0001-kep-process.md#L168-L173

The OWNER that is most closely associated with this KEP. If there is code or other artifacts that will result from this KEP, then it is expected that this OWNER will take responsibility for the bulk of those artifacts.

@asekretenko will you be able to serve in this capacity and own feature?

@kensipe the intention is to have @asekretenko become an owner down the line and we're earning our way to it via contributions in the form of bug-fixes and other KEPs down the line.

keps/0036-kudo-operator-toggle-behavior.md

kensipe · 2021-05-14T15:19:34Z

keps/0036-kudo-operator-toggle-behavior.md

+
+This KEP aims to add an ability to switch an operator dependency on and off
+via a specified parameter of a parent operator. Namely, as a result of
+implementing this KEP, `KudoOperator` task definition will get an optional field


Suggested change

implementing this KEP, `KudoOperator` task definition will get an optional field

implementing this KEP, `KudoOperator` task definition will have an optional field

kensipe · 2021-05-14T15:24:34Z

keps/0036-kudo-operator-toggle-behavior.md

+via a specified parameter of a parent operator. Namely, as a result of
+implementing this KEP, `KudoOperator` task definition will get an optional field
+containing a name of a boolean parameter (of the parent operator), the value
+of which will determine whether the child operator instance should exist.


this doesn't make sense... in particular should exist. I'm guessing.. but perhaps you mean that the operator will expect that the child operator exists and will not be created as part of a deployment.

Also... it might be worth editing this paragraph with a focus on "why" and less on how. The how will be outlined in the KEP.. the summary should strongly support "why" it adds value. what the world is likely without and what the world will be like with it.... which then be amplified by the motivation below.

kensipe · 2021-05-14T15:26:31Z

keps/0036-kudo-operator-toggle-behavior.md

+## Motivation
+From [#1775](https://github.com/kudobuilder/kudo/issues/1775):
+> Currently, KudoOperator tasks don't support toggle behavior which
+> significantly complicates the handling of the optional dependencies. A good


'handling of "the" optional dependencies'... gives the impression of "one" dependencies and requires a lot of assumptions of the reader. Likely we mean "handling of optional dependencies", which is explained below.

Suggested change

> significantly complicates the handling of the optional dependencies. A good

> significantly complicates the handling of optional dependencies. A good

kensipe · 2021-05-14T15:29:32Z

keps/0036-kudo-operator-toggle-behavior.md

+of which will determine whether the child operator instance should exist.
+
+## Motivation
+From [#1775](https://github.com/kudobuilder/kudo/issues/1775):


kensipe · 2021-05-14T15:33:25Z

keps/0036-kudo-operator-toggle-behavior.md

+
+* Add an ability to enforce existence/**non-existence** of the child
+  operator instance (and all its children) via a parent operator parameter,
+  i.e. to toggle the dependency.


Could we please rephrase? It is hard to understand and it is the goal. Are we expecting KUDO to "enforce" something? That isn't the mental model I formed to this point of reading. I am expecting that a defined dependency is already installed and those I don't want KUDO to install it (as a dependency) of the parent kudo operator. KUDO isn't "enforcing" anything at that point... it seems to be allowing a deviation from the current norm.

@kensipe thanks, I'll rephrase this (and also other locations).

I should have been much more careful with the word "enforce". While an execution of a task (Apply/Delete/Toggle/KudoOperator) is enforcing existence/deletion of a resource (probably "is ensuring" would be a better word), neither tasks nor plans nor KUDO are really enforcing anything; such an enforcement is very far from the scope of this KEP.
Sorry for confusion.

kensipe · 2021-05-14T15:35:35Z

keps/0036-kudo-operator-toggle-behavior.md

+  operator instance (and all its children) via a parent operator parameter,
+  i.e. to toggle the dependency.
+
+* Make parent operators specifying an invalid parameter for toggling fail


I'm question "invalid" parameter... I don't understand what that means in this context. It also sounds inverted... perhaps more like "required" dependency which can be set false. It is unclear what we are communicating here. What exactly are we toggling?

keps/0036-kudo-operator-toggle-behavior.md

kensipe · 2021-05-14T15:51:57Z

keps/0036-kudo-operator-toggle-behavior.md

+* Introducing an `UninstallKudoOperator` task. An ability to unconditionally
+  remove a child operator (for example, installed by a previous version), while
+  no more difficult, is not a goal of this proposal.
+


before getting to "proposal"... we need more information outlined to set expectations..
For examples questions / comments below, assuming Spark operator dependent on History Server and the KUDO spark operator is configured for a dependent KUDO History Server.

When KUDO Spark is installed... it will install KUDO History Server. Resulting in KUDO having "ownership" as a controller over the history server. Resulting in param changes for a kudo update being managed for the history server.

Questions:

IF we opt-out of the dependency, does KUDO own it or not own it? It seems it would not but the KEP should make that clear... with all the responsibilities for the dependency being removed from kudo. (like updates, deletes etc.)

IF we opt-out, (and I'm going off really old memory here).. I can't remember if we have the ability to have output values from a dependency... for instance a dependent zookeeper, provide a zookeeper client details to a parent.. if we have that, how does that work for a parent that opts out?

If we opt-out, what happens for kudo update for parameters which are for an opt-out dependency? Do we error out? does it ignore it?

same for a kudo upgrade

for an opt-out, can an kudo update reverse the decision? Opting in and provisioning a new dependency?

What does the plan status look like for an opt-out... now it shows a tree of phases including all dependencies... does it show the dependencies? Does it reflect the opt-out?

There is likely more... we need these questions answered in the KEP to frame a mental model for discussions, expectations and awareness. A feature of this magnitude requires some thoughts around impact to all other features and usability. It really is required prior to any proposal as the proposal will be evaluated against all the edge cases.

We don't have to boil the ocean.. this KEP is "provisional"... the first go of it, doesn't require a proposal... just a lot of detail of all the edge cases and a strong case around the acceptance criteria. It is also reasonable to identify cases which are too complex and will not be supported but they need to called out in this doc which becomes the foundation by which we add the feature.

@kensipe Thanks for these questions!

Regarding (1), I've tried to reword this KEP to make it clear that the goal is not toggling of a dependency relationship between two operator instances, but controlling whether the dependency instance exists by means of a parameter.

(2) is not an issue: as of today, there are no means for obtaining an output from the dependency.
I've tried to address (3) - (6) in the Non-Goals section.

Please take a new look; hopefully, this makes more sense now.

@asekretenko sorry for delays.. I will make time to review again tomorrow morning... it should be there by your mid-day.

Signed-off-by: Andrei Sekretenko <[email protected]>

kensipe · 2021-05-19T01:22:07Z

keps/0036-conditional-operator-dependencies.md

+  - "@asekretenko"
+editor: @asekretenko
+creation-date: 2021-04-30
+last-updated: 2021-04-30


could we update this?

Updated. Sorry, forgot about this timestamp.

Signed-off-by: Andrei Sekretenko <[email protected]>

kensipe

As this is still provisional... I'm ok with approval at this stage. It still seems refinement is necessary to make it implementable.

It is unclear when an parent operator has an opted out child... after provisioning the operator... what happens when a child parameter is updated? is it ignored? by the webhook? Is it an error?

I know it is a lot of work driving all the details... it would be great to ensure that the mental model creating when reading this provides a complete picture such that anyone can see the acceptance criteria and implement the feature. Nice job driving this!

kensipe · 2021-05-19T12:24:53Z

keps/0036-conditional-operator-dependencies.md

@@ -0,0 +1,106 @@
+---
+kep-number: 36
+short-desc: Allowing existence of an operator dependency to be conditional on a parameter value


title is prescriptive (and shouldn't be)... we should leave the "how" off... parameter value is a way... it should include a short desc of why or when
Suggest: Allowing for the installation of an operator with preexisting dependents already provisioned

kensipe · 2021-05-19T12:29:53Z

keps/0036-conditional-operator-dependencies.md

+
+## Summary
+
+This KEP aims to make it possible to create operators with conditionally existing child operators. Namely, as a result of implementing this KEP, it will be possible to create a task, execution of which, depending on a value of a specified parameter, will ensure either


"existing child operators"... I think we mean previously installed child operators. It isn't clear in which form these child operators exist IMO. Somewhat nuanced. non-blocking for me.. just feedback that we could be more clear.

kensipe · 2021-05-19T12:31:39Z

keps/0036-conditional-operator-dependencies.md

+## Summary
+
+This KEP aims to make it possible to create operators with conditionally existing child operators. Namely, as a result of implementing this KEP, it will be possible to create a task, execution of which, depending on a value of a specified parameter, will ensure either
+ * that a child operator instance exists, with appropriate parameters and operator version (this is what `KudoOperator` task currently does)


"this is what KudoOperator task currently does" is very confusing... as written it seems the operator does what we are looking for... we could be more clear the kudoOperator currently takes ownership of all dependent child operators.

kensipe · 2021-05-19T12:35:15Z

keps/0036-conditional-operator-dependencies.md

+
+## Motivation
+
+While `KudoOperator` task type greatly simplifies handling operator dependencies by introducing the notion of a child operator, it is almost of no help when one needs to make existence of a child operator conditional. Good examples of cases where this is necessary are running Spark with an optional History Server or Kafka with an optional Schema Registry.


"it is almost of no help when one needs to make existence of a child operator conditional" this is an awkward phrase... we are saying the a tool is almost of no help when it doesn't do the thing it wasn't designed to do. can we rephrase this more positively, in that there is use-case which if implemented would expand kudos utility. Or we have discovered a common use-case to support...

kensipe · 2021-05-19T12:35:39Z

keps/0036-conditional-operator-dependencies.md

+
+While `KudoOperator` task type greatly simplifies handling operator dependencies by introducing the notion of a child operator, it is almost of no help when one needs to make existence of a child operator conditional. Good examples of cases where this is necessary are running Spark with an optional History Server or Kafka with an optional Schema Registry.
+
+In both cases, it would be more convenient to control the optional dependencies via separate child operators; however, the only ways to optionally have a dependency today are either to include templates as-is into the parent operator and to control the contents at the template level, or to implement a job (via a `Toggle` task) which deploys/removes the dependency based on a parameter value. Also, cascading updates/upgrades become complicated in these scenarios. (See [#1775](https://github.com/kudobuilder/kudo/issues/1775)).


Suggested change

In both cases, it would be more convenient to control the optional dependencies via separate child operators; however, the only ways to optionally have a dependency today are either to include templates as-is into the parent operator and to control the contents at the template level, or to implement a job (via a `Toggle` task) which deploys/removes the dependency based on a parameter value. Also, cascading updates/upgrades become complicated in these scenarios. (See [#1775](https://github.com/kudobuilder/kudo/issues/1775)).

In both cases, it would be more convenient to control the optional dependencies via separate child operators; however, the only way to optionally have a dependency today are either to include templates as-is into the parent operator and to control the contents at the template level, or to implement a job (via a `Toggle` task) which deploys/removes the dependency based on a parameter value. Also, cascading updates/upgrades become complicated in these scenarios. (See [#1775](https://github.com/kudobuilder/kudo/issues/1775)).

kensipe · 2021-05-19T12:44:43Z

keps/0036-conditional-operator-dependencies.md

+
+* Adding a way to turn a child instance into an independent one (i.e. to detach/disown a dependency).
+
+* Adding a way to distinguish "dependency parameters" from other parameters of the parent operator. Removing a dependency instance will not prevent users from modifying parent operator parameters that are only passed through into the child operator. In the Spark History Server example, history server options will be configured via a parameter of a parent operator; switching off the history server dependency will have no effect on the user's ability to change this parameter.


it is hard to understand the example provided... it sounds like the parent operator for a non-owned child can still work on child parameters... that doesn't make sense. if the child isn't owned by kudo... do we want kudo to make changes to the operator.

This non-goal is about the parent parameters; I've tried to reword it, hopefully now it looks less confusing.

kensipe · 2021-05-19T12:46:47Z

keps/0036-conditional-operator-dependencies.md

+
+* OperatorVersion validation. This proposal adds one more way to create an operator version that cannot be installed regardless of instance parameter values.
+
+* Introducing a task that unconditionally removes a child operator instance.


I assume a non-goal includes: Ability to take ownership of an existing operator as a child

Added to non-goals, thanks!

kensipe · 2021-05-19T12:50:46Z

keps/0036-conditional-operator-dependencies.md

+    enablingParameter: "installDependency"
+```
+In this example, when the parameter `installDependency` equals to `true`, the task will be handled as a normal, unconditional `KudoOperator`. When the parameter equals to `false`, execution of the task will ensure that the corresponding operator instance is not installed (uninstalling it if necessary).
+


what about conditional optionality? is this a goal or non-goal
to be clear... what if operator A has 2 children B and C. what if B and C or 2 separate operators but 1 depends on the other such that if B is not a child there is no value in C. Do we want to support this declaratively? or do we force the user to pass false for both operators?

Thanks, this is a good example which in many cases (at least, in the simple ones) can be reasonably resolved without conditional optionality. Added to non-goals.

kensipe · 2021-05-19T12:53:53Z

keps/0036-conditional-operator-dependencies.md

+#### Validation
+A `KudoOperator` task referencing a non-existing parent parameter will be treated as invalid: instance admission (and plan execution, if instance admission is not used) will fail.
+
+Task execution and instance validation will require that the type of the specified parameter is either "boolean" or a string convertible to boolean according to rules used by Go's `strconv.ParseBool()`.


this is a strange use of validation... I'm not sure what or where this description applies. We have a capability for validating an operator, which is what I thought this section would cover. which should provide validation around the following:

is the enableParameter required? if not required I assume it's default is true

is the parameter value of enableParameter referencing a property which returns a boolean.

Renamed and extended this subsection.

Looks like "validation" has to be added to the KUDO's list of reserved terms which should only be used in a specific context (operator package validation in this case). While I don't observe any consistency regarding the use of this word in the previous KEPs, maybe it is indeed the time...

Thanks for pointing to the package validation! This is indeed one more case where the validity can and should be checked.

kensipe · 2021-05-19T12:57:49Z

keps/0036-conditional-operator-dependencies.md

+
+Task execution and instance validation will require that the type of the specified parameter is either "boolean" or a string convertible to boolean according to rules used by Go's `strconv.ParseBool()`.
+
+#### Changing `enablingParameter` on upgrade


it is unclear if you mean upgrading from a kudo operator previous version to a new version with this feature... or if you mean upgrading kudo manager (the owner of the kudo operators). I will assume the latter but is there value in covering the former?

I meant upgrading the OV; however, the value of covering the OV upgrade is indeed questionable, as no special handling is intended. Removed this subsection.

Signed-off-by: Andrei Sekretenko <[email protected]>

asekretenko · 2021-05-27T20:11:04Z

@kensipe Sorry, it is only now that I've realized how using the word "dependency" makes it easy to misinterpret this KEP due to having two loosely related meanings: "dependency" as a relationship between two entities, and "dependency" as one of these two entities.

I've tried to reword this KEP by replacing "dependency" used in the second meaning with less ambiguous words ("child", etc.) in most locations; please take a look.

KEP for adding toggle behaviour to KudoOperator

febd5d6

Signed-off-by: Andrei Sekretenko <[email protected]>

asekretenko added the kep/36 label May 5, 2021

asekretenko self-assigned this May 5, 2021

asekretenko requested review from alenkacz, gerred, kensipe and zen-dog as code owners May 5, 2021 16:16

asekretenko requested review from zmalik, akirillov and rishabh96b May 5, 2021 16:17

akirillov approved these changes May 13, 2021

View reviewed changes

kensipe mentioned this pull request May 14, 2021

Look into Failure for Cluster Teardown #1797

Open

kensipe requested changes May 14, 2021

View reviewed changes

Use a single line per paragraph

04a1e44

Signed-off-by: Andrei Sekretenko <[email protected]>

asekretenko force-pushed the asekretenko/kep_toggle_behavior_in_kudo_operator_task branch from 7477a3e to ebb20f8 Compare May 14, 2021 20:13

asekretenko added 2 commits May 17, 2021 11:48

Reword Summary, Motivation, Goals and Alternatives

477c1c7

Signed-off-by: Andrei Sekretenko <[email protected]>

Extend Non-Goals

0979e92

Signed-off-by: Andrei Sekretenko <[email protected]>

asekretenko force-pushed the asekretenko/kep_toggle_behavior_in_kudo_operator_task branch from ebb20f8 to 2951485 Compare May 17, 2021 11:13

Rename KEP file

9deb66d

Signed-off-by: Andrei Sekretenko <[email protected]>

asekretenko force-pushed the asekretenko/kep_toggle_behavior_in_kudo_operator_task branch from 2951485 to 9deb66d Compare May 17, 2021 11:14

asekretenko requested a review from kensipe May 17, 2021 11:23

kensipe reviewed May 19, 2021

View reviewed changes

Update last-updated

a9e1187

Signed-off-by: Andrei Sekretenko <[email protected]>

asekretenko force-pushed the asekretenko/kep_toggle_behavior_in_kudo_operator_task branch from 962f5d6 to a9e1187 Compare May 19, 2021 08:22

asekretenko requested a review from kensipe May 19, 2021 08:24

kensipe approved these changes May 19, 2021

View reviewed changes

asekretenko added 2 commits May 27, 2021 21:45

Avoid using the word "dependency"; clarify non-goals and other points

73e82cd

Signed-off-by: Andrei Sekretenko <[email protected]>

Remove the subsection on upgrade handling

e0319bf

Signed-off-by: Andrei Sekretenko <[email protected]>

	implementing this KEP, `KudoOperator` task definition will get an optional field
	implementing this KEP, `KudoOperator` task definition will have an optional field

	> significantly complicates the handling of the optional dependencies. A good
	> significantly complicates the handling of optional dependencies. A good


		## Summary

		This KEP aims to make it possible to create operators with conditionally existing child operators. Namely, as a result of implementing this KEP, it will be possible to create a task, execution of which, depending on a value of a specified parameter, will ensure either


		## Motivation

		While `KudoOperator` task type greatly simplifies handling operator dependencies by introducing the notion of a child operator, it is almost of no help when one needs to make existence of a child operator conditional. Good examples of cases where this is necessary are running Spark with an optional History Server or Kafka with an optional Schema Registry.


		While `KudoOperator` task type greatly simplifies handling operator dependencies by introducing the notion of a child operator, it is almost of no help when one needs to make existence of a child operator conditional. Good examples of cases where this is necessary are running Spark with an optional History Server or Kafka with an optional Schema Registry.

		In both cases, it would be more convenient to control the optional dependencies via separate child operators; however, the only ways to optionally have a dependency today are either to include templates as-is into the parent operator and to control the contents at the template level, or to implement a job (via a `Toggle` task) which deploys/removes the dependency based on a parameter value. Also, cascading updates/upgrades become complicated in these scenarios. (See [#1775](https://github.com/kudobuilder/kudo/issues/1775)).


		* Adding a way to turn a child instance into an independent one (i.e. to detach/disown a dependency).

		* Adding a way to distinguish "dependency parameters" from other parameters of the parent operator. Removing a dependency instance will not prevent users from modifying parent operator parameters that are only passed through into the child operator. In the Spark History Server example, history server options will be configured via a parameter of a parent operator; switching off the history server dependency will have no effect on the user's ability to change this parameter.


		* OperatorVersion validation. This proposal adds one more way to create an operator version that cannot be installed regardless of instance parameter values.

		* Introducing a task that unconditionally removes a child operator instance.


		Task execution and instance validation will require that the type of the specified parameter is either "boolean" or a string convertible to boolean according to rules used by Go's `strconv.ParseBool()`.

		#### Changing `enablingParameter` on upgrade

KEP for adding toggle behaviour to KudoOperator #1794

Are you sure you want to change the base?

KEP for adding toggle behaviour to KudoOperator #1794

Conversation

asekretenko commented May 5, 2021

akirillov left a comment

Choose a reason for hiding this 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

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

asekretenko May 17, 2021 • 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

kensipe 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

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

asekretenko commented May 27, 2021

asekretenko May 17, 2021 •

edited

Loading