federated api servers: Adding a discovery summarizer server #20358

nikhiljindal · 2016-01-30T00:31:09Z

Ref #20229

The server reads a config file to get the list of federated servers and then surfaces the combined discovery information at /api and /apis.

cc @kubernetes/sig-api-machinery @lavalamp

This change is

nikhiljindal · 2016-02-01T18:32:04Z

@k8s-bot ok to test

nikhiljindal · 2016-02-04T01:19:17Z

@lavalamp Can you please take a look once?

For now, I have redefined the discovery types in pkg/apimachinery/discoverysummarizer/types.go instead of changing the current ones in pkg/api/unversioned/types.go.
I have added a single ServerIP field, which will be set to the public IP address. We can add another field for internal address, when we figure that out.

I redefined the type instead of changing the existing one since that is unversioned and I did not want to make too many changes to that.
But the disadvantage is that clients will have to understand different types depending on whether it is talking to the summarizer or the server directly.

WDYT?

smarterclayton · 2016-02-04T01:23:43Z

pkg/apimachinery/discoverysummarizer/types.go

+	// The IP address of the server that is serving this group.
+	// TODO: Support returning both internal and external IP addresses:
+	// http://issues.k8s.io/20229
+	ServerIP string `json:"serverIP"`


Why are we assuming this is an API address. This should be a HostName that can also be an IP.

you are right. Will rename to HostName

smarterclayton · 2016-02-04T01:24:27Z

I don't like summarizer and server not having the same types. I think that defeats the purpose of this API originally, which is that clients just hit a discovery doc. Having two... isn't much discovery.

lavalamp · 2016-02-04T01:52:02Z

Yeah, I agree, I think it has to have the same types. But the concern is that we're stuck with whatever we add, so we'd better get it right.

nikhiljindal · 2016-02-04T02:07:54Z

ok then, how about I add publicHostName field to unversioned.APIGroup?
Later, when we figure out when to return internal IP of server, we can add another field preferredHostName which will be internalIP for internal clients and external IP for external clients.

Does that sound good?

nikhiljindal · 2016-02-11T08:42:35Z

Have updated the code so that the summarizer now reads a list of serverAddresses and list of paths exposed by each server. It then fetches the groups exposed by all servers and provides a summarized view. Groups exposed at same path by multiple servers are summarized together at same path.

For ex: if there are 2 servers: one supporting /api and /apis and other supporting /oapi and /apis, then the summarizer will support 3 paths: /api which will list resources from server1/api; /apis which will list resources from both - server1/apis and server2/apis; and /oapi which will list resources from server2/oapi.

In future, we can easily extend the FederatedAPIServer struct to include the list of groupVersions themselves, in which case the summarizer will use that list rather than fetching the group versions from the server.

@lavalamp @smarterclayton PTAL

k8s-bot · 2016-02-17T00:15:54Z

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

lavalamp · 2016-03-25T00:09:32Z

pkg/apimachinery/discoverysummarizer/server/main.go

@@ -0,0 +1,35 @@
+/*


Move to cmd/kube-discover. Or plugin/cmd/kube-discover?

Moved to cmd/kubernetes-discover.
Chose kubernetes-discover over kube-discover as per comment here: #23509 (comment)

Doesn't seem moved?

lavalamp · 2016-03-25T00:24:18Z

Sorry for delay. Hopefully that's enough for a first pass :)

smarterclayton · 2016-05-09T20:00:16Z

pkg/apimachinery/discoverysummarizer/discoverysummarizer.go

+	return &apiVersions, nil
+}
+
+func writeRawJSON(statusCode int, object interface{}, w http.ResponseWriter) {


I don't like that these have to be duplicated - add a TODO that common response negotiation and version handling should be split out of pkg/apiserver into a more fundamental package.

Added a TODO to share this with kubernetes apiserver.

lavalamp · 2016-05-09T20:52:45Z

cmd/kubernetes-discovery/server/main.go

+limitations under the License.
+*/
+
+package main


Any reason not to put this directly in the cmd/kubernetes-discovery directory?

I can, but you didnt like types.go directly in top level directory so I thought you might not like this directly at top level directory as well :)
I can move it there if you think thats better.

I think it's OK for the main.go file to be top level. :)

Moved to top level

lavalamp · 2016-05-09T20:55:36Z

Just a few final comments.

nikhiljindal · 2016-05-09T22:11:06Z

Updated code as per comments

lavalamp · 2016-05-09T22:25:02Z

LGTM-- good to get this started!

k8s-github-robot · 2016-05-10T17:44:20Z

@nikhiljindal
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.

nikhiljindal · 2016-05-10T17:53:33Z

@k8s-bot e2e test this issue: #IGNORE

k8s-github-robot · 2016-05-10T19:47:11Z

@nikhiljindal
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.

nikhiljindal · 2016-05-10T19:51:54Z

@k8s-bot node e2e test this issue: #IGNORE
@k8s-bot unit test this issue: #IGNORE
@k8s-bot test this issue: #IGNORE

nikhiljindal · 2016-05-10T20:40:17Z

@k8s-bot unit test this issue: #IGNORE

nikhiljindal · 2016-05-13T20:53:44Z

Fixed the test by changing the port so that it does not conflict with examples/apiserver.
The tests should pass now.
Adding back the LGTM tag.

nikhiljindal · 2016-05-13T22:04:39Z

As per logs all tests seem to have passed, not sure why it says FAILED at the end :)
@k8s-bot node e2e test this issue: #IGNORE

k8s-bot · 2016-05-14T12:55:11Z

GCE e2e build/test passed for commit 65c1dc8.

k8s-github-robot · 2016-05-14T12:55:33Z

Automatic merge from submit-queue

…containers UPSTREAM: 65549: Fix flexvolume in containerized kubelets Origin-commit: 4b8fef9c7c9971e1857e901c4bdc7a18f60188ac

googlebot added the cla: yes label Jan 30, 2016

nikhiljindal assigned lavalamp Jan 30, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 30, 2016

nikhiljindal force-pushed the federatedProxy branch 2 times, most recently from dfa7ef8 to 53d0cf9 Compare January 30, 2016 00:55

nikhiljindal mentioned this pull request Feb 4, 2016

Moving /apis handler to generic api server #20532

Merged

smarterclayton reviewed Feb 4, 2016
View reviewed changes

nikhiljindal mentioned this pull request Feb 4, 2016

Adding hostname to groups discovery information #20626

Merged

nikhiljindal force-pushed the federatedProxy branch 2 times, most recently from 0f1dafa to a9def28 Compare February 11, 2016 08:30

nikhiljindal force-pushed the federatedProxy branch from a9def28 to 02dd9f8 Compare February 11, 2016 08:43

lavalamp reviewed Mar 25, 2016
View reviewed changes

smarterclayton reviewed May 9, 2016
View reviewed changes

nikhiljindal force-pushed the federatedProxy branch from 8521b14 to 400a7ba Compare May 9, 2016 20:29

lavalamp reviewed May 9, 2016
View reviewed changes

nikhiljindal force-pushed the federatedProxy branch 3 times, most recently from a9ff150 to bc5abcc Compare May 9, 2016 22:04

lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 9, 2016

nikhiljindal closed this May 10, 2016

nikhiljindal reopened this May 10, 2016

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

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

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

Adding a discovery summarizer server

65c1dc8

nikhiljindal force-pushed the federatedProxy branch from bc5abcc to 65c1dc8 Compare May 13, 2016 20:52

nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 13, 2016

k8s-github-robot merged commit da3bd75 into kubernetes:master May 14, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

federated api servers: Adding a discovery summarizer server #20358

federated api servers: Adding a discovery summarizer server #20358

nikhiljindal commented Jan 30, 2016 •

edited by mikedanese

Loading

nikhiljindal commented Feb 1, 2016

nikhiljindal commented Feb 4, 2016

smarterclayton Feb 4, 2016

nikhiljindal Feb 4, 2016

smarterclayton commented Feb 4, 2016

lavalamp commented Feb 4, 2016

nikhiljindal commented Feb 4, 2016

nikhiljindal commented Feb 11, 2016

k8s-bot commented Feb 17, 2016

lavalamp Mar 25, 2016

nikhiljindal Apr 5, 2016

lavalamp May 9, 2016

lavalamp commented Mar 25, 2016

smarterclayton May 9, 2016

nikhiljindal May 9, 2016

lavalamp May 9, 2016

nikhiljindal May 9, 2016

lavalamp May 9, 2016

nikhiljindal May 9, 2016

lavalamp commented May 9, 2016

nikhiljindal commented May 9, 2016

lavalamp commented May 9, 2016

k8s-github-robot commented May 10, 2016

nikhiljindal commented May 10, 2016

k8s-github-robot commented May 10, 2016

nikhiljindal commented May 10, 2016

nikhiljindal commented May 10, 2016

nikhiljindal commented May 13, 2016

nikhiljindal commented May 13, 2016

k8s-bot commented May 14, 2016

k8s-github-robot commented May 14, 2016

federated api servers: Adding a discovery summarizer server #20358

federated api servers: Adding a discovery summarizer server #20358

Conversation

nikhiljindal commented Jan 30, 2016 • edited by mikedanese Loading

nikhiljindal commented Feb 1, 2016

nikhiljindal commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Feb 4, 2016

lavalamp commented Feb 4, 2016

nikhiljindal commented Feb 4, 2016

nikhiljindal commented Feb 11, 2016

k8s-bot commented Feb 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Mar 25, 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

lavalamp commented May 9, 2016

nikhiljindal commented May 9, 2016

lavalamp commented May 9, 2016

k8s-github-robot commented May 10, 2016

nikhiljindal commented May 10, 2016

k8s-github-robot commented May 10, 2016

nikhiljindal commented May 10, 2016

nikhiljindal commented May 10, 2016

nikhiljindal commented May 13, 2016

nikhiljindal commented May 13, 2016

k8s-bot commented May 14, 2016

k8s-github-robot commented May 14, 2016

nikhiljindal commented Jan 30, 2016 •

edited by mikedanese

Loading