separate discovery from the apiserver #43003

deads2k · 2017-03-13T17:57:45Z

This decouples the API discovery handlers from the core API server code. It separates the code into a new package and clarifies interfaces with existing TPR code.

@kubernetes/sig-api-machinery-pr-reviews

k8s-reviewable · 2017-03-13T17:58:56Z

This change is

sttts · 2017-03-13T18:12:04Z

staging/src/k8s.io/apiserver/pkg/endpoints/discovery/legacy.go

+type legacyRootAPIDiscoveryHandler struct {
+	// DiscoveryAddresses is used to build cluster IPs for discovery.
+	discoveryAddresses DiscoveryAddresses
+


nit: empty lines

sttts · 2017-03-13T18:13:02Z

Haven't done a line-by-line review, but overall this looks good!

deads2k · 2017-03-15T11:29:09Z

@k8s-bot gce etcd3 e2e test this
@k8s-bot cvm gce e2e test this

deads2k · 2017-03-29T12:50:34Z

Rebased. @liggitt yes, I preserved the discovery ordering tests.

deads2k · 2017-04-12T12:09:31Z

rebased again. bump.

deads2k · 2017-05-01T16:06:16Z

@ncdc rebased again

ncdc · 2017-05-01T18:29:30Z

staging/src/k8s.io/apiserver/pkg/endpoints/discovery/legacy.go

+
+// AddApiWebService adds a service to return the supported api versions at the legacy /api.
+func (s *legacyRootAPIDiscoveryHandler) WebService() *restful.WebService {
+	// Because in release 1.1, /api returns response with empty APIVersion, we


Remove this comment since you have it a few lines up?

Remove this comment since you have it a few lines up?

done

ncdc · 2017-05-01T18:37:09Z

staging/src/k8s.io/apiserver/pkg/endpoints/discovery/root.go

+	"k8s.io/apiserver/pkg/endpoints/handlers/responsewriters"
+)
+
+type DiscoveryGroupManager interface {


godoc please

Since the package is discovery, call this GroupManager?

Since the package is discovery, call this GroupManager?

sure.

And AddGroup + RemoveGroup

godoc please

Since the package is discovery, call this GroupManager

And AddGroup + RemoveGroup

done

ncdc · 2017-05-01T18:37:27Z

staging/src/k8s.io/apiserver/pkg/endpoints/discovery/legacy.go

+)
+
+// legacyRootAPIDiscoveryHandler creates a webservice serving api group discovery.
+// Note: during the server runtime apiGroups might change.


What exactly does this note mean?

What exactly does this note mean?

Doesn't belong here. Will remove.

ncdc · 2017-05-01T20:03:44Z

staging/src/k8s.io/apiserver/pkg/endpoints/discovery/group.go

+
+// apiGroupDiscoveryHandler creates a webservice serving the supported versions, preferred version, and name
+// of a group. E.g., such a web service will be registered at /apis/extensions.
+type apiGroupDiscoveryHandler struct {


Theme: remove discovery from names? apiGroupHandler

ncdc · 2017-05-01T20:03:54Z

staging/src/k8s.io/apiserver/pkg/endpoints/discovery/group.go

+	group metav1.APIGroup
+}
+
+func NewAPIGroupDiscoveryHandler(serializer runtime.NegotiatedSerializer, group metav1.APIGroup) *apiGroupDiscoveryHandler {


Theme: remove discovery from names? NewAPIGroupHandler

ncdc · 2017-05-01T20:04:18Z

staging/src/k8s.io/apiserver/pkg/endpoints/discovery/legacy.go

+
+// legacyRootAPIDiscoveryHandler creates a webservice serving api group discovery.
+// Note: during the server runtime apiGroups might change.
+type legacyRootAPIDiscoveryHandler struct {


Theme: remove discovery from names? legacyRootAPIHandler

ncdc · 2017-05-01T20:04:55Z

staging/src/k8s.io/apiserver/pkg/endpoints/discovery/root.go

+
+// rootAPIsDiscoveryHandler creates a webservice serving api group discovery.
+// Note: during the server runtime apiGroups might change.
+type rootAPIsDiscoveryHandler struct {


Theme: remove discovery from names? rootAPIsHandler

ncdc · 2017-05-01T20:05:21Z

staging/src/k8s.io/apiserver/pkg/endpoints/discovery/root.go

+}
+
+// rootAPIsDiscoveryHandler creates a webservice serving api group discovery.
+// Note: during the server runtime apiGroups might change.


Is this comment appropriate here? If so, what is it saying exactly?

Is this comment appropriate here? If so, what is it saying exactly?

This list of API groups is not fixed and (at least currently) this is accomplished by mutating the handler itself. You cannot safely cache this information.

Is this comment appropriate here? If so, what is it saying exactly?

Clarified comment

deads2k · 2017-05-02T12:36:44Z

Comments addressed.

sttts · 2017-05-02T12:39:43Z

staging/src/k8s.io/apiserver/pkg/server/genericapiserver.go

-	apiGroupsForDiscovery     map[string]metav1.APIGroup
-	apiGroupNamesForDiscovery []string
+	// RootDiscoveryConfig serves /apis
+	RootDiscoveryConfig discovery.GroupManager


s/RootDiscoveryConfig/DiscoveryGroupManager/

s/RootDiscoveryConfig/DiscoveryGroupManager/

done

sttts · 2017-05-02T13:22:32Z

/lgtm

k8s-github-robot · 2017-05-02T15:55:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sttts

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

~~pkg/master/OWNERS~~ [deads2k,sttts]
~~staging/src/k8s.io/apiserver/pkg/endpoints/OWNERS~~ [deads2k,sttts]
~~staging/src/k8s.io/apiserver/pkg/server/OWNERS~~ [deads2k,sttts]

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-05-02T18:24:53Z

Automatic merge from submit-queue (batch tested with PRs 45227, 43003, 45231)

deads2k added this to the v1.7 milestone Mar 13, 2017

deads2k assigned ncdc Mar 13, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 13, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Mar 13, 2017

sttts reviewed Mar 13, 2017

View reviewed changes

sttts added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Mar 13, 2017

deads2k force-pushed the server-05-discovery branch 2 times, most recently from df2fbd7 to 4f9e8d2 Compare March 14, 2017 19:49

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

deads2k force-pushed the server-05-discovery branch from 4f9e8d2 to 2a36ef9 Compare March 29, 2017 12:49

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

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

deads2k force-pushed the server-05-discovery branch from 2a36ef9 to 73f00d6 Compare April 12, 2017 12:09

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

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

deads2k force-pushed the server-05-discovery branch from 73f00d6 to bdcdf5c Compare May 1, 2017 15:14

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 1, 2017

ncdc reviewed May 1, 2017

View reviewed changes

deads2k force-pushed the server-05-discovery branch from bdcdf5c to 0fb85cb Compare May 2, 2017 12:36

sttts reviewed May 2, 2017

View reviewed changes

separate discovery from the apiserver

e099f5e

deads2k force-pushed the server-05-discovery branch from 0fb85cb to e099f5e Compare May 2, 2017 12:50

k8s-ci-robot assigned sttts May 2, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 2, 2017

k8s-github-robot merged commit e10c59a into kubernetes:master May 2, 2017

deads2k deleted the server-05-discovery branch August 3, 2017 20:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

separate discovery from the apiserver #43003

separate discovery from the apiserver #43003

deads2k commented Mar 13, 2017

k8s-reviewable commented Mar 13, 2017

sttts Mar 13, 2017

sttts commented Mar 13, 2017 •

edited

Loading

deads2k commented Mar 15, 2017

deads2k commented Mar 29, 2017

deads2k commented Apr 12, 2017

deads2k commented May 1, 2017

ncdc May 1, 2017

deads2k May 2, 2017

ncdc May 1, 2017

ncdc May 1, 2017

deads2k May 1, 2017

sttts May 2, 2017

deads2k May 2, 2017

ncdc May 1, 2017

deads2k May 1, 2017

ncdc May 1, 2017

ncdc May 1, 2017

ncdc May 1, 2017

ncdc May 1, 2017

ncdc May 1, 2017

deads2k May 1, 2017

deads2k May 2, 2017

deads2k commented May 2, 2017

sttts May 2, 2017

deads2k May 2, 2017

sttts commented May 2, 2017

k8s-github-robot commented May 2, 2017

k8s-github-robot commented May 2, 2017

separate discovery from the apiserver #43003

separate discovery from the apiserver #43003

Conversation

deads2k commented Mar 13, 2017

k8s-reviewable commented Mar 13, 2017

Choose a reason for hiding this comment

sttts commented Mar 13, 2017 • edited Loading

deads2k commented Mar 15, 2017

deads2k commented Mar 29, 2017

deads2k commented Apr 12, 2017

deads2k commented May 1, 2017

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 May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented May 2, 2017

k8s-github-robot commented May 2, 2017

k8s-github-robot commented May 2, 2017

sttts commented Mar 13, 2017 •

edited

Loading