Match GroupVersionKind against specific version #34010

soltysh · 2016-10-04T12:09:03Z

Currently when multiple GVK match a specific kind in KindForGroupVersionKinds only the first will be matched, which not necessarily will be the correct one. I'm proposing to extend this to pick the best match, instead.

Here's my problematic use-case, of course it involves ScheduledJobs 😉:
I have a GroupVersions with batch/v1 and batch/v2alpha1 in that order. I'm calling KindForGroupVersionKinds with kind batch/v2alpha1 ScheduledJob and that currently results this matching first GroupVersion, instead of picking more concrete one. There's a clear description why it is on single GroupVersion, but GroupVersions should pick this more carefully.

@deads2k this is your baby, wdyt?

Match GroupVersionKind against specific version

This change is

deads2k · 2016-10-04T12:13:09Z

Looks reasonable to me, but this is definitely @smarterclayton's baby. :)

smarterclayton · 2016-10-04T17:35:40Z

pkg/api/unversioned/group_version.go

@@ -268,13 +268,30 @@ type GroupVersions []GroupVersion

 // KindForGroupVersionKinds identifies the preferred GroupVersionKind out of a list. It returns ok false
 // if none of the options match the group.
-func (gvs GroupVersions) KindForGroupVersionKinds(kinds []GroupVersionKind) (target GroupVersionKind, ok bool) {
+func (gvs GroupVersions) KindForGroupVersionKinds(kinds []GroupVersionKind) (GroupVersionKind, bool) {
+	targets := []GroupVersionKind{}


Always do var targets []GroupVersionKind

Always do var targets []GroupVersionKind

Not obvious to me. Why?

smarterclayton · 2016-10-04T17:36:54Z

pkg/api/unversioned/group_version_test.go

+			input:  []GroupVersionKind{{Group: "policy", Version: "v1alpha1", Kind: "PodDisruptionBudget"}},
+			target: GroupVersionKind{Group: "policy", Version: "v1alpha1", Kind: "PodDisruptionBudget"},
+			ok:     true,
+		},


Add all the other scenarios to this test.

smarterclayton · 2016-10-04T17:38:17Z

pkg/api/unversioned/group_version.go

+	}
+	if len(targets) == 1 {
+		return targets[0], true
+	} else if len(targets) > 1 {


Not an else

smarterclayton · 2016-10-04T17:38:44Z

pkg/api/unversioned/group_version.go

+		for _, k := range kinds {
+			if k == gvk {
+				return k, true
+			}


This doesn't seem logically equivalent to what we had before - if no exact match exists, you need to pick the first target.

smarterclayton

Requested changes

soltysh · 2016-10-04T20:32:41Z

@smarterclayton all comments addressed ptal

k8s-ci-robot · 2016-10-04T20:55:49Z

Jenkins unit/integration failed for commit 0d3ff1d3bac292b9830613df0ebfb968e70d4507. 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.

smarterclayton · 2016-10-04T22:47:30Z

Only allocate if you need it. This is a hot code path.

On Oct 4, 2016, at 1:54 PM, David Eads notifications@github.com wrote:

@deads2k commented on this pull request.

In pkg/api/unversioned/group_version.go
#34010:

@@ -268,13 +268,30 @@ type GroupVersions []GroupVersion

// KindForGroupVersionKinds identifies the preferred GroupVersionKind
out of a list. It returns ok false
// if none of the options match the group.
-func (gvs GroupVersions) KindForGroupVersionKinds(kinds
[]GroupVersionKind) (target GroupVersionKind, ok bool) {
+func (gvs GroupVersions) KindForGroupVersionKinds(kinds
[]GroupVersionKind) (GroupVersionKind, bool) {

targets := []GroupVersionKind{}

Always do var targets []GroupVersionKind

Not obvious to me. Why?

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#34010, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p1t1Igstl97b7OESIpnmi66qTNkRks5qwpLBgaJpZM4KNmXk
.

soltysh · 2016-10-05T10:52:17Z

I had to drop one test case from pkg/runtime/scheme_test.go which was explicitly relying on picking the first object from GroupVersionKinds list, instead of the most matching one, @smarterclayton ptal

smarterclayton · 2016-10-05T16:39:25Z

pkg/runtime/scheme_test.go

@@ -602,13 +602,6 @@ func TestConvertToVersion(t *testing.T) {
 			gv:     unversioned.GroupVersion{Version: "__internal"},
 			out:    &TestType1{A: "test"},
 		},
-		// prefers the first group version in the list


Don't remove the test, change the output and then add a second test

2nd test is not needed since the previous one is touching the conversion from external to internal if such GV is passed. I'll fix the output for this test, only.

soltysh · 2016-10-05T18:59:38Z

Updated with reverted and fixed tests. @smarterclayton ptal

soltysh · 2016-10-10T08:19:39Z

@smarterclayton bump, I'm blocked with this downstream, ptal

k8s-github-robot · 2016-10-10T12:38:42Z

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

k8s-github-robot · 2016-10-10T13:16:29Z

Automatic merge from submit-queue

flavianmissi · 2016-10-11T15:08:22Z

I see this didn't make it to v1.4.1, any idea of when we'll have a v1.4.2? (sorry if this is the wrong channel to ask, please do redirect me if there's a better one:)

soltysh · 2016-10-12T10:14:07Z

@jessfraz any chance to have this cherry-picked for 1.4.2?

jessfraz · 2016-10-12T17:06:31Z

will do!

soltysh · 2016-10-12T18:57:10Z

Thank you!

#34468-#34416-#34010-origin-release-1.4 Automatic merge from submit-queue Automated cherry pick of #33147 #34468 #34416 #34010 origin release 1.4 Cherry pick of #33147 #34468 #34416 #34010 on release-1.4. #33147: fix base image pinning during upgrades via #34468: Fix upgrade.sh image setup #34416: hyperkube image: add cifs-utils #34010: Match GroupVersionKind against specific version

k8s-cherrypick-bot · 2016-10-13T01:26:28Z

Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

flavianmissi · 2016-10-18T08:37:47Z

Looking forward to see this on GKE, thank you! o/

…ck-of-#33147-kubernetes#34468-kubernetes#34416-kubernetes#34010-origin-release-1.4 Automatic merge from submit-queue Automated cherry pick of kubernetes#33147 kubernetes#34468 kubernetes#34416 kubernetes#34010 origin release 1.4 Cherry pick of kubernetes#33147 kubernetes#34468 kubernetes#34416 kubernetes#34010 on release-1.4. kubernetes#33147: fix base image pinning during upgrades via kubernetes#34468: Fix upgrade.sh image setup kubernetes#34416: hyperkube image: add cifs-utils kubernetes#34010: Match GroupVersionKind against specific version

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

soltysh assigned deads2k Oct 4, 2016

googlebot added the cla: yes label Oct 4, 2016

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 4, 2016

deads2k assigned smarterclayton and unassigned deads2k Oct 4, 2016

smarterclayton reviewed Oct 4, 2016

View reviewed changes

smarterclayton requested changes Oct 4, 2016

View reviewed changes

soltysh force-pushed the fix_edit_sj branch from 3ab7278 to 0d3ff1d Compare October 4, 2016 20:32

soltysh force-pushed the fix_edit_sj branch from 0d3ff1d to 0ba115a Compare October 5, 2016 10:51

smarterclayton reviewed Oct 5, 2016

View reviewed changes

Match GroupVersionKind against specific version

69f751a

soltysh force-pushed the fix_edit_sj branch from 0ba115a to 69f751a Compare October 5, 2016 18:59

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2016

k8s-github-robot merged commit 049ad98 into kubernetes:master Oct 10, 2016

soltysh deleted the fix_edit_sj branch October 11, 2016 06:28

soltysh added the cherrypick-candidate label Oct 12, 2016

soltysh added this to the v1.4 milestone Oct 12, 2016

jessfraz added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Oct 12, 2016

jessfraz added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 12, 2016

jessfraz mentioned this pull request Oct 12, 2016

Automated cherry pick of #33147 #34468 #34416 #34010 origin release 1.4 #34628

Merged

k8s-cherrypick-bot removed the cherrypick-candidate label Oct 13, 2016

vreon mentioned this pull request Oct 19, 2016

kubectl apply emits error when patching a ScheduledJob #35149

Closed

mengqiy mentioned this pull request Oct 25, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Match GroupVersionKind against specific version #34010

Match GroupVersionKind against specific version #34010

soltysh commented Oct 4, 2016 •

edited by jessfraz

Loading

deads2k commented Oct 4, 2016

smarterclayton Oct 4, 2016

deads2k Oct 4, 2016

smarterclayton Oct 4, 2016

smarterclayton Oct 4, 2016

smarterclayton Oct 4, 2016

smarterclayton left a comment

soltysh commented Oct 4, 2016

k8s-ci-robot commented Oct 4, 2016

smarterclayton commented Oct 4, 2016

soltysh commented Oct 5, 2016

smarterclayton Oct 5, 2016

soltysh Oct 5, 2016

soltysh commented Oct 5, 2016

soltysh commented Oct 10, 2016

k8s-github-robot commented Oct 10, 2016

k8s-github-robot commented Oct 10, 2016

flavianmissi commented Oct 11, 2016

soltysh commented Oct 12, 2016

jessfraz commented Oct 12, 2016

soltysh commented Oct 12, 2016

k8s-cherrypick-bot commented Oct 13, 2016

flavianmissi commented Oct 18, 2016

Match GroupVersionKind against specific version #34010

Match GroupVersionKind against specific version #34010

Conversation

soltysh commented Oct 4, 2016 • edited by jessfraz Loading

deads2k commented Oct 4, 2016

smarterclayton Oct 4, 2016

Choose a reason for hiding this comment

deads2k Oct 4, 2016

Choose a reason for hiding this comment

smarterclayton Oct 4, 2016

Choose a reason for hiding this comment

smarterclayton Oct 4, 2016

Choose a reason for hiding this comment

smarterclayton Oct 4, 2016

Choose a reason for hiding this comment

smarterclayton left a comment

Choose a reason for hiding this comment

soltysh commented Oct 4, 2016

k8s-ci-robot commented Oct 4, 2016

smarterclayton commented Oct 4, 2016

@deads2k commented on this pull request.

soltysh commented Oct 5, 2016

smarterclayton Oct 5, 2016

Choose a reason for hiding this comment

soltysh Oct 5, 2016

Choose a reason for hiding this comment

soltysh commented Oct 5, 2016

soltysh commented Oct 10, 2016

k8s-github-robot commented Oct 10, 2016

k8s-github-robot commented Oct 10, 2016

flavianmissi commented Oct 11, 2016

soltysh commented Oct 12, 2016

jessfraz commented Oct 12, 2016

soltysh commented Oct 12, 2016

k8s-cherrypick-bot commented Oct 13, 2016

flavianmissi commented Oct 18, 2016

soltysh commented Oct 4, 2016 •

edited by jessfraz

Loading