Leader to give up leadership lock on graceful shutdown #1460
Conversation
k8s-ci-robot
added
cncf-cla: yes
himanshug
force-pushed
the
leader_shutdown
branch
from
6e4360c
to
9cb357c
Compare
himanshug
force-pushed
the
leader_shutdown
branch
from
9cb357c
to
a70bb73
Compare
| */ | ||
| @Test(timeout = 20000L) | ||
| public void testLeaderGracefulShutdown() throws Exception { | ||
| LockSmith lockSmith = new LockSmith(); |
There was a problem hiding this comment.
I feel like there is value in using WireMock and actually talking HTTP for the locks. How hard is it to switch to testing using WireMock like we do elsewhere?
There was a problem hiding this comment.
Thanks for the review.
Here we are more interested in testing the code of LeaderElector which can be tested as long as a well-behaving Lock implementation is provided. Also, truth is that it was just super simple/quick to do and does the job :) ... and followed in spirit the existing unit tests for leader election related code.
With WireMock, we would actually also test the implementation of Lock itself i.e. ConfigMapLock class etc. I haven't used WireMock in the past, so not sure about the work required, but will try to look into it.
that said, not sure but, I see there is an e2e build that appears to setup a real k8s cluster, how would you feel about this test running as an e2e integration test against real k8s cluster instead of the WireMock ?
There was a problem hiding this comment.
e2e integration is even better :)
|
Thanks, this looks good to me, but I'd rather see the tests actually use HTTP rather than an in-memory mock. Also, I'd like @yue9944882 to take a look at this before we merge it, since he wrote most of this code. |
k8s-ci-robot
added
the
lgtm
|
/approve Would love to have the integration test as a follow on PR. Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, himanshug 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 |
k8s-ci-robot
added
the
approved
So that other candidate can become the next leader quickly or else other candidate might need to wait for full
leaseDurationbefore getting the lock.