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

Clean up the kubeadm high availability instructions. Fixes Issue 15284 #15411

Closed
wants to merge 5 commits into from
Closed

Clean up the kubeadm high availability instructions. Fixes Issue 15284 #15411

wants to merge 5 commits into from

Conversation

rcythr
Copy link

@rcythr rcythr commented Jul 13, 2019

@k8s-ci-robot
Copy link
Contributor

Welcome @rcythr!

It looks like this is your first PR to kubernetes/website 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/website has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 13, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign kbarnard10
You can assign the PR to them by writing /assign @kbarnard10 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 13, 2019
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Jul 13, 2019
@netlify
Copy link

netlify bot commented Jul 13, 2019

Deploy preview for kubernetes-io-master-staging ready!

Built with commit a586b71

https://deploy-preview-15411--kubernetes-io-master-staging.netlify.com

… specific method for setting up a load balancer. Removed extra space. Fixed indentation.
@rcythr rcythr changed the title First attempt to clean up the kubeadm high availability instructions. Clean up the kubeadm high availability instructions. Fixes #15284 Jul 13, 2019
@rcythr rcythr changed the title Clean up the kubeadm high availability instructions. Fixes #15284 Clean up the kubeadm high availability instructions. Fixes Issue 15284 Jul 13, 2019
@rcythr
Copy link
Author

rcythr commented Jul 13, 2019

/sig cluster-lifecycle

@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Jul 13, 2019
@kbhawkey
Copy link
Contributor

hello @rcythr , would you coordinate with PR #15372. Thanks!

Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

@rcythr Thanks for this PR!
I'm ok with the change, with only a minor nit


- Make sure the address of the load balancer always matches
the address of kubeadm's `ControlPlaneEndpoint`.
- Check if your deployment environment has a highly available load balancer, and use that if possible. Otherwise, setup a TCP forwarding load balancer (such as [HAProxy](http://www.haproxy.org/)). Make sure to follow the documentation to properly setup this load balancer in a highly-available configuration. Otherwise, your load balancer becomes a single point of failure.
Copy link
Member

Choose a reason for hiding this comment

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

After re-reading, I think this sentence should become the first point (first check what you have, second create). Also, plese change "Otherwise, setup a TCP forwarding load balancer (such as HAProxy)" into Otherwise, "select a TCP forwarding load balancer such as HAProxy", in order to be more in line with the "check what you have" message

@tengqm
Copy link
Contributor

tengqm commented Jul 21, 2019

@rcythr Feedback reasonable to you?

@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jul 21, 2019
@rcythr
Copy link
Author

rcythr commented Jul 21, 2019

@fabriziopandini How about this? Sorry for the delay, I've been swamped this week.

@tengqm Thanks for the reminder.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jul 21, 2019
@fabriziopandini
Copy link
Member

@rcythr thanks for the updates! I'm ok with the changes, I leave to @neolit123 giving the final green light

Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 22, 2019
@rcythr
Copy link
Author

rcythr commented Jul 23, 2019

/assign @kbarnard10

@rcythr
Copy link
Author

rcythr commented Jul 25, 2019

/hold

In light of kubernetes/kubeadm#1685 we should revise our advice to be more explicit about what kind of things the LB needs to be able to do. Azure's LB currently cannot fulfil those requirements.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 25, 2019
…want to get into details of specific LB implementation. Added HA LB as a required piece of infrastructure where it belongs.
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 28, 2019
@rcythr
Copy link
Author

rcythr commented Jul 28, 2019

@neolit123 @fabriziopandini I modified the text to make the HA LB a requirement instead of trying to provide steps for setting it up -- we can't get into details about the specific implementations. I added a note about hairpin NAT in light of kubernetes/kubeadm#1685

What do you think?

@rcythr
Copy link
Author

rcythr commented Jul 28, 2019

/hold cancel
I have adjusted this to incorporate content from kubernetes/kubeadm#1685.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 28, 2019
@@ -40,6 +40,7 @@ For both methods you need this infrastructure:
the masters
- Three machines that meet [kubeadm's minimum
requirements](/docs/setup/production-environment/tools/kubeadm/install-kubeadm/#before-you-begin) for the workers
- A highly available load balancer that operates at Layer 4 (such as one provided by a cloud provider, or [HAProxy](http://www.haproxy.org/)) with it's own name in DNS. The load balancer should be configured to forward the apiserver port (default: 6443) on the load balancer to the apiserver port on all control plane nodes. Before using a cloud provider provided load balancer, double check the documentation for [NAT hairpinning](https://en.wikipedia.org/wiki/Hairpinning) support.
Copy link
Member

Choose a reason for hiding this comment

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

we should probably mention HTTPS and health checks here.
@ereslibre @fabriziopandini how would you word that?

Copy link
Contributor

@ereslibre ereslibre Jul 28, 2019

Choose a reason for hiding this comment

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

I would say that even if it's a layer 4 LB it should have the ability to perform HTTPS health checks (even if the certificate chain is ignored) against every apiserver /healthz endpoint. That way it will remove from the active pool unhealthy apiservers.

…h-availability.md


Fixed it's vs its.

Co-Authored-By: Rafael Fernández López <[email protected]>
@zacharysarah
Copy link
Contributor

@rcythr 👋 Any updates?

@zacharysarah
Copy link
Contributor

@rcythr 👋 Please feel free to /reopen when you're ready to continue.

/close

@k8s-ci-robot
Copy link
Contributor

@zacharysarah: Closed this PR.

In response to this:

@rcythr 👋 Please feel free to /reopen when you're ready to continue.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with k8s.io/docs/setup/production-environment/tools/kubeadm/high-availability/
9 participants