Skip to content

KEP for adding toggle behaviour to KudoOperator #1794

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.

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
106 changes: 106 additions & 0 deletions keps/0036-conditional-operator-dependencies.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
---
kep-number: 36
short-desc: Allowing existence of an operator dependency to be conditional on a parameter value
Copy link
Member

Choose a reason for hiding this comment

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

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

title: Conditional Operator Dependencies
authors:
- "@asekretenko"
owners:
- "@asekretenko"
editor: @asekretenko
creation-date: 2021-04-30
last-updated: 2021-04-30
Copy link
Member

Choose a reason for hiding this comment

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

could we update this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated. Sorry, forgot about this timestamp.

status: provisional
see-also:
- KEP-23
- KEP-29
---

# Conditional Operator Dependencies

## Table of Contents
* [Summary](#summary)
* [Motivation](#motivation)
* [Goals](#goals)
* [Non-Goals](#non-goals)
* [Proposal](#proposal)
* [Interface](#interface)
* [Validation](#validation)
* [Changing enablingParameter on upgrade](#changing-enablingparameter-on-upgrade)
* [Implementation Notes](#implementation-notes)
* [Risks and Mitigations](#risks-and-mitigations)
* [Drawbacks](#drawbacks)
* [Alternatives](#alternatives)


## 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
Copy link
Member

Choose a reason for hiding this comment

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

"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.

* that a child operator instance exists, with appropriate parameters and operator version (this is what `KudoOperator` task currently does)
Copy link
Member

Choose a reason for hiding this comment

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

"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.

* or that the child instance does not exist (by cleaning it up if it does)

## 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.
Copy link
Member

Choose a reason for hiding this comment

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

"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...


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)).
Copy link
Member

Choose a reason for hiding this comment

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

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)).


### Goals

* Add an ability to toggle a `KudoOperator` task between ensuring existence (as it does now) and ensuring **non-existence** of the child operator instance (and its children) via a parent operator parameter. Toggling in both directions will be possibe.
Copy link
Member

Choose a reason for hiding this comment

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

this is still very confusing... when it says "ensuring existence", I think what you mean is that it takes ownership of the operator through provisioning, upgrading etc. It is very confusing to say that we want to ensure the non-existence of a child operator... It seems like we want the existence to be owned and managed external to kudo... right? or are you literally saying that this child operator should not exist at all?


### Non-Goals

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

Choose a reason for hiding this comment

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

👏


* 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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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


* Displaying whether the dependency is switched on or off in an output of `kudo plan status`. As for now, the latter only descends to the status of individual plan steps of the parent instance and tells nothing about the dependencies. The dependency being switched off is a property of an individual task and not of a step.

* Chain validation of child instances. As for now, `KudoOperator` task executions that result in failed instance admission of a child do not fail parent instance admission, only plan execution. Functionality introduced by this proposal is also affected by this: for example, if a parent switches on a child that uses a non-existent parameter for toggling a grandchild, parent instance admission will succeed, only the corresponding parent plan will fail.

* 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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Added to non-goals, thanks!


## Proposal

### Interface
An optional field `enablingParameter` will be added to the `KudoOperator` task:
```
- name: dependency
kind: KudoOperator
spec:
package: "./packages/dependency"
parameterFile: dependency-params.yaml
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).

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

#### 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()`.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


#### Changing `enablingParameter` on upgrade
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

No special handling for upgrade is planned.

Some consequencees of this choice:
* Dropping the parameter will convert the task into an unconditional `KudoOperator` task managing the same instance.

* Switching to a parent operator parameter with a different name will have an effect determined only by the value of the new parameter.

### Implementation Notes

Implementation is relatively straightforward, including the instance removal case: the child instance to be removed will be identified via the same mechanism as one used for identifying the instance to patch on upgrade/update.

## Risks and Mitigations

## Drawbacks

This proposal makes the operator developer API even less robust with regards to developer errors than it is now (see [Non-Goals](#non-goals)).

## Alternatives

Currently existing alternatives:
* Pulling the would-be contents of a child operator into the main operator as `Apply` tasks with heavily parametrized templates, or as `Toggle` tasks. This becomes complicated when the would-be child operator has to implement something more complex than a simple unconditional set of `Apply`/`Toggle` tasks.
* Conditionally creating/deleting child operator instance by means of `Toggle` tasks. This is extremely cumbersome in the existing form, but might become much more convenient if we are to implement a stable API for managing operators via a single custom resource (aka CRD-based installation; KEP yet to be filed). However, one might argue that `KudoOperator` is still going to provide a much more convenient framework for cases when a parent operator has multiple children that are never used on their own.