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

kubeadm: Remove multiple API server endpoints support upon join #69812

Merged
merged 1 commit into from Oct 30, 2018

Conversation

@rosti
Contributor

rosti commented Oct 15, 2018

What this PR does / why we need it:

In the past the discovery configuration expected, that we can support multiple API server endpoints. In practice, we always end up with a single API server endpoint, because, even in HA setups, we use a load balancer scheme for API servers.
Therefore, to reduce complexity and improve readability of the config, the multiple API server endpoints support is removed from the bootstrap token discovery join method and configuration.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
refs kubernetes/kubeadm#911

Special notes for your reviewer:

Depends on:

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews
/area kubeadm
/kind api-change
/kind cleanup
/assign @luxas
/assign @timothysc
/assign @fabriziopandini

Release note:

kubeadm: Multiple API server endpoints support upon join is removed as it is now redundant.
@neolit123

thanks, added a couple of minor comments.

DiscoveryTimeout *metav1.Duration
// Discovery specifies the options for the kubelet to use during the TLS Bootstrap process
Discovery Discovery

This comment has been minimized.

@neolit123

neolit123 Oct 15, 2018

Member

hm, some of the fields in the types structs have empty lines separating them.
the rest don't have them.

@fabriziopandini @rosti what coding convention should we end up doing here?

This comment has been minimized.

@rosti

rosti Oct 16, 2018

Contributor

@neolit123 it appears, that up until now, the empty line is used to distinguish between different groups of fields in a single structure. Like the ones, who are dependent in one-another.

With the redesign of config, most of the related fields were moved into substructs and therefore single fields, unrelated to others have become more common. Hence most of the field structures have empty lines between them.

func AddJoinBootstrapTokenDiscoveryFlags(flagSet *flag.FlagSet, btd *kubeadmapiv1beta1.BootstrapTokenDiscovery) {
flagSet.StringVar(
&btd.Token, "discovery-token", "",
"A token used to validate cluster information fetched from the api server.")

This comment has been minimized.

@neolit123

neolit123 Oct 15, 2018

Member

maybe take the opportunity to api -> API here?

This comment has been minimized.

@rosti

rosti Oct 16, 2018

Contributor

Fixed in #67763

func AddJoinFileDiscoveryFlags(flagSet *flag.FlagSet, fd *kubeadmapiv1beta1.FileDiscovery) {
flagSet.StringVar(
&fd.KubeConfigPath, "discovery-file", "",
"A file or url from which to load cluster information.")

This comment has been minimized.

@neolit123

neolit123 Oct 15, 2018

Member

url -> URL

This comment has been minimized.

@rosti

rosti Oct 16, 2018

Contributor

Fixed in #67763

@rosti

This comment has been minimized.

Contributor

rosti commented Oct 16, 2018

Waiting on #67763 to merge.

/hold

@rosti

This comment has been minimized.

Contributor

rosti commented Oct 17, 2018

This is now ready for final review.

/hold cancel

@rosti

This comment has been minimized.

Contributor

rosti commented Oct 17, 2018

/test pull-kubernetes-e2e-kops-aws

@neolit123

/lgtm
thanks for the update.

@fabriziopandini

@rosti many thanks for this PR!
only one minor nit and then ready to lgtm

@@ -177,7 +177,7 @@ func NewCmdJoin(out io.Writer) *cobra.Command {
cfg.Discovery.File = fd
} else {
cfg.Discovery.BootstrapToken = btd
cfg.Discovery.BootstrapToken.APIServerEndpoints = args
cfg.Discovery.BootstrapToken.APIServerEndpoint = args[0]

This comment has been minimized.

@fabriziopandini

fabriziopandini Oct 19, 2018

Contributor

What about adding a check that args has one element?

This comment has been minimized.

@rosti

rosti Oct 19, 2018

Contributor

The checks need to be active if --config is not used though. In the --config case this code is redundant and warning or error messages by the checks will be misleading.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 19, 2018

@rosti

This comment has been minimized.

Contributor

rosti commented Oct 19, 2018

@fabriziopandini updated with your feedback, also added another check in the conversion code to avoid possible panic.

@fabriziopandini

/approve
IMO this PR is now ready to merge. Please rebase to get the test passing

/assign @timothysc

@k8s-ci-robot k8s-ci-robot added approved and removed needs-rebase labels Oct 26, 2018

err := fmt.Errorf("abort connecting to API servers after timeout of %v", discoveryTimeout)
fmt.Printf("[discovery] %v\n", err)
wg.Wait()
<-finishedChan

This comment has been minimized.

@timothysc

timothysc Oct 29, 2018

Member

I think the wait/waitgroup is a lot cleaner here imo.

The double use of the channel for both the case statement and within the case is an anti-pattern.

kubeadm: APIServerEndpoints -> APIServerEndpoint
In the past the discovery configuration expected, that we can support multiple
API server endpoints. In practice, we always end up with a single API server
endpoint, because, even in HA setups, we use a load balancer scheme for API
servers.
Therefore, to reduce complexity and improve readability of the config, the
multiple API server endpoints support is removed from the bootstrap token
discovery join method and configuration.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>
@fabriziopandini

@rosti Thanks for this PR!
/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 30, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 30, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, rosti

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

The pull request process is described 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 merged commit 0a405f4 into kubernetes:master Oct 30, 2018

18 checks passed

cla/linuxfoundation rosti authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment