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

rosti · 2018-10-15T16:30:21Z

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:

kubeadm: Split discovery from JoinConfiguration #67763

/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.

neolit123 · 2018-10-15T16:57:10Z

cmd/kubeadm/app/apis/kubeadm/types.go

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


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?

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

neolit123 · 2018-10-15T17:38:39Z

cmd/kubeadm/app/cmd/join.go

+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.")


maybe take the opportunity to api -> API here?

Fixed in #67763

neolit123 · 2018-10-15T17:39:18Z

cmd/kubeadm/app/cmd/join.go

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


url -> URL

Fixed in #67763

rosti · 2018-10-16T12:32:05Z

Waiting on #67763 to merge.

/hold

rosti · 2018-10-17T08:28:34Z

This is now ready for final review.

/hold cancel

rosti · 2018-10-17T10:52:11Z

/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

fabriziopandini · 2018-10-19T08:06:53Z

cmd/kubeadm/app/cmd/join.go

@@ -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]


What about adding a check that args has one element?

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.

rosti · 2018-10-19T09:20:58Z

@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

timothysc · 2018-10-29T20:09:22Z

cmd/kubeadm/app/discovery/token/token.go

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


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.

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 <[email protected]>

fabriziopandini

@rosti Thanks for this PR!
/approve
/lgtm

k8s-ci-robot · 2018-10-30T15:53:20Z

[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:

~~cmd/kubeadm/OWNERS~~ [fabriziopandini]

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 assigned fabriziopandini Oct 15, 2018

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 15, 2018

k8s-ci-robot assigned luxas and timothysc Oct 15, 2018

rosti force-pushed the single-api-endpoint branch from 5407c34 to 0bc6bd1 Compare October 15, 2018 16:32

neolit123 reviewed Oct 15, 2018

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2018

rosti force-pushed the single-api-endpoint branch from 0bc6bd1 to 6871f76 Compare October 17, 2018 08:27

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 17, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 17, 2018

neolit123 approved these changes Oct 17, 2018

View reviewed changes

k8s-ci-robot assigned neolit123 Oct 17, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2018

fabriziopandini suggested changes Oct 19, 2018

View reviewed changes

rosti force-pushed the single-api-endpoint branch from 6871f76 to 0907edd Compare October 19, 2018 09:18

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2018

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2018

fabriziopandini approved these changes Oct 26, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2018

rosti force-pushed the single-api-endpoint branch from 0907edd to 3e2ce98 Compare October 29, 2018 11:43

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

timothysc suggested changes Oct 29, 2018

View reviewed changes

rosti force-pushed the single-api-endpoint branch from 3e2ce98 to 76ad45f Compare October 30, 2018 13:51

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2018

rosti force-pushed the single-api-endpoint branch from 76ad45f to a3e7d7e Compare October 30, 2018 14:11

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2018

fabriziopandini approved these changes Oct 30, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 30, 2018

k8s-ci-robot merged commit 0a405f4 into kubernetes:master Oct 30, 2018

rosti deleted the single-api-endpoint branch November 22, 2018 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rosti commented Oct 15, 2018 •

edited

Loading

neolit123 left a comment

neolit123 Oct 15, 2018

rosti Oct 16, 2018

neolit123 Oct 15, 2018

rosti Oct 16, 2018

neolit123 Oct 15, 2018

rosti Oct 16, 2018

rosti commented Oct 16, 2018

rosti commented Oct 17, 2018

rosti commented Oct 17, 2018

neolit123 left a comment

fabriziopandini left a comment

fabriziopandini Oct 19, 2018

rosti Oct 19, 2018

rosti commented Oct 19, 2018

fabriziopandini left a comment

timothysc Oct 29, 2018

fabriziopandini left a comment

k8s-ci-robot commented Oct 30, 2018

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

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

Conversation

rosti commented Oct 15, 2018 • edited Loading

neolit123 left a 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

rosti commented Oct 16, 2018

rosti commented Oct 17, 2018

rosti commented Oct 17, 2018

neolit123 left a comment

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosti commented Oct 19, 2018

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 30, 2018

rosti commented Oct 15, 2018 •

edited

Loading