nil vs zero in componentconfig

[Justin Santa Barbara]

Looking at the kubescheduler componentconfig (for example), I am thinking if we should differentiate between values that are unset vs values that are zero, thoughout all of componentconfig.

For example, this is good:

BindTimeoutSeconds *int64 `json:"bindTimeoutSeconds"`

https://github.com/kubernetes/kube-scheduler/blob/48c345b70465ae710e616991d45403f14b291cd5/config/v1alpha1/types.go#L82-L85

When we don't set that, the field is omitted (well, it would be easy enough to add omitempty!), and it's clear that the field wasn't set by the user, and the default value should be used.

But... how can we tell the difference between "not set" and "false" here?

DisablePreemption bool `json:"disablePreemption"`

https://github.com/kubernetes/kube-scheduler/blob/48c345b70465ae710e616991d45403f14b291cd5/config/v1alpha1/types.go#L69-L70

Relatedly https://github.com/kubernetes/kube-scheduler/blob/48c345b70465ae710e616991d45403f14b291cd5/config/v1alpha1/types.go#L61-L63

MetricsBindAddress string `json:"metricsBindAddress"`

metricsBindAddress defaults to 0.0.0.0:10251, which means that although the value "" appears, that value would actually mean "0.0.0.0:10251".  So how would a user turn off the metrics endpoint?  We typically end up with magic values - like "0" meaning don't bind.  Also this means when we are serving it, we probably would specify "0.0.0.0:10251" and not "", but then that obscures the fact that this is in fact the default value.

Some observations:

• Today we can tell the difference if we use a non-standard yaml parser - but only if we are sourcing from a configmap or a CRD.  But one day CRDs may be backed by protobuf or a comparable binary serializer, and we might not be able to differentiate.  More problematically today, this also means a componentconfig object is not the canonical representation of the configuration; we need the yaml.
• It makes it harder to generate the configuration - we can't use the k8s structs if we want to differentiate between disablePreemption=false and =nil, or if we just want a compact configuration that only specifies a few values (particularly useful if we are one day overlaying these configurations).
• If the default value changes I suspect this makes life a lot harder (c.f. kube-proxy --resource-container flag, kubelet --allow-privileged flag)
• A policy of populating the default values has lead to user dissatisfaction with the size of the yaml - it's harder to see the settings that are changed from the default.

The following are easy changes that don't break existing yaml:

DisablePreemption *bool `json:"disablePreemption,omitempty"`
MetricsBindAddress *string `json:"metricsBindAddress,omitempty"`

I think having a consistent behaviour for the bind addresses that makes explicit and leaves room for additional options e.g. authn/authz would be nice, but would break existing yaml:

Metrics *EndpointConfiguration `json:"metrics,omitempty"`


type EndpointConfiguration struct {
  Enabled *bool `json:"enabled,omitempty"`
  BindAddress string `json:"bindAddress,omitempty"`
  ...
}

My proposal:

• We should today change all the `bool` -> `*bool`, `string` -> `*string`, `int32` -> `*int32` etc, and include `omitempty`.  This should not break any yaml configurations, and it should give us more compact output, and the ability to differentiate not-set vs zero.
• We should look at structuring common behaviors using objects in the next alpha release of these types, in particular so that we have consistency and don't have magic values.

[Michael Taufen]

1) Agree with using pointers for all primitive types. In yaml, everything is a Maybe, anyway. Historically, the advice has been "if you need the distinction, use a pointer," where "if you need the distinction" is usually read as "the default is the same as the zero value." But this has always been harder to reason about than just using pointers to indicate set vs. unset.

2) Housing common componentconfig types is one of the purposes of https://github.com/kubernetes/component-base. We haven't done a whole lot of work to start consolidating things there yet, but your EndpointConfiguration is a good example of a common type.

--
You received this message because you are subscribed to the Google Groups "kubernetes-wg-component-standard" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-componen...@googlegroups.com.
To post to this group, send email to kubernetes-wg-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-wg-component-standard/4e262c7a-8c82-415a-bafe-3fab92bc489b%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

--

Michael Taufen

Google SWE

[Michael Taufen]

It's probably worth a short KEP and API review, since it's a project-wide (albeit compatible) change in API conventions.

[Lucas Käldström]

Agree on the above proposals. Also want to note that the good thing with internal/external types here is that we can have more or less everything as pointers in the external type, but convert into non-pointers as needed in the internal type.

Thanks Justin!

---

From: 'Michael Taufen' via kubernetes-wg-component-standard <kubernetes-wg-co...@googlegroups.com>
Sent: Wednesday, July 10, 2019 9:39:02 PM
To: Justin Santa Barbara
Cc: kubernetes-wg-component-standard
Subject: Re: nil vs zero in componentconfig

 

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-wg-component-standard/CAHJadQJCECrKk1e2-V%3DVOxc8Vk8Mrq41bPH4-VhDQjXhgg6O%2Bg%40mail.gmail.com.

[Stefan Schimanski]

On Wed, Jul 10, 2019 at 7:01 PM 'Justin Santa Barbara' via kubernetes-wg-component-standard <kubernetes-wg-co...@googlegroups.com> wrote:

My proposal:

• We should today change all the `bool` -> `*bool`, `string` -> `*string`, `int32` -> `*int32` etc, and include `omitempty`.  This should not break any yaml configurations, and it should give us more compact output, and the ability to differentiate not-set vs zero.

For componentconfigs–which do not run through defaulting as REST resources do–this makes a lot of sense IMO.

Such a convention is especially important if we want to implement merging of multiple configs.

• We should look at structuring common behaviors using objects in the next alpha release of these types, in particular so that we have consistency and don't have magic values.

+1

[Justin Santa Barbara]

Thanks for the great input all.  Sent an initial draft of a KEP for this: https://github.com/kubernetes/enhancements/pull/1173

I don't know if we'll be able to get this approved/done for 1.16, I mostly wanted to get it up for discussion so I could point to it from the other KEPs that seem to be proposing promoting e.g. kube-scheduler componentconfig to beta; we need to make a sequencing decision!

On Wednesday, July 10, 2019 at 4:03:55 PM UTC-4, Stefan Schimanski wrote:

[Michael Taufen]

Thanks Justin, I'll try to take a look by Monday.

--

You received this message because you are subscribed to the Google Groups "kubernetes-wg-component-standard" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-wg-componen...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-wg-component-standard/54c488eb-5c84-469c-b1ed-39497508fa57%40googlegroups.com.