add lease endpoint reconciler #51698

rphillips · 2017-08-31T11:35:49Z

What this PR does / why we need it: Adds OpenShift's LeaseEndpointReconciler to register kube-apiserver endpoints within the storage registry.

Adds a command-line argument alpha-endpoint-reconciler-type to the kube-apiserver.

Defaults to the old MasterCount reconciler.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes kubernetes/community#939 fixes #22609

Release note:

Adds a command-line argument to kube-apiserver called
--alpha-endpoint-reconciler-type=(master-count, lease, none) (default
"master-count"). The original reconciler is 'master-count'. The 'lease'
reconciler uses the storageapi and a TTL to keep alive an endpoint within the
`kube-apiserver-endpoint` storage namespace. The 'none' reconciler is a noop
reconciler that does not do anything. This is useful for self-hosted
environments.

/cc @lavalamp @smarterclayton @ncdc

k8s-ci-robot · 2017-08-31T11:35:56Z

Hi @rphillips. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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. I understand the commands that are listed here.

lpabon · 2017-08-31T14:26:43Z

@rphillips Could you describe which files are equal to OpenShift's and which lines/files you added/edited to make it easier to review?

ericchiang · 2017-08-31T14:49:04Z

/ok-to-test

lpabon · 2017-08-31T14:57:59Z

pkg/master/master.go

+	ttl := c.MasterEndpointReconcileTTL
+	config, err := c.StorageFactory.NewConfig(kapi.Resource("apiServerIPInfo"))
+	if err != nil {
+		glog.Fatalf("Error determining service IP ranges: %v", err)


Is this an acceptable pattern to exit the application at this point instead of returning an error?

Yes, I think so. It is during config setup

rphillips · 2017-08-31T15:45:25Z

@lpabon The OpenShift files are marked with their original location. Everything else is written or generated for this PR

calebamiles · 2017-08-31T15:55:21Z

/release-note-none

calebamiles · 2017-08-31T16:10:47Z

/test pull-kubernetes-unit
/test pull-kubernetes-bazel-test
/test pull-kubernetes-node-e2e

calebamiles · 2017-08-31T16:11:36Z

/release-note

klausenbusk · 2017-08-31T20:29:29Z

pkg/master/master.go

+func (c *Config) createEndpointReconciler() EndpointReconciler {
+	glog.Infof("Using reconciler: %v", c.EndpointReconcilerType)
+	switch c.EndpointReconcilerType {
+	case "":


A option to disable the reconciler, would be useful for people who use self-hosted k8s.

Then they can just create a kubernetes service which use selector.

See: kubernetes/community#939 (comment) and kubernetes/community#939 (comment)

i added a none reconciler to not do anything on the reconcile loop.

calebamiles · 2017-08-31T22:04:46Z

/test pull-kubernetes-e2e-gce-bazel

lavalamp

I haven't yet reviewed the code that you're upstreaming from openshift.

lavalamp · 2017-08-31T21:58:01Z

cmd/kube-apiserver/app/options/options.go

@@ -164,6 +166,9 @@ func (s *ServerRunOptions) AddFlags(fs *pflag.FlagSet) {
 	fs.IntVar(&s.MasterCount, "apiserver-count", s.MasterCount,
 		"The number of apiservers running in the cluster, must be a positive number.")

+	fs.StringVar(&s.EndpointReconcilerType, "alpha-endpoint-reconciler-type", "master-count",


Default should be s.EndpointReconcilerType, and that should be initialized properly (look for where other defaults are set).

Description must clearly specify what the options are.

lavalamp · 2017-08-31T21:58:50Z

cmd/kube-apiserver/app/options/options_test.go

-		AllowPrivileged:      false,
+		ServiceNodePortRange:   DefaultServiceNodePortRange,
+		MasterCount:            5,
+		EndpointReconcilerType: "master-count",


Make a type and constants for this rather than write the string everywhere.

lavalamp · 2017-08-31T22:26:15Z

pkg/election/doc.go

+limitations under the License.
+*/
+
+// Package election provides objects for managing the list of active masters via leases.


Please make this a sub-directory under pkg/master/

Also, add a disclaimer here that this is not the intended way for any apiserver other than kube-apiserver to accomplish this task.

lavalamp · 2017-08-31T22:27:48Z

pkg/master/master.go

+
+const (
+	// DefaultMasterCountReconciler will select the original reconciler
+	MasterCountReconciler = "master-count"


Oh, you already have these constants! give them a type and use them everywhere. :)

(you may need to put them in a more visible place)

Probably should make them CamelCase to be consistent with other constants for type

The problem was the circular dependency it caused, but I'll look into it.

lavalamp · 2017-08-31T22:45:23Z

pkg/master/master.go

@@ -155,6 +180,58 @@ type completedConfig struct {
 	*Config
 }

+func (c *Config) createMasterCountReconciler() EndpointReconciler {


consider adding a "reconcilers.go" file to collect these?

lavalamp · 2017-08-31T22:45:54Z

pkg/master/master.go

+	switch c.EndpointReconcilerType {
+	case "":
+		fallthrough
+	case MasterCountReconciler:


case "", MasterCountReconciler: is more concise.

But consider rejecting "" as a flag value on start up.

lavalamp · 2017-08-31T22:46:38Z

pkg/master/master.go

+	case NoneEndpointReconciler:
+		return c.createNoneReconciler()
+	default:
+		glog.Fatalf("Reconciler not implemented: %v", c.EndpointReconcilerType)


lavalamp · 2017-08-31T22:49:16Z

pkg/master/master.go

+		StorageConfig:           endpointConfig,
+		Decorator:               generic.UndecoratedStorage,
+		DeleteCollectionWorkers: 0,
+		ResourcePrefix:          c.StorageFactory.ResourcePrefix(kapi.Resource("endpoints")),


I would use something that clearly won't collide with the endpoints api. "kube-apiserver-endpoint"?

@ncdc I don't remember whether there was a reason we didn't do that before?

smarterclayton · 2017-09-01T01:29:53Z

@ncdc since you're the original author if you want to review

bgrant0607 · 2017-09-01T02:12:47Z

@rphillips Was there a specific aspect you thought I should look at?

This just changes how Endpoints is populated for the apiserver?

This needs a better release note. See kubernetes/community#484 for guidance.

rphillips · 2017-09-11T17:44:06Z

rebased

calebamiles · 2017-09-11T18:44:12Z

/test pull-kubernetes-federation-e2e-gce

smarterclayton · 2017-09-12T02:18:29Z

/lgtm
/retest

But we need to @kubernetes/sig-release-feature-requests to approve this if the intent is still to deliver for 1.8.

k8s-github-robot · 2017-09-12T02:18:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rphillips, smarterclayton

Associated issue: 939

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

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kube-apiserver/OWNERS~~ [smarterclayton]
~~pkg/master/OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

fejta-bot · 2017-09-12T14:21:19Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

fejta-bot · 2017-09-16T12:51:00Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-09-23T16:14:01Z

Automatic merge from submit-queue (batch tested with PRs 52240, 48145, 52220, 51698, 51777). If you want to cherry-pick this change to another branch, please follow the instructions here..

alexbrand · 2017-12-18T15:20:47Z

Great to see this. Thanks!

I think user-facing documentation about the problem and the reconcilers would be useful? Maybe in https://kubernetes.io/docs/admin/high-availability/?

Happy to open a PR if it makes sense.

luxas · 2017-12-25T18:32:24Z

@alexbrand An user-facing docs PR for this would make sense to me 👍

@rphillips Please graduate this to beta in v1.10 and add e2e tests #57617

tengqm · 2017-12-27T04:57:58Z

@luxas FYI, this has been documented kubernetes/website#6695.

luxas · 2017-12-27T14:14:59Z

Great! Thanks @tengqm!

rphillips · 2018-01-08T15:42:38Z

@tengqm I've been on vacation. thanks!

When a cluster is bootstrapped with multiple kube-apiservers, the `kubernetes` service contains a list of all of these endpoints. By default, this list of endpoints will *not* be updated if one of the apiservers goes down. This can lead to the api becoming unresponsive and breaking it. To have the endpoints automatically keep track of the apiservers that are available the `--endpoint-reconciler-type` option `lease` needs to be added. (The default option for 1.10 `master-count` only changes the endpoint when the count changes: https://github.com/apprenda/kismatic/issues/987) See: kubernetes/kubernetes#22609 kubernetes/kubernetes#56584 kubernetes/kubernetes#51698

k8s-ci-robot requested review from bgrant0607, bgrant0607-cc and lavalamp August 31, 2017 11:35

k8s-ci-robot added size/XL cncf-cla: yes labels Aug 31, 2017

k8s-ci-robot added the needs-ok-to-test label Aug 31, 2017

k8s-github-robot assigned lavalamp and janetkuo Aug 31, 2017

k8s-github-robot added the do-not-merge/release-note-label-needed label Aug 31, 2017

k8s-ci-robot removed the needs-ok-to-test label Aug 31, 2017

lpabon reviewed Aug 31, 2017

View reviewed changes

k8s-ci-robot added the release-note-none label Aug 31, 2017

k8s-github-robot removed the do-not-merge/release-note-label-needed label Aug 31, 2017

k8s-ci-robot added release-note and removed release-note-none labels Aug 31, 2017

rphillips force-pushed the feat/lease_endpoint_reconciler branch from a462c9c to 1c9ec47 Compare August 31, 2017 18:30

klausenbusk reviewed Aug 31, 2017

View reviewed changes

lavalamp unassigned janetkuo Aug 31, 2017

lavalamp reviewed Aug 31, 2017

View reviewed changes

k8s-ci-robot removed the size/XL label Sep 1, 2017

k8s-ci-robot assigned smarterclayton Sep 12, 2017

k8s-ci-robot added lgtm sig/release kind/feature labels Sep 12, 2017

k8s-github-robot merged commit fd3c1f4 into kubernetes:master Sep 23, 2017

tatsuhiro-t mentioned this pull request Sep 26, 2017

give the kubernetes service client ip session affinity #23129

Merged

s-urbaniak mentioned this pull request Oct 9, 2017

modules/net/calico: enable BGP networking coreos/tectonic-installer#1993

Merged

sym3tri mentioned this pull request Oct 11, 2017

Kubernetes Control Plane apiserver never reaches consensus between enlisted endpoints and num_masters config setting coreos/tectonic-forum#189

Closed

ncdc mentioned this pull request Oct 12, 2017

Lease reconciler creates incorrect kubernetes endpoint and lease files #53803

Closed

This was referenced Dec 4, 2017

Disable session affinity for internal kuberntes service #56690

Merged

kubenetes svc endpoints are not updated when one of API is down (when running HA api) #56584

Closed

mumoshu mentioned this pull request Dec 6, 2017

Reconsider apiserver-count kubernetes-retired/kube-aws#499

Closed

liggitt mentioned this pull request Dec 11, 2017

kube-apiserver endpoint is not automatically removed from the endpoints of "kubernetes" SVC if the master node goes down #56477

Closed

luxas mentioned this pull request Dec 25, 2017

Graduate the Lease Endpoint Reconciler to beta in v1.11 #57617

Closed

bergmannf mentioned this pull request Feb 27, 2019

Automatically update the kubernetes-service endpoint. SUSE/caasp-salt#752

Merged

add lease endpoint reconciler #51698

add lease endpoint reconciler #51698

Conversation

rphillips commented Aug 31, 2017 • edited Loading

k8s-ci-robot commented Aug 31, 2017

lpabon commented Aug 31, 2017

ericchiang commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphillips commented Aug 31, 2017

calebamiles commented Aug 31, 2017

calebamiles commented Aug 31, 2017

calebamiles commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebamiles commented Aug 31, 2017

lavalamp 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

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

smarterclayton commented Sep 1, 2017

bgrant0607 commented Sep 1, 2017

rphillips commented Sep 11, 2017

calebamiles commented Sep 11, 2017

smarterclayton commented Sep 12, 2017

k8s-github-robot commented Sep 12, 2017

fejta-bot commented Sep 12, 2017

fejta-bot commented Sep 16, 2017

k8s-github-robot commented Sep 23, 2017

alexbrand commented Dec 18, 2017

luxas commented Dec 25, 2017

tengqm commented Dec 27, 2017

luxas commented Dec 27, 2017

rphillips commented Jan 8, 2018

rphillips commented Aug 31, 2017 •

edited

Loading