Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow the restclient to contact multiple apiservers #25428

Closed

Conversation

mikedanese
Copy link
Member

@mikedanese mikedanese commented May 10, 2016

WIP
cc @krousey

fixes #18174

@mikedanese mikedanese self-assigned this May 10, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 10, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2016
@mikedanese mikedanese force-pushed the multi-apiserver-client branch 2 times, most recently from a1bce58 to a7d5f5d Compare May 10, 2016 23:56
@mikedanese
Copy link
Member Author

mikedanese commented May 10, 2016

this is turning out to be a pretty big change. I will invest more time into fixing tests if we decide this is an okay thing to do.

cc @liggitt @deads2k @aaronlevy @lavalamp

@mikedanese mikedanese force-pushed the multi-apiserver-client branch 3 times, most recently from 3355e64 to 78ddf84 Compare May 11, 2016 00:31
@liggitt
Copy link
Member

liggitt commented May 11, 2016

Is the intent to make clients know about multiple identical back ends? As a client I find that harder to use than an actual HA/load-balanced server

@mikedanese
Copy link
Member Author

Forgot to link the issue: #18174, the desire is to use this specifically for kube-proxy and kubelet. Everything else should communicate to the apiserver through the service vip.

As a client I find that harder to use than an actual HA/load-balanced server

This approach requires one less outside dependency and is thus more portable

@aaronlevy
Copy link
Contributor

Overall this approach seems good to me.

@@ -58,6 +58,8 @@ type Preferences struct {
type Cluster struct {
// Server is the address of the kubernetes cluster (https://hostname:port).
Server string `json:"server"`
// Servers is an array of addresses to the kubernetes apiservers with the format: (https://hostname:port)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs doc about how server overrides this. It might be better to make this alternateServers so that it's clear which one should be first

@aaronlevy
Copy link
Contributor

Any thoughts if this is a reasonable approach? I selfishly would love to have this in the 1.3 release, but I know the freeze is end of week. If it helps, I could try and take a look at updating the tests.

@mikedanese
Copy link
Member Author

@aaronlevy I will fix this up today. I'd appreciate you guys verifying the change on your deployment once it's ready. Currently we don't have any multimaster e2e ci setup.

@mikedanese mikedanese force-pushed the multi-apiserver-client branch 2 times, most recently from 614f482 to d86a29c Compare May 16, 2016 20:26
@smarterclayton
Copy link
Contributor

smarterclayton commented May 16, 2016 via email

@aaronlevy
Copy link
Contributor

@mikedanese sure, happy to help with multi-master testing.

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 16, 2016
@k8s-github-robot
Copy link

@mikedanese PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 19, 2016
@aaronlevy
Copy link
Contributor

I've been playing around with this change a bit, and noticed a couple things (also added some minor modifications: 1cdb969).

  • The error rate limiting is far too high (in my minimal tests). At the current setting kubelet/kube-proxy running a couple pods will never roll over to the next host. In this current approach, would there be a reason we couldn't roll-over to next host on first failure?
  • It is somewhat odd behavior with kubectl - where as a user I might expect it to log failure and retry, but instead it randomly picks a host and fails/exits.

@therc
Copy link
Member

therc commented May 23, 2016

Is this still intended for 1.3?

@lavalamp
Copy link
Member

This has missed the cut-off date for 1.3.

@k8s-bot
Copy link

k8s-bot commented Jul 12, 2016

GCE e2e build/test passed for commit 1c2cff6.

@k8s-github-robot
Copy link

This PR hasn't been active in 54 days. Will be closed in 35 days.

You can add 'keep-open' label to prevent this from happening.

@mikedanese mikedanese closed this Jul 20, 2016
@dshulyak
Copy link
Contributor

dshulyak commented Aug 5, 2016

@mikedanese do you plan to work on this in nearest time? Asking because we can not rely on multiple dns records and i could try to fix it myself

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/HA needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"don't require a load balancer between cluster and control plane and still be HA"
10 participants