Pluralizing existing API fields - pattern needed

[Tim Hockin]

Since I will be out for the next 3 sig-arch meetings, I am putting this here instead of on the agenda.

context: https://github.com/kubernetes/community/pull/2254

We have an existing field `Pod.spec.podIP` which holds a single value.  The proposal is to support dual-stack networking (IPv6 and IPv4 at the same time, very common), which requires that a pod report multiple IP addresses.

Our API changes doc (https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#on-compatibility) talks about adding a second field for "extra values", e.g. `extraPodIPs`.  The point of debate here is how to do this is a maximally compatible and safe way in the face of version skew, and what guarantees can be made.

If we demand that the new field (the plural) is exclusive of the singular field, we face a compat problem.  Older clients might not know to update the plural.  Consider an existing pod with `PodIP = "X"` and `PodIPs" = [ "Y", "Z" ]`.  If we have a client that only knows `PodIP`, and it updates that field to "Z", the exclusivity invariant is violated.

That effectively means that a new field has changed the meaning of an old field, and a client operation which used to work will no longer work.  Bad.

If we demand that the new field (the plural) is inclusive of the singular field, we face a similar problem in the other direction.  Consider `PodIP = "X"` and `PodIPs" = [ "X", "Y" ]`.  If an older client updates `podIP` field to "Z", the inclusivity invariant is violated.  Again, a new field has changed the meaning of an old field.

So we're basically forced to make it an optionally-inclusive relationship.  And now we have to answer situations like this: Consider `PodIP = "X"` and `PodIPs" = [ "X", "Y" ]`.  If an older client updates `podIP` field to "Z", should "X" be removed from the plural field?

It gets a little more complex because we want to include metadata about IPs in the plural structure, so simple replacement is more nuanced.

There are other options like making the fields more "linked" (e.g. one piece of metadata is "is-primary" which is always in sync with the singular field (and defaulting / validation could maybe handle the corner cases).  This crosses boundaries of changing user-provided data in ways that we don't generally approve of.

Other ideas are welcome.

I wanted to put this in from of the sig-arch group for consideration, since I suspect this will happen again as APIs evolve.  This could set a precedent.

Tim

[Brian Grant]

Something similar was discussed with nodeSelector, but wasn't implemented:

https://github.com/kubernetes/kubernetes/issues/341#issuecomment-140809259

podIP is in status, so the main concern is readers rather than writers. 

Are there other fields we need to consider?

--
You received this message because you are subscribed to the Google Groups "kubernetes-sig-architecture" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-arch...@googlegroups.com.
To post to this group, send email to kubernetes-si...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/CAO_RewaAx6gRmwyL5nipr7wL5Pvu86xzrFCPjD-0W%3D%3DpKa%2BKVw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Brian Grant]

On Mon, Jul 16, 2018 at 10:58 PM Brian Grant <brian...@google.com> wrote:

Something similar was discussed with nodeSelector, but wasn't implemented:

https://github.com/kubernetes/kubernetes/issues/341#issuecomment-140809259

podIP is in status, so the main concern is readers rather than writers. 

Looking at the podIP case, it seems like the additional IPs all need to be aliases for the same interface, so a new field called podIPAliases or some such would make sense. The ipv4 address, if any, would go into the existing field, and an ipv6 address would go into the aliases field, as it's effectively a new feature.

Multiple interfaces would need some kind of structure that would permit association of addresses and interfaces.

More generally, old external clients and configuration must be able to be oblivious to new fields and still be able to work. If a client utilizes new fields, it also needs to understand pre-existing fields. 

We assume that spec fields may be written explicitly by programmatic clients, may be part of read-modify-write sequences, and may be applied (as in kubectl apply) as declarative configuration specified by the user. The system cannot change the manner in which fields are specified by the user. And representing the same information in multiple ways in the spec in the same API version is impossible to make work.

Status fields are written by controllers and read by other clients. For pluggable controllers (e.g. the scheduler), status fields may be written by external clients, but we put the burden of compatibility on controller authors, since otherwise evolution would be practically impossible.

Interoperation of multiple clients can be hard in the case of inter-related fields. Introduction of new fields that change the meanings of existing fields are highly problematic. Technically pre-existing optional fields may be omitted, but practically may break clients if they've so far always been present.

Since it's so challenging to maintain strict backward compatibility, we've typically had to make judgement calls of risk/value on a case-by-case basis.

[Paul Michali]

On Tuesday, July 17, 2018 at 12:45:17 PM UTC-4, Brian Grant wrote:

On Mon, Jul 16, 2018 at 10:58 PM Brian Grant <brian...@google.com> wrote:

Something similar was discussed with nodeSelector, but wasn't implemented:

https://github.com/kubernetes/kubernetes/issues/341#issuecomment-140809259

podIP is in status, so the main concern is readers rather than writers. 

Looking at the podIP case, it seems like the additional IPs all need to be aliases for the same interface, so a new field called podIPAliases or some such would make sense. The ipv4 address, if any, would go into the existing field, and an ipv6 address would go into the aliases field, as it's effectively a new feature.

A small, but important correction to your statement...

As of v1.9, the existing field could have an IPv4 address or an IPv6 address. One could have a network with each pod only having an IPv6 address (the IPv6 only case).

With the dual stack proposal, the added cases are: (only) multiple IPv6 addressed, and an IPv4 address and one or more IPv6 addresses. The challenge being handling how to represent these additional cases and maintain backwards compatibility.

Regards,

PCM

Multiple interfaces would need some kind of structure that would permit association of addresses and interfaces.

More generally, old external clients and configuration must be able to be oblivious to new fields and still be able to work. If a client utilizes new fields, it also needs to understand pre-existing fields. 

We assume that spec fields may be written explicitly by programmatic clients, may be part of read-modify-write sequences, and may be applied (as in kubectl apply) as declarative configuration specified by the user. The system cannot change the manner in which fields are specified by the user. And representing the same information in multiple ways in the spec in the same API version is impossible to make work.

Status fields are written by controllers and read by other clients. For pluggable controllers (e.g. the scheduler), status fields may be written by external clients, but we put the burden of compatibility on controller authors, since otherwise evolution would be practically impossible.

Interoperation of multiple clients can be hard in the case of inter-related fields. Introduction of new fields that change the meanings of existing fields are highly problematic. Technically pre-existing optional fields may be omitted, but practically may break clients if they've so far always been present.

Since it's so challenging to maintain strict backward compatibility, we've typically had to make judgement calls of risk/value on a case-by-case basis.

Are there other fields we need to consider?

On Thu, Jul 12, 2018 at 12:47 PM 'Tim Hockin' via kubernetes-sig-architecture <kubernetes-si...@googlegroups.com> wrote:

Since I will be out for the next 3 sig-arch meetings, I am putting this here instead of on the agenda.

context: https://github.com/kubernetes/community/pull/2254

We have an existing field `Pod.spec.podIP` which holds a single value.  The proposal is to support dual-stack networking (IPv6 and IPv4 at the same time, very common), which requires that a pod report multiple IP addresses.

Our API changes doc (https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#on-compatibility) talks about adding a second field for "extra values", e.g. `extraPodIPs`.  The point of debate here is how to do this is a maximally compatible and safe way in the face of version skew, and what guarantees can be made.

If we demand that the new field (the plural) is exclusive of the singular field, we face a compat problem.  Older clients might not know to update the plural.  Consider an existing pod with `PodIP = "X"` and `PodIPs" = [ "Y", "Z" ]`.  If we have a client that only knows `PodIP`, and it updates that field to "Z", the exclusivity invariant is violated.

That effectively means that a new field has changed the meaning of an old field, and a client operation which used to work will no longer work.  Bad.

If we demand that the new field (the plural) is inclusive of the singular field, we face a similar problem in the other direction.  Consider `PodIP = "X"` and `PodIPs" = [ "X", "Y" ]`.  If an older client updates `podIP` field to "Z", the inclusivity invariant is violated.  Again, a new field has changed the meaning of an old field.

So we're basically forced to make it an optionally-inclusive relationship.  And now we have to answer situations like this: Consider `PodIP = "X"` and `PodIPs" = [ "X", "Y" ]`.  If an older client updates `podIP` field to "Z", should "X" be removed from the plural field?

It gets a little more complex because we want to include metadata about IPs in the plural structure, so simple replacement is more nuanced.

There are other options like making the fields more "linked" (e.g. one piece of metadata is "is-primary" which is always in sync with the singular field (and defaulting / validation could maybe handle the corner cases).  This crosses boundaries of changing user-provided data in ways that we don't generally approve of.

Other ideas are welcome.

I wanted to put this in from of the sig-arch group for consideration, since I suspect this will happen again as APIs evolve.  This could set a precedent.

Tim

--
You received this message because you are subscribed to the Google Groups "kubernetes-sig-architecture" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-architecture+unsub...@googlegroups.com.

[Brian Grant]

On Wed, Jul 18, 2018 at 3:40 AM Paul Michali <pcmi...@gmail.com> wrote:

On Tuesday, July 17, 2018 at 12:45:17 PM UTC-4, Brian Grant wrote:

On Mon, Jul 16, 2018 at 10:58 PM Brian Grant <brian...@google.com> wrote:

Something similar was discussed with nodeSelector, but wasn't implemented:

https://github.com/kubernetes/kubernetes/issues/341#issuecomment-140809259

podIP is in status, so the main concern is readers rather than writers. 

Looking at the podIP case, it seems like the additional IPs all need to be aliases for the same interface, so a new field called podIPAliases or some such would make sense. The ipv4 address, if any, would go into the existing field, and an ipv6 address would go into the aliases field, as it's effectively a new feature.

A small, but important correction to your statement...

As of v1.9, the existing field could have an IPv4 address or an IPv6 address. One could have a network with each pod only having an IPv6 address (the IPv6 only case).

If an existing client expects an ipv4 address in the podIP field, and there is an ipv6 address there, or vice versa, then there's pretty much no way to work this work.

I'll read and comment on the proposal so that I can better understand the scenarios you're trying to make work.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-arch...@googlegroups.com.

To post to this group, send email to kubernetes-si...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/CAO_RewaAx6gRmwyL5nipr7wL5Pvu86xzrFCPjD-0W%3D%3DpKa%2BKVw%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--

You received this message because you are subscribed to the Google Groups "kubernetes-sig-architecture" group.

To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-arch...@googlegroups.com.

To post to this group, send email to kubernetes-si...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/4160a123-7fa2-47d9-ba36-bb4cabd180c1%40googlegroups.com.

[Paul Michali]

If the podIP has an IPv6 address, the older clients would be expecting an IPv6 address.

Let me try to state the cases...

Existing:

Cluster is set up for IPv4 (only). Each pod has a single IPv4 address, and the podIP field has that address.

Cluster is set up for IPv6(only). Each pod has a single IPv6 address, and the podIP field has that address.

New:

Cluster is dual-stack. Pods would have an IPv4 and 1+ IPv6 addresses.

Two proposals have been floated for either having the IP in podIP be inclusive or exclusive, w.r.t. the podIPs field. The thought has been that the podIP field would be the "primary" address, and I guess that could be IPv4 or IPv6.

The concern is how to handle this with backwards compatibility, especially when older clients may be replacing the IP, using one that is in the podIPs field.

Does that clarify the issue a bit more?

Regards,

PCM

[Tim Hockin]

The existing field can be either v4 or v6, but is singular.  The proposal at hand is to support both v4 and v6, though I think that any design that doesn't extend to multiple addresses of the same family will be trouble in the not very distant future anyway, so we're better off so solve this at the same time.

And these are not necessarily aliases, they could be multiple interfaces in a pod.

That said, I agree that there's almost certainly only one writer of this info (kubelet), so the discussion here is mostly academic.

Perhaps the "clean" answer is to link the fields.  If a client provides the singular form only, we clear the plural form.  If a client provides only the plural, it must be inclusive (or exclusive, but inclusive seems more useful with metadata) of the singular (validated statically).

That is backwards compatible (old clients work as before) and future safe (new API versions would have only the plural).  Practically this makes rollback to older nodes safe, too.

I don't know if we have precedent for this sort of behavior, or if we would want to consider this precedent-setting.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/CAKCBhs62JcKiWxT8eSd7qwzmGtZxfOUwUJvHKPDZtV3nsUpKFw%40mail.gmail.com.

[Brian Grant]

On Wed, Jul 18, 2018 at 9:09 AM Tim Hockin <tho...@google.com> wrote:

The existing field can be either v4 or v6, but is singular.  The proposal at hand is to support both v4 and v6, though I think that any design that doesn't extend to multiple addresses of the same family will be trouble in the not very distant future anyway, so we're better off so solve this at the same time.

And these are not necessarily aliases, they could be multiple interfaces in a pod.

I don't see how this approach can work for multiple interfaces. Should we solve that use case also at the same time? 

[Tim Hockin]

The number or kind of interfaces should not be part of the  conversation, if we can avoid it.  The construct is connectivity - can a pod reach X.  Whether that is multi-network or multi-family or link-local addressing or something else.

What issue do you see for multi-interface? 

NB we may want to list interface name in the metadata (or not, TBD) but it's not required.

[Brian Grant]

On Wed, Jul 18, 2018 at 11:06 AM Tim Hockin <tho...@google.com> wrote:

The number or kind of interfaces should not be part of the  conversation, if we can avoid it.  The construct is connectivity - can a pod reach X.  Whether that is multi-network or multi-family or link-local addressing or something else.

What issue do you see for multi-interface? 

NB we may want to list interface name in the metadata (or not, TBD) but it's not required.

All of the use cases of which I'm aware segregate different types of traffic to different networks: red/black, control/data, primary/fallback, internal/external, etc. Which means applications need to know which addresses correspond to which. I can see that some single-network components could use network locality to prefer addresses on the same network as they are on (or link-local addresses), but that doesn't seem sufficient for multi-network applications, NetworkPolicy, other networking infrastructure, and probably other cases.

[Tim Hockin]

We do have to address that, though a large portion of those problems should be handled by simple IP routing.  The idea of "why" is part of the metadata proposed in the KEP (or in the comments thereof).

[Brian Grant]

On Wed, Jul 18, 2018 at 11:25 AM Tim Hockin <tho...@google.com> wrote:

We do have to address that, though a large portion of those problems should be handled by simple IP routing.  The idea of "why" is part of the metadata proposed in the KEP (or in the comments thereof).

It's in the KEP: The pod status API changes will include a per-IP string map for arbitrary annotations, as a placeholder for future Kubernetes enhancements. This mapping is not required for this dual-stack design, but will allow future annotations, e.g. allowing a CNI network plugin to indicate to which network a given IP address applies. 

[Tim Hockin]

Does that address your concerns on that topic?

[Brian Grant]

On Wed, Jul 18, 2018 at 9:09 AM Tim Hockin <tho...@google.com> wrote:

The existing field can be either v4 or v6, but is singular.  The proposal at hand is to support both v4 and v6, though I think that any design that doesn't extend to multiple addresses of the same family will be trouble in the not very distant future anyway, so we're better off so solve this at the same time.

And these are not necessarily aliases, they could be multiple interfaces in a pod.

That said, I agree that there's almost certainly only one writer of this info (kubelet), so the discussion here is mostly academic.

Perhaps the "clean" answer is to link the fields.  If a client provides the singular form only, we clear the plural form.  If a client provides only the plural, it must be inclusive (or exclusive, but inclusive seems more useful with metadata) of the singular (validated statically).

A client can't provide only the plural. That would break all consumers of the singular form, and a client understanding the plural must be a newer client, so putting the burden on it to provide both makes sense.

We should look at whether syncPod would clear the plural field in the case of a rollback:

https://github.com/kubernetes/kubernetes/blob/f4668d281cec0a8286f7bff8fe7f4109d8dfa88b/pkg/kubelet/status/status_manager.go#L495

[Brian Grant]

On Wed, Jul 18, 2018 at 11:40 AM Tim Hockin <tho...@google.com> wrote:

Does that address your concerns on that topic?

I finished reading the proposal.

A list that includes the extra properties for all IP addresses does address my concerns, and is what I expected, but sounded different from what was being discussed in this thread.

    // Default IP address allocated to the pod. Routable at least within the
    // cluster. Empty if not yet allocated.
    PodIP string `json:"podIP,omitempty" protobuf:"bytes,6,opt,name=podIP"`

    // IP address information. Each entry includes:
    //    IP: An IP address allocated to the pod. Routable at least within
    //        the cluster.
    //    Properties: Arbitrary metadata associated with the allocated IP.
    // This list is inclusive, i.e. it includes the default IP stored in the
    // "PodIP" field. It is empty if no IPs have been allocated yet.
    type PodIPInfo struct {
        IP string
        Properties map[string]string
    }

    // IP addresses allocated to the pod with associated metadata.
    PodIPs []PodIPInfo `json:"podIPs,omitempty" protobuf:"bytes,6,opt,name=podIPs"`

 

[Tim Hockin]

I would be fine requiring "new" clients to set both singular and plural.  I was offering a minor shortcut for cases where no change to the primary IP is needed.

Another twist could be to have "is primary" as metadata in the new field. new clients" would set the plural field, which would (server-side) sync the primary to the singular field.  Old clients would set the singular field which would clear the plural field.  Any update which set both singular and plural would have to agree on primary.

But simply demanding the client set both, even if not changing the singular, could be fine.  There's really only one writer of this - kubelet.

[Tim Hockin]

Circling back on this before I take a little break from keyboards.

I am proposing to establish the following as a convention for cases of an existing field which needs to be pluralized.

Given an existing field `widget`, and a desire to add support for multiple values:

* Add a new optional field, e.g. `widgets`.

* The new plural field, if specified, is inclusive of the old singular value

* If a client writes only the old singular field, the new plural field is cleared

* If a client writes the new plural field, they must also write the singular field

  - the client may omit the singular field if and only if the value would not have changed

  - the inclusive property must be maintained

* Validation logic must enforce these rules synchronously - bad API requests must fail

[Daniel Smith]

On Thu, Jul 12, 2018 at 12:47 PM 'Tim Hockin' via kubernetes-sig-architecture <kubernetes-si...@googlegroups.com> wrote:

Since I will be out for the next 3 sig-arch meetings, I am putting this here instead of on the agenda.

context: https://github.com/kubernetes/community/pull/2254

We have an existing field `Pod.spec.podIP` which holds a single value.  The proposal is to support dual-stack networking (IPv6 and IPv4 at the same time, very common), which requires that a pod report multiple IP addresses.

Our API changes doc (https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#on-compatibility) talks about adding a second field for "extra values", e.g. `extraPodIPs`.  The point of debate here is how to do this is a maximally compatible and safe way in the face of version skew, and what guarantees can be made.

If we demand that the new field (the plural) is exclusive of the singular field, we face a compat problem.  Older clients might not know to update the plural.  Consider an existing pod with `PodIP = "X"` and `PodIPs" = [ "Y", "Z" ]`.  If we have a client that only knows `PodIP`, and it updates that field to "Z", the exclusivity invariant is violated.

That effectively means that a new field has changed the meaning of an old field, and a client operation which used to work will no longer work.  Bad.

If we demand that the new field (the plural) is inclusive of the singular field, we face a similar problem in the other direction.  Consider `PodIP = "X"` and `PodIPs" = [ "X", "Y" ]`.  If an older client updates `podIP` field to "Z", the inclusivity invariant is violated.  Again, a new field has changed the meaning of an old field.

I have not read the rest of this thread, maybe it is covered, but this is wrong. The old client--for most values of old clients--will also completely omit the PodIPs field, so it is not ambiguous at the server what happened: PodIPs[0] should be automatically changed. We can actually do it the other way, too-- if PodIPs is present and PodIP is not, we can automatically update PodIP. The only ambiguous case is when a client sends both PodIP AND PodIPs, AND PodIP != PodIPs[0]. We can send an error on that case, as clients have to go out of their way to actually do that.

... at least that is how it should work. Until the apply work is finished, some server-side special casing would be needed to make e.g. PATCH do the right thing.

OK, I skipped ahead to the end of the thread.

I am proposing to establish the following as a convention for cases of an existing field which needs to be pluralized.
Given an existing field `widget`, and a desire to add support for multiple values:
* Add a new optional field, e.g. `widgets`.
* The new plural field, if specified, is inclusive of the old singular value

 

Yes.

 

* If a client writes only the old singular field, the new plural field is cleared

I think this is not necessary, though it may be expedient.

 

* If a client writes the new plural field, they must also write the singular field
  - the client may omit the singular field if and only if the value would not have changed
  - the inclusive property must be maintained

This kind of breaks apply.

 

* Validation logic must enforce these rules synchronously - bad API requests must fail

I think I would have a different set of rules.

1) The client must specify only the old or the new field and omit the other; it may send both fields only if they match.

2) The server must include a defaulting function which fixes it up in either direction.

The losers in this situation are "dynamic" clients which GET the entire object and use PUT to modify the field instead of PATCH. (Clients with static types compiled in will drop the unknown fields and work fine.)

We can, with more work, make even these clients work by comparing old and new objects in the PUT handler: for this we'd require, instead of 1) above, that if the client sends both PodIP and PodIPs, it must change at most one of those fields-- we require one of them to exactly match the prior object in order to determine which the user changed.

 

[Tim Hockin]

On Fri, Aug 3, 2018 at 4:39 PM Daniel Smith <dbs...@google.com> wrote:

>> I am proposing to establish the following as a convention for cases of an existing field which needs to be pluralized.
>> Given an existing field `widget`, and a desire to add support for multiple values:
>> * Add a new optional field, e.g. `widgets`.
>> * The new plural field, if specified, is inclusive of the old singular value
>
>
> Yes.
>
>>
>> * If a client writes only the old singular field, the new plural field is cleared
>
>
> I think this is not necessary, though it may be expedient.

If we don't do this, we can't guarantee the inclusivity. We can't
simply add singular to plural because we don't know the intention.
E.g.

Singular: X
Plural: [X, Y]

User sets Singular=Z

Did they want [X, Y, Z] or [Y, Z] or [Z, Y]? What if plural carries
metadata? We can't necessarily infer it. Easier to just nuke plural
in that case - client used teh old API which only afforded for a
single value.

>> * If a client writes the new plural field, they must also write the singular field
>> - the client may omit the singular field if and only if the value would not have changed
>> - the inclusive property must be maintained
>
>
> This kind of breaks apply.

Explain? Think of it this way - the update verifier will verify
inclusivity. If a patch maintains inclusivity, it should be OK?

>> * Validation logic must enforce these rules synchronously - bad API requests must fail
>
>
> I think I would have a different set of rules.
>
> 1) The client must specify only the old or the new field and omit the other; it may send both fields only if they match.
> 2) The server must include a defaulting function which fixes it up in either direction.

This could work, but it would involve changing fields that were
previously user-set. How does that map to apply and patch and teh
saved snapshots?

E.g.

T0: User sets singular to X
T1: User sets plural to [Y, Z] ; server changes singular to Y (plural[0])

Is that OK?

> The losers in this situation are "dynamic" clients which GET the entire object and use PUT to modify the field instead of PATCH. (Clients with static types compiled in will drop the unknown fields and work fine.)
>
> We can, with more work, make even these clients work by comparing old and new objects in the PUT handler: for this we'd require, instead of 1) above, that if the client sends both PodIP and PodIPs, it must change at most one of those fields-- we require one of them to exactly match the prior object in order to determine which the user changed.

I want to write this case down - it's come up a few times and people
are not thinking it through well enough or consistently.

[Brian Grant]

Sorry, catching up.

On Wed, Jul 18, 2018 at 3:50 PM Tim Hockin <tho...@google.com> wrote:

I would be fine requiring "new" clients to set both singular and plural.  I was offering a minor shortcut for cases where no change to the primary IP is needed.

Another twist could be to have "is primary" as metadata in the new field. new clients" would set the plural field, which would (server-side) sync the primary to the singular field.  Old clients would set the singular field which would clear the plural field.  Any update which set both singular and plural would have to agree on primary.

I'd strongly prefer that we don't introduce complex inter-field and/or inter-component interactions such as that. We almost always eventually regret it.

[Daniel Smith]

On Mon, Aug 13, 2018 at 11:20 AM Tim Hockin <tho...@google.com> wrote:

On Fri, Aug 3, 2018 at 4:39 PM Daniel Smith <dbs...@google.com> wrote:
>> * If a client writes only the old singular field, the new plural field is cleared
>
>
> I think this is not necessary, though it may be expedient.

If we don't do this, we can't guarantee the inclusivity.  We can't
simply add singular to plural because we don't know the intention.
E.g.

Singular: X
Plural: [X, Y]

User sets Singular=Z

Did they want [X, Y, Z] or [Y, Z] or [Z, Y]?

IMO it's completely unambiguous that they want [Z, Y]. They deleted X and added Z.

 

  What if plural carries metadata?

Metadata for X should be cleared.

 

  We can't necessarily infer it.  Easier to just nuke plural
in that case

They did not ask to delete Y, so it is wrong to do so.

(Note: I am assuming this is an associative list. If it is an atomic list then I agree it's correct to delete the whole thing, as definitionally merging it is not possible. But I think an expansion resulting in an atomic list will happen rarely or never.)

 

- client used teh old API which only afforded for a
single value.

>> * If a client writes the new plural field, they must also write the singular field
>>   - the client may omit the singular field if and only if the value would not have changed
>>   - the inclusive property must be maintained
>
>
> This kind of breaks apply.

Explain?  Think of it this way - the update verifier will verify
inclusivity.  If a patch maintains inclusivity, it should be OK?

It makes it impossible to maintain a desired state object that only expresses an opinion on the new field.

 

> I think I would have a different set of rules.
>
> 1) The client must specify only the old or the new field and omit the other; it may send both fields only if they match.
> 2) The server must include a defaulting function which fixes it up in either direction.

This could work, but it would involve changing fields that were
previously user-set.  How does that map to apply and patch and teh
saved snapshots?

E.g.

T0: User sets singular to X
T1: User sets plural to [Y, Z] ; server changes singular to Y (plural[0])

Is that OK?

Emphatically yes. It should generate all the standard errors/warnings when acquiring ownership of a new field, of course. Apply must support ownership acquisition and relinquishment.

[Brian Grant]

+Antoine Pelisse 

On Tue, Jul 31, 2018 at 4:35 PM Tim Hockin <tho...@google.com> wrote:

Circling back on this before I take a little break from keyboards.

I am proposing to establish the following as a convention for cases of an existing field which needs to be pluralized.

Given an existing field `widget`, and a desire to add support for multiple values:

* Add a new optional field, e.g. `widgets`.

* The new plural field, if specified, is inclusive of the old singular value

* If a client writes only the old singular field, the new plural field is cleared

Ok. This is similar to what I proposed for discriminated unions.

https://github.com/kubernetes/kubernetes/issues/35345

 

* If a client writes the new plural field, they must also write the singular field

  - the client may omit the singular field if and only if the value would not have changed

In a patch, presumably. (That is, not PUT.)

[Brian Grant]

On Fri, Aug 3, 2018 at 4:39 PM 'Daniel Smith' via kubernetes-sig-architecture <kubernetes-si...@googlegroups.com> wrote:

On Thu, Jul 12, 2018 at 12:47 PM 'Tim Hockin' via kubernetes-sig-architecture <kubernetes-si...@googlegroups.com> wrote:

Since I will be out for the next 3 sig-arch meetings, I am putting this here instead of on the agenda.

context: https://github.com/kubernetes/community/pull/2254

We have an existing field `Pod.spec.podIP` which holds a single value.  The proposal is to support dual-stack networking (IPv6 and IPv4 at the same time, very common), which requires that a pod report multiple IP addresses.

Our API changes doc (https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#on-compatibility) talks about adding a second field for "extra values", e.g. `extraPodIPs`.  The point of debate here is how to do this is a maximally compatible and safe way in the face of version skew, and what guarantees can be made.

If we demand that the new field (the plural) is exclusive of the singular field, we face a compat problem.  Older clients might not know to update the plural.  Consider an existing pod with `PodIP = "X"` and `PodIPs" = [ "Y", "Z" ]`.  If we have a client that only knows `PodIP`, and it updates that field to "Z", the exclusivity invariant is violated.

That effectively means that a new field has changed the meaning of an old field, and a client operation which used to work will no longer work.  Bad.

If we demand that the new field (the plural) is inclusive of the singular field, we face a similar problem in the other direction.  Consider `PodIP = "X"` and `PodIPs" = [ "X", "Y" ]`.  If an older client updates `podIP` field to "Z", the inclusivity invariant is violated.  Again, a new field has changed the meaning of an old field.

I have not read the rest of this thread, maybe it is covered, but this is wrong. The old client--for most values of old clients--will also completely omit the PodIPs field, so it is not ambiguous at the server what happened: PodIPs[0] should be automatically changed. We can actually do it the other way, too-- if PodIPs is present and PodIP is not, we can automatically update PodIP. The only ambiguous case is when a client sends both PodIP AND PodIPs, AND PodIP != PodIPs[0]. We can send an error on that case, as clients have to go out of their way to actually do that.

Problem with that approach:

Old client does a GET, patches PodIP (but not PodIPs) on the client side, and PUT.

 

... at least that is how it should work. Until the apply work is finished, some server-side special casing would be needed to make e.g. PATCH do the right thing.

OK, I skipped ahead to the end of the thread.

I am proposing to establish the following as a convention for cases of an existing field which needs to be pluralized.
Given an existing field `widget`, and a desire to add support for multiple values:
* Add a new optional field, e.g. `widgets`.
* The new plural field, if specified, is inclusive of the old singular value

 

Yes.

 

* If a client writes only the old singular field, the new plural field is cleared

I think this is not necessary, though it may be expedient.

 

* If a client writes the new plural field, they must also write the singular field
  - the client may omit the singular field if and only if the value would not have changed
  - the inclusive property must be maintained

This kind of breaks apply.

Why? Clients that understand both need to deal with both. We already impose that rule on apply for fields that have default values set by other fields.

 

 

* Validation logic must enforce these rules synchronously - bad API requests must fail

I think I would have a different set of rules.

1) The client must specify only the old or the new field and omit the other; it may send both fields only if they match.

2) The server must include a defaulting function which fixes it up in either direction.

Why is this better? 

The losers in this situation are "dynamic" clients which GET the entire object and use PUT to modify the field instead of PATCH.

Right.

 

(Clients with static types compiled in will drop the unknown fields and work fine.) 

A case where upgrading client-go can actually break client behavior, not just require coping with signature changes.

 

We can, with more work, make even these clients work by comparing old and new objects in the PUT handler: for this we'd require, instead of 1) above, that if the client sends both PodIP and PodIPs, it must change at most one of those fields-- we require one of them to exactly match the prior object in order to determine which the user changed.

 

--

You received this message because you are subscribed to the Google Groups "kubernetes-sig-architecture" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-arch...@googlegroups.com.
To post to this group, send email to kubernetes-si...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/CAB_J3bZ%2B08eR%3DG3XvOL9CnNav7xntYynqd_tQbEjT1tqoMyEQQ%40mail.gmail.com.

[Brian Grant]

Yes, we should add that here:

https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#backward-compatibility-gotchas 

--
You received this message because you are subscribed to the Google Groups "kubernetes-sig-architecture" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-arch...@googlegroups.com.
To post to this group, send email to kubernetes-si...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/CAO_RewZ6EpQ1t8dQ4%2Bcui%2Bpa3gCZ4gPjXYXYpLc7U78ALMMb5Q%40mail.gmail.com.

[Brian Grant]

On Mon, Aug 13, 2018 at 11:38 AM 'Daniel Smith' via kubernetes-sig-architecture <kubernetes-si...@googlegroups.com> wrote:

On Mon, Aug 13, 2018 at 11:20 AM Tim Hockin <tho...@google.com> wrote:

On Fri, Aug 3, 2018 at 4:39 PM Daniel Smith <dbs...@google.com> wrote:
>> * If a client writes only the old singular field, the new plural field is cleared
>
>
> I think this is not necessary, though it may be expedient.

If we don't do this, we can't guarantee the inclusivity.  We can't
simply add singular to plural because we don't know the intention.
E.g.

Singular: X
Plural: [X, Y]

User sets Singular=Z

Did they want [X, Y, Z] or [Y, Z] or [Z, Y]?

IMO it's completely unambiguous that they want [Z, Y]. They deleted X and added Z.

I disagree. They should have also specified [Z, Y], which is what an auto-generated patch would contain.

 

 

  What if plural carries metadata?

Metadata for X should be cleared.

 

  We can't necessarily infer it.  Easier to just nuke plural
in that case

They did not ask to delete Y, so it is wrong to do so.

(Note: I am assuming this is an associative list. If it is an atomic list then I agree it's correct to delete the whole thing, as definitionally merging it is not possible. But I think an expansion resulting in an atomic list will happen rarely or never.)

 

- client used teh old API which only afforded for a
single value.

>> * If a client writes the new plural field, they must also write the singular field
>>   - the client may omit the singular field if and only if the value would not have changed
>>   - the inclusive property must be maintained
>
>
> This kind of breaks apply.

Explain?  Think of it this way - the update verifier will verify
inclusivity.  If a patch maintains inclusivity, it should be OK?

It makes it impossible to maintain a desired state object that only expresses an opinion on the new field.

Yes, that's true, for that API version.

 

 

> I think I would have a different set of rules.
>
> 1) The client must specify only the old or the new field and omit the other; it may send both fields only if they match.
> 2) The server must include a defaulting function which fixes it up in either direction.

This could work, but it would involve changing fields that were
previously user-set.  How does that map to apply and patch and teh
saved snapshots?

E.g.

T0: User sets singular to X
T1: User sets plural to [Y, Z] ; server changes singular to Y (plural[0])

Is that OK?

Emphatically yes. It should generate all the standard errors/warnings when acquiring ownership of a new field, of course. Apply must support ownership acquisition and relinquishment.

 

> The losers in this situation are "dynamic" clients which GET the entire object and use PUT to modify the field instead of PATCH. (Clients with static types compiled in will drop the unknown fields and work fine.)
>
> We can, with more work, make even these clients work by comparing old and new objects in the PUT handler: for this we'd require, instead of 1) above, that if the client sends both PodIP and PodIPs, it must change at most one of those fields-- we require one of them to exactly match the prior object in order to determine which the user changed.

I want to write this case down - it's come up a few times and people
are not thinking it through well enough or consistently. 

 

--

You received this message because you are subscribed to the Google Groups "kubernetes-sig-architecture" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-arch...@googlegroups.com.
To post to this group, send email to kubernetes-si...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/CAB_J3bb-b9h5nZqevSGc6fhgpYHEoEwVOtiEEgQaB9-6DRshdA%40mail.gmail.com.

[Daniel Smith]

On Mon, Aug 13, 2018 at 12:24 PM Brian Grant <brian...@google.com> wrote:

On Mon, Aug 13, 2018 at 11:38 AM 'Daniel Smith' via kubernetes-sig-architecture <kubernetes-si...@googlegroups.com> wrote:

On Mon, Aug 13, 2018 at 11:20 AM Tim Hockin <tho...@google.com> wrote:

On Fri, Aug 3, 2018 at 4:39 PM Daniel Smith <dbs...@google.com> wrote:
>> * If a client writes only the old singular field, the new plural field is cleared
>
>
> I think this is not necessary, though it may be expedient.

If we don't do this, we can't guarantee the inclusivity.  We can't
simply add singular to plural because we don't know the intention.
E.g.

Singular: X
Plural: [X, Y]

User sets Singular=Z

Did they want [X, Y, Z] or [Y, Z] or [Z, Y]?

IMO it's completely unambiguous that they want [Z, Y]. They deleted X and added Z.

I disagree. They should have also specified [Z, Y], which is what an auto-generated patch would contain.

This client doesn't understand the plural field and therefore can't do that.

 

[Daniel Smith]

On Mon, Aug 13, 2018 at 12:20 PM Brian Grant <brian...@google.com> wrote:

On Fri, Aug 3, 2018 at 4:39 PM 'Daniel Smith' via kubernetes-sig-architecture <kubernetes-si...@googlegroups.com> wrote:

On Thu, Jul 12, 2018 at 12:47 PM 'Tim Hockin' via kubernetes-sig-architecture <kubernetes-si...@googlegroups.com> wrote:

Since I will be out for the next 3 sig-arch meetings, I am putting this here instead of on the agenda.

context: https://github.com/kubernetes/community/pull/2254

We have an existing field `Pod.spec.podIP` which holds a single value.  The proposal is to support dual-stack networking (IPv6 and IPv4 at the same time, very common), which requires that a pod report multiple IP addresses.

Our API changes doc (https://github.com/kubernetes/community/blob/master/contributors/devel/api_changes.md#on-compatibility) talks about adding a second field for "extra values", e.g. `extraPodIPs`.  The point of debate here is how to do this is a maximally compatible and safe way in the face of version skew, and what guarantees can be made.

If we demand that the new field (the plural) is exclusive of the singular field, we face a compat problem.  Older clients might not know to update the plural.  Consider an existing pod with `PodIP = "X"` and `PodIPs" = [ "Y", "Z" ]`.  If we have a client that only knows `PodIP`, and it updates that field to "Z", the exclusivity invariant is violated.

That effectively means that a new field has changed the meaning of an old field, and a client operation which used to work will no longer work.  Bad.

If we demand that the new field (the plural) is inclusive of the singular field, we face a similar problem in the other direction.  Consider `PodIP = "X"` and `PodIPs" = [ "X", "Y" ]`.  If an older client updates `podIP` field to "Z", the inclusivity invariant is violated.  Again, a new field has changed the meaning of an old field.

I have not read the rest of this thread, maybe it is covered, but this is wrong. The old client--for most values of old clients--will also completely omit the PodIPs field, so it is not ambiguous at the server what happened: PodIPs[0] should be automatically changed. We can actually do it the other way, too-- if PodIPs is present and PodIP is not, we can automatically update PodIP. The only ambiguous case is when a client sends both PodIP AND PodIPs, AND PodIP != PodIPs[0]. We can send an error on that case, as clients have to go out of their way to actually do that.

Problem with that approach:

Old client does a GET, patches PodIP (but not PodIPs) on the client side, and PUT.

It's only a problem for dynamic clients which pass the fields they don't understand though.

As I said in that section, "We can, with more work, make even these clients work by comparing old and new objects in the PUT handler: for this we'd require ... that if the client sends both PodIP and PodIPs, it must change at most one of those fields-- we require one of them to exactly match the prior object in order to determine which the user changed."

This kind of breaks apply.

Why? Clients that understand both need to deal with both. We already impose that rule on apply for fields that have default values set by other fields.

The point of apply (one of them, anyway) is to manage only fields that you care about. Ideally you'd only manage the plural field in your manifests. Maybe "breaks" is too strong of a word. It's not ideal.

 

 

 

* Validation logic must enforce these rules synchronously - bad API requests must fail

I think I would have a different set of rules.

1) The client must specify only the old or the new field and omit the other; it may send both fields only if they match.

2) The server must include a defaulting function which fixes it up in either direction.

Why is this better? 

Clients have to do less work.

 

The losers in this situation are "dynamic" clients which GET the entire object and use PUT to modify the field instead of PATCH.

Right.

 

(Clients with static types compiled in will drop the unknown fields and work fine.) 

A case where upgrading client-go can actually break client behavior, not just require coping with signature changes.

Yes, good point, so my suggested "extra work" maybe is mandatory for this approach.

[Brian Grant]

Ah, old client. I don't think that can be inferred, in general. If a client knows about the fields and doesn't specify them, that's a different story.

[Tim Hockin]

On Mon, Aug 13, 2018 at 12:24 PM Brian Grant <brian...@google.com> wrote:
>
>
>
> On Mon, Aug 13, 2018 at 11:38 AM 'Daniel Smith' via kubernetes-sig-architecture <kubernetes-si...@googlegroups.com> wrote:
>>
>>
>>
>> On Mon, Aug 13, 2018 at 11:20 AM Tim Hockin <tho...@google.com> wrote:
>>>
>>> On Fri, Aug 3, 2018 at 4:39 PM Daniel Smith <dbs...@google.com> wrote:
>>> >> * If a client writes only the old singular field, the new plural field is cleared
>>> >
>>> >
>>> > I think this is not necessary, though it may be expedient.
>>>
>>> If we don't do this, we can't guarantee the inclusivity. We can't
>>> simply add singular to plural because we don't know the intention.
>>> E.g.
>>>
>>> Singular: X
>>> Plural: [X, Y]
>>>
>>> User sets Singular=Z
>>>
>>> Did they want [X, Y, Z] or [Y, Z] or [Z, Y]?
>>
>>
>> IMO it's completely unambiguous that they want [Z, Y]. They deleted X and added Z.
>
>
> I disagree. They should have also specified [Z, Y], which is what an auto-generated patch would contain.

If the client doesn't know the new field, it can't. That's why I said
to clear the plural. It has to be one or the other.

>>> What if plural carries metadata?
>>
>>
>> Metadata for X should be cleared.
>>
>>>
>>> We can't necessarily infer it. Easier to just nuke plural
>>> in that case
>>
>>
>> They did not ask to delete Y, so it is wrong to do so.
>>
>> (Note: I am assuming this is an associative list. If it is an atomic list then I agree it's correct to delete the whole thing, as definitionally merging it is not possible. But I think an expansion resulting in an atomic list will happen rarely or never.)
>>
>>>
>>> - client used teh old API which only afforded for a
>>> single value.
>>>
>>> >> * If a client writes the new plural field, they must also write the singular field
>>> >> - the client may omit the singular field if and only if the value would not have changed
>>> >> - the inclusive property must be maintained
>>> >
>>> >
>>> > This kind of breaks apply.
>>>
>>> Explain? Think of it this way - the update verifier will verify
>>> inclusivity. If a patch maintains inclusivity, it should be OK?
>>
>>
>> It makes it impossible to maintain a desired state object that only expresses an opinion on the new field.
>
>
> Yes, that's true, for that API version.

Yeah, and I am saying that's OK. Clients just need to know that.
It's fugly, but the alternative is to assume that plural[0] ===
singular, with the corollary that any plural-associated metadata must
be totally optional or inferable.

[Daniel Smith]

On Mon, Aug 13, 2018 at 1:37 PM Tim Hockin <tho...@google.com> wrote:

>> It makes it impossible to maintain a desired state object that only expresses an opinion on the new field.
>
>
> Yes, that's true, for that API version.

Yeah, and I am saying that's OK.  Clients just need to know that.
It's fugly, but the alternative is to assume that plural[0] ===
singular, with the corollary that any plural-associated metadata must
be totally optional or inferable.

Or we could require it have sensible defaults and require that plural[0]'s metadata is always the default--basically, the defaults are defined by what you want the behavior for the first element to be.

[Brian Grant]

I'm worried about violating the principle of least surprise, as well as the standing policy of not mutating fields set by the user. We haven't implemented the diffing in the apiserver before -- it would be a new pattern. And if a client ever cleared the singular in an apply, they'd thereafter get a conflict with the field.

 

[Daniel Smith]

Actually, I think clearing singular and setting plural in the same apply operation should work as expected if the defaulter works like I said and is invoked at the right time.

We already have cases where automated systems do (or can) take over content that's initially user-set, e.g. HPA.

 

 

[Tim Hockin]

I still feel that clearing the plural is the right thing to do. If I
have a client that is managing a resource, and it gets rolled back to
a version that only writes the singular field, I do not think the
plural fields should be retained - they hold data that is no longer
being actively managed or understood.

In this specific case, the IP addresses could (conceptually) change on
the host (think DHCP) and they won't get updated in the API because
the client doesn't know to update the plural field.

Minor revision on my previous statement. I propose that this become
the guidance for pluralizing:

Given an existing field `widget`, and a new desire to add support for

multiple values:
* Add a new optional field, e.g. `widgets`.

* The new plural field, if specified, must be inclusive of the old
singular value
- if the old singular field is not included in the new plural field,
the API operation must fail

* If a client writes only the old singular field, the new plural field

is cleared and populated with the old value
- any metadata in the new plural field must be defaultable in this case
* If a client writes the new plural field, they must ensure that it is
inclusive of the old singular field
- if the old singular field is not included in the new plural field,
the API operation must fail

I am explicitly pushing this back on the client. It might be OK to
say that defaulting logic (do we run defaults on updates?) is allowed
to set the singular from the plural based on position or metadata. I
am pretty convinced that a write to *just* the singular field should
result in the plural being cleared. It's a little weird if the
singular is "X" and plural is ["X", "Y"], and I want to change
singular to "Y", but we can argue that the singular must be plural[0]
or must carry metadata, in which case plural must be written anyway.

[Daniel Smith]

On Wed, Aug 15, 2018 at 10:39 PM Tim Hockin <tho...@google.com> wrote:

I still feel that clearing the plural is the right thing to do.  If I
have a client that is managing a resource, and it gets rolled back to
a version that only writes the singular field, I do not think the
plural fields should be retained - they hold data that is no longer
being actively managed or understood.

I think your user model is "one client writes the whole thing, either singular or plural" and mine is "different clients are responsible for different items in the list". Quite plausibly we are both right at the same time, just for different users. I think if we do this to an atomic list, it should behave as you say; if we do it to an associative list, it should behave as I say. Those two concepts are meant for the two different user models.

 

In this specific case, the IP addresses could (conceptually) change on
the host (think DHCP) and they won't get updated in the API because
the client doesn't know to update the plural field.

Minor revision on my previous statement.  I propose that this become
the guidance for pluralizing:

Given an existing field `widget`, and a new desire to add support for
multiple values:
* Add a new optional field, e.g. `widgets`.
* The new plural field, if specified, must be inclusive of the old
singular value
  - if the old singular field is not included in the new plural field,
the API operation must fail
* If a client writes only the old singular field, the new plural field
is cleared and populated with the old value
  - any metadata in the new plural field must be defaultable in this case
* If a client writes the new plural field, they must ensure that it is
inclusive of the old singular field
  - if the old singular field is not included in the new plural field,
the API operation must fail

Maybe this would be OK for atomic lists, but it's super annoying to clients using the new field, which includes human users writing YAML. Imagine having to do this for the containers array as a user.

I have a bunch of stuff on my plate so this isn't a complete response but I wanted to get it out there.

 

I am explicitly pushing this back on the client.  It might be OK to
say that defaulting logic (do we run defaults on updates?) is allowed

(yes)

[Tim Hockin]

On Thu, Aug 16, 2018 at 2:11 PM Daniel Smith <dbs...@google.com> wrote:
>
>
>
> On Wed, Aug 15, 2018 at 10:39 PM Tim Hockin <tho...@google.com> wrote:
>>
>> I still feel that clearing the plural is the right thing to do. If I
>> have a client that is managing a resource, and it gets rolled back to
>> a version that only writes the singular field, I do not think the
>> plural fields should be retained - they hold data that is no longer
>> being actively managed or understood.
>
>
> I think your user model is "one client writes the whole thing, either singular or plural" and mine is "different clients are responsible for different items in the list". Quite plausibly we are both right at the same time, just for different users. I think if we do this to an atomic list, it should behave as you say; if we do it to an associative list, it should behave as I say. Those two concepts are meant for the two different user models.

The difference in perspective is true, and I am arguing that there
should not be two owners of the same field, and that the plural form
is a logical extension of the singular, so different clients really
should not be writing those fields.

>>
>> In this specific case, the IP addresses could (conceptually) change on
>> the host (think DHCP) and they won't get updated in the API because
>> the client doesn't know to update the plural field.
>>
>> Minor revision on my previous statement. I propose that this become
>> the guidance for pluralizing:
>>
>> Given an existing field `widget`, and a new desire to add support for
>> multiple values:
>> * Add a new optional field, e.g. `widgets`.
>> * The new plural field, if specified, must be inclusive of the old
>> singular value
>> - if the old singular field is not included in the new plural field,
>> the API operation must fail
>> * If a client writes only the old singular field, the new plural field
>> is cleared and populated with the old value
>> - any metadata in the new plural field must be defaultable in this case
>> * If a client writes the new plural field, they must ensure that it is
>> inclusive of the old singular field
>> - if the old singular field is not included in the new plural field,
>> the API operation must fail
>
>
> Maybe this would be OK for atomic lists, but it's super annoying to clients using the new field, which includes human users writing YAML. Imagine having to do this for the containers array as a user.

In order to get into this mode we would have had to start with
`container`, want to evolve to `containers` and have 2 different users
writing that field of the resource. How would that happen?

User submits singular. Admission control mutates that by populating
plural and adding a sidecar. User updates the resource to change
`container`. Does admission control have an opportunity to mutate
again?

[Daniel Smith]

Yes, but imagine that scenario with a controller instead of an admission webhook.

Or imagine an existing ipv4 system reports assigned IP & its routing status in the singluar field, and we're adding an ipv6 system in parallel to do the same for the subsequent entries. Clearly we don't want all statuses and IPs to be deleted when the ipv4 system changes anything. It's drastically more convenient over upgrades to leave the existing system in place and turn on/off a new one than to require that the client and apiserver move in lock-step.

I think it's a fundamental goal of the API system that edits from different systems, both machine and human, play nice together. Some lists just can't work that way and they're marked as atomic, but many can.

[Tim Hockin]

On Thu, Aug 16, 2018 at 2:56 PM 'Daniel Smith' via
kubernetes-sig-architecture

In this scenario, assuming the v4 address changes, the older v4 client
writes the singular field, and violates the inclusiveness rule.
Plural no longer includes it. I guess your model says that defaulting
logic force sync's singular into plural[0], so no problem. But now
we're back to the case I suggested - the plural list is no longer
being updated.

I don't see a model that satisfies both. I guess in the realm of
evils, mine is less significant.

Does this match what you are thinking, then?

----------

Given an existing field `widget`, and a new desire to add support for
multiple values:

* Add a new plural field, e.g. `widgets`.
* The plural field must be inclusive of the singular field
* On an API write:
- if only the singular field is specified, API logic must populate
plural[0] from it
- if only the plural field is specified, API logic must populate
the singular field from plural[0]
- if both the singular and plural are specified, API logic must
validate that the singular field matches plural[0].
----------

>> > I have a bunch of stuff on my plate so this isn't a complete response but I wanted to get it out there.
>> >
>> >>
>> >>
>> >> I am explicitly pushing this back on the client. It might be OK to
>> >> say that defaulting logic (do we run defaults on updates?) is allowed
>> >
>> >
>> > (yes)
>> >
>> >>
>> >> to set the singular from the plural based on position or metadata. I
>> >> am pretty convinced that a write to *just* the singular field should
>> >> result in the plural being cleared. It's a little weird if the
>> >> singular is "X" and plural is ["X", "Y"], and I want to change
>> >> singular to "Y", but we can argue that the singular must be plural[0]
>> >> or must carry metadata, in which case plural must be written anyway.
>> >> On Mon, Aug 13, 2018 at 2:00 PM Daniel Smith <dbs...@google.com> wrote:
>> >> >
>> >> >
>> >> >
>> >> > On Mon, Aug 13, 2018 at 1:56 PM Brian Grant <brian...@google.com> wrote:
>> >> >>
>> >> >> On Mon, Aug 13, 2018 at 1:42 PM Daniel Smith <dbs...@google.com> wrote:
>> >> >>>
>> >> >>>
>> >> >>>
>> >> >>> On Mon, Aug 13, 2018 at 1:37 PM Tim Hockin <tho...@google.com> wrote:
>> >> >>>>
>> >> >>>> >> It makes it impossible to maintain a desired state object that only expresses an opinion on the new field.
>> >> >>>> >
>> >> >>>> >
>> >> >>>> > Yes, that's true, for that API version.
>> >> >>>>
>> >> >>>> Yeah, and I am saying that's OK. Clients just need to know that.
>> >> >>>> It's fugly, but the alternative is to assume that plural[0] ===
>> >> >>>> singular, with the corollary that any plural-associated metadata must
>> >> >>>> be totally optional or inferable.
>> >> >>>
>> >> >>>
>> >> >>> Or we could require it have sensible defaults and require that plural[0]'s metadata is always the default--basically, the defaults are defined by what you want the behavior for the first element to be.
>> >> >>
>> >> >>
>> >> >> I'm worried about violating the principle of least surprise, as well as the standing policy of not mutating fields set by the user. We haven't implemented the diffing in the apiserver before -- it would be a new pattern. And if a client ever cleared the singular in an apply, they'd thereafter get a conflict with the field.
>> >> >
>> >> >
>> >> > Actually, I think clearing singular and setting plural in the same apply operation should work as expected if the defaulter works like I said and is invoked at the right time.
>> >> >
>> >> > We already have cases where automated systems do (or can) take over content that's initially user-set, e.g. HPA.
>> >> >
>> >> >>
>> >> >>
>

> --
> You received this message because you are subscribed to the Google Groups "kubernetes-sig-architecture" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-arch...@googlegroups.com.
> To post to this group, send email to kubernetes-si...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/CAB_J3bZF2Ybk2s9grdUneGhwAg490CJRMWgHq7FqwBfCHngotg%40mail.gmail.com.

[Daniel Smith]

Yes, for a create that is how I think it should work. For an update, I'd replace the last bullet point with:

- if both singular and plural are provided and plural[0] does not match singular, then api logic must:

  - if plural matches the prior object's plural field, copy singular to plural[0]

  - if singular matches the prior object's singular field, copy plural[0] to singular

  - else error

I think this will help humans using apply and dynamic clients that don't understand both fields do the right thing.

If plural is marked atomic (example: a container's args) then it does not make sense to propagate a change to singular to plural[0]; plural should be replaced instead.

[Daniel Smith]

+Haowei, since you read this entire thread already for the CRD versioning work, and neither Tim nor I have summarized this thread somewhere visible, and I seem to be directing people to this conversation daily, can you write a summary of this in the form of a PR to our API conventions?

[Tim Hockin]

I have a PR started, but I was looking to find an alternate example of
changing the API in a compatible way that wasn't adding a new field or
pluralizing.

Below is the short form. I want to work up a rationale based on this
conversation.

```

Given an existing field `widget`, and a new desire to add support for
multiple values:

* Add a new plural field, e.g. `widgets[]`.

* This new field MUST be inclusive of the singular field
* On an API create or patch operation:
- if only `widget` is specified, API logic must populate
`widgets[0]` from `widget`
- if only `widgets[]` is specified, API logic must populate `widget`
from `widgets[0]`
- if both `widget` and `widgets[]` are specified, API logic must
validate that `widget` matches `widgets[0]`
* On an API update operation:
- if `widget` is changed but `widgets[]` is not, API logic must
populate `widgets[0]` from `widget`
- if `widgets[]` is changed and `widget` is not, API logic must
populate `widget` from `widgets[0]`
- if both `widget` and `widgets[]` are changed, API logic must
validate that `widget` matches `widgets[0]`
```

I am not sure the distinction is necessary, if we define "is
specified" to mean "is present in a create or patch or is changed in
an update.
On Thu, Sep 6, 2018 at 1:48 PM Haowei Cai <hao...@google.com> wrote:
>
> Yes, I'd like to.

[Clayton]

I swear we tried this exact flow really early on in v1beta3 and it broke something. Patch? Will go through my history and look.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/CAO_RewaNfDoVb4pwOeAZ152BAf81JmDYPTvr%3Dzj2eByOMKTFJA%40mail.gmail.com.

[Antoine Pelisse]

On Fri, Sep 14, 2018 at 5:18 PM 'Tim Hockin' via kubernetes-sig-architecture <kubernetes-si...@googlegroups.com> wrote:

I have a PR started, but I was looking to find an alternate example of
changing the API in a compatible way that wasn't adding a new field or
pluralizing.

Below is the short form.  I want to work up a rationale based on this
conversation.

```
Given an existing field `widget`, and a new desire to add support for
multiple values:

* Add a new plural field, e.g. `widgets[]`.
* This new field MUST be inclusive of the singular field
* On an API create or patch operation:
  - if only `widget` is specified, API logic must populate
    `widgets[0]` from `widget`

Should it also remove the value of widget from widgets[] if it's already there?

A controller that only knows about the former version (with only widget) and that oscillates between A and B, would create a long widgets list: [A, B, A, B, ...].

 

  - if only `widgets[]` is specified, API logic must populate `widget`
    from `widgets[0]`
  - if both `widget` and `widgets[]` are specified, API logic must
    validate that `widget` matches `widgets[0]`
* On an API update operation:
  - if `widget` is changed but `widgets[]` is not, API logic must
    populate `widgets[0]` from `widget`
  - if `widgets[]` is changed and `widget` is not, API logic must
    populate `widget` from `widgets[0]`
  - if both `widget` and `widgets[]` are changed, API logic must
    validate that `widget` matches `widgets[0]`
```

I am not sure the distinction is necessary, if we define "is
specified" to mean "is present in a create or patch or is changed in
an update.

I thought about this as well, it lead me to think about this use-case: what if I set widgets[] and unset widget in my update, should it fail the validation or should it just set widget to widgets[0]? 

[Haowei Cai]

Hi Tim, would you like to summarize the guideline? I will leave it to you if you've started the documentation :) Looking forward to the PR.

 I am not sure the distinction is necessary, if we define "is specified" to mean "is present in a create or patch or is changed in an update.

Maybe not necessary but we need to make it clear what does "specified" mean in an update. For update, I suppose that "changed" means the field presents and is different from the prior object's field. If a client wants to clear a field, the client should setting the field to it's zero value instead of nil value.

A controller that only knows about the former version (with only widget) and that oscillates between A and B, would create a long widgets list: [A, B, A, B, ...].

My understanding is that only widgets[0] will be overwritten. 

what if I set widgets[] and unset widget in my update, should it fail the validation or should it just set widget to widgets[0]?

IIUC in the update, if widget is nil, API logic should set widget to widgets[0]; if widget is zero value and doesn't match widgets[0], API logic should fail the validation. 

[Tim Hockin]

On Mon, Sep 17, 2018 at 12:00 PM Antoine Pelisse <apel...@google.com> wrote:
>

> On Fri, Sep 14, 2018 at 5:18 PM 'Tim Hockin' via kubernetes-sig-architecture <kubernetes-si...@googlegroups.com> wrote:
>>
>> I have a PR started, but I was looking to find an alternate example of
>> changing the API in a compatible way that wasn't adding a new field or
>> pluralizing.
>>
>> Below is the short form. I want to work up a rationale based on this
>> conversation.
>>
>> ```
>> Given an existing field `widget`, and a new desire to add support for
>> multiple values:
>>
>> * Add a new plural field, e.g. `widgets[]`.
>> * This new field MUST be inclusive of the singular field
>> * On an API create or patch operation:
>> - if only `widget` is specified, API logic must populate
>> `widgets[0]` from `widget`
>
>
> Should it also remove the value of widget from widgets[] if it's already there?
> A controller that only knows about the former version (with only widget) and that oscillates between A and B, would create a long widgets list: [A, B, A, B, ...].

Great question. I think I would argue yes. Concretely:

* widget=A, widgets=[A, B, C]
* user sets widget=B
* widget=B, widgets=[B, C]

The alternative would be to go back to an earlier proposal to force
clear widgets if only widget is set. I am still OK with that
guidance. If you expect a single writer, clearing is probably
correct. If you expect multiple writers syncing is probably correct.
The problem is that any such assumption is likely wrong eventually..

One could argue that a) we don't really support or encourage multiple
writers of a single field and b) widgets and widget are conceptually a
single field. So clearing might be safer. But assumptions change
over time.

>> - if only `widgets[]` is specified, API logic must populate `widget`
>> from `widgets[0]`
>> - if both `widget` and `widgets[]` are specified, API logic must
>> validate that `widget` matches `widgets[0]`
>> * On an API update operation:
>> - if `widget` is changed but `widgets[]` is not, API logic must
>> populate `widgets[0]` from `widget`
>> - if `widgets[]` is changed and `widget` is not, API logic must
>> populate `widget` from `widgets[0]`
>> - if both `widget` and `widgets[]` are changed, API logic must
>> validate that `widget` matches `widgets[0]`
>> ```
>>
>> I am not sure the distinction is necessary, if we define "is
>> specified" to mean "is present in a create or patch or is changed in
>> an update.
>
>
> I thought about this as well, it lead me to think about this use-case: what if I set widgets[] and unset widget in my update, should it fail the validation or should it just set widget to widgets[0]?

If you explicitly un-set widget, you have "specified" it, and it is in
conflict so it should be an error. IMO

[Davanum Srinivas]

Tim,

How do we ensure an old kubectl (that does not support widgets[]) still work? (Do we need to support this scenario?)

-- Dims

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/CAO_RewaNfDoVb4pwOeAZ152BAf81JmDYPTvr%3Dzj2eByOMKTFJA%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Davanum Srinivas :: https://twitter.com/dims

[Jordan Liggitt]

Since 1.6, the generic kubectl read/write (get, create, replace) operations preserve all data submitted via manifests and received from the API, behaving more like a "dumb" client than one with detailed schema knowledge. 

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/CANw6fcFoS6cQguX_70b9adqFW%3DxkGnP5Rura34eArmW85yx6Mg%40mail.gmail.com.

[Tim Hockin]

For an older client, you would only write the singular field. That is
where the decision comes in - should you sync widget to widgets[] in a
way that makes sense for the field in question, or should you clear
the widgets[] field completely? I can argue both, I think syncing is
less likely to surprise people down the road (which was Daniel's
argument, IIRC) but it can be debated...

Now, Jordan points out that `kubectl replace` will preserve fields
(news to me!) which means that a client which does a RMW mutation of
widget falls into the cases described above, requiring a sync (or
clear if we decide that)

[Jordan Liggitt]

to be clear, I meant it preserves all the fields specified in the manifest fed in (`replace -f <file>`)

there's no merging with the current fields from the API in a read-mutate-write loop with `replace`…

[Tim Hockin]

Oh, sure, so in that case the plural field is either present (in which
case it has to match) or absent (in which case it will be defaulted) -
right?

[Daniel Smith]

I think the best way to continue this discussion is in comments on a PR to the API conventions :)

[Tim Hockin]

I don't want to spend a lot of time writing pretty words and making
tables if we have fundamental disagreement among likely approvers.

Clayton: "I swear we tried this ... and it broke something"

And still some debate about semantics of rollbacks (clear vs sync)...

Would it make more sense to write a doc with the whole rationale (I
was thinking about doing a blog post on this, since it is so
interesting) ? We could argue in comments there, but I'm also happy
with email...

[Daniel Smith]

Yes, a doc would also work for me. It's difficult to track where each comment goes in the structure of the argument in email.

[Mehdy Bohlool]

Did we start a doc on this? I have some question/comments. If you guys didn't start it yet, I can do it.

[Tim Hockin]

Doc, mostly cut-and-paste from this thread.

https://docs.google.com/document/d/1Z8Vbo7RmV_Wvs4k8mluHQC2_Zs8cEwMJmHwWBf9BcaA/edit
On Wed, Oct 3, 2018 at 1:37 PM 'Mehdy Bohlool' via
kubernetes-sig-architecture

> --
> You received this message because you are subscribed to the Google Groups "kubernetes-sig-architecture" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-arch...@googlegroups.com.
> To post to this group, send email to kubernetes-si...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/2595bebc-8c73-4fc2-940f-7fedc3b0fab1%40googlegroups.com.

[Brian Grant]

+Jordan Liggitt 

Did this get resolved?

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-architecture/CAO_RewYSP0%3DYr8Kc0bc6CJosvw%3DDMtrfp60np9QFRAA3HX-W0w%40mail.gmail.com.

[Tim Hockin]

Yes, I think.  I wrote a PR and I think it got merged.  Can't check easily, flight leaves soon. :)

[Mehdy Bohlool]

This is the PR: https://github.com/kubernetes/community/pull/2838

Mehdy Bohlool |

Software Engineer |

me...@google.com |

mbohlool@github