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.

Sign up for GitHub

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

Jump to bottom

Pass runtime.Object to Helper.Create/Replace #16441

Merged
merged 1 commit into from
Oct 30, 2015
Merged

Pass runtime.Object to Helper.Create/Replace #16441

merged 1 commit into from
Oct 30, 2015

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Oct 28, 2015

kubectl commands were serializing using the info.Mapping.Codec, rather than letting the RESTClient serialize the object using the Codec for the version it expected to speak to the server with.

This also meant that in modify cases, the object would get encoded, decoded, and re-encoded, which would apply defaulting client-side, which is not ideal.

@k8s-github-robot
Copy link

Labelling this PR as size/L

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 28, 2015
@liggitt
Copy link
Member Author

liggitt commented Oct 28, 2015

cc @deads2k @smarterclayton @kubernetes/kubectl

liggitt
liggitt reviewed Oct 28, 2015
View reviewed changes
pkg/kubectl/resource/helper_test.go
Object: &api.Pod{
ObjectMeta: api.ObjectMeta{Name: "foo"},
Spec: apitesting.DeepEqualSafePodSpec(),
},
ExpectObject: &api.Pod{
ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"},
Spec: apitesting.DeepEqualSafePodSpec(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this illustrates the encode/decode/defaulting/encode that was happening client-side

@deads2k
Copy link
Contributor

deads2k commented Oct 28, 2015

lgtm

@smarterclayton
Copy link
Contributor

LGTM

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2015
@k8s-bot
Copy link

k8s-bot commented Oct 28, 2015

GCE e2e test build/test passed for commit b58e62e.

@jackgr
Copy link
Contributor

jackgr commented Oct 28, 2015

We are serializing the object client side using the mapping codec to create the annotation for kubectl apply. We are also deserializing it client side to retrieve the previous version for the three way diff. I'm wondering if this process could introduce api version artifacts into the patch that we generate, and/or if we should fail patch generation if the live version is different from the version in the annotation?

However, I can't think of any specific examples of problems caused by this process.

  • In the event of conflicts between user supplied values and defaults, the user supplied values override the defaults, so differences in defaults between versions should not have any effect.
  • Properties added to the resource schema in a later version will be correctly treated as additions by apply.
  • Properties removed from the resource schema might trigger errors if the user supplied configuration provides values for them, but apply doesn't change the existing behavior in that case. Also, the presence of such properties in the annotation won't cause them to be included in the patch. It would cause tombstones to be generated for them in the patch if the user supplied configuration no longer provides values for them, but the tombstones will be noops against the current state, so there's no net difference there, either.

I'm therefore inclined to suggest that we keep the annotation and patch generation version independent. Is there anything I'm overlooking here that might argue otherwise?

cc @bgrant0607, @ghodss, @janetkuo, @kubernetes/kubectl

@bgrant0607
Copy link
Member

cc @jackgr

@bgrant0607
Copy link
Member

Please cherrypick this into 1.1.

@bgrant0607 bgrant0607 added this to the v1.1 milestone Oct 28, 2015
@liggitt
Copy link
Member Author

liggitt commented Oct 28, 2015

I'm therefore inclined to suggest that we keep the annotation and patch generation version independent. Is there anything I'm overlooking here that might argue otherwise?

Not sure what you're proposing... generating the JSON for the annotation using the internal codec? Anything serialized has to use a versioned codec.

@liggitt liggitt mentioned this pull request Oct 28, 2015
Merged
@jackgr
Copy link
Contributor

jackgr commented Oct 28, 2015

@liggitt it serializes the user input to the command using json.Marshal to generate the annotation, when the user input is present, and the object retrieved from the server using Mapping.Codec.Encode, to generate the annotation when no user input is present. See this method.

The object retrieved from the server is serialized for the patch computation using Mapping.Codec.Encode here.

PTAL.

@bgrant0607
Copy link
Member

We do not want defaults applied in the annotation, which means we can't apply conversion, either.

@liggitt
Copy link
Member Author

liggitt commented Oct 28, 2015

The straight json.Marshal gives me pause as well... I'd need to dig to convince myself VersionedObject had all the fields we cared about already populated. @jackgr, did you compare behavior of using info.Mapping.Codec to encode?

@jackgr
Copy link
Contributor

jackgr commented Oct 28, 2015

See this method for the difference between them. As I read it, VersionedObject is the raw input from the command line deserialized with api.Scheme.Raw().DecodeToVersionedObject, while obj is initially the same input deserialized with mapping.Codec.Decode. Later, however, obj is the object returned by the server, deserialized by RESTClient.

@j3ffml
Copy link
Contributor

j3ffml commented Oct 29, 2015

@k8s-bot unit test this

@bgrant0607 bgrant0607 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2015
@bgrant0607
Copy link
Member

I don't have time to look at the code, but the whole point of the annotations is to capture user intent and to detect changes in that intent (removal of specified items, such as containers, in particular).

Is the problem that we don't understand what the alternative implementations do, or that we don't agree on which is preferred? If the latter, could someone please clearly explain the difference in behavior between the two?

@liggitt
Copy link
Member Author

liggitt commented Oct 29, 2015

can we move the discussion of the annotation encoding to a new issue? that is unrelated to the issue this PR is fixing

@jackgr jackgr mentioned this pull request Oct 29, 2015
Closed
@jackgr
Copy link
Contributor

jackgr commented Oct 29, 2015

Created #16543 to continue this discussion.

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2015
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Oct 29, 2015

GCE e2e build/test failed for commit b58e62e.

@wojtek-t
Copy link
Member

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Oct 30, 2015

GCE e2e test build/test passed for commit b58e62e.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Oct 30, 2015

GCE e2e test build/test passed for commit b58e62e.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Oct 30, 2015

GCE e2e test build/test passed for commit b58e62e.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Oct 30, 2015
Merge pull request #16441 from liggitt/helper_codec
a9534bf 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit a9534bf into kubernetes:master Oct 30, 2015
k8s-github-robot referenced this pull request Oct 30, 2015
Merge pull request #16450 from liggitt/automated-cherry-pick-of-#1644…
e58f415 
…1-upstream-release-1.1

Auto commit by PR queue bot
@liggitt liggitt deleted the helper_codec branch November 4, 2015 04:54
shyamjvs referenced this pull request in shyamjvs/kubernetes Dec 1, 2016
Merge pull request kubernetes#16450 from liggitt/automated-cherry-pic…
697c2e6 
…k-of-#16441-upstream-release-1.1

Auto commit by PR queue bot
shouhong referenced this pull request in shouhong/kubernetes Feb 14, 2017
Merge pull request kubernetes#16450 from liggitt/automated-cherry-pic…
ea4538a 
…k-of-#16441-upstream-release-1.1

Auto commit by PR queue bot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bgrant0607 bgrant0607

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.1
Development

Successfully merging this pull request may close these issues.

None yet