KEP-30: Immutable parameters #1485
Conversation
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
ANeumann82
requested review from
alenkacz,
gerred,
kensipe,
nfnt and
zen-dog
as code owners
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
keps/0030-immutable-parameters.md
Outdated
|
|
||
| ### Implementation Details/Notes/Constraints | ||
|
|
||
| The KUDO CLI will not check if a parameter is immutable to allow providing an immutable parameter on the CLI or in a parameter file without changing it's value. This allows the user to keep the full operator configuration in a parameter file and use that for installation and updates. |
There was a problem hiding this comment.
| The KUDO CLI will not check if a parameter is immutable to allow providing an immutable parameter on the CLI or in a parameter file without changing it's value. This allows the user to keep the full operator configuration in a parameter file and use that for installation and updates. | |
| The KUDO CLI will not check if a parameter is immutable to allow providing an immutable parameter on the CLI or in a parameter file without changing its value. This allows the user to keep the full operator configuration in a parameter file and use that for installation and updates. |
There was a problem hiding this comment.
I don't understand this... IF we have params which are mutable and immutable... it seems to reason that an example of A, B and C params, where B is immutable, would result in an installation of A, B and C but and update of A and C. So the declaration that the same file could be used for installation and updates doesn't make sense.
There was a problem hiding this comment.
Well, the alternative is that the KUDO CLI checks that a parameter is immutable independently of the value. If the CLI detects that kudo update is called with an immutable parameter it would error out.
The alternative is that immutable parameters can be specified with kudo update but their value will be silently ignored, which goes against "Fail loud and fast" principle
There was a problem hiding this comment.
I don't see the alt approach (last paragraph) as a violation of "fail load and fast". I'm huge fan of "fail load and fast"... but to me that doesn't mean punish the user or reduce UX. It means when the current experience is outside of the defined experience and expectations than fail... don't assume / hide. We should design a good and convenient user experience.
There was a problem hiding this comment.
in the example of params: A, B and C, where B is immutable.. it should be ok for an update to have params for all 3 (as long as B hasn't changed values)... but it should also be ok for an update where A and C are specified (which implicitly means there is no change on B).
I believe this now states this is possible ... but we should accept the grammar change suggestion.
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
Updated KEP overview Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
keps/0030-immutable-parameters.md
Outdated
| - When executing `kudo upgrade` the user has to explicitly set the value for parameters that are changed from mutable to immutable | ||
| - If an parameter is changed from mutable to immutable and the user does not explicitly specifies a parameter the upgrade will abort with an error message | ||
| - This ensures that the user knows about the new immutability |
There was a problem hiding this comment.
For upgrades, we could silently use the current value, but I think it's good practice to get an informed user decision. This case shouldn't happen too often and therefore I think it's ok to make it a bit harder to use.
| - When executing `kudo upgrade` the user has to explicitly set the value for the new parameter, even if the parameter has a default value | ||
| - If the user does not provide a value on `kudo upgrade`, the upgrade will abort with an error message that contains the (optional) default value | ||
| - This ensures that the user knows about the new parameter and that it can not be changed after the upgrade |
There was a problem hiding this comment.
Same here - if the new param has a default value, we could silently use it, but this won't work without any default value, and as above, I think it's good practice to let users know about things they can't undo later on.
There was a problem hiding this comment.
soooo close
A couple of things to resolve then lets get this merged.
I really don't like the error without specifying all params... but it is mainly because we don't know the number of params impacted... we could guess it is low..
I would also like to get the non-goal cleaned up more (as specified)
Also I thought more about it as I'm submitting... there isn't anything blocking really anymore... I'm switching to approved... that said I would still prefer some cleanup or ack of counter proposal before a merge.
keps/0030-immutable-parameters.md
Outdated
| - Upgrades to a new OperatorVersions: | ||
| - A parameter can be made immutable in a newly released OperatorVersion. | ||
| - When executing `kudo upgrade` the user has to explicitly set the value for parameters that are changed from mutable to immutable | ||
| - If an parameter is changed from mutable to immutable and the user does not explicitly specifies a parameter the upgrade will abort with an error message |
There was a problem hiding this comment.
| - If an parameter is changed from mutable to immutable and the user does not explicitly specifies a parameter the upgrade will abort with an error message | |
| - If an parameter is changed from mutable to immutable and the user does not explicitly specify a parameter the upgrade will abort with an error message |
fixed the grammar... however I'm not sure I like this...
There was a problem hiding this comment.
after some thought... if you want user input.. perhaps an ack flag of some sort. --force or --params-upgrade
specifying every param is the cognitive load we should avoid
There was a problem hiding this comment.
Mmmm, i'm torn on this one. I really want to make sure users do not accidentally set an immutable parameter they don't know about.
I see two options:
- The user has to specify all new immutable parameters explicitly. This has to be done for immutable parameters without a default in any case, and I don't think there will be that many new immutable parameters introduced to a new operator version.
- Add a new parameter to
kudo upgrade. If this is specified, the upgrade process would automatically use the default values. I think this would be an extra parameter for a very specific use case that won't be used often, and it may lead to users adding this parameter to their tooling, ignoring new immutable parameters.
As said above, I think the use case here will happen very seldom and I think it's acceptable for users to specify all parameters. Especially if parameter-files are used. KUDO should error with all missing parameters so this would only be one round trip: Call kudo upgrade .... -> See missing parameters -> Add parameters to param-file -> Call kudo upgrade again.
If you insist on a parameter I'm ok with it but I would prefer to not use one.
There was a problem hiding this comment.
I don't know how it is possible to know the freq or likelihood of this situation. we also don't know the magnitude of impact (number of changes for an upgrade). It seems that the goal is to make sure the user is aware of this situation. It is a reasonable assumption that 1 of 2 things will happen for an upgrade of this kind... either they know or they don't. IF they don't they will try to do an upgrade and an error will occur... now they are aware. I suggest that the error returned is a list of all violations (all params that were mutable that became immutable). Now they are aware.. to force them to specify each one is to punish the user... (and to punish them for something the operator dev did.. not them). A flag that indicates they are aware should be good enough. I could imagine that if the error was generic that in your model, specifying each param is ensuring they know of each and every one... but it would like result in a try/fail/trywith(a)/fail/trywith(a,b)/fail/trywith(a,b,c)/succeed. We should provide an error with a list of all parameters to be aware of... if they have that information, then why punish them with jump through hoops?
your model seems to expect param-file... I'm not sure if they will have this... also, they may be upgrading without any changed params (so no param-file at all).
I think the better UX is to have the ack flag.
While I think it is a much better UX... It isn't a blocker for me.
Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>
|
@ANeumann82 nice work on this KEP! I suggestion a rebase (to rid the testing error) and accept the grammar changes and lets merge it. As stated, I have a preference for a change with upgrades of mutable to immutable... but if you disagree after reading and considering... lets merge with what you think is best. |
Signed-off-by: Andreas Neumann aneumann@mesosphere.com