KEP: ComponentConfig fields should be optional #1173
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
kind/kep
Proposing that we make all fields in ComponentConfig optional/nilable, as initially discussed in https://groups.google.com/d/topic/kubernetes-wg-component-standard/hO7Lmi0cwQU/discussion
justinsb
force-pushed
the
component-config-fields-should-be-optional
branch
from
1c12898 to
a05f968
Compare
|
@justinsb: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
|
||
| After demonstrating this in v1.16 (ideally in v1.17) we will change the types of | ||
| the fields in kubelet so that they are nilable. This may require introduction | ||
| of a v1beta2 (TBD). |
There was a problem hiding this comment.
Yeah we have to think about this carefully. It would probably "just work" as far as config files, though something else like kubeadm may be relying on the v1beta1 struct types to generate files.
@liggitt can you think of any other edge cases here?
There was a problem hiding this comment.
there are two types of clients to consider:
- clients using the go types... changing fields to pointers would introduce NPE risks when they updated to new versions
- clients using serialized config files. as long as the fields changed to pointers set omitempty, we would not serialize
nullin unexpected places
There was a problem hiding this comment.
changing fields to pointers would introduce NPE risks
the same is true for the kubelet itself, actually, so a very diligent sweep would be required to guard against that
There was a problem hiding this comment.
If we break it up field-by-field and write really good instructions on how to modify and test consumer code, we can get a broad swath of people attacking this. But the isntructions have to be ACTIONABLE.
| We will begin with kube-scheduler and kube-proxy, as those are the smallest; | ||
| there is also more desire to promote those to beta. | ||
|
|
||
| Implementation details TBD! |
There was a problem hiding this comment.
Off the top of my head:
- Make everything a pointer in the external type(s).
- Update defaulting code to check nil everywhere instead of zero values (yay for consistency!).
- Backfill zero-value defaults for previously-non-pointer fields.
- Run codegen.
- Run tests.
Hopefully most logic is based off the internal type and changes are minimal. Some tests that round-trip may need updates.
Might get interesting with deeply-nested structures. Best way to find out is to try it.
There was a problem hiding this comment.
Making everything a pointer is a nice thing from consistency standpoint, however it also looks annoying in Go code. It won't have much impact on users, that treat component configs as YAML templates only, but Go code vendoring users are going to be in for a hard time (or at least, if I understand correctly, for a huge use of k8s.io/utils/pointer).
The real problem here is tied to omitempty and the JSON library, which is very cautious in respecting it. In fact, in non-trivial cases, it won't respect it at all. The Go-YAML library has a IsZeroer interface, but that is not supported in the builtin Go JSON library.
| allows us to prove the value before we change types with stricter versioning | ||
| requirements. | ||
|
|
||
| We risk changing the behaviour of an unspecified field in the componentconfig |
There was a problem hiding this comment.
Is there still a risk if we backfill zero-value defaults?
| ### Goals | ||
|
|
||
| - Allow generation of compact componentconfig using the same types | ||
| - Start to replace some of the "magic sentinel" values that are used to mean "not set" |
There was a problem hiding this comment.
I think conversion v1 <-> v2 will lose set/unset information because it goes through the internal type, even if v1 and v2 consist of all pointers. There's still significant utility to this proposal, but conversions that preserve intent would also be very useful. It's complicated, because the convertor needs to understand defaults for both versions so it can shim correctly, and not sure how easily the current machinery would extend to that.
There was a problem hiding this comment.
you would have to make the fields in the internal type pointers as well if you want to be able to preserve the set/unset aspect after conversion to the internal hub version
| pointer-types in our componentconfig. | ||
|
|
||
| This also allows for generation of a minimal componentconfig from code, and | ||
| should faciliate patching and merging of componentconfig. |
| participating-sigs: | ||
| - sig-cluster-lifecycle | ||
| reviewers: | ||
| - @mtaufen |
There was a problem hiding this comment.
These might need to be quoted, verify-kep-metadata failed.
There was a problem hiding this comment.
Thanks @justinsb !
This is indeed a pressing issue (as we found with kubeadm and v1beta1).
However, I do think, that we need to start at the top. We need to figure out component config storage, conversion and validation responsibilities with respect of cluster lifecycle tools. I talked to @mtaufen about a possible design meeting with regards to these questions on last week's WG Component Standard meeting.
Surely, once these questions are cleared, we would have a lot more understanding to what to do with questions like this one. Like, to what extent to make versioned Go types hard to use.
|
|
||
| ## Proposal | ||
|
|
||
| In v1.16 we will change the types of the fields in the kube-proxy, |
There was a problem hiding this comment.
1.16 is a very optimistic time frame, given the fact, that there is only a month before code freeze and a bit more than a day for the enhancement freeze.
| We will begin with kube-scheduler and kube-proxy, as those are the smallest; | ||
| there is also more desire to promote those to beta. | ||
|
|
||
| Implementation details TBD! |
There was a problem hiding this comment.
Making everything a pointer is a nice thing from consistency standpoint, however it also looks annoying in Go code. It won't have much impact on users, that treat component configs as YAML templates only, but Go code vendoring users are going to be in for a hard time (or at least, if I understand correctly, for a huge use of k8s.io/utils/pointer).
The real problem here is tied to omitempty and the JSON library, which is very cautious in respecting it. In fact, in non-trivial cases, it won't respect it at all. The Go-YAML library has a IsZeroer interface, but that is not supported in the builtin Go JSON library.
| However, while flags make the distinction between not-specified and | ||
| set-to-empty-value, our api types generally do not. By using pointer-types it | ||
| is possible to preserve this difference, this KEP proposes that we should use | ||
| pointer-types in our componentconfig. |
There was a problem hiding this comment.
+1! Lack of this ability has already caused problem for the scheduler. We cannot turn on pprof by default for the scheduler. In order to set the default value of "EnableProfiling" to true, we first need to find that the flag is not provided, but we cannot, because the flag is false both when it is explicitly set to false and when it is not provided. This applies to other boolean and integer options.
There was a problem hiding this comment.
@bsalamat you can tell whether a flag was provided or not using pflag flagset's Changed method:
kubernetes-sigs/cluster-addons@d5f8d8d#diff-fe99745b837e4e6db3d1c39b19f39ed3R41
That can be used for the scheduler's flag logic 👍.
I think what is being expressed here is that is that when using a config file, once you have the struct, you won't be able to get this information unless we make all fields pointers or encode the information somewhere else.
|
/assign @timothysc @neolit123 |
|
/assign |
|
/assign @fabriziopandini |
| should faciliate patching and merging of componentconfig. | ||
|
|
||
| ComponentConfig types are used with v1alpha1 versions for kube-scheduler, | ||
| kube-proxy, kube-controller-manager, cloud-controller-manager; I intend to make |
There was a problem hiding this comment.
kubeadm already consumes kube-proxy v1alpha1, and fear this change - without cutting a new version - might create some problem...
There was a problem hiding this comment.
@justinsb thanks for this write up!
Only one additional point from my side related to the migration from the current version to "all pointer" version.
Currently, conversions are implemented in the "private" part of the component config, while consumers like kubeadm are using the public API. This potentially can lead to forcing each component config consumer to re-implement the conversion logic, which is far from ideal.
IMO, before proceeding with this KEP, it should be discussed how to ensure "smooth" migration from public versions of the same API
| kube-proxy, kube-controller-manager, cloud-controller-manager; I intend to make | ||
| these nilable. kubelet uses a ComponentConfig type that is v1beta1, I propose | ||
| not changing those at the current time, but rather proving out the approach with | ||
| the v1alpha1 types. |
There was a problem hiding this comment.
Might want to put details here wrt if successful we will... This should also serve as a blocker towards graduating other component configs.
|
|
||
| I initially raised this in a [mailing list | ||
| thread](https://groups.google.com/d/topic/kubernetes-wg-component-standard/hO7Lmi0cwQU/discussion) | ||
| with wg-component-standard. When trying to create componentconfig from code, |
|
|
||
| Supporting merging of configurations that better reflects intent has come up a | ||
| few times: there have been discussions of merging fields with flags and the | ||
| precedence when doing so, merging fields set at different levels of a hierarchy |
There was a problem hiding this comment.
I get what you're saying, but I don't think most others would. An example on hierarchical ordering I think would help make your case.
|
/wg component-standard |
k8s-ci-robot
added
the
wg/component-standard
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
|
@fejta-bot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Proposing that we make all fields in ComponentConfig optional/nilable,
as initially discussed in
https://groups.google.com/d/topic/kubernetes-wg-component-standard/hO7Lmi0cwQU/discussion