Fix strategic patch for list of primitive type with merge sementic

[mengqiy]

Fix strategic patch for list of primitive type when the patch strategy is merge.
Before: we cannot replace or delete an item in a list of primitive, e.g. string, when the patch strategy is merge. It will always append new items to the list.
This patch will generate a map to update the list of primitive type.
The server with this patch will accept either a new patch or an old patch.
The client will found out the APIserver version before generate the patch.

Fixes #35163, #32398

cc: @pwittrock @fabianofranz

Fix strategic patch for list of primitive type when patch strategy is `merge` to remove deleted objects.

---

This change is

[mengqiy]

This change will break the current tests, since part of the format of the format has changed.
I will fix them and add new tests when everyone is happy with the change.

[mengqiy]

Currently the old the client is able to talk to the new API server and the API server will behave the same when merging the patch.
But an old server will not accept a patch that generated by the new client. It will fail.
I can add some logic to let the kubectl get the API server version first before generating the patch to make the new client be able to talk to an old API server

[mengqiy]

@k8s-bot ok to test

[pwittrock]

@ymqytw Any idea why the tests failed?

[mengqiy]

I guess something went wrong with the test-infra, since I found many tests were failing because of IsUp or Up exit with status 1.

[mengqiy]

@k8s-bot test this

[k8s-ci-robot]

Jenkins Kubemark GCE e2e failed for commit 01f0aa0e47c14c387c6bf74b9cea98287b33d05a. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

[k8s-ci-robot]

Jenkins GCE e2e failed for commit 14b5d7d1d14d9d5c37f8d1de8c9802218a22b12e. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

[pwittrock]

@ymqytw Let me know if I should take a look again

[mengqiy]

@smarterclayton If we encounter error when calling GetServerSupportedSMPatchVersion(), it's safe to default to the old behavior as v1.4 which won't break any older server. But it will leave apply broken for list of primitives.
What is the solution?

[pwittrock]

@smarterclayton WDYT of defaulting to the new patch version when we can't get the supported version from the server? Is it safe to require that all servers will support the same STMRG version? I am not sure if there is a perfect solution here and want to make sure we make a good set of tradeoffs.

[smarterclayton]

I'm ok with defaulting to the latest (if we are broken with the old way).
Just need a default and not to error out.

On Wed, Nov 16, 2016 at 11:31 AM, Phillip Wittrock <notifications@github.com

wrote:

@smarterclayton https://github.com/smarterclayton WDYT of defaulting to
the new patch version when we can't get the supported version from the
server? Is it safe to require that all servers will support the same STMRG
version? I am not sure if there is a perfect solution here and want to make
sure we make a good set of tradeoffs.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#35647 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p1IoNhhmuu8Aj8gYUDNY3h7cRIp7ks5q-1odgaJpZM4KhlTL
.

[bgrant0607]

@pwittrock @ymqytw Sorry I didn't think about this very hard before.

I don't like that the patch deviates from the schema of the resource. Among other things, that prevents us from using the schema validator on patches.

Was a parallel list considered instead?

Also, we need to update the existing SMP doc:
https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api-conventions.md#strategic-merge-patch

And ideally we'd write a more complete user-facing version of that section.

Also, I didn't have time to read the whole discussion, but I don't think it's valid to base any functionality on the release version. For instance, it's not clear to me that every use of Kubernetes source code is going to report the same release info. If we agree that this is the case, we need to document it in the api conventions.

cc @smarterclayton

[mengqiy]

I don't think it's valid to base any functionality on the release version.

@bgrant0607 This part has been removed in #36916. In that PR, we change the behavior to:
first try the new patch, if it fails because the server don't recognize it, it will try the old patch.

[bgrant0607]

Thanks @ymqytw.

Also, mergeprimitiveslist deviates from our API conventions -- everything else in the API is camelCase or CamelCase. Since the other directives are lowercase, I'd expect mergePrimitivesList.

However, if we changed to parallel lists, we'd replace mergeprimitiveslist. For example, to delete one list entry from this:

finalizers: [ "A", "B", "C" ]

We could write something like:

"$deleteFromStringList/finalizers": ["C"]

[pwittrock]

This makes sense to me @ymqytw given that the 1.5 cut date is fast approaching, what is your confidence that we could change the implementation to match @bgrant0607 's suggestion before the deadline?

[mengqiy]

@pwittrock I can try to create a PR for review before thanksgiving.

[pwittrock]

Sounds good. Otherwise we may need to consider rolling back the client changes.

[bgrant0607]

@pwittrock Is there a reason why we can't just rollback and try again in 1.6? What's blocked by this?

[bgrant0607]

With my proposed approach, I believe older apiservers would just ignore the additional directives, though we should confirm that is true.

[bgrant0607]

@ymqytw @pwittrock Please email sig-api-machinery to give them the opportunity to chime in before we merge the new approach.

[pwittrock]

@bgrant0607 Nothing is blocked by this. The finalizers field in ObjectMeta were changed to use the a merge strategy, so this would be newly broken in 1.5. @caesarxuchao, I believe that you indicated that this was acceptable for 1.5 - would you confirm? If that is the case, rolling back the changes SGTM.

[caesarxuchao]

Rolling back is fine. The garbage collector does NOT use patch to update the finalizers.

[mengqiy]

@pwittrock I think we should try the new way in 1.6. It may be too risky to do that for 1.5. Since there is only 1 week left, because of the Thanksgiving holiday.
I'm reverting these PR now.

[pwittrock]

Agreed

[mengqiy]

Reverting related PRs in #37343.

[lavalamp]

Not that we should necessarily copy them, but mongo does it this way: https://docs.mongodb.com/manual/reference/operator/update/pull/

(I think 'pull' is a silly name for this.)

I do think $deleteFromStringList is overly specific, though. Perhaps 'primitive' instead of string?

[mengqiy]

@lavalamp I created a design doc PR #37652 for this. PTAL