Force using versioned objects in apply's patch #35688

soltysh · 2016-10-27T11:30:29Z

Until now kubectl apply was working with internal version and manually invoking encoding, which resulted in problems converting object to versioned counterpart. It worked until now mostly because each group had only single version, but with multiple versions usually the first/preferred is chosen, which in case of ScheduledJobs isn't true. (ScheduledJobs exists in batch/v2alpha1, but preferred version is batch/v1). Resource helper has all the necessary information to perform necessary conversion so I've added conversion before actually calling the patch method and passing it as versioned to that method.

@janetkuo or anyone from @kubernetes/kubectl ptal

This change is

soltysh · 2016-10-27T11:31:22Z

/fyi @zreigz @bes @vreon

zreigz · 2016-10-27T11:59:08Z

Great! It works.

smarterclayton · 2016-10-27T14:06:14Z

Ha! Fixes #25106 (i was right)

smarterclayton · 2016-10-27T14:06:49Z

pkg/kubectl/cmd/apply.go

@@ -220,7 +230,13 @@ func RunApply(f cmdutil.Factory, cmd *cobra.Command, out io.Writer, options *App
 			helper := resource.NewHelper(info.Client, info.Mapping)
 			patcher := NewPatcher(encoder, decoder, info.Mapping, helper, overwrite)

-			patchBytes, err := patcher.patch(info.Object, modified, info.Source, info.Namespace, info.Name)
+			infos := []*resource.Info{info}


Actually... you should have info.VersionedObject set here. Can you verify that you do (it should be the external version)?

Hmmm... I'll check, but from what I see kubectl edit is using the same approach.

Actually that's where I copied it from 😉

info.VersionedObject to my surprise is of type interface {} and might be nil as well, so I'd have to cast it to runtime.Object and if that fails fall back to resource.AsVersionedObject. Or stay with current solution? Which one you prefer?

If Apply has VersionedObject as nil, other things will fail. VersionedObject was added in order to solve the Apply use case, so it's expected to work in general.

brendandburns · 2016-10-28T04:22:57Z

This LGTM, but I defer to @smarterclayton for the final call.

soltysh · 2016-10-28T15:36:34Z

Grrrr.... after using info.VersionedObject the test fail badly :/ looking into it...

soltysh · 2016-11-01T08:07:25Z

.VersionedObject doesn't work with lists, it looks like the code does not support that case, yet. @smarterclayton are you ok with a temporary fix I've provided initially (iow. encoding in place) for now with a TODO to fix the list case? I'm not sure I have enough time to debug this now and people are sort of blocked by this. If I find time I'll fix it this or next week.

smarterclayton · 2016-11-01T13:42:19Z

Please open a follow up issue for it. If versioned object doesn't work, we
shouldn't be supporting it elsewhere (it's a massive time sink).

On Nov 1, 2016, at 4:08 AM, Maciej Szulik notifications@github.com wrote:

.VersionedObject doesn't work with lists, it looks like the code does not
support that case, yet. @smarterclayton https://github.com/smarterclayton
are you ok with a temporary fix I've provided initially (iow. encoding in
place) for now with a TODO to fix the list case? I'm not sure I have enough
time to debug this now and people are sort of blocked by this. If I find
time I'll fix it this or next week.

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

soltysh · 2016-11-01T20:34:13Z

it's a massive time sink

Tell me about it 😉

I've created #35999 and assigned it to myself. Can we merge this as is in that case?

smarterclayton · 2016-11-02T20:26:37Z

pkg/kubectl/cmd/apply.go

@@ -220,7 +230,13 @@ func RunApply(f cmdutil.Factory, cmd *cobra.Command, out io.Writer, options *App
 			helper := resource.NewHelper(info.Client, info.Mapping)
 			patcher := NewPatcher(encoder, decoder, info.Mapping, helper, overwrite)

-			patchBytes, err := patcher.patch(info.Object, modified, info.Source, info.Namespace, info.Name)
+			infos := []*resource.Info{info}
+			originalObj, err := resource.AsVersionedObject(infos, false, defaultVersion, encoder)


This will fail if you try to apply two objects in different API versions, it looks like. Is there a test that proves it doesn't?

I'll verify that and add a proper test.

Actually, the reason this doesn't have VersionedObject is because you are calling Get() up above. Why are you calling Get() up above?

Defending I didn't write that part won't probably help 😜 . I'll test proposed approaches.

I dropped Get, but that still suffers from list not having VersionedObject set :/ I think I'll leave that for later in that case. Hopefully when we enter codefreeze I'll find some time to dig into this.

smarterclayton · 2016-11-03T15:01:32Z

This is a bug so it can be worked post feature freeze.

k8s-ci-robot · 2016-11-03T15:11:40Z

Jenkins unit/integration failed for commit 9cafc7e. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit 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 · 2016-11-03T15:29:43Z

Jenkins GCE etcd3 e2e failed for commit 9cafc7e. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 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 · 2016-11-03T15:30:41Z

Jenkins GCI GCE e2e failed for commit 9cafc7e. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce 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 · 2016-11-03T15:32:13Z

Jenkins GCE e2e failed for commit 9cafc7e. 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.

jlutzhbo · 2016-11-09T00:16:59Z

Do we have an ETA for this? Also, what version will this be fixed in? Thanks!

soltysh · 2016-11-09T21:52:47Z

No strict ETA, rather wishful thinking of mine to have it in for 1.5

adohe-zz · 2016-11-17T14:45:49Z

@soltysh please rebase this, I think this has blocked users using HPA.

soltysh · 2016-11-19T20:09:13Z

@adohe rebase is one thing, most importantly this needs final solution :/

adohe-zz · 2016-11-20T01:25:56Z

@soltysh Ok, I will take a look.

liggitt · 2016-12-08T21:55:20Z

pkg/kubectl/cmd/apply.go

 		if !dryRun {
 			overwrite := cmdutil.GetFlagBool(cmd, "overwrite")
 			helper := resource.NewHelper(info.Client, info.Mapping)
 			patcher := NewPatcher(encoder, decoder, info.Mapping, helper, overwrite)

-			patchBytes, err := patcher.patch(info.Object, modified, info.Source, info.Namespace, info.Name)


I don't think this is right... VersionedObject is what the client provided, Object came from the server

against master, I'm seeing an error inside patchSimple, where this line is returning an internal object:

versionedObject, _, err := p.decoder.Decode(current, nil, nil)

using the info from the mapping instead let the apply unit tests pass for me:

versionedObject, err := api.Scheme.New(p.mapping.GroupVersionKind)

edit: I take it back... that may only work for types registered into the scheme

@liggitt thx for the feedback, it's on my list for this week, hopefully.

mengqiy · 2016-12-16T22:25:45Z

pkg/kubectl/cmd/apply.go

@@ -182,45 +182,17 @@ func RunApply(f cmdutil.Factory, cmd *cobra.Command, out io.Writer, options *App
 			return cmdutil.AddSourceToErr(fmt.Sprintf("retrieving modified configuration from:\n%v\nfor:", info), info.Source, err)
 		}

-		if err := info.Get(); err != nil {


We should not remove all of this. Otherwise, we can't use kubectl apply to create non-existing resource.

mengqiy · 2016-12-16T22:27:31Z

pkg/kubectl/cmd/apply.go

 		if !dryRun {
 			overwrite := cmdutil.GetFlagBool(cmd, "overwrite")
 			helper := resource.NewHelper(info.Client, info.Mapping)
 			patcher := NewPatcher(encoder, decoder, info.Mapping, helper, overwrite)

-			patchBytes, err := patcher.patch(info.Object, modified, info.Source, info.Namespace, info.Name)
+			originalObj, ok := info.VersionedObject.(runtime.Object)


info.VersionedObject is no longer a interface. Don't need to cast.

k8s-github-robot · 2017-01-03T22:35:18Z

@soltysh PR needs rebase

liggitt · 2017-01-03T22:39:09Z

I'm not sure this is needed anymore. Can you double check with the tests you added and see if this is already fixed in master?

soltysh · 2017-01-04T10:50:50Z

The problem still persists. I just checked on lastest master and I'm getting following error:

$ kubectl apply -f cronjob.yaml 
Warning: kubectl apply should be used on resource created by either kubectl create --save-config or kubectl apply
error: error when applying patch:

to:
&{0xc42061afc0 0xc420139500 default pi cronjob.yaml &CronJob{ObjectMeta:k8s_io_kubernetes_pkg_api_v1.ObjectMeta{Name:pi,GenerateName:,Namespace:,SelfLink:,UID:,ResourceVersion:,Generation:0,CreationTimestamp:0001-01-01 00:00:00 +0000 UTC,DeletionTimestamp:<nil>,DeletionGracePeriodSeconds:nil,Labels:map[string]string{},Annotations:map[string]string{kubectl.kubernetes.io/last-applied-configuration: ,},OwnerReferences:[],Finalizers:[],ClusterName:,},Spec:CronJobSpec{Schedule:*/25 * * * ?,StartingDeadlineSeconds:nil,ConcurrencyPolicy:,Suspend:nil,JobTemplate:JobTemplateSpec{ObjectMeta:k8s_io_kubernetes_pkg_api_v1.ObjectMeta{Name:,GenerateName:,Namespace:,SelfLink:,UID:,ResourceVersion:,Generation:0,CreationTimestamp:0001-01-01 00:00:00 +0000 UTC,DeletionTimestamp:<nil>,DeletionGracePeriodSeconds:nil,Labels:map[string]string{},Annotations:map[string]string{},OwnerReferences:[],Finalizers:[],ClusterName:,},Spec:JobSpec{Parallelism:nil,Completions:nil,ActiveDeadlineSeconds:nil,Selector:nil,ManualSelector:nil,Template:k8s_io_kubernetes_pkg_api_v1.PodTemplateSpec{ObjectMeta:ObjectMeta{Name:pi,GenerateName:,Namespace:,SelfLink:,UID:,ResourceVersion:,Generation:0,CreationTimestamp:0001-01-01 00:00:00 +0000 UTC,DeletionTimestamp:<nil>,DeletionGracePeriodSeconds:nil,Labels:map[string]string{},Annotations:map[string]string{},OwnerReferences:[],Finalizers:[],ClusterName:,},Spec:PodSpec{Volumes:[],Containers:[{pi perl [perl -Mbignum=bpi -wle print bpi(2000)] []  [] [] [] {map[] map[]} [] nil nil nil   nil false false false}],RestartPolicy:Never,TerminationGracePeriodSeconds:nil,ActiveDeadlineSeconds:nil,DNSPolicy:,NodeSelector:map[string]string{},ServiceAccountName:,DeprecatedServiceAccount:,NodeName:,HostNetwork:false,HostPID:false,HostIPC:false,SecurityContext:nil,ImagePullSecrets:[],Hostname:,Subdomain:,Affinity:nil,},},},},},Status:CronJobStatus{Active:[],LastScheduleTime:<nil>,},} &TypeMeta{Kind:,APIVersion:,} 211 false}
for: "cronjob.yaml": error when serializing current configuration from:
&TypeMeta{Kind:,APIVersion:,}
for: "cronjob.yaml": no kind "CronJob" is registered for version "batch/v1"

I'll try to fix it this week.

soltysh · 2017-01-09T12:31:07Z

Here's the current state. I can't use info.VersionedObject since it's user-provided versioned object, so my initial approach was wrong and I have to stick with info.Object in that case.
I started digging more into this and it's failing apply.patchSimple, which is going all the way down to scheme.convertToVersion, which fails getting batch/v2alpha1.CronJob when an input is __internal.CronJob. KindForGroupVersionKinds is getting both possible batch versions in the for loop, and since there's no direct match against actual Kind, it picks the first one, which in this case is wrong.

@deads2k @liggitt @caesarxuchao @kubernetes/sig-api-machinery-misc any ideas how this could be fixed? I'm thinking about two possible approaches:

extend GroupVersions with actual Kinds` in here, but that's quite tedious, imho.
create additional runtime.Encode method which would accept GVK, because here info.Mapping is holding the right GVK I'd expect.

Are there other options I'm missing here?

soltysh · 2017-01-09T13:04:14Z

I can't use info.VersionedObject since it's user-provided versioned object,

Btw, am I the only one who finds this confusing. I've always thought info.VersionedObject is the same as info.Object but just versioned. Shouldn't that have a different name based on its description?

huggsboson · 2017-01-13T02:00:23Z

At Box we're hitting an issue like this trying to apply (update an already created one, first time apply works just fine) instances of a thirdpartyresource after upgrading to 1.4.8. I can't tell if this or #38982 will fix this problem for instances of a ThirdPartyResource.

Our error message looks like:

&{0xc820016d80 0xc82013bea0 user-dev pki-95c36eb44c4a9132082142f7d5e47bf0 /box/deployment-config/release/dsv31/dev/user/app.json &TypeMeta{Kind:,APIVersion:extensions/v1beta1,} &TypeMeta{Kind:,APIVersion:extensions/v1beta1,} 178335915 false}
for: "/box/deployment-config/release/dsv31/dev/user/app.json": error when getting instance of versioned object for box-pki.com/v1, Kind=Pki: "/box/deployment-config/release/dsv31/dev/user/app.json": no kind "Pki" is registered for version "box-pki.com/v1"

mengqiy · 2017-01-13T15:55:35Z

@huggsboson This is the PR #39396 merged in 1.4 branch. But the patch is not for fixing TPR.

liggitt · 2017-01-23T15:18:05Z

I think #40260 is more likely to be what we want, switching apply to use unstructured and not do any conversion or defaulting from the input the user gives

soltysh · 2017-01-23T15:52:33Z

I'll check, thx for the pointer.

k8s-github-robot · 2017-01-24T00:12:52Z

[APPROVALNOTIFIER] Needs approval from an approver in each of these OWNERS Files:

pkg/kubectl/OWNERS

We suggest the following people:
cc @sig-cli-maintainers
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

soltysh · 2017-01-24T10:58:30Z

@liggitt confirmed, just verified this works as expected with #40260. Closing in favor of the other one in that case.

soltysh added area/kubectl release-note-none Denotes a PR that doesn't merit a release note. labels Oct 27, 2016

googlebot added the cla: yes label Oct 27, 2016

k8s-github-robot assigned brendandburns Oct 27, 2016

k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 27, 2016

smarterclayton reviewed Oct 27, 2016

View reviewed changes

brendandburns added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 28, 2016

brendandburns assigned smarterclayton and unassigned brendandburns Oct 28, 2016

soltysh mentioned this pull request Oct 28, 2016

Propagate explicit null values when using kubectl apply #35496

Closed

soltysh mentioned this pull request Nov 1, 2016

Switch kubectl apply to use info.VersionedObject #35999

Closed

smarterclayton reviewed Nov 2, 2016

View reviewed changes

Force using versioned objects in apply's patch

9cafc7e

soltysh force-pushed the issue35149 branch from 81867a2 to 9cafc7e Compare November 3, 2016 14:54

soltysh added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 3, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 3, 2016

smarterclayton added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 3, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 11, 2016

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2016

caesarxuchao mentioned this pull request Dec 8, 2016

Remove json serialization annotations from internal types #38406

Merged

14 tasks

liggitt reviewed Dec 8, 2016

View reviewed changes

matchstick mentioned this pull request Dec 16, 2016

Intermittent error when doing kubectl apply -f for a HorizontalPodAutoscaler: 'unable to find api field in struct HorizontalPodAutoscalerSpec for the json field "scaleTargetRef"' #34413

Closed

mengqiy reviewed Dec 16, 2016

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 3, 2017

huggsboson mentioned this pull request Jan 13, 2017

kubectl apply does not update ThirdPartyResource object. #29542

Closed

soltysh closed this Jan 24, 2017

soltysh deleted the issue35149 branch January 24, 2017 10:59

Force using versioned objects in apply's patch #35688

Force using versioned objects in apply's patch #35688

Conversation

soltysh commented Oct 27, 2016 • edited

soltysh commented Oct 27, 2016

zreigz commented Oct 27, 2016

smarterclayton commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendandburns commented Oct 28, 2016

soltysh commented Oct 28, 2016

soltysh commented Nov 1, 2016

smarterclayton commented Nov 1, 2016

soltysh commented Nov 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

k8s-ci-robot commented Nov 3, 2016

jlutzhbo commented Nov 9, 2016

soltysh commented Nov 9, 2016

adohe-zz commented Nov 17, 2016

soltysh commented Nov 19, 2016

adohe-zz commented Nov 20, 2016

Choose a reason for hiding this comment

liggitt Dec 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Jan 3, 2017

liggitt commented Jan 3, 2017

soltysh commented Jan 4, 2017

soltysh commented Jan 9, 2017

soltysh commented Jan 9, 2017

huggsboson commented Jan 13, 2017 • edited

mengqiy commented Jan 13, 2017

liggitt commented Jan 23, 2017

soltysh commented Jan 23, 2017

k8s-github-robot commented Jan 24, 2017

soltysh commented Jan 24, 2017

soltysh commented Oct 27, 2016 •

edited

liggitt Dec 8, 2016 •

edited

huggsboson commented Jan 13, 2017 •

edited