Add LeaseLock for leader election - #2690
Conversation
ff94d3b to
6073f7f
Compare
|
@yliaog this one is ready when you get a chance. @Invictus17 reviewed it and approved, and I pushed the change he asked for in 6073f7f. He isn't in OWNERS though, so Prow still shows it as not approved ,, it needs /lgtm and /approve from you or @fabianvf before it can go in.. |
Leader election could only use a ConfigMap. client-go moved to the coordination.k8s.io Lease resource and dropped ConfigMap support, so add a LeaseLock next to ConfigMapLock and use it in the example. Based on the implementation from kubernetes-client#2314, with four corrections: use the package logger instead of calling logging.basicConfig at import, accept the times str(datetime) produces when the microseconds are zero, keep the created lease so a following update has a reference, and store the times as the real UTC instant rather than local wall clock labeled as UTC. Co-authored-by: Lane Richard <rick.lane@nokia.com>
6073f7f to
9b87f18
Compare
|
Fixed all three in 9b87f18 - both copyright years and the spelling. Thanks for the review. |
|
thanks for the PR /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chala2001, Invictus17, yliaog The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ffd05cd
into
kubernetes-client:master
What type of PR is this?
/kind feature
What this PR does / why we need it:
Leader election can currently only use a
ConfigMap.client-gomoved to thecoordination.k8s.ioLeaseresource and removed ConfigMap support, so this adds aLeaseLocknext toConfigMapLock. The example now uses it, with the ConfigMap line kept commented out.Which issue(s) this PR fixes:
Fixes #1877
Special notes for your reviewer:
This picks up @rvlane's #2314, which you reviewed with "LGTM overall. Could you add a test?" and which the triage robot later closed as rotten. The original work is credited with a
Co-authored-bytrailer. The tests you asked for are inleaselock_test.py.Four things changed from that version while rebasing onto current master:
logging.basicConfig(level=logging.INFO)at import, which reconfigures the root logger of whatever application imports the library. This package moved tologger = logging.getLogger("leaderelection")in the meantime, so it uses that now.str(datetime)leaves the microseconds out when they are exactly zero, and the original only accepted the%fspellings. Those times fell through and returnedNone, so the lease was written with a nullacquireTime. Covered bytest_record_without_microseconds_survives_a_write_and_read.lease_referenceis kept aftercreate, so a followingupdatehas something to work from.updatealso returnsFalserather than raisingAttributeErrorwhen it is called before aget, which matches client-go'slease not initializedcheck.leaderelection.pybuilds its times withdatetime.fromtimestamp(), which is naive local time. The original appendedZto it, which labels local wall clock as UTC and writes a timestamp that is wrong by the host's offset. Since the point of a Lease is that other clients read it, the times are now converted to the real UTC instant and converted back on read, so the election record still round trips unchanged.test_times_are_written_as_the_real_utc_instantpins that underAsia/Kolkata.One thing I left out, happy to follow up if you would like it:
leaseTransitionsis neither read nor written, becauseLeaderElectionRecordhas no field for it. client-go writes it on every update. Adding it would touch the shared record class andConfigMapLockas well, so it seemed better as a separate change.Verified: 17 tests pass under
kubernetes/base/leaderelection(5 existing, 12 new), and reverting each of the four fixes above makes the matching test fail.pycodestyle --max-line-length=100is clean on both new files. I have not run the functional or e2e suites.