thirdpartyresource add multiple version support #36977

adohe-zz · 2016-11-17T06:08:50Z

This change is

deads2k · 2016-11-17T21:15:44Z

pkg/api/meta/priority.go

 	// any versions the user provides take priority
 	priorities := m.KindPriority
 	if len(versions) > 0 {
 		priorities = make([]unversioned.GroupVersionKind, 0, len(m.KindPriority)+len(versions))
 		for _, version := range versions {
+			if strings.Count(version, "/") == 0 {


This doesn't look right. Doesn't ParseGroupVersion already do this? Also, open an issue to fix this method signature. Either it gets a version or a groupversion, but we shouldn't have strings floating around anymore.

The problem with ParseGroupVersion is if version parameter doesn't match group/version, for example v2, it will return GroupVersion{"", "v2"} with empty group. And this is not correct if a TPR schema has multi versions: v1, v2, and user provides v2, this will fail to recognize. Why not use group field of groupkind?

v2 with empty group is not valid. All TPR would have somegroup/v2

the question is when version would be group/version or version when call restmapper?

@deads2k @liggitt I want to know why ParseGroupVersion use empty group when version is not group/version format, I think it should respect groupKind not use empty group.

@deads2k @liggitt I want to know why ParseGroupVersion use empty group when version is not group/version format, I think it should respect groupKind not use empty group.

versions should be just the version part. Group should come from the GroupKind.

I will open a separate issue to describe this.

deads2k · 2016-11-17T21:20:27Z

pkg/apiserver/api_installer.go

@@ -108,6 +108,7 @@ func (a *APIInstaller) NewWebService() *restful.WebService {
 	mediaTypes, streamMediaTypes := mediaTypesForSerializer(a.group.Serializer)
 	ws.Produces(append(mediaTypes, streamMediaTypes...)...)
 	ws.ApiVersion(a.group.GroupVersion.String())
+	ws.SetDynamicRoutes(true)


This change isn't obvious to me. Why would make this the new default?

Given our failed TPR integration test for example, if we create two TPR schemas: foo.company.com and bar.company.com, both with v1, thirdparty server will create a WebService, but if we delete bar.company.com, thirdparty server does nothing. And of course we cannot remove the entire web service, but we can remove bar.company.com related routes, as for go-restful, if we don't enable dynamic routes, we are note allowed to remove these routes.

I'm wary of changes that affect things other than the TPR endpoints. What if we made the resource endpoint a nested web service? Those were allowed to be added/removed previously without this setting

@liggitt this attribute is just webservice level, which gives us chance to remove routes handler of a specific webservice. I am not quite clear what you mean nested web service were allowed to be removed, would you mind explain more?

@liggitt this attribute is just webservice level, which gives us chance to remove routes handler of a specific webservice. I am not quite clear what you mean nested web service were allowed to be removed, would you mind explain more?

Can you create a webservice with a narrower scope for each TPR group? This sort of change is why we're interested in breaking TPRs out of the main API server.

+1 for putting TPRs in a separate ws, so that there's no way of accidentally removing a route for a built-in API.

@lavalamp I will try to update this, but what I am really confused is whether I should go on this PR against current apiserver or I should start working on a add-on server?

@adohe is there already an addon server in developing status?

IMO it's not bad to make improvements to the existing TPR implementation. We won't be moving it this quarter, I'm pretty sure.

deads2k · 2016-11-17T21:20:39Z

pkg/apiserver/api_installer.go

@@ -192,6 +193,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
 	if err != nil {
 		return nil, err
 	}
+	fmt.Printf("register resource handler for path: %s and the fqKind is: %v\n", path, fqKindToRegister)


deads2k · 2016-11-17T21:20:46Z

pkg/kubectl/resource/mapper.go

@@ -54,12 +54,14 @@ func (m *Mapper) InfoForData(data []byte, source string) (*Info, error) {
 		return nil, fmt.Errorf("unable to decode %q: %v", source, err)
 	}

+	fmt.Printf("info for data gvk is: %v\n", gvk)


liggitt · 2016-11-18T12:37:36Z

pkg/master/thirdparty/thirdparty.go

+			return false, nil
+		}
+	}
+	return true, nil
 }

 func (m *ThirdPartyResourceServer) removeThirdPartyStorage(path, resource string) error {


Can we change the signature so we get the info we need without string parsing?

how about change this to removeThirdPartyStorage(group, version, resource string), wdyt?

or gvr unversioned.GroupVersionResource

liggitt · 2016-11-18T12:39:13Z

pkg/master/thirdparty/thirdparty.go

+	}
+
+	if ws != nil {
+		pattern := path + "/.*/" + resource + ".*"


Why is the trailing .* needed

there should be some problem with this regexp, I am fixing it.

k8s-ci-robot · 2016-12-01T12:38:10Z

Jenkins GCE etcd3 e2e failed for commit 3d6a4c9. 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-12-01T12:38:10Z

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

k8s-ci-robot · 2016-12-01T12:38:10Z

Jenkins GCE e2e failed for commit 3d6a4c9. 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.

k8s-ci-robot · 2016-12-01T12:38:40Z

Jenkins GCI GKE smoke e2e failed for commit 3d6a4c9. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke 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-12-01T12:38:42Z

Jenkins kops AWS e2e failed for commit 3d6a4c9. Full PR test history.

The magic incantation to run this job again is @k8s-bot kops aws 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-12-01T12:40:41Z

Jenkins GCE Node e2e failed for commit 3d6a4c9. 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.

adohe-zz · 2016-12-13T15:16:50Z

I get some time to get this updated, and fix all tests failure. @deads2k @liggitt @smarterclayton please take a look, and give a deep review. I would like to get this merged and then go on with TPR finalizers.

deads2k · 2016-12-16T16:48:54Z

pkg/registry/extensions/rest/thirdparty_controller.go

@@ -76,19 +78,31 @@ func (t *ThirdPartyController) SyncResources() error {
 }

 func (t *ThirdPartyController) syncResourceList(list runtime.Object) error {
-	existing := sets.String{}
+	existing := map[metav1.GroupVersionResource]sets.Empty{}


map[]struct{}

deads2k · 2016-12-16T16:50:36Z

pkg/registry/extensions/rest/thirdparty_controller.go

 	switch list := list.(type) {
 	case *extensions.ThirdPartyResourceList:
 		// Loop across all schema objects for third party resources
 		for ix := range list.Items {
 			item := &list.Items[ix]
 			// extract the api group and resource kind from the schema
-			_, group, err := thirdpartyresourcedata.ExtractApiGroupAndKind(item)
+			kind, group, err := thirdpartyresourcedata.ExtractApiGroupAndKind(item)


Seems like this API should return GroupKind and GroupResource for you.

I will check this when I return from ooo.

maybe we can return GroupKind here, seems we cannot extract GroupResource from item, wdyt?

deads2k · 2016-12-16T16:52:10Z

pkg/registry/extensions/rest/thirdparty_controller.go

-	ListThirdPartyResources() []string
+	// List all currently installed third party resources, return
+	// collection of all groupVersionResource
+	ListThirdPartyResources() []metav1.GroupVersionResource


good change.

deads2k · 2016-12-16T16:54:04Z

pkg/registry/extensions/rest/thirdparty_controller.go

 				found = true
 				break
 			}
 		}
 		// not expected, delete the resource
 		if !found {
-			if err := t.master.RemoveThirdPartyResource(installedAPI); err != nil {
+			if err := t.master.RemoveThirdPartyResource(installedGvr); err != nil {


I see the fake for this changed, and the interface, but I can't seem to find where the actual impl changed.

nm, github helpfully hid the diff.

actually, the implementation changed a lot :)

deads2k · 2016-12-16T16:59:14Z

pkg/master/thirdparty/thirdparty.go

@@ -84,12 +88,25 @@ func NewThirdPartyResourceServer(genericAPIServer *genericapiserver.GenericAPISe
 	return ret
 }

-// thirdPartyEntry combines objects storage and API group into one struct
+type thirdPartyGroup struct {
+	versionedEntry map[string]*thirdPartyEntry


document the key for this map. Version?

deads2k · 2016-12-16T16:59:40Z

pkg/master/thirdparty/thirdparty.go

+	group          metav1.APIGroup
+}
+
+// thirdPartyEntry combines objects storage into one struct
 // for easy lookup.
 type thirdPartyEntry struct {


tprVersionStorage?

good suggestion :)

deads2k · 2016-12-16T17:00:08Z

pkg/master/thirdparty/thirdparty.go

+
+func (g *thirdPartyGroup) versionInstalled(version string) bool {
+	for installedVersion := range g.versionedEntry {
+		if reflect.DeepEqual(version, installedVersion) {


deep equal on string is weird.

deads2k · 2016-12-16T17:00:50Z

pkg/master/thirdparty/thirdparty.go

-	// map from api path to a tuple of (storage for the objects, APIGroup)
-	thirdPartyResources map[string]*thirdPartyEntry
+	// map from api path to a tuple of (versioned storage for the objects, APIGroup)
+	thirdPartyResources map[string]*thirdPartyGroup


keying by path is weird. Why not key by group name and have the tprGroup hold a path value?

deads2k · 2016-12-16T17:01:34Z

pkg/master/thirdparty/thirdparty.go

 		return false, nil
 	}
 	plural, _ := meta.KindToResource(schema.GroupVersionKind{
 		Group:   group,
 		Version: rsrc.Versions[0].Name,
 		Kind:    kind,
 	})
-	_, found := entry.storage[plural.Resource]
-	return found, nil
+	for _, version := range rsrc.Versions {


Please rename rsrc to tpr. I keep having to look up what this is.

deads2k · 2016-12-16T17:02:33Z

pkg/master/thirdparty/thirdparty.go

+			return true
+		}
+	}
+	return false
 }

 // HasThirdPartyResource returns true if a particular third party resource currently installed.


if its installed in one version and not another, what's the behavior?

I think I actually expect this to take a GVR. That would make it unambiguous.

if we change HasThirdPartyResource(rsrc) to HasThirdPartyResource(gvr), then I would suppose we should make the same change to InstallThirdPartyResource, which means we should update the thirdparty controller. wdyt?

one TPR schema could define multiple GroupVersionResource identified resource, so update method to accept gvr parameter could make sense.

deads2k · 2016-12-16T17:04:43Z

pkg/master/thirdparty/thirdparty.go

 	if !found {
 		return nil
 	}
-	storage, found := entry.storage[resource]
+	storage, found := entry.storage[gvr.Resource]
 	if !found {
 		return nil
 	}
 	if err := m.removeAllThirdPartyResources(storage); err != nil {


Is this method just misnamed? Is it deleting a single storage?

actually it is deleting all TPR data in this storage registry.

deads2k · 2016-12-16T17:05:37Z

pkg/master/thirdparty/thirdparty.go

+	}
+
+	if ws != nil {
+		pattern := path + "/.*" + gvr.Resource + ".*"


why the wildcarding? This looks like it would hit false positives.

yes, this is still not correct.

deads2k · 2016-12-16T17:07:50Z

I think this may just be too big to fix inside of our current API server. The changes are pervasive and are changing attributes our current RESTHandler to allow removal. I worry about accidentally removing something important.

@lavalamp @kubernetes/sig-api-machinery-misc If we ran TPRs out of a addon API server, we could have a completely separate handler chain and have confidence of containment for changes like this.

k8s-github-robot · 2016-12-20T01:38:00Z

@adohe PR needs rebase

adohe-zz · 2016-12-20T13:09:44Z

@lavalamp could you please take a look? If we decide to run TPRs out of a addon API server, I would start building this addon server, and add this to that server.

k8s-github-robot · 2017-01-24T02:04:02Z

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

pkg/OWNERS
test/OWNERS

We suggest the following people:
cc @brendandburns @ixdy
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

bgrant0607 · 2017-02-07T14:48:03Z

See also #24299

liggitt · 2017-02-07T14:52:25Z

Since TPRs cannot be converted between versions by the server, we cannot enable multiple versions in the API server and behave like a standard Kubernetes API.

adohe-zz · 2017-02-10T06:11:56Z

@liggitt yes, so I am going to close this and will re-implement it in an addon server.

jamiehannaford · 2017-04-20T13:36:17Z

@adohe Did you get a chance to start work on the addon server? I'm interested in helping out with TPRs, so was interested in the current status of things

deads2k · 2017-04-21T16:50:06Z

@adohe Did you get a chance to start work on the addon server? I'm interested in helping out with TPRs, so was interested in the current status of things

We're looking at this proposal to move things forward: kubernetes/community#524.

k8s-github-robot · 2017-05-22T22:48:26Z

This PR hasn't been active in 31 days. It will be closed in 58 days (Jul 20, 2017).

cc @adohe @smarterclayton

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

googlebot added the cla: yes label Nov 17, 2016

adohe-zz mentioned this pull request Nov 17, 2016

[WIP] fix TPR single group, multiple versions issue #36157

Closed

k8s-github-robot assigned smarterclayton Nov 17, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Nov 17, 2016

adohe-zz force-pushed the tpr branch 3 times, most recently from a5489e4 to c24d4e1 Compare November 17, 2016 15:04

deads2k reviewed Nov 17, 2016

View reviewed changes

adohe-zz mentioned this pull request Nov 18, 2016

Require at least one version in ThirdPartyResource objects #36996

Merged

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

adohe-zz force-pushed the tpr branch from c24d4e1 to ae0c0b0 Compare November 18, 2016 11:57

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

liggitt reviewed Nov 18, 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 Nov 19, 2016

adohe-zz force-pushed the tpr branch from ccb1fb4 to b16be79 Compare November 21, 2016 07:38

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 21, 2016

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 21, 2016

adohe-zz force-pushed the tpr branch from b16be79 to 3d6a4c9 Compare December 1, 2016 12:32

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

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

adohe-zz force-pushed the tpr branch from d40b181 to da6aa81 Compare December 14, 2016 14:52

thirdpartyresource add multi version support

da6aa81

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

deads2k 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 Dec 20, 2016

foxish mentioned this pull request Jan 5, 2017

CustomResourceDefinitions kubernetes/enhancements#95

Closed

adohe-zz closed this May 31, 2017

thirdpartyresource add multiple version support #36977

thirdpartyresource add multiple version support #36977

Conversation

adohe-zz commented Nov 17, 2016 • edited by k8s-oncall

Choose a reason for hiding this comment

adohe-zz Nov 18, 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

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

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

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

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 1, 2016

k8s-ci-robot commented Dec 1, 2016

k8s-ci-robot commented Dec 1, 2016

k8s-ci-robot commented Dec 1, 2016

k8s-ci-robot commented Dec 1, 2016

k8s-ci-robot commented Dec 1, 2016

adohe-zz commented Dec 13, 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

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

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

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

deads2k commented Dec 16, 2016

k8s-github-robot commented Dec 20, 2016

adohe-zz commented Dec 20, 2016

k8s-github-robot commented Jan 24, 2017

bgrant0607 commented Feb 7, 2017

liggitt commented Feb 7, 2017 • edited

adohe-zz commented Feb 10, 2017

jamiehannaford commented Apr 20, 2017

deads2k commented Apr 21, 2017

k8s-github-robot commented May 22, 2017

adohe-zz commented Nov 17, 2016 •

edited by k8s-oncall

adohe-zz Nov 18, 2016 •

edited

liggitt commented Feb 7, 2017 •

edited