Fixed the lease endpoint reconciler creation of kubernetes endpoint and lease file ttl. #53803 #53809

rrati · 2017-10-12T15:12:45Z

Fixed the prefix used to create the kubernetes endpoint and the ttl value used for lease files.

k8s-ci-robot · 2017-10-12T15:12:45Z

@rrati: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

rrati · 2017-10-12T15:13:37Z

@ncdc @rphillips

ncdc · 2017-10-12T15:16:03Z

pkg/master/reconcilers/lease.go

@@ -82,7 +82,7 @@ func (s *storageLeases) UpdateLease(ip string) error {
 			},
 		}

-		leaseTime := uint64(s.leaseTime)
+		leaseTime := uint64(s.leaseTime / time.Second)


Worth commenting that leaseTime has to be in seconds and hence the division?

Yep, will add a comment

ncdc · 2017-10-12T15:16:27Z

pkg/master/master.go

@@ -94,7 +94,7 @@ const (
 	// DefaultEndpointReconcilerTTL is the default TTL timeout for the storage layer
 	DefaultEndpointReconcilerTTL = 15 * time.Second
 	// DefaultStorageEndpoint is the default storage endpoint for the lease controller
-	DefaultStorageEndpoint = "kube-apiserver-endpoint"
+	DefaultStorageEndpoint = "services/endpoints"


@deads2k @sttts @liggitt is there any better way to get at this value?

we shouldn't need this at all. this is getting passed like this:

ResourcePrefix: c.ExtraConfig.StorageFactory.ResourcePrefix(kapi.Resource(DefaultStorageEndpoint)),

that should be

ResourcePrefix: c.ExtraConfig.StorageFactory.ResourcePrefix(kapi.Resource("endpoints")),

Partial diff here

ah, the api.Resource("endpoints") is even better

I think we can get it from StorageFactory.ResourcePrefix(api.Resource("endpoints"))

ncdc · 2017-10-12T15:17:10Z

pkg/master/master.go

@@ -94,7 +94,7 @@ const (
 	// DefaultEndpointReconcilerTTL is the default TTL timeout for the storage layer
 	DefaultEndpointReconcilerTTL = 15 * time.Second
 	// DefaultStorageEndpoint is the default storage endpoint for the lease controller


This comment doesn't really make sense insofar as what it's really used for, and we should probably change the name of the constant too.

Agreed. What about EndpointPrefix?

Actually, I think we can get rid of this const alltogether

ncdc · 2017-10-12T15:30:15Z

As best I can tell, the lease endpoint reconciler is only on master and not in 1.8, therefore:

/release-note-none

ncdc · 2017-10-12T15:30:48Z

@kubernetes/sig-api-machinery-pr-reviews

liggitt · 2017-10-12T15:31:18Z

As best I can tell, the lease endpoint reconciler is only on master and not in 1.8, therefore:

Yes, merged after 1.8 was cut

mml · 2017-10-12T19:36:34Z

/assign @cheftako

…nd lease file ttl

ncdc · 2017-10-16T15:05:44Z

@rrati I assume you've tested this locally and it's all good?

@liggitt any other comments?

liggitt · 2017-10-16T17:25:20Z

pkg/master/master.go

@@ -206,18 +204,19 @@ func (c *Config) createLeaseReconciler() reconcilers.EndpointReconciler {
 	if err != nil {
 		glog.Fatalf("Error creating storage factory: %v", err)
 	}
-	endpointConfig, err := c.ExtraConfig.StorageFactory.NewConfig(kapi.Resource(DefaultStorageEndpoint))
+	endpointPrefix := c.ExtraConfig.StorageFactory.ResourcePrefix(api.Resource("endpoints"))
+	endpointConfig, err := c.ExtraConfig.StorageFactory.NewConfig(kapi.Resource(endpointPrefix))


this looks right:

endpointPrefix := c.ExtraConfig.StorageFactory.ResourcePrefix(api.Resource("endpoints"))

this looks wrong:

c.ExtraConfig.StorageFactory.NewConfig(kapi.Resource(endpointPrefix))

why are you passing the prefix into kapi.Resource(endpointPrefix)? I expected:

c.ExtraConfig.StorageFactory.NewConfig(kapi.Resource("endpoints"))

StorageFactory.ResourcePrefix actually returns a string. I'm passing to kapi.Resource to create a schema.GroupResource which is what NewConfig needs.

the output of c.ExtraConfig.StorageFactory.ResourcePrefix is an etcd prefix, the location in etcd where we want to store that resource.

taking that value and making a schema.GroupResource out of it doesn't make any sense. this should be

endpointConfig, err := c.ExtraConfig.StorageFactory.NewConfig(kapi.Resource("endpoints"))

liggitt · 2017-10-16T17:25:49Z

pkg/master/master.go

 	if err != nil {
 		glog.Fatalf("Error getting storage config: %v", err)
 	}
 	endpointsStorage := endpointsstorage.NewREST(generic.RESTOptions{
 		StorageConfig:           endpointConfig,
 		Decorator:               generic.UndecoratedStorage,
 		DeleteCollectionWorkers: 0,
-		ResourcePrefix:          c.ExtraConfig.StorageFactory.ResourcePrefix(kapi.Resource(DefaultStorageEndpoint)),
+		ResourcePrefix:          c.ExtraConfig.StorageFactory.ResourcePrefix(kapi.Resource(endpointPrefix)),


should be ResourcePrefix: endpointPrefix, right?

Same comment as above

same response as above :)

liggitt · 2017-10-16T17:26:35Z

pkg/master/master.go

 	})
 	endpointRegistry := endpoint.NewRegistry(endpointsStorage)
-	masterLeases := reconcilers.NewLeases(leaseStorage, "/masterleases/", ttl)
+	masterLeases := reconcilers.NewLeases(leaseStorage, "/masterleases", ttl)


normalize inside NewLeases to ensure we work with or without leading/trailing slashes?

also, does List depend on a trailing slash? does removing this mean we could see things at /masterleases1?

Internally the prefix is appended with a / and a key for getting a specific key or just uses the prefix when listing, so shouldn't have an issue with /masterleases1. The adding of a / is currently hard-coded so "masterleases/" would result in "masterleases//" on lookup. Using path.join is the better way to handle the filename.

or just the prefix, so shouldn't have an issue with /masterleases1

my question was whether List("/masterleases") would return /masterleases1 if it existed

Updated the internals to use path.join when hunting for a key

my question was whether List("/masterleases") would return /masterleases1 if it existed

I believe it would. The list function is just using a prefix. In my experience List("masterleases") would return /masterleses1 and/or /masterleases/1

so this needs to change back to "/masterleases/"

You're right. I'll fix

rrati · 2017-10-17T15:08:40Z

@ncdc Yes, I've tested on a 3 node apiserver setup and verified the lease files are created, expire, and that the endpoint is created in the proper location and attached to the kubernetes service

ncdc · 2017-10-17T15:22:22Z

LGTM. Will defer to @liggitt to tag

liggitt · 2017-10-17T15:51:44Z

/lgtm

rrati · 2017-10-17T16:45:08Z

/assign @derekwaynecarr

derekwaynecarr · 2017-10-19T14:08:01Z

/approve

k8s-github-robot · 2017-10-19T14:08:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, liggitt, rrati

Associated issue: 53803

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

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

~~pkg/master/OWNERS~~ [derekwaynecarr]

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

rrati · 2017-10-19T16:13:58Z

/test pull-kubernetes-unit

k8s-github-robot · 2017-10-19T17:55:32Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-10-19T18:44:56Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-github-robot assigned yujuhong and derekwaynecarr Oct 12, 2017

ncdc reviewed Oct 12, 2017

View reviewed changes

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 12, 2017

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Oct 12, 2017

rrati force-pushed the lease-reconciler-fixes branch from e160bef to 700f41d Compare October 12, 2017 16:09

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 12, 2017

k8s-ci-robot assigned cheftako Oct 12, 2017

Fixed the lease endpoint reconciler creation of kubernetes endpoint a…

099404a

…nd lease file ttl

ncdc mentioned this pull request Oct 16, 2017

kube-proxy does not update iptables for default/kubernetes Endpoints #51445

Closed

liggitt reviewed Oct 16, 2017

View reviewed changes

rrati force-pushed the lease-reconciler-fixes branch 2 times, most recently from 2f4c12e to bc80909 Compare October 16, 2017 20:08

yujuhong removed their assignment Oct 16, 2017

rrati force-pushed the lease-reconciler-fixes branch from bc80909 to a52bcd4 Compare October 17, 2017 12:52

k8s-ci-robot assigned liggitt Oct 17, 2017

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

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 19, 2017

rrati force-pushed the lease-reconciler-fixes branch from a52bcd4 to 099404a Compare October 19, 2017 14:30

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

k8s-github-robot merged commit c7a1a06 into kubernetes:master Oct 19, 2017

rrati mentioned this pull request Jul 10, 2020

REQUEST: New membership for rrati kubernetes/org#2016

Closed

6 tasks

Fixed the lease endpoint reconciler creation of kubernetes endpoint and lease file ttl. #53803 #53809

Fixed the lease endpoint reconciler creation of kubernetes endpoint and lease file ttl. #53803 #53809

Conversation

rrati commented Oct 12, 2017 • edited

k8s-ci-robot commented Oct 12, 2017

rrati commented Oct 12, 2017

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

ncdc commented Oct 12, 2017

ncdc commented Oct 12, 2017

liggitt commented Oct 12, 2017

mml commented Oct 12, 2017

ncdc commented Oct 16, 2017

liggitt Oct 16, 2017 • edited

Choose a reason for hiding this comment

rrati Oct 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrati Oct 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Oct 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrati Oct 16, 2017 • edited

Choose a reason for hiding this comment

liggitt Oct 16, 2017 • edited

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

rrati commented Oct 17, 2017

ncdc commented Oct 17, 2017

liggitt commented Oct 17, 2017

rrati commented Oct 17, 2017

derekwaynecarr commented Oct 19, 2017

k8s-github-robot commented Oct 19, 2017

rrati commented Oct 19, 2017

k8s-github-robot commented Oct 19, 2017

k8s-github-robot commented Oct 19, 2017

rrati commented Oct 12, 2017 •

edited

liggitt Oct 16, 2017 •

edited

rrati Oct 16, 2017 •

edited

rrati Oct 16, 2017 •

edited

liggitt Oct 16, 2017 •

edited

rrati Oct 16, 2017 •

edited

liggitt Oct 16, 2017 •

edited