Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge feature-serverside-apply back in master #72947

Merged
merged 10 commits into from Feb 5, 2019

Conversation

@apelisse
Copy link
Member

apelisse commented Jan 16, 2019

What type of PR is this?
/kind api-change
/kind feature

Special notes for your reviewer:

Status of this pull-request:

  • CRDs are missing, and that's the main reason for being WIP. We're working on this.
  • Merging objects should be working mostly fine (there is a known bug with lists),
  • Conflict detection and reporting should work,
  • All Update/Create paths update the managedFields when the feature is enabled, allowing conflicts detection with all the actors of the system.

Most of the logic lives in vendor/sigs.k8s.io/structured-merge-diff, the code strictly in kubernetes is for:

  1. Kubectl changes (kubectl apply and kubectl diff now support server-side apply),
  2. Types: we've added the managedFields field to manage "ownership".
  3. api-machinery to plug all the things together, create the new content-type and use it, update the managedFields as needed on all modification paths.

@deads2k @justinsb @liggitt

Does this PR introduce a user-facing change?:

NONE

for the moment.

@apelisse

This comment has been minimized.

Copy link
Member Author

apelisse commented Jan 16, 2019

cc @jennybuckley @kwiesmueller @lavalamp

I'm not sure all the verify scripts are passing yet, I'll fix as they fail, maybe (since this is just a wip, I don't want to spend too much time fixing these).

@apelisse apelisse force-pushed the apelisse:wip-feature-serverside-apply-merge branch 2 times, most recently from aa63ea8 to db01a25 Jan 16, 2019

@apelisse

This comment has been minimized.

Copy link
Member Author

apelisse commented Jan 17, 2019

I've updated the description, I'll fix the build/tests.

@@ -49,7 +49,6 @@ func TestDefaulting(t *testing.T) {
{Group: "", Version: "v1", Kind: "ConfigMap"}: {},
{Group: "", Version: "v1", Kind: "ConfigMapList"}: {},
{Group: "", Version: "v1", Kind: "Endpoints"}: {},
{Group: "", Version: "v1", Kind: "EndpointsList"}: {},

This comment has been minimized.

@justinsb

justinsb Jan 17, 2019

Member

Deliberate?

This comment has been minimized.

@lavalamp

lavalamp Jan 30, 2019

Member

Please fix.

@fedebongio

This comment has been minimized.

Copy link
Contributor

fedebongio commented Jan 17, 2019

/assign @mbohlool

@deads2k

This comment has been minimized.

Copy link
Contributor

deads2k commented Jan 21, 2019

How is the effort to bring in CRD handling going?

@apelisse

This comment has been minimized.

Copy link
Member Author

apelisse commented Jan 21, 2019

apelisse and others added some commits Jan 17, 2019

@apelisse apelisse force-pushed the apelisse:wip-feature-serverside-apply-merge branch from 2766ef4 to fcd4985 Feb 4, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Feb 4, 2019

@apelisse

This comment has been minimized.

Copy link
Member Author

apelisse commented Feb 4, 2019

/priority important-soon

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Feb 4, 2019

/lgtm

// let's try the live object. This is to prevent clients who
// don't understand managedFields from deleting it accidentally.
if err != nil || len(managed) == 0 {
managed, err = internal.DecodeObjectManagedFields(liveObj)

This comment has been minimized.

@tedyu

tedyu Feb 4, 2019

Contributor

Is it possible that managed is empty and err == nil after the call is made ?

@@ -0,0 +1,116 @@
/*
Copyright 2018 The Kubernetes Authors.

This comment has been minimized.

@tedyu

tedyu Feb 4, 2019

Contributor

It would be nice if year is updated.

for _, modelName := range models.ListModels() {
model := models.LookupModel(modelName)
if model == nil {
panic("ListModels returns a model that can't be looked-up.")

This comment has been minimized.

@tedyu

tedyu Feb 4, 2019

Contributor

Please include modelName in the message

gvkList := parseGroupVersionKind(model)
for _, gvk := range gvkList {
if len(gvk.Kind) > 0 {
parser.gvks[gvk] = modelName

This comment has been minimized.

@tedyu

tedyu Feb 4, 2019

Contributor

What if parser.gvks[gvk] already had some value ?

@jennybuckley

This comment has been minimized.

Copy link
Contributor

jennybuckley commented Feb 4, 2019

@tedyu Thanks for the review, we will address these comments in a follow-up PR, since this one is about to merge.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 5, 2019

@apelisse: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 0ae307b link /test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@jennybuckley

This comment has been minimized.

Copy link
Contributor

jennybuckley commented Feb 5, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 2a5a41a into kubernetes:master Feb 5, 2019

15 of 16 checks passed

pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation apelisse authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
@tedyu

This comment has been minimized.

Copy link
Contributor

tedyu commented Feb 5, 2019

@jennybuckley
I can send PR for the points I made, if you're busy.

Cheers

@tedyu

This comment has been minimized.

Copy link
Contributor

tedyu commented Feb 5, 2019

@jennybuckley
Please see #73731.
For my last comment, once we decide what to do with duplicate gvk key, I can add it to the PR.

@@ -25,4 +25,5 @@ const (
JSONPatchType PatchType = "application/json-patch+json"
MergePatchType PatchType = "application/merge-patch+json"
StrategicMergePatchType PatchType = "application/strategic-merge-patch+json"
ApplyPatchType PatchType = "application/apply-patch+yaml"

This comment has been minimized.

@felixfbecker

felixfbecker Feb 13, 2019

Will application/apply-patch+json be supported too?

This comment has been minimized.

@apelisse

apelisse Feb 13, 2019

Author Member

Good question, we've decided not to do that because json is valid yaml. Feel free to send json directly to this end-point/Content-Type

This comment has been minimized.

@apelisse

apelisse Feb 13, 2019

Author Member

Adding a comment there might be helpful.

@jennybuckley

This comment has been minimized.

Copy link
Contributor

jennybuckley commented Feb 13, 2019

/wg apply

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.