Fix kubectl sort-by metadata issue. #21694

aveshagarwal · 2016-02-22T18:19:07Z

Internal types are not supposed to have json metadata (though in kubernetes
they do) as it is true with openshift types. That means sort-by must work
on versioned objects for sorting, otherwise it produces "error: metadata
is not found" error if it sorts internal types without json metadata.

This PR converts internal types objects to versioned objects and sort-by
sorts them correctly without medata error, and then it prints
corresponding internal objects in sorted order.

aveshagarwal · 2016-02-22T18:19:19Z

@kubernetes/rh-cluster-infra

k8s-github-robot · 2016-02-22T18:24:40Z

Labelling this PR as size/S

k8s-bot · 2016-02-22T18:27:26Z

GCE e2e build/test failed for commit 71412f2.

ncdc · 2016-02-22T18:30:04Z

cc @kubernetes/rh-ux @kubernetes/kubectl

liggitt · 2016-02-22T18:37:00Z

pkg/kubectl/cmd/get.go

+		if err != nil {
+			return err
+		}
+
 		// TODO: questionable
 		if sorter, err = kubectl.SortObjects(f.Decoder(true), objs, sorting); err != nil {


at first glance, rather than converting into versions objects, here, I think would expect to pass the output version to SortObjects... it is taking a fieldInput, which is not really meaningful unless paired with a group version. In SortObjects, can we decode to the specified groupversion? I'd like @smarterclayton's take on that as well...

I would expect us to be sorting based on the JSON and the field path
qualifier only, or at minimum on the map[string]interface{}. It's a
riskier change though.

On Mon, Feb 22, 2016 at 1:37 PM, Jordan Liggitt notifications@github.com
wrote:

In pkg/kubectl/cmd/get.go
#21694 (comment)
:

clientConfig, err := f.ClientConfig()

if err != nil {

return err

}

version, err := cmdutil.OutputVersion(cmd, clientConfig.GroupVersion)

if err != nil {

return err

}

objs, err = resource.AsVersionedObjects(infos, version.String(), f.JSONEncoder())

if err != nil {

return err

}

// TODO: questionable
if sorter, err = kubectl.SortObjects(f.Decoder(true), objs, sorting); err != nil {

at first glance, rather than converting into versions objects, here, I
think would expect to pass the output version to SortObjects... it is
taking a fieldInput, which is not really meaningful unless paired with a
group version. In SortObjects, can we decode to the specified
groupversion? I'd like @smarterclayton https://github.com/smarterclayton's
take on that as well...

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/21694/files#r53669745.

@liggitt I am looking into what you suggested and i think it should be possible. The only issue with that I see is that once we decode objects into specified groupversion inside SortObjects, after sorting, any caller of sort-by would have converted versioned objects and it would require handling printing of those objects in the similar way as it is done currently in kubectl get. Because as I understand, it is not possible to print versioned objects directly but only internal types objects.

Or another way might be to convert sorted and decoded obejcts back to their internal types and then return them. Although I am not sure how to handle this,

ncdc · 2016-02-22T21:12:01Z

e2e failed because of

Error pulling image (0.3) from gcr.io/google_containers/kube-registry-proxy, endpoint: https://gcr.io/v1/, Get https://storage.googleapis.com/artifacts.google-containers.appspot.com/containers/images/9b9342c134bbda6f57c95c52cafd4ae40333dd6cc0fce4bace9296f957042d22/ancestry: dial tcp 74.125.202.128:443: i/o timeout

ncdc · 2016-02-22T21:26:50Z

@k8s-bot e2e test this issue: #IGNORE

k8s-bot · 2016-02-22T21:57:47Z

GCE e2e build/test failed for commit 71412f2.

k8s-bot · 2016-02-22T21:59:10Z

GCE e2e test build/test passed for commit 71412f2.

k8s-github-robot · 2016-02-22T22:21:48Z

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

k8s-github-robot · 2016-02-23T22:07:22Z

Labelling this PR as size/M

aveshagarwal · 2016-02-23T22:24:10Z

I have updated this PR and it seems to be working wth kubectl get. I am still trying to see if it could affect annotate, create, rollingupdate, expose, run, autoscale, label, although I have yet to check if these commands really need sort-by or not.

liggitt · 2016-02-23T22:28:04Z

Wouldn't anything that outputs objects potentially use SortObjects and be affected? That's why I think the work of applying the sort by field to the correct output version should be done inside the common sorting code

k8s-bot · 2016-02-23T22:36:20Z

GCE e2e test build/test passed for commit 01632d0f7daf16e5d37c4fb57d2112db56b49501.

k8s-bot · 2016-02-23T22:38:57Z

GCE e2e test build/test passed for commit b0624ef08311eacb4267f33aa4e74d2e03d1acd3.

0xmichalis · 2016-02-23T22:43:55Z

I think sort-by was added only for kubectl get and no other command really uses it.

liggitt · 2016-02-23T22:55:59Z

is it a global flag? If so, we should stop adding global flags that only apply to a single command

lavalamp · 2016-02-24T00:46:00Z

pkg/kubectl/sorting_printer.go

@@ -63,7 +66,7 @@ func (s *SortingPrinter) sortObj(obj runtime.Object) error {
 		return nil
 	}

-	sorter, err := SortObjects(s.Decoder, objs, s.SortField)
+	sorter, err := SortObjects(s.Decoder, objs, s.SortField, "")


We should probably never sort by the "" version or group

lavalamp · 2016-02-24T00:46:39Z

Wouldn't it be a lot easier to just convert to the output version to begin with, and then nothing else would need to change?

0xmichalis · 2016-02-24T09:38:05Z

is it a global flag? If so, we should stop adding global flags that only apply to a single command

Agreed

aveshagarwal · 2016-03-01T02:57:29Z

e2e flake:

[Fail] Deployment [It] deployment should delete old replica sets 
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/deployment.go:440

#22088 where this flake is being tracked and also has a fix: #22223

liggitt · 2016-03-01T15:49:26Z

this is obscure enough that it deserves at least one test, I think

aveshagarwal · 2016-03-01T15:51:56Z

@liggitt ok will look into that.

k8s-github-robot · 2016-03-01T15:54:11Z

@aveshagarwal
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

k8s-bot · 2016-03-01T16:16:37Z

GCE e2e build/test passed for commit 5207cb5317bf81204b2c7d3930bdd80e336c365e.

Internal types are not supposed to have json metadata (though in kubernetes they do) as it is true with openshift types. That means sort-by must work on versioned objects for sorting, otherwise it produces "error: metadata is not found" error if it sorts internal types without json metadata. This PR converts internal types objects to versioned objects and sort-by sorts them correctly without medata error, and then it prints corresponding internal objects in sorted order.

aveshagarwal · 2016-03-04T21:16:38Z

@liggitt Added a test case. PTAL.

k8s-github-robot · 2016-03-04T21:25:00Z

Labelling this PR as size/M

k8s-bot · 2016-03-04T21:51:37Z

GCE e2e build/test passed for commit be06003.

liggitt · 2016-03-07T14:22:03Z

LGTM

k8s-github-robot · 2016-03-07T14:31:16Z

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

k8s-bot · 2016-03-07T15:05:28Z

GCE e2e build/test passed for commit be06003.

k8s-github-robot · 2016-03-07T15:37:14Z

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

k8s-bot · 2016-03-07T16:07:41Z

GCE e2e build/test passed for commit be06003.

k8s-github-robot · 2016-03-07T16:09:55Z

Automatic merge from submit-queue

Auto commit by PR queue bot

aveshagarwal mentioned this pull request Feb 22, 2016

Fix metadata issue for deployment and build configs openshift/origin#7299

Closed

googlebot added the cla: yes label Feb 22, 2016

k8s-github-robot assigned lavalamp Feb 22, 2016

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

liggitt reviewed Feb 22, 2016
View reviewed changes

k8s-github-robot added the needs-ok-to-merge label Feb 22, 2016

aveshagarwal force-pushed the master-sort-by-versioned branch from 71412f2 to b0624ef Compare February 23, 2016 22:06

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 Feb 23, 2016

aveshagarwal force-pushed the master-sort-by-versioned branch from b0624ef to 01632d0 Compare February 23, 2016 22:09

lavalamp reviewed Feb 24, 2016
View reviewed changes

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2016

bgrant0607 assigned liggitt and unassigned lavalamp Mar 2, 2016

bgrant0607 added the area/kubectl label Mar 2, 2016

aveshagarwal force-pushed the master-sort-by-versioned branch from 5207cb5 to be06003 Compare March 4, 2016 21:15

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 Mar 4, 2016

liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2016

k8s-github-robot pushed a commit that referenced this pull request Mar 7, 2016

Merge pull request #21694 from aveshagarwal/master-sort-by-versioned

492c5d6

Auto commit by PR queue bot

k8s-github-robot merged commit 492c5d6 into kubernetes:master Mar 7, 2016

aveshagarwal deleted the master-sort-by-versioned branch March 8, 2016 14:38

bgrant0607 added this to the v1.2 milestone Mar 8, 2016

bgrant0607 added the cherrypick-candidate label Mar 8, 2016

eparis removed the cherrypick-candidate label Mar 9, 2016

liggitt mentioned this pull request Mar 18, 2016

Remove json tags from pkg/api/types.go #3933

Closed

Fix kubectl sort-by metadata issue. #21694

Fix kubectl sort-by metadata issue. #21694

Conversation

aveshagarwal commented Feb 22, 2016

aveshagarwal commented Feb 22, 2016

k8s-github-robot commented Feb 22, 2016

k8s-bot commented Feb 22, 2016

ncdc commented Feb 22, 2016

liggitt Feb 22, 2016

Choose a reason for hiding this comment

smarterclayton Feb 23, 2016

Choose a reason for hiding this comment

aveshagarwal Feb 23, 2016

Choose a reason for hiding this comment

aveshagarwal Feb 23, 2016

Choose a reason for hiding this comment

ncdc commented Feb 22, 2016

ncdc commented Feb 22, 2016

k8s-bot commented Feb 22, 2016

k8s-bot commented Feb 22, 2016

k8s-github-robot commented Feb 22, 2016

k8s-github-robot commented Feb 23, 2016

aveshagarwal commented Feb 23, 2016

liggitt commented Feb 23, 2016

k8s-bot commented Feb 23, 2016

k8s-bot commented Feb 23, 2016

0xmichalis commented Feb 23, 2016

liggitt commented Feb 23, 2016

lavalamp Feb 24, 2016

Choose a reason for hiding this comment

lavalamp commented Feb 24, 2016

0xmichalis commented Feb 24, 2016

aveshagarwal commented Mar 1, 2016

liggitt commented Mar 1, 2016

aveshagarwal commented Mar 1, 2016

k8s-github-robot commented Mar 1, 2016

k8s-bot commented Mar 1, 2016

aveshagarwal commented Mar 4, 2016

k8s-github-robot commented Mar 4, 2016

k8s-bot commented Mar 4, 2016

liggitt commented Mar 7, 2016

k8s-github-robot commented Mar 7, 2016

k8s-bot commented Mar 7, 2016

k8s-github-robot commented Mar 7, 2016

k8s-bot commented Mar 7, 2016

k8s-github-robot commented Mar 7, 2016