Fix AsVersionedObject to deal with external schema #36066

mfojtik · 2016-11-02T13:54:02Z

This fixes errors where the internal object omits json tags or has a different field structure than the versioned object

At also add typer and encoder to AsVersionedObject() to deal with custom schemes.

This change is

mfojtik · 2016-11-02T13:54:26Z

@Kargakis @smarterclayton PTAL

@soltysh 🍺

soltysh · 2016-11-02T13:58:03Z

LGTM

0xmichalis · 2016-11-02T13:59:18Z

@kubernetes/sig-api-machinery @kubernetes/sig-cli

mfojtik · 2016-11-02T14:07:02Z

(need to add unit test)

mfojtik · 2016-11-02T14:30:49Z

(added unit test)

soltysh · 2016-11-02T14:36:24Z

@liggitt || @deads2k any objections for merge at this point?

liggitt · 2016-11-02T14:49:14Z

talked with @mfojtik, would like to see a test that fails without the fix in place first

mfojtik · 2016-11-03T13:42:50Z

@liggitt now that I spend couple hours making that test working, I think I would defer this as making this work in kube will require more work.

Basically, when I register new schema and add my types there, the AsVersionedObject method have api.Schema.ObjectKinds(info.Object) hardcoded, so my custom types will get converted to runtime.Unknown and the test will fail....
This also explains why this fix is working just fine for origin (Origin types are registered in api.Schema?).

To make the test work (I can show you WIP) we will need to add runtime.ObjectTyper to AsVersionedObject (or even runtime.Schema) and plumb this all the way up to factory and all tests...

So what I suggest is to merge this as it is now and open a follow up to refactor this post-mortem. We also need this for Origin to fix the regression bug which is due to tomorrow. WDYT?

mfojtik · 2016-11-03T13:58:45Z

@liggitt the WIP of the test is here:

mfojtik@2c351af

I can submit this with the test disabled until we fix the `AsVersionedObject (

kubernetes/pkg/kubectl/resource/result.go

Line 254 in 56bbfd2

    
           if _, _, err := api.Scheme.ObjectKinds(info.Object); runtime.IsNotRegisteredError(err) {

)

smarterclayton · 2016-11-03T14:30:31Z

The problem here is the use of Get and the structured scheme. Get rid of /
fix either and you fix this issue.

On Nov 3, 2016, at 9:59 AM, Michal Fojtik notifications@github.com wrote:

@liggitt https://github.com/liggitt the WIP of the test is here:

mfojtik/kubernetes@2c351af
mfojtik@2c351af

I can submit this with the test disabled until we fix the
`AsVersionedObject (

kubernetes/pkg/kubectl/resource/result.go

Line 254 in 56bbfd2

    
           if _, _, err := api.Scheme.ObjectKinds(info.Object); runtime.IsNotRegisteredError(err) {

)

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

mfojtik · 2016-11-03T14:31:44Z

@smarterclayton i'm not sure I understand what you're suggesting

mfojtik · 2016-11-03T15:57:41Z

@liggitt added typer/encoder into AsVersionedObject and added unit test.

mfojtik · 2016-11-03T15:57:51Z

@liggitt PTAL

mfojtik · 2016-11-03T15:58:25Z

pkg/kubectl/cmd/util/helpers_test.go

+	s.AddKnownTypes(externalGV, &ExternalTestType{})
+	s.AddKnownTypeWithName(externalGV.WithKind("TestType"), &ExternalTestType{})
+	s.AddKnownTypeWithName(internalGV.WithKind("TestType"), &TestType{})
+	// TODO: Use AddToScheme(s) here instead of explicitely calling the conversion function.


@liggitt if I do this I will get:

unexpected error: converting (util.TestType).CreationTimestamp.Time.sec to (util.ExternalTestType).CreationTimestamp.Time.sec: Cannot set dest. (Tried to deep copy something with unexported fields?)

#35728 will fix it so that runtime.NewScheme() will get you the deep copies needed for objectmeta, so this is fine for now

k8s-ci-robot · 2016-11-03T16:51:42Z

Jenkins GCI GCE e2e failed for commit 5953e45. 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-04T10:22:40Z

Jenkins unit/integration failed for commit 61720752b222679a27e6ffca4d3f04f7510cb577. 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-04T10:49:05Z

Jenkins GCE Node e2e failed for commit 86e6fd6. Full PR test history.

The magic incantation to run this job again is @k8s-bot node 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-04T10:51:06Z

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

soltysh · 2016-11-04T11:38:28Z

@k8s-bot node e2e test this

soltysh · 2016-11-04T11:58:20Z

Still LGTM

liggitt · 2016-11-04T12:12:40Z

pkg/kubectl/cmd/edit.go

@@ -340,10 +340,10 @@ func getPrinter(cmd *cobra.Command) (*editPrinterOptions, error) {
 	}
 }

-func getMapperAndResult(f cmdutil.Factory, args []string, options *resource.FilenameOptions, editMode EditMode) (meta.RESTMapper, *resource.Mapper, *resource.Result, string, error) {
+func getMapperAndResult(f cmdutil.Factory, args []string, options *resource.FilenameOptions, editMode EditMode) (meta.RESTMapper, *resource.Mapper, runtime.ObjectTyper, *resource.Result, string, error) {


don't you already return the typer in resourceMapper.ObjectTyper?

you're right! will fix it

liggitt · 2016-11-04T12:13:27Z

pkg/kubectl/cmd/get.go

@@ -150,6 +150,9 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
 	allNamespaces := cmdutil.GetFlagBool(cmd, "all-namespaces")
 	showKind := cmdutil.GetFlagBool(cmd, "show-kind")
 	mapper, typer := f.Object()


when is this returning nil? (I assume it was nil or you wouldn't have added options.Typer)

?

(I need to expose typer in GetOptions for a unit test)

it's possible the test typer we mocked up in NewMixedFactory is wrong, or it's possible something else is wrong, but we need to figure out the problem there and fix it, not swap in a fake typer here for test

the test MixedFactory was actually returning a typer that would recognize the test types, but the point of TestGetUnknownSchemaObjectListGeneric is to test serialization of objects the compiled-in typer doesn't recognize. Because you plumbed the typer correctly, we needed a way to simulate the typer not recognizing the test type.

fixed in mfojtik#1, and removed this field and test-only hack

liggitt · 2016-11-04T12:14:58Z

pkg/kubectl/cmd/get_test.go

@@ -225,10 +225,10 @@ func TestGetUnknownSchemaObjectListGeneric(t *testing.T) {
 		}
 		arr := out["items"].([]interface{})
 		if arr[0].(map[string]interface{})["apiVersion"] != test.testtypeVersion {


save locally and reuse, rather than repeating the index+cast:

if version := arr[0].(map[string]interface{})["apiVersion"]; version != test.testtypeVersion { ... version ... }

i can drop this change, just something I encountered and was hard to debug.

liggitt · 2016-11-04T18:08:31Z

pkg/kubectl/cmd/get_test.go

@@ -210,7 +210,7 @@ func TestGetUnknownSchemaObjectListGeneric(t *testing.T) {
 		cmd.Flags().Set("output", "json")

 		cmd.Flags().Set("output-version", test.outputVersion)
-		err := RunGet(f, buf, errBuf, cmd, []string{"type/foo", "replicationcontrollers/foo"}, &GetOptions{})
+		err := RunGet(f, buf, errBuf, cmd, []string{"type/foo", "replicationcontrollers/foo"}, &GetOptions{Typer: api.Scheme})


we need to revert this change and figure out whether the problem is in the typer returned from NewMixedFactory, or in AsVersionedObject, or elsewhere

mfojtik · 2016-11-07T08:24:09Z

@liggitt I think @deads2k already took care of the "get" here:

#36085

So no plumbing is needed with this PR.

soltysh · 2016-11-07T16:40:51Z

It's possible that this is fixed in #36369 much more cleanly. @mfojtik mind checking that?

deads2k · 2016-11-07T21:05:06Z

It's possible that this is fixed in #36369 much more cleanly. @mfojtik mind checking that?

That ended up too big for 1.5

liggitt · 2016-11-07T21:56:40Z

yeah, #36085 fixed the get case

mfojtik · 2016-11-08T13:03:49Z

@liggitt @deads2k you OK merging this as it is?

k8s-github-robot · 2016-11-11T22:49:04Z

@mfojtik PR needs rebase

k8s-github-robot · 2017-01-24T00:13:22Z

[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

k8s-github-robot · 2017-02-06T13:12:43Z

This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review.

cc @adohe @deads2k @fabianofranz @mfojtik

You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days

googlebot added the cla: yes label Nov 2, 2016

k8s-github-robot assigned deads2k Nov 2, 2016

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Nov 2, 2016

mfojtik force-pushed the fix-record branch from 9fe6cb3 to e72f1ed Compare November 2, 2016 14:30

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 2, 2016

soltysh added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Nov 2, 2016

mfojtik force-pushed the fix-record branch from e72f1ed to 5953e45 Compare November 3, 2016 15:55

mfojtik commented Nov 3, 2016

View reviewed changes

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

mfojtik changed the title ~~Use versioned object when recording changes using --record~~ Fix AsVersionedObject to deal with external schema Nov 3, 2016

mfojtik force-pushed the fix-record branch from 5953e45 to 912acd0 Compare November 3, 2016 16:57

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

mfojtik force-pushed the fix-record branch from 9b9a3af to 6172075 Compare November 4, 2016 10:05

mfojtik force-pushed the fix-record branch from 6172075 to 86e6fd6 Compare November 4, 2016 10:26

liggitt reviewed Nov 4, 2016

View reviewed changes

mfojtik force-pushed the fix-record branch from 86e6fd6 to 3b7ad4f Compare November 4, 2016 12:18

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

liggitt reviewed Nov 4, 2016

View reviewed changes

Fix AsVersionedObject to deal with external schemas

61bab61

mfojtik force-pushed the fix-record branch from 3b7ad4f to 61bab61 Compare November 7, 2016 08:20

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 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 assigned adohe-zz and unassigned deads2k Jan 30, 2017

apelisse assigned deads2k Jan 31, 2017

k8s-github-robot assigned fabianofranz Jan 31, 2017

k8s-github-robot closed this Feb 6, 2017

Fix AsVersionedObject to deal with external schema #36066

Fix AsVersionedObject to deal with external schema #36066

Conversation

mfojtik commented Nov 2, 2016 • edited by k8s-github-robot Loading

mfojtik commented Nov 2, 2016

soltysh commented Nov 2, 2016

0xmichalis commented Nov 2, 2016

mfojtik commented Nov 2, 2016

mfojtik commented Nov 2, 2016

soltysh commented Nov 2, 2016

liggitt commented Nov 2, 2016

mfojtik commented Nov 3, 2016

mfojtik commented Nov 3, 2016

smarterclayton commented Nov 3, 2016

mfojtik commented Nov 3, 2016

mfojtik commented Nov 3, 2016

mfojtik commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 3, 2016

k8s-ci-robot commented Nov 4, 2016

k8s-ci-robot commented Nov 4, 2016

k8s-ci-robot commented Nov 4, 2016

soltysh commented Nov 4, 2016

soltysh commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Nov 4, 2016 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Nov 7, 2016

soltysh commented Nov 7, 2016

deads2k commented Nov 7, 2016

liggitt commented Nov 7, 2016

mfojtik commented Nov 8, 2016

k8s-github-robot commented Nov 11, 2016

k8s-github-robot commented Jan 24, 2017

k8s-github-robot commented Feb 6, 2017

mfojtik commented Nov 2, 2016 •

edited by k8s-github-robot

Loading

liggitt Nov 4, 2016 •

edited

Loading