KEP-28: Transient parameters

[ANeumann82]

Relates to #1395

Signed-off-by: Andreas Neumann [email protected]

[kensipe]

I would like to drop this back into draft and have a discussion...

First, the use cases from the issues #1395 should be included in this KEP.. they provide a lot of useful default IMO.
Second, the goal is defined... but there is only 1 approach assumed / defined (with variations of that same approach). that approach being resetting a param.

Thoughts:

1. The goal is an ephemeral value (one that survives failures, until complete). Perhaps we should consider a passed in value that can be passed into plans. Something params like but it handled complete different.
2. I would like to think about convention over configuration if possible and what that would look like. perhaps there are "specially" defined params backup.name which are required and only good for the life of the plan. In other words it doesn't take a bunch of verbosity to define, and configure to make this happen. Perhaps the plan defines these "passed" params in the plan, so they could be defined there with behavior that isn't special for a param... but is defined for that type of variable.
3. perhaps, instead of proposal 2, where "resetAfterPlan", there is s a list of "reset-params" defined for a plan. Flipping on its head, but puts it in the right place.
4. there is also the question of is this strings only, or?

I would rather consider several alt approaches before we go impl.

[kensipe]

I'm not sure you want it on FATAL

[ANeumann82]

I'm not sure. I think FATAL_ERROR is a valid case to reset, as the plan is finished.
If the plan should be restarted, the user would have to set the parameter again. (An possibly fix the problem that led to the FATAL_ERROR)

[ANeumann82]

@kensipe Great comment, very interesting alternatives that I haven't thought about yet. I'll integrate and extend those!

[ANeumann82]

@kensipe I've integrated 1., 2. and 3. into the document.

With regards to 4.: I don't think this is "string" specific, I think this would apply to all types of parameters.

[kensipe]

the question on type is important to vet IMO.... if we are talk about something someone would pass to a CLI (like path, or backup name or... ) these seem to meet the use case and are obviously strings. If we are talking any type of param... than params make more sense, but we don't have a use case to support it.

[zen-dog]

I'd added a fifth proposal 😉. PTAL

[zen-dog]

Here is a Proposal 5:

TL;DR: we use recently introduced Spec.PlanExecution to hold transient parameter values. So a CLI command like:

$ k kudo plan trigger backup --BACKUP_NAME foo

will result in the following PlanExecution:

...
spec:
  planExecution:
    planName: backup
    uid: xxx-yyy-zzzz
    parameters:
        BACKUP_NAME: foo

And the planPlanExecution is already reset after the plan is terminal so not much to do there.

Pros:

• This design captures closely the mental model of passing parameters with a triggered plan and leaves the parameters where they belong: with the current plan execution

Cons:

• This works best with manually triggered plans which are, I believe, the 90/01 use case. Mixing transient and normal parameters during a plan execution triggered by a parameter update seem very confusing to me so I would want to avoid this

[ANeumann82]

I'm not sure if that's a new Proposal, it looks a little bit like an implementation variant of Proposal 3?

I don't see any kind of parameter definition in your proposal, would that be part of it somewhere?

With regards to mixing transient and normal parameters: Have a look at the Restore operator User story. In that case it's not a parameter update, but the installation, but it has a convincing use case for mixing transient and normal parameters.
I'm not sure if we could find a use case for parmeter updates, but I wouldn't rule it out.

In any case, I assume you're talking about updating/setting transient and normal parameters, correct? Because we certainly need to read/use both kinds when rendering specific resources.

[zen-dog]

I'm not sure if that's a new Proposal, it looks a little bit like an implementation variant of Proposal 3?

Sort of. Though where we persist parameters in the Instance is an important enough detail 😉

I don't see any kind of parameter definition in your proposal, would that be part of it somewhere?

There is no need for changing the parameter definitions.

In that case, it's not a parameter update, but the installation, but it has a convincing use case for mixing transient and normal parameters.

Yeah, I saw it. So k kudo install ... -p RESTORE_NAME=foo command would end up with a PlanExecution like:

...
spec:
  planExecution:
    planName: deploy
    parameters:
        RESTORE_NAME: foo

This still fits the model nicely.

In any case, I assume you're talking about updating/setting transient and normal parameters, correct?

Exactly. A simple k kudo update should not mix both IMHO. But even if there is an absolutely compelling story for it, we could still do it.

[ANeumann82]

I've thought about this a bit more:

I'm not sure if that's a new Proposal, it looks a little bit like an implementation variant of Proposal 3?

Sort of. Though where we persist parameters in the Instance is an important enough detail 😉

I think storing the parameters in the planExecution makes a lot of sense in all proposals, I've updated the sections accordingly.

I don't see any kind of parameter definition in your proposal, would that be part of it somewhere?

There is no need for changing the parameter definitions.

I'm not sure this is true. If we don't change the param definitions, it would be possible to set a BACKUP_NAME in a kudo update invocation. It would then be stored in the permanent section in the instance. KUDO wouldn't have any way to determine that a parameter is (or should) only be used in kudo plan trigger.

In that case, it's not a parameter update, but the installation, but it has a convincing use case for mixing transient and normal parameters.

Yeah, I saw it. So k kudo install ... -p RESTORE_NAME=foo command would end up with a PlanExecution like:

...
spec:
  planExecution:
    planName: deploy
    parameters:
        RESTORE_NAME: foo

This still fits the model nicely.

It does, KUDO still needs a way to know which parameters end up in spec.planExecution.parameters and which one should be in spec.parameters.

In any case, I assume you're talking about updating/setting transient and normal parameters, correct?

Exactly. A simple k kudo update should not mix both IMHO. But even if there is an absolutely compelling story for it, we could still do it.

I think this use case will be common enough that we should consider it. I think it would be good to be able to distinguish between permanent and transient parameters in the invocation though:

kudo install -p NODE_COUNT=3 -p install.RESTORE_BACKUP_NAME=MyBackup
vs
kudo install -p NODE_COUNT=3 -p RESTORE_NAME=MyBackup

woud make it at least a bit more clear that something is different between the parameters.

[zen-dog]

It does, KUDO still needs a way to know which parameters end up in spec.planExecution.parameters and which one should be in spec.parameters.

If a parameter is in spec.planExecution it's transient. If it's in the spec.parameters it's persistent.

[ANeumann82]

If a parameter is in spec.planExecution it's transient. If it's in the spec.parameters it's persistent.

Yes, for the KUDO manager side that's all good and well, but how does the KUDO CLI decide whether to put a parameter into spec.planExecution or spec.parameters?

[zen-dog]

1. if you kudo plan trigger -p ... we treat the parameters as transient
2. kudo update --instance -p ... we treat the parameters as persistent

The rationale behind it being: if you need to update parameters and trigger a plan, you need simply update the parameters - the plan is triggered anyway. Triggering a plan directly (1) doesn't need -p so we can treat the parameters as transient.

[ANeumann82]

1. If you kudo plan update you can't set transient parameters. I agree with that
2. If you kudo plan trigger you can't change permanent parameters. I kind of agree with that. It might be nice, but it's not really necessary.
3. For kudo install I think there are reasonable use cases to specify both transient and permanent parameters.

The big issue I have when the operator developer can't specify which parameters are transient and which are permanent is that the user can accidentally use them in the wrong context:

For example:

• BACKUP_NAME transient parameter
• NODE_COUNT permanent parameter

If we follow your approach, there is no way to prevent a user from
a) kudo update --instance -p BACKUP_NAME=asdf Now the BACKUP_NAME is stored in spec.parameters where it really doesn't belong
b) kudo plan trigger -p NODE_COUNT=5 The given value would be in spec.parameters with the old value and in spec.planExecution with a transient value. If the triggered plan uses NODE_COUNT it has to decide which value to use. Additionally, the user might expect the NODE_COUNT to be saved.

So, TL;DR: We can't error out when a user uses a parameter in the wrong context and it might lead to wrong assumptions how a parameter is used

[kensipe]

this came up in the community meeting today... there seems to be some agreement that this should not be handled as proposed... it was suggested that we bring back PlanExecution CRD even... which I'm sure would need to have a larger conversation around. I really feel that having different kinds of params isn't the best approach... it seems like a cognitive load on the operator developer. Lets setup a meeting.

[kensipe]

I would also propose change the name... "Resettable Parameters" seems prescriptive..... It seems more like enable arguments to imperative operations or... plan parms... plan args?

[zen-dog]

In summary:

1. this is probably the cleanest solution that keeps most of the existing semantics. The params.yaml clearly states which parameters are transient. Both, kudoctl trigger & update commands work as expected. A mix of transient and persistent parameters is allowed. And we could extend kudoctl to check for all the empty & transient plan parameters before executing a plan to see if the user missed one.
2. second approach, on the other hand, is very intuitive (from a developer point of view) and resembles the most a function call with passed parameters e.g. func backup(BACKUP_NAME string). However, it also moves us further away from the declarative territory towards the YAML-y programming language one.
3. third approach introduces an additional semantic to the plan trigger command. Initially, I liked this one, but after sleeping on it, I believe it has no benefits over the first approach while introducing additional complexity.

This plan->parameter connection is probably the crux here: KUDO's model goes from changing a parameter -> to triggering a defined plan. This KEP basically needs the opposite: a direct plan trigger call -> requires parameters to be passed thus introducing a bidirectional plan <-> parameter connection. This is a confusing UX and will probably be a complex implementation.

After thinking about all approaches my vote goes to the first one.

[zen-dog]

Afaik the main justification for the kudo plan trigger command was always the ability to trigger a plan without a corresponding parameter change. Otherwise, the user might as well simply kudo update the parameter directly. So we might want restrict the kudo plan trigger command to only accept transient parameters (which are marked as such in the definition).

However, this is probably an unnecessary restriction that won't allow a mix of persistent and transient parameters.

[ANeumann82]

Well, we do have a restriction that the changed parameters can not trigger multiple plans, right? So we already have a restriction there, although it does not distinguish between transient/persistent parameters. So I agree, I think we don't need an additional restriction here.

[zen-dog]

This missing link from the parameter-to-its-plan the biggest drawback with approaches (1) and (3). Only (2) solves it but at the cost of introducing a plan specific (and thus non-reusable) resources.

and kudoctl will not be able to determine that a parameter is required to render a plan-specific resource.

I'm not sure this is true. A transient parameter still needs a trigger field with its corresponding plan value:

  - name: BACKUP_NAME
    description: "The name under which the backup is saved."
    transient: true
    trigger: backup

We could use the kudo update command the same way:

$ kudo update --instance op-instance -p BACKUP_NAME=foo

but the parameter is never persisted.

[ANeumann82]

Tbh, at the moment we don't have a parameter-to-plan mapping either. We may have a trigger field on the parameter, but the param can be used in any other plans as well, so I am not sure how strong our mapping here is anyway.

The more I think about it, i'd rather get rid of the trigger and always specify plans explicitly. But that's a discussion for a different KEP.

[zen-dog]

The trigger is always there, it just defaults to deploy if not specified. And to be precise:

• one parameter can be used in many templates so 1:n
• one parameter can trigger only one plan so 1:1
• one plan can be triggered by many parameters so 1:n

[ANeumann82]

Yeah - the "default to deploy" is problematic as well.

There is a relation missing:

• one template can be used in many plans so 1:n

I feel this relationship is flawed. If one parameter can be used in a template, and that template can be used in multiple plans, why does the parameter trigger a single plan?

One of the next things I'll KEP out is probably the "Define No-Plan Trigger" on parameters, because with the BACKUP_NAME parameter we get a problem:

BACKUP_NAME is used by backup plan and restore plan. If the BACKUP_NAME parameter has the backup plan as the trigger, it can't be used together with the restore plan - because specifying multiple parameters that trigger different plans is prevented by the webhook...

[ANeumann82]

Or maybe it's enough to say "If a plan is explicitly triggered, it doesn't matter what trigger attributes the parameters have" - that might work as well...

[zen-dog]

I feel this relationship is flawed. If one parameter can be used in a template, and that template can be used in multiple plans, why does the parameter trigger a single plan?

I guess, the predominant plan is still deploy. Additionally, an n:n mapping would be probably very confusing from the UX perspective. But I'm guessing here.

If the BACKUP_NAME parameter has the backup plan as the trigger, it can't be used together with the restore plan

True. But having a second RESTORE_NAME parameter seems like a small price to pay 🤷

[zen-dog]

The other cons don't look bad but this means that we lose reusability of some resources. Maybe this isn't that bad because these resources aren't reusable in the first place (e.g. backup Job) but I still see this somewhat critical.

[zen-dog]

this is basically:

func backup(NAME string)

which makes it the most intuitive approach (at least for me). However, this also moves us further away from the declarative into YAML-y programming language territory.