Remove fields that are omitted from applied configurations if there are no other owners

[jpbetz]

Implements the first part of the WG Apply: Unsetting fields design:

Omitting a shared field from an applied configuration:

• Applier gives up ownership but the field remains unchanged.

Omit an exclusively owned field from an applied configuration:

• Optional field: field is removed or defaulted.
• Required field: the apply operation will fail with a validation error.

This makes fields consistent with how associative list/map items already behave.

Added a test converter that renames fields (e.g. conversion of v1 to v2 renames "field_v1" to "field_v2") and used it to ensure that fields are unset when the unset is applied at a different version than the previous apply.

cc @apelisse @jennybuckley @lavalamp

[apelisse]

Could you please specifically pinpoint the limitation of the code in this pull-request? I'm assuming it won't work perfectly across versions?

[apelisse]

By the way, while you're looking at this code, we should understand and fix the bug with "complex nested types".

[jpbetz]

Could you please specifically pinpoint the limitation of the code in this pull-request? I'm assuming it won't work perfectly across versions?

This goes through Updater.prune which does the required conversions and field set comparisons to handle multi version. I‘ll add more tests to showcase this.

[jpbetz]

By the way, while you're looking at this code, we should understand and fix the bug with "complex nested types".

Complex types should be handled properly. I’ll add test cases for this too. If there are specific bugs I should be aware of please send them my way.

[apelisse]

This goes through Updater.prune which does the required conversions and field set comparisons to handle multi version. I‘ll add more tests to showcase this.

Awesome yes, I'd be very surprised if this actually works to be honest. Let me think about it for a minute.

[apelisse]

@jpbetz Let me know if I can help you write additional tests!

[jpbetz]

This goes through Updater.prune which does the required conversions and field set comparisons to handle multi version. I‘ll add more tests to showcase this.

Awesome yes, I'd be very surprised if this actually works to be honest. Let me think about it for a minute.

I spend a while with venn diagrams and couldn't find anything wrong with it. Let me know if you find any problems.

[jpbetz]

@jpbetz Let me know if I can help you write additional tests!

I added field unsetting tests for scalar and complex fields. I also added multi version testing with a converter that does field renames to make sure that prune handles multi version apply correctly.

[apelisse]

I added field unsetting tests for scalar and complex fields. I also added multi version testing with a converter that does field renames to make sure that prune handles multi version apply correctly.

Awesome, I was looking forward to it, and will definitely look closely at that code!

[jpbetz]

@apelisse I had a look at how defaulting w/r/t this PR.

• if I add defaulting to conversion (https://github.com/jpbetz/structured-merge-diff/pull/new/test-defaults-in-converter) and re-ran the TestMultipleAppliersFieldUnsetting test added by this PR. It fails as we expected. prune is unable to observe the field is being deleted, so the field ends up ownership orphaned, but its value is not unset as it should be.
• If I use go modules to replace the SMD dependency in k8s to depend on this PR, and then try to reproduce the defaulting problem, using fields like replicas, I don't observe the issue. The fields are unset as they should be. I've reviewed the code and it looks like while defaulting does happen after apply (https://github.com/kubernetes/kubernetes/blob/ad29e168dc21aae6abe342a9501f40ec1cfe365d/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/structuredmerge.go#L172), it does not happen during conversion, which is performed using the UnsafeObjectConverter (https://github.com/kubernetes/kubernetes/blob/1ff857e1269fc100c8dda64287180cbfb5b2e657/staging/src/k8s.io/apimachinery/pkg/runtime/helper.go#L45). I've looked over the converter and it doesn't seem to do defaulting anywhere.

So it looks like this approach is safe?

[apelisse]

As discussed offline, this is probably because we don't run the conversion (and defaulting) if the version is the same in k8s:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/versionconverter.go#L78-L80.

[jpbetz]

As discussed offline, this is probably because we don't run the conversion (and defaulting) if the version is the same in k8s:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager/internal/versionconverter.go#L78-L80.

I checked and we don't do defaulting when converting across versions either. I've added some test cases to the design doc showing that we get the behavior we want.

I also instrumented a default function and verified that it is not called from within Updater.Apply (in SMD), even when multiple conversions are called from prune, but that it is called from the defaulter.Default call in structuredMergeManager.Apply (in k8s), after Updater.Applycompletes.

E0629 21:07:50.056798    3186 update.go:164] Start Apply
E0629 21:07:50.056903    3186 update.go:229] prune: Converting merged to apiextensions.k8s.io/v1beta1
E0629 21:07:50.057022    3186 update.go:265] addBackOwnedItems: Converting merged to apiextensions.k8s.io/v1
E0629 21:07:50.060735    3186 update.go:273] addBackOwnedItems: Converting pruned to apiextensions.k8s.io/v1
E0629 21:07:50.060894    3186 update.go:298] addBackDanglingItems: Converting pruned to apiextensions.k8s.io/v1beta1
E0629 21:07:50.061051    3186 update.go:247] prune: Converting pruned to apiextensions.k8s.io/v1
E0629 21:07:50.061167    3186 update.go:210] End Apply

If defaulting happens, we would see "setDefaults_CustomResourceDefinitionSpec called" logged in the above output.

Here's the output of defaulter.Default being called (which happens after SMD apply is completed):

E0629 21:07:21.335972    3186 structuredmerge.go:176] Start Calling default
E0629 21:07:21.336211    3186 defaults.go:46] setDefaults_CustomResourceDefinitionSpec called
E0629 21:07:21.336663    3186 defaults.go:46] setDefaults_CustomResourceDefinitionSpec called
E0629 21:07:21.336676    3186 structuredmerge.go:178] End calling default

[jpbetz]

/hold

k8s CI has been a bit flaky, but existing k8s unit tests and integration are passing for this change (kubernetes/kubernetes#92661). I'm going to remove this hold once I've added a couple of integration tests in k8s that ensure unsetting fields happens as expected for native types, and they are passing.

Feel free to tag this when it looks ready.

[jpbetz]

Tests added to kubernetes/kubernetes#92661

/hold cancel

[jpbetz]

This is ready to be merged once reviewers are happy with it. I'd like to get it going today if possible so we can bump k8s to use it for 1.19.

[apelisse]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, jpbetz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [apelisse]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment