From d57eb62895ce450b0de22b17f23fb71dfec3e97d Mon Sep 17 00:00:00 2001 From: Andreas Neumann Date: Tue, 31 Mar 2020 11:39:53 +0200 Subject: [PATCH 1/8] Add KEP for resettable parameters Signed-off-by: Andreas Neumann --- keps/0028-resettable-parameters.md | 96 ++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 keps/0028-resettable-parameters.md diff --git a/keps/0028-resettable-parameters.md b/keps/0028-resettable-parameters.md new file mode 100644 index 000000000..aca5edec9 --- /dev/null +++ b/keps/0028-resettable-parameters.md @@ -0,0 +1,96 @@ +--- +kep-number: 28 +short-desc: A parameter flag to reset parameter values after a plan is executed +title: Resettable parameters +authors: + - @aneumann82 +owners: + - @aneumann82 +editor: +creation-date: 2020-03-31 +last-updated: 2020-03-31 +status: provisional +see-also: +replaces: +superseded-by: +--- + +## Summary + +This KEP describes addition of a flag for parameters that allows a parameter to be reset after it was used in a plan. + +## Motivation + +The new flag allows an operator to define a parameter that is basically one-time-use. Especially for manually triggered +plans this will be an often used case: +- Start a backup with a specific name +- Evict a specific pod from a stateful set +- Start a repair plan for a specific node or datacenter + +All these examples have in common that they require input parameters. The parameters are required, and the user should be +forced to set them. If we do not have resettable parameters, it might happen that a parameters are still set from a +previous execution and the user forgets to set it. + +When parameters are marked as resettable, they are set to the default value after plan execution, and KUDO can error +out if a required parameter is not set. + +### Goals + +Make it possible to automatically reset a parameter after a plan is executed. + +### Non-Goals + +- Set parameters to specific values (except for a default) +- Set parameters at other moments than at the end of a plan + +## Proposal 1 + +Add an additional attribute `resettable` to parameter specifications in `params.yaml`: + +```yaml + - name: BACKUP_NAME + description: "The name under which the backup is saved." + resettable: "true" +``` + +The default value for this flag would be `false`. + +If the flag is set to `true`, the parameter will be set to the default value when *any* plan finishes. This change +of parameter value should *not* trigger any plan execution. This is the preferred proposal. + +## Proposal 2 + +An alternative would be a "string" type parameter that allows a user to set a specific plan after which the parameter +is reset: + +```yaml + - name: BACKUP_NAME + description: "The name under which the backup is saved." + resetAfterPlan: "backup" +``` + +This would reset the parameter after the plan `backup` is executed. The downside with this approach is that a parameter +could be set at some point and then be unknowingly used later. + +### User Stories + +- [#1395](https://github.com/kudobuilder/kudo/issues/1395) Resettable parameters + +### Implementation Details/Notes/Constraints + +- The parameter reset should happen if a plan reaches a terminal state, either `COMPLETED` or `FATAL_ERROR`. + +### Risks and Mitigations + + + +## Implementation History + +- 2020-03-31 - Initial draft. (@aneumann) + +## Alternatives + +An alternative would be to have a new step type `SetParameter` that can modify a parameter and set it to a custom value. +This would allow a lot more flexibility, but also introduce a lot more complexity: Parameter values could then change +in the middle of a plan execution, triggering new plans might happen, etc. This might be an interesting idea for +an upcoming enhancement. From 4a855ac7a3baae79161e9cb2e5377e92a21c1c31 Mon Sep 17 00:00:00 2001 From: Andreas Neumann Date: Fri, 3 Apr 2020 15:05:54 +0200 Subject: [PATCH 2/8] Added more alternatives Added user stories Signed-off-by: Andreas Neumann --- keps/0028-resettable-parameters.md | 138 +++++++++++++++++++++++++++-- 1 file changed, 129 insertions(+), 9 deletions(-) diff --git a/keps/0028-resettable-parameters.md b/keps/0028-resettable-parameters.md index aca5edec9..8583e27a7 100644 --- a/keps/0028-resettable-parameters.md +++ b/keps/0028-resettable-parameters.md @@ -58,24 +58,147 @@ The default value for this flag would be `false`. If the flag is set to `true`, the parameter will be set to the default value when *any* plan finishes. This change of parameter value should *not* trigger any plan execution. This is the preferred proposal. -## Proposal 2 - -An alternative would be a "string" type parameter that allows a user to set a specific plan after which the parameter +An alternative would be a "string" type parameter that allows a user to set plans after which the parameter is reset: ```yaml - name: BACKUP_NAME - description: "The name under which the backup is saved." - resetAfterPlan: "backup" + description: "The name for backups to create or restore." + resetAfterPlan: [ "backup", "restore" ] ``` This would reset the parameter after the plan `backup` is executed. The downside with this approach is that a parameter could be set at some point and then be unknowingly used later. +Pros: +- Both variants would be an easy extension for parameters from the definition point of view + +Cons: +- The parameters for a specific plan are not separated from "normal" parameters +- It's not easy to determine + +## Proposal 2 + +Add new task type, `SetParameters`: +```yaml + - name: backup-done + kind: SetParameters + spec: + params: + - name: 'RESTORE_FLAG' + value: nil +``` +This is a lot more powerful, but also provides a lot more ways to introduce complexity: Parameter values could change +inside a plan execution, what about triggered plans from param changes, etc. + +Pros: +- Very powerful +- Would allow easy extension to set parameters to custom values + +Cons: +- Very complex +- Parameters could change while the plan is executed +- What happens when a plan is triggered by a changed parameter + +## Proposal 3 + +Specific plan parameters: These parameters would only be valid inside a specific plan and could be defined inside the +operator: + +```yaml +plans: + backup: + params: + - name: 'NAME' + description: "The name under which the backup is saved." + strategy: serial + phases: + - name: nodes + strategy: parallel + steps: + - name: node + tasks: + - backup +``` + +Alternatively it might be possible to define this in the `params.yaml` +```yaml + - name: backup.NAME + plan: backup + description: "The name under which the backup is saved." +``` + +These parameters would only be valid during the execution of that plan. + +To use the parameter, it would have to be prefixed with the plan name in which it is defined: +`kudo update -p backup.NAME=MyBackup` + +``` +apiVersion: batch/v1 +kind: Job +metadata: + name: {{ $.PlanParams.backup.NAME }} +``` + +Pros: +- Would work well for manually triggered plans +- The parameter scope would be clearly defined + +Cons: +- Could potentially increase the size of the operator.yaml + + +## Proposal 4 + +Define a list of parameters to be reset when a plan is finished: + +```yaml +plans: + backup: + resetAfterPlan: + - BACKUP_NAME + strategy: serial + phases: + - name: nodes + strategy: parallel + steps: + - name: node + tasks: + - backup +``` + +Pros: +- Does not add a new flag on parameter definition + +Cons: +- It won't be obvious from the parameter list that this is a plan specific parameter + + ### User Stories - [#1395](https://github.com/kudobuilder/kudo/issues/1395) Resettable parameters +#### A backup plan for the Cassandra Operator + +A plan that starts a backup for the whole cluster which is manually triggered + +It has a BACKUP_NAME parameter that specifies the name of a backup that is to be created. +This parameter needs to be unique on every execution and should not be reused. If the parameter is not unset after the +backup plan is finished, a user could forget to set it again for the next execution. + +- Plan is manually triggered + +#### The restore operation in the Cassandra Operator + +The restore operation on the Cassandra Operator can be used to create a new cluster from an existing backup. + +It uses a RESTORE_FLAG parameter that can be used on installation of a new C* cluster to restore an existing backup. +It sets up an initContainer that downloads the data and prepares the new cluster to use this data. +The initContainer is only used on the very first start of the cluster and should not exist on subsequent restarts of +the nodes, additionally the RESTORE_FLAG is useless/not used after the initial deploy plan is done. + +- Plan is `deploy`, used on installation + ### Implementation Details/Notes/Constraints - The parameter reset should happen if a plan reaches a terminal state, either `COMPLETED` or `FATAL_ERROR`. @@ -87,10 +210,7 @@ could be set at some point and then be unknowingly used later. ## Implementation History - 2020-03-31 - Initial draft. (@aneumann) +- 2020-04-03 - Added alternatives and user stories ## Alternatives -An alternative would be to have a new step type `SetParameter` that can modify a parameter and set it to a custom value. -This would allow a lot more flexibility, but also introduce a lot more complexity: Parameter values could then change -in the middle of a plan execution, triggering new plans might happen, etc. This might be an interesting idea for -an upcoming enhancement. From eb299ee796c7b2cdf24e281632a76ba3dfe546fd Mon Sep 17 00:00:00 2001 From: Andreas Neumann Date: Fri, 17 Apr 2020 14:43:16 +0200 Subject: [PATCH 3/8] Added proposal 5, removed hard breaks Signed-off-by: Andreas Neumann --- keps/0028-resettable-parameters.md | 59 +++++++++++++++++------------- 1 file changed, 34 insertions(+), 25 deletions(-) diff --git a/keps/0028-resettable-parameters.md b/keps/0028-resettable-parameters.md index 8583e27a7..cb60146ae 100644 --- a/keps/0028-resettable-parameters.md +++ b/keps/0028-resettable-parameters.md @@ -21,18 +21,14 @@ This KEP describes addition of a flag for parameters that allows a parameter to ## Motivation -The new flag allows an operator to define a parameter that is basically one-time-use. Especially for manually triggered -plans this will be an often used case: +The new flag allows an operator to define a parameter that is basically one-time-use. Especially for manually triggered plans this will be an often used case: - Start a backup with a specific name - Evict a specific pod from a stateful set - Start a repair plan for a specific node or datacenter -All these examples have in common that they require input parameters. The parameters are required, and the user should be -forced to set them. If we do not have resettable parameters, it might happen that a parameters are still set from a -previous execution and the user forgets to set it. +All these examples have in common that they require input parameters. The parameters are required, and the user should be forced to set them. If we do not have resettable parameters, it might happen that a parameters are still set from a previous execution and the user forgets to set it. -When parameters are marked as resettable, they are set to the default value after plan execution, and KUDO can error -out if a required parameter is not set. +When parameters are marked as resettable, they are set to the default value after plan execution, and KUDO can error out if a required parameter is not set. ### Goals @@ -43,7 +39,7 @@ Make it possible to automatically reset a parameter after a plan is executed. - Set parameters to specific values (except for a default) - Set parameters at other moments than at the end of a plan -## Proposal 1 +## Proposal 1 - Reset flag on parameter Add an additional attribute `resettable` to parameter specifications in `params.yaml`: @@ -55,11 +51,9 @@ Add an additional attribute `resettable` to parameter specifications in `params. The default value for this flag would be `false`. -If the flag is set to `true`, the parameter will be set to the default value when *any* plan finishes. This change -of parameter value should *not* trigger any plan execution. This is the preferred proposal. +If the flag is set to `true`, the parameter will be set to the default value when *any* plan finishes. This change of parameter value should *not* trigger any plan execution. This is the preferred proposal. -An alternative would be a "string" type parameter that allows a user to set plans after which the parameter -is reset: +An alternative would be a "string" type parameter that allows a user to set plans after which the parameter is reset: ```yaml - name: BACKUP_NAME @@ -67,8 +61,7 @@ is reset: resetAfterPlan: [ "backup", "restore" ] ``` -This would reset the parameter after the plan `backup` is executed. The downside with this approach is that a parameter -could be set at some point and then be unknowingly used later. +This would reset the parameter after the plan `backup` is executed. The downside with this approach is that a parameter could be set at some point and then be unknowingly used later. Pros: - Both variants would be an easy extension for parameters from the definition point of view @@ -77,7 +70,7 @@ Cons: - The parameters for a specific plan are not separated from "normal" parameters - It's not easy to determine -## Proposal 2 +## Proposal 2 - SetParameters Task Add new task type, `SetParameters`: ```yaml @@ -88,8 +81,7 @@ Add new task type, `SetParameters`: - name: 'RESTORE_FLAG' value: nil ``` -This is a lot more powerful, but also provides a lot more ways to introduce complexity: Parameter values could change -inside a plan execution, what about triggered plans from param changes, etc. +This is a lot more powerful, but also provides a lot more ways to introduce complexity: Parameter values could change inside a plan execution, what about triggered plans from param changes, etc. Pros: - Very powerful @@ -100,10 +92,9 @@ Cons: - Parameters could change while the plan is executed - What happens when a plan is triggered by a changed parameter -## Proposal 3 +## Proposal 3 - Plan specific parameters -Specific plan parameters: These parameters would only be valid inside a specific plan and could be defined inside the -operator: +Specific plan parameters: These parameters would only be valid inside a specific plan and could be defined inside the operator: ```yaml plans: @@ -148,7 +139,7 @@ Cons: - Could potentially increase the size of the operator.yaml -## Proposal 4 +## Proposal 4 - Parameter reset after plan finish Define a list of parameters to be reset when a plan is finished: @@ -173,6 +164,25 @@ Pros: Cons: - It won't be obvious from the parameter list that this is a plan specific parameter +## Proposal 5 - Transient parameters only on `kudo plan trigger` + +This proposal would not require new configuration options in the operator definition. Normal parameter definitions can be used, the handling depends on the usage: + +```bash +kudo update --instance op-instance -p PARAM1=value +``` +This would set a parameter persistently, the parameter value will be saved in the Instance CRD + +```bash +kudo plan trigger someplan --instance op-instance -p PARAM1=value +``` +This would use a parameter transiently, the value would be available while rendering templates, but the value would not be saved in the instance CRD. When the plan execution ends, the parameter value is discarded. + +Open Questions: +- Should transient parameters marked in the parameter definition? + - If not, they could be set permanently with `kudo update`, which would reverse most of the ideas of transient parameters + - If yes, it may allow setting permanent parameters with `kudo plan trigger`. The question would be if the definition would be more like Proposal 1 or 3 + ### User Stories @@ -183,8 +193,7 @@ Cons: A plan that starts a backup for the whole cluster which is manually triggered It has a BACKUP_NAME parameter that specifies the name of a backup that is to be created. -This parameter needs to be unique on every execution and should not be reused. If the parameter is not unset after the -backup plan is finished, a user could forget to set it again for the next execution. +This parameter needs to be unique on every execution and should not be reused. If the parameter is not unset after the backup plan is finished, a user could forget to set it again for the next execution. - Plan is manually triggered @@ -194,8 +203,7 @@ The restore operation on the Cassandra Operator can be used to create a new clus It uses a RESTORE_FLAG parameter that can be used on installation of a new C* cluster to restore an existing backup. It sets up an initContainer that downloads the data and prepares the new cluster to use this data. -The initContainer is only used on the very first start of the cluster and should not exist on subsequent restarts of -the nodes, additionally the RESTORE_FLAG is useless/not used after the initial deploy plan is done. +The initContainer is only used on the very first start of the cluster and should not exist on subsequent restarts of the nodes, additionally the RESTORE_FLAG is useless/not used after the initial deploy plan is done. - Plan is `deploy`, used on installation @@ -211,6 +219,7 @@ the nodes, additionally the RESTORE_FLAG is useless/not used after the initial d - 2020-03-31 - Initial draft. (@aneumann) - 2020-04-03 - Added alternatives and user stories +- 2020-04-17 - Added proposal 5 ## Alternatives From dc63cc4b6c815b4e79bb4c8ec2a61b4893577294 Mon Sep 17 00:00:00 2001 From: Andreas Neumann Date: Fri, 24 Apr 2020 15:48:58 +0200 Subject: [PATCH 4/8] expanded user stories Signed-off-by: Andreas Neumann --- keps/0028-resettable-parameters.md | 37 +++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/keps/0028-resettable-parameters.md b/keps/0028-resettable-parameters.md index cb60146ae..dc743eb3e 100644 --- a/keps/0028-resettable-parameters.md +++ b/keps/0028-resettable-parameters.md @@ -137,7 +137,7 @@ Pros: Cons: - Could potentially increase the size of the operator.yaml - +- If the same parameter is used for two different plans, it would have to be repeated. ( for example BACKUP_NAME, used for backup and restore plans) ## Proposal 4 - Parameter reset after plan finish @@ -192,20 +192,45 @@ Open Questions: A plan that starts a backup for the whole cluster which is manually triggered -It has a BACKUP_NAME parameter that specifies the name of a backup that is to be created. +It has a `BACKUP_NAME` parameter that specifies the name of a backup that is to be created. This parameter needs to be unique on every execution and should not be reused. If the parameter is not unset after the backup plan is finished, a user could forget to set it again for the next execution. -- Plan is manually triggered +The expected initiation is: + +`kudo plan trigger --instance cassandra backup -p BACKUP_NAME=NewBackup` + +- A call without defined parameter should fail. +- The parameter BACKUP_NAME should not be stored in the instance and must be set the next time the backup plan is triggered. + +The operator has another parameter, `BACKUP_PREFIX`. This parameter describes the prefix that is used in the S3 bucket. This parameter should be saved in the instance, as it usually does not change often. + +It could be an option to set permanent parameters as well on plan trigger: + +`kudo plan trigger --instance cassandra backup -p BACKUP_NAME=2020-06 -p BACKUP_PREFIX=monthly` + +- BACKUP_PREFIX would be saved in the instance, BACKUP_NAME would be transient + +This is not a requirement, it's also possible to make two calls: +```bash +kudo update --instance cassandra -p BACKUP_PREFIX=monthly +kudo plan trigger --instance cassandra -p BACKUP_NAME=2020-06 +``` #### The restore operation in the Cassandra Operator The restore operation on the Cassandra Operator can be used to create a new cluster from an existing backup. -It uses a RESTORE_FLAG parameter that can be used on installation of a new C* cluster to restore an existing backup. -It sets up an initContainer that downloads the data and prepares the new cluster to use this data. -The initContainer is only used on the very first start of the cluster and should not exist on subsequent restarts of the nodes, additionally the RESTORE_FLAG is useless/not used after the initial deploy plan is done. +It uses a RESTORE_FLAG parameter that can be used on installation of a new C* cluster to restore an existing backup, and additional parameters like RESTORE_OLD_NAME and RESTORE_OLD_NAMESPACE to define which backup should be restored. +When RESTORE_FLAG is set, the deployment includes an initContainer that downloads the data and prepares the new cluster to use this data. +The initContainer is only used on the very first start of the cluster and should not exist on subsequent restarts of the nodes, additionally the RESTORE_FLAG, RESTORE_OLD_NAME and RESTORE_OLD_NAMESPACE are useless/not used after the initial deploy plan is done. + +The usage would be: + +`kudo install cassandra -p RESTORE_FLAG=true -p RESTORE_OLD_NAME=OldCluster -p RESTORE_OLD_NAMESPACE=OldNamespace -p NODECOUNT=3 ...` - Plan is `deploy`, used on installation +- The command line includes transient and permanent parameters +- The transient parameters should not be saved in the instance ### Implementation Details/Notes/Constraints From b48929403c10438a5f92f002290848e3a6ca2d8e Mon Sep 17 00:00:00 2001 From: Andreas Neumann Date: Tue, 28 Apr 2020 13:06:08 +0200 Subject: [PATCH 5/8] Renamed KEP, updated proposals Signed-off-by: Andreas Neumann --- ...meters.md => 0028-transient-parameters.md} | 143 ++++++++++-------- 1 file changed, 77 insertions(+), 66 deletions(-) rename keps/{0028-resettable-parameters.md => 0028-transient-parameters.md} (67%) diff --git a/keps/0028-resettable-parameters.md b/keps/0028-transient-parameters.md similarity index 67% rename from keps/0028-resettable-parameters.md rename to keps/0028-transient-parameters.md index dc743eb3e..e4323ec4e 100644 --- a/keps/0028-resettable-parameters.md +++ b/keps/0028-transient-parameters.md @@ -1,14 +1,14 @@ --- kep-number: 28 -short-desc: A parameter flag to reset parameter values after a plan is executed -title: Resettable parameters +short-desc: Transient parameters that are not saved permanently in the instance +title: Transient parameters authors: - @aneumann82 owners: - @aneumann82 editor: creation-date: 2020-03-31 -last-updated: 2020-03-31 +last-updated: 2020-04-28 status: provisional see-also: replaces: @@ -17,31 +17,31 @@ superseded-by: ## Summary -This KEP describes addition of a flag for parameters that allows a parameter to be reset after it was used in a plan. +This KEP describes addition of transient parameters that allows a parameter to be used only in a certain plan without saving the value in the instance. ## Motivation -The new flag allows an operator to define a parameter that is basically one-time-use. Especially for manually triggered plans this will be an often used case: +Transient parameters allow a specific use-case that is basically one-time-use. Especially for manually triggered plans this will be an often used case: - Start a backup with a specific name - Evict a specific pod from a stateful set - Start a repair plan for a specific node or datacenter -All these examples have in common that they require input parameters. The parameters are required, and the user should be forced to set them. If we do not have resettable parameters, it might happen that a parameters are still set from a previous execution and the user forgets to set it. +All these examples have in common that they require input parameters. The parameters are required, and the user should be forced to set them. If we do not have transient parameters, it might happen that a parameters are still set from a previous invocation and the user forgets to set it. Additionally, using permanent parameters for these use cases would clutter the instance parameter store with values that are not required to be saved. -When parameters are marked as resettable, they are set to the default value after plan execution, and KUDO can error out if a required parameter is not set. +When parameters are transient, they should be set to their default value after plan execution, and KUDO can error out if a required parameter is not set. ### Goals -Make it possible to automatically reset a parameter after a plan is executed. +Make it possible to have transient parameters that are only available withing a single plan execution. ### Non-Goals - Set parameters to specific values (except for a default) -- Set parameters at other moments than at the end of a plan +- Set or change parameters at other moments than at the end of a plan -## Proposal 1 - Reset flag on parameter +## Resettable parameters -Add an additional attribute `resettable` to parameter specifications in `params.yaml`: +Add an additional attribute `resettable` or `transient` to parameter specifications in `params.yaml`: ```yaml - name: BACKUP_NAME @@ -53,7 +53,7 @@ The default value for this flag would be `false`. If the flag is set to `true`, the parameter will be set to the default value when *any* plan finishes. This change of parameter value should *not* trigger any plan execution. This is the preferred proposal. -An alternative would be a "string" type parameter that allows a user to set plans after which the parameter is reset: +An alternative would be a list of strings that allows a user to set plans after which the parameter is reset: ```yaml - name: BACKUP_NAME @@ -63,36 +63,16 @@ An alternative would be a "string" type parameter that allows a user to set plan This would reset the parameter after the plan `backup` is executed. The downside with this approach is that a parameter could be set at some point and then be unknowingly used later. -Pros: -- Both variants would be an easy extension for parameters from the definition point of view - -Cons: -- The parameters for a specific plan are not separated from "normal" parameters -- It's not easy to determine - -## Proposal 2 - SetParameters Task - -Add new task type, `SetParameters`: -```yaml - - name: backup-done - kind: SetParameters - spec: - params: - - name: 'RESTORE_FLAG' - value: nil -``` -This is a lot more powerful, but also provides a lot more ways to introduce complexity: Parameter values could change inside a plan execution, what about triggered plans from param changes, etc. +Transient parameters can be easily identified and stored only in the `planExecution` section of the instance. As the `planExecution` is reset after the plan is finished, the transient parameters there would be discarded as well. Pros: -- Very powerful -- Would allow easy extension to set parameters to custom values +- Both variants would be an easy extension for parameters from the definition point of view +- Parameters are not bound to specific plans. Multiple plans can use the same transient parameters. (For example `backup` and `restore` could use the same `BACKUP_NAME` parameter) Cons: -- Very complex -- Parameters could change while the plan is executed -- What happens when a plan is triggered by a changed parameter +- The parameters for a specific plan are not separated from permanent parameters - It would be easy to miss the attribute. -## Proposal 3 - Plan specific parameters +## Plan specific parameters Specific plan parameters: These parameters would only be valid inside a specific plan and could be defined inside the operator: @@ -119,11 +99,12 @@ Alternatively it might be possible to define this in the `params.yaml` description: "The name under which the backup is saved." ``` -These parameters would only be valid during the execution of that plan. +The parameters would only be valid during the execution of that plan and can only be used with the defining plan. As with the previous proposal, they can be stored in the `planExecution` part of the instance where the get discarded after plan execution. To use the parameter, it would have to be prefixed with the plan name in which it is defined: `kudo update -p backup.NAME=MyBackup` + ``` apiVersion: batch/v1 kind: Job @@ -134,37 +115,14 @@ metadata: Pros: - Would work well for manually triggered plans - The parameter scope would be clearly defined +- Transient and permanent parameters can be used at the same time +- A prefix will make it clear which type is used when invoking `kudo install`, `kudo update` or `kudo plan trigger` Cons: -- Could potentially increase the size of the operator.yaml +- Could potentially increase the size of the operator.yaml (If parameters are defined there) - If the same parameter is used for two different plans, it would have to be repeated. ( for example BACKUP_NAME, used for backup and restore plans) -## Proposal 4 - Parameter reset after plan finish - -Define a list of parameters to be reset when a plan is finished: - -```yaml -plans: - backup: - resetAfterPlan: - - BACKUP_NAME - strategy: serial - phases: - - name: nodes - strategy: parallel - steps: - - name: node - tasks: - - backup -``` - -Pros: -- Does not add a new flag on parameter definition - -Cons: -- It won't be obvious from the parameter list that this is a plan specific parameter - -## Proposal 5 - Transient parameters only on `kudo plan trigger` +## Transient parameters only on `kudo plan trigger` This proposal would not require new configuration options in the operator definition. Normal parameter definitions can be used, the handling depends on the usage: @@ -183,6 +141,9 @@ Open Questions: - If not, they could be set permanently with `kudo update`, which would reverse most of the ideas of transient parameters - If yes, it may allow setting permanent parameters with `kudo plan trigger`. The question would be if the definition would be more like Proposal 1 or 3 +Pros: +- Potentially no change to operator definition + ### User Stories @@ -234,7 +195,9 @@ The usage would be: ### Implementation Details/Notes/Constraints -- The parameter reset should happen if a plan reaches a terminal state, either `COMPLETED` or `FATAL_ERROR`. +- The parameter value will be discarded when plan reaches a terminal state, either `COMPLETED` or `FATAL_ERROR`. +- Transient parameters will be safed in the `planExecution` structure in the instance CRD. + ### Risks and Mitigations @@ -245,6 +208,54 @@ The usage would be: - 2020-03-31 - Initial draft. (@aneumann) - 2020-04-03 - Added alternatives and user stories - 2020-04-17 - Added proposal 5 +- 2020-04-28 - Moved two proposals to alternatives, renamed KEP ## Alternatives +### Parameter reset after plan finish + +This is a variant of the previous proposal: Plans will have a list of parameters that are reset when a plan is finished: + +```yaml +plans: + backup: + resetAfterPlan: + - BACKUP_NAME + strategy: serial + phases: + - name: nodes + strategy: parallel + steps: + - name: node + tasks: + - backup +``` + +Pros: +- Does not add a new flag on parameter definition + +Cons: +- It won't be obvious from the parameter list that this is a plan specific parameter +- It will be possible for a parameter to be transient for one plan but permanent for another. It would not be possible to store transient parameters in the `planExecution` structure of the instance + +### SetParameters Task + +Add new task type, `SetParameters`: +```yaml + - name: backup-done + kind: SetParameters + spec: + params: + - name: 'RESTORE_FLAG' + value: nil +``` +This would be a very powerful task, but it would also provide a lot more ways to introduce complexity: Parameter values could change inside a plan execution, the change could trigger additional plans, etc. + +Pros: +- Very powerful +- Would allow easy extension to set parameters to custom values + +Cons: +- Very complex +- Parameters could change while the plan is executed +- What happens when a plan is triggered by a changed parameter \ No newline at end of file From f12d1b0049a2fc0ce0d3a99bd5961109512b294a Mon Sep 17 00:00:00 2001 From: Andreas Neumann Date: Wed, 5 Aug 2020 14:07:32 +0200 Subject: [PATCH 6/8] Cleaned up first proposal Signed-off-by: Andreas Neumann --- keps/0028-transient-parameters.md | 35 +++++++++++++------------------ 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/keps/0028-transient-parameters.md b/keps/0028-transient-parameters.md index e4323ec4e..646290432 100644 --- a/keps/0028-transient-parameters.md +++ b/keps/0028-transient-parameters.md @@ -39,38 +39,28 @@ Make it possible to have transient parameters that are only available withing a - Set parameters to specific values (except for a default) - Set or change parameters at other moments than at the end of a plan -## Resettable parameters +## Persistent vs. Transient parameters -Add an additional attribute `resettable` or `transient` to parameter specifications in `params.yaml`: +Add an attribute `persistent` to parameter specifications in `params.yaml`: ```yaml - name: BACKUP_NAME description: "The name under which the backup is saved." - resettable: "true" + persistent: "false" ``` -The default value for this flag would be `false`. +The default value for this flag would be `true`, all parameters that are defined without the attribute are persistent by default. -If the flag is set to `true`, the parameter will be set to the default value when *any* plan finishes. This change of parameter value should *not* trigger any plan execution. This is the preferred proposal. +If the flag is set to `false`, the parameter is transient and its value will not be persisted in the instance resource. -An alternative would be a list of strings that allows a user to set plans after which the parameter is reset: - -```yaml - - name: BACKUP_NAME - description: "The name for backups to create or restore." - resetAfterPlan: [ "backup", "restore" ] -``` - -This would reset the parameter after the plan `backup` is executed. The downside with this approach is that a parameter could be set at some point and then be unknowingly used later. - -Transient parameters can be easily identified and stored only in the `planExecution` section of the instance. As the `planExecution` is reset after the plan is finished, the transient parameters there would be discarded as well. +Transient parameters can be easily identified and will only be stored in the `planExecution` section of the instance. As the `planExecution` is reset after the plan finishes, the transient parameters from the plan execution will be discarded as well. Pros: -- Both variants would be an easy extension for parameters from the definition point of view +- Easy extension for parameters from the definition point of view - Parameters are not bound to specific plans. Multiple plans can use the same transient parameters. (For example `backup` and `restore` could use the same `BACKUP_NAME` parameter) Cons: -- The parameters for a specific plan are not separated from permanent parameters - It would be easy to miss the attribute. +- The parameters for a specific plan are not separated from permanent parameters - It can be easy to miss the attribute. ## Plan specific parameters @@ -101,10 +91,10 @@ Alternatively it might be possible to define this in the `params.yaml` The parameters would only be valid during the execution of that plan and can only be used with the defining plan. As with the previous proposal, they can be stored in the `planExecution` part of the instance where the get discarded after plan execution. -To use the parameter, it would have to be prefixed with the plan name in which it is defined: -`kudo update -p backup.NAME=MyBackup` - +To specify the parameter, it would have to be prefixed with the plan name in which it is defined: +`kudo plan trigger backup -p backup.NAME=MyBackup` +Plan parameters will be stored in a separate map named `PlanParams` which contains a map for the currently executed plan. ``` apiVersion: batch/v1 kind: Job @@ -121,6 +111,8 @@ Pros: Cons: - Could potentially increase the size of the operator.yaml (If parameters are defined there) - If the same parameter is used for two different plans, it would have to be repeated. ( for example BACKUP_NAME, used for backup and restore plans) +- The format of `{{ $.PlanParams.backup.NAME }}` is problematic if the same resource is used in multiple plans +- Implementation would be more complex (New PlanParams map, enhanced verification to make sure that resources that use a plan parameter are only used in the correct plans, ...) ## Transient parameters only on `kudo plan trigger` @@ -209,6 +201,7 @@ The usage would be: - 2020-04-03 - Added alternatives and user stories - 2020-04-17 - Added proposal 5 - 2020-04-28 - Moved two proposals to alternatives, renamed KEP +- 2020-08-05 - Improved wording, cleaned up naming ## Alternatives From c137ce648d2c1e23fa27a60587a3958b83874e79 Mon Sep 17 00:00:00 2001 From: Andreas Neumann Date: Wed, 5 Aug 2020 14:29:15 +0200 Subject: [PATCH 7/8] More details Signed-off-by: Andreas Neumann --- keps/0028-transient-parameters.md | 54 ++++++++++++++++++------------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/keps/0028-transient-parameters.md b/keps/0028-transient-parameters.md index 646290432..42915a957 100644 --- a/keps/0028-transient-parameters.md +++ b/keps/0028-transient-parameters.md @@ -26,7 +26,7 @@ Transient parameters allow a specific use-case that is basically one-time-use. E - Evict a specific pod from a stateful set - Start a repair plan for a specific node or datacenter -All these examples have in common that they require input parameters. The parameters are required, and the user should be forced to set them. If we do not have transient parameters, it might happen that a parameters are still set from a previous invocation and the user forgets to set it. Additionally, using permanent parameters for these use cases would clutter the instance parameter store with values that are not required to be saved. +All these examples have in common that they require input parameters. The parameters are required, and the user should be forced to set them. If we do not have transient parameters, it might happen that a parameters are still set from a previous invocation and the user forgets to set it. Additionally, using persistent parameters for these use cases would clutter the instance parameter store with values that are not required to be saved. When parameters are transient, they should be set to their default value after plan execution, and KUDO can error out if a required parameter is not set. @@ -39,7 +39,9 @@ Make it possible to have transient parameters that are only available withing a - Set parameters to specific values (except for a default) - Set or change parameters at other moments than at the end of a plan -## Persistent vs. Transient parameters +## Proposals + +#### Persistent vs. Transient parameters Add an attribute `persistent` to parameter specifications in `params.yaml`: @@ -55,14 +57,21 @@ If the flag is set to `false`, the parameter is transient and its value will not Transient parameters can be easily identified and will only be stored in the `planExecution` section of the instance. As the `planExecution` is reset after the plan finishes, the transient parameters from the plan execution will be discarded as well. +- Transient parameters cannot be `immutable` + - They are reset after every plan execution and are therefore mutable by design +- Transient parameters can have a default value +- Transient parameters cannot be `required` + - This would be a desired property and could be theoretically allowed. But transient parameters are often plan specific, and the `required` scope is global, it would be mostly too wide of a scope. + Pros: - Easy extension for parameters from the definition point of view - Parameters are not bound to specific plans. Multiple plans can use the same transient parameters. (For example `backup` and `restore` could use the same `BACKUP_NAME` parameter) Cons: -- The parameters for a specific plan are not separated from permanent parameters - It can be easy to miss the attribute. +- The parameters for a specific plan are not separated from persistent parameters - It can be easy to miss the attribute. +- Without plan specific parameters, it is hard to determine if a plan requires a specific parameter. It will be easy for a user to forget to specify a transient parameter, and `kudoctl` will not be able to determine that a parameter is required to render a plan-specific resource. -## Plan specific parameters +#### Plan specific parameters Specific plan parameters: These parameters would only be valid inside a specific plan and could be defined inside the operator: @@ -105,16 +114,19 @@ metadata: Pros: - Would work well for manually triggered plans - The parameter scope would be clearly defined -- Transient and permanent parameters can be used at the same time +- Transient and persistent parameters can be used at the same time - A prefix will make it clear which type is used when invoking `kudo install`, `kudo update` or `kudo plan trigger` Cons: - Could potentially increase the size of the operator.yaml (If parameters are defined there) - If the same parameter is used for two different plans, it would have to be repeated. ( for example BACKUP_NAME, used for backup and restore plans) - The format of `{{ $.PlanParams.backup.NAME }}` is problematic if the same resource is used in multiple plans -- Implementation would be more complex (New PlanParams map, enhanced verification to make sure that resources that use a plan parameter are only used in the correct plans, ...) +- Plan specific parameters are not necessarily transient - Some persistent parameters may be plan specific as well +- More complex implementation (New PlanParams map, enhanced verification to make sure that resources which use a plan parameter are only used in the correct plans, ...) + +Plan specific parameters may not really be the solution for the User stories in this KEP, but they might be useful in other circumstances. -## Transient parameters only on `kudo plan trigger` +### Transient parameters only on `kudo plan trigger` This proposal would not require new configuration options in the operator definition. Normal parameter definitions can be used, the handling depends on the usage: @@ -131,13 +143,21 @@ This would use a parameter transiently, the value would be available while rende Open Questions: - Should transient parameters marked in the parameter definition? - If not, they could be set permanently with `kudo update`, which would reverse most of the ideas of transient parameters - - If yes, it may allow setting permanent parameters with `kudo plan trigger`. The question would be if the definition would be more like Proposal 1 or 3 + - If yes, it may allow setting persistent parameters with `kudo plan trigger`. The question would be if the definition would be more like Proposal 1 or 3 Pros: - Potentially no change to operator definition +### Implementation Details/Notes/Constraints + +- The parameter value will be discarded when plan reaches a terminal state, either `COMPLETED` or `FATAL_ERROR`. +- Transient parameters will be safed in the `planExecution` structure in the instance CRD. + + +### Risks and Mitigations + -### User Stories +## User Stories - [#1395](https://github.com/kudobuilder/kudo/issues/1395) Resettable parameters @@ -157,7 +177,7 @@ The expected initiation is: The operator has another parameter, `BACKUP_PREFIX`. This parameter describes the prefix that is used in the S3 bucket. This parameter should be saved in the instance, as it usually does not change often. -It could be an option to set permanent parameters as well on plan trigger: +It could be an option to set persistent parameters as well on plan trigger: `kudo plan trigger --instance cassandra backup -p BACKUP_NAME=2020-06 -p BACKUP_PREFIX=monthly` @@ -182,19 +202,9 @@ The usage would be: `kudo install cassandra -p RESTORE_FLAG=true -p RESTORE_OLD_NAME=OldCluster -p RESTORE_OLD_NAMESPACE=OldNamespace -p NODECOUNT=3 ...` - Plan is `deploy`, used on installation -- The command line includes transient and permanent parameters +- The command line includes transient and persistent parameters - The transient parameters should not be saved in the instance -### Implementation Details/Notes/Constraints - -- The parameter value will be discarded when plan reaches a terminal state, either `COMPLETED` or `FATAL_ERROR`. -- Transient parameters will be safed in the `planExecution` structure in the instance CRD. - - -### Risks and Mitigations - - - ## Implementation History - 2020-03-31 - Initial draft. (@aneumann) @@ -229,7 +239,7 @@ Pros: Cons: - It won't be obvious from the parameter list that this is a plan specific parameter -- It will be possible for a parameter to be transient for one plan but permanent for another. It would not be possible to store transient parameters in the `planExecution` structure of the instance +- It will be possible for a parameter to be transient for one plan but persistent for another. It would not be possible to store transient parameters in the `planExecution` structure of the instance ### SetParameters Task From c6e357f718d0e69989f51fa2be27eddaea6056de Mon Sep 17 00:00:00 2001 From: Andreas Neumann Date: Thu, 20 Aug 2020 15:54:13 +0200 Subject: [PATCH 8/8] Inverted transient/persistent naming in params.yaml definition, fixed typo Signed-off-by: Andreas Neumann --- keps/0028-transient-parameters.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/keps/0028-transient-parameters.md b/keps/0028-transient-parameters.md index 42915a957..6084d7eb7 100644 --- a/keps/0028-transient-parameters.md +++ b/keps/0028-transient-parameters.md @@ -43,17 +43,17 @@ Make it possible to have transient parameters that are only available withing a #### Persistent vs. Transient parameters -Add an attribute `persistent` to parameter specifications in `params.yaml`: +Add an attribute `transient` to parameter specifications in `params.yaml`: ```yaml - name: BACKUP_NAME description: "The name under which the backup is saved." - persistent: "false" + transient: "true" ``` -The default value for this flag would be `true`, all parameters that are defined without the attribute are persistent by default. +The default value for this flag would be `false`, all parameters that are defined without the attribute are persistent by default. -If the flag is set to `false`, the parameter is transient and its value will not be persisted in the instance resource. +If the flag is set to `true`, the parameter is transient and its value will not be persisted in the instance resource. Transient parameters can be easily identified and will only be stored in the `planExecution` section of the instance. As the `planExecution` is reset after the plan finishes, the transient parameters from the plan execution will be discarded as well. @@ -61,7 +61,7 @@ Transient parameters can be easily identified and will only be stored in the `pl - They are reset after every plan execution and are therefore mutable by design - Transient parameters can have a default value - Transient parameters cannot be `required` - - This would be a desired property and could be theoretically allowed. But transient parameters are often plan specific, and the `required` scope is global, it would be mostly too wide of a scope. + - This would be a desired property and could be theoretically allowed. But transient parameters are often plan specific, and the `required` scope is global, it would be mostly too wide of a scope. Having plan-specific required parameter might be the scope of a different KEP. Pros: - Easy extension for parameters from the definition point of view @@ -151,7 +151,7 @@ Pros: ### Implementation Details/Notes/Constraints - The parameter value will be discarded when plan reaches a terminal state, either `COMPLETED` or `FATAL_ERROR`. -- Transient parameters will be safed in the `planExecution` structure in the instance CRD. +- Transient parameters will be saved in the `planExecution` structure in the instance CRD. ### Risks and Mitigations