From 057fdbd902a06468b9e5df63845519428c66d03c Mon Sep 17 00:00:00 2001 From: Steve Hawkins Date: Tue, 19 Sep 2023 06:07:15 -0400 Subject: [PATCH] fix: ensure that onStopLeading is called closes #5463 --- CHANGELOG.md | 1 + .../leaderelection/LeaderElector.java | 47 +++++++++++-------- .../leaderelection/LeaderElectorTest.java | 32 +++++++++++++ 3 files changed, 61 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb1762dd85b..94e05feafae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ #### Bugs * Fix #5382: [java-generator] Allow to deserialize more valid RFC3339 date-time and make the format customizable * Fix #5380: [java-generator] Avoid to emit Java Keywords as package names +* Fix #5463: ensures that onStopLeading is called with releaseOnCancel even when leadership is already lost * Fix #5423: OkHttpClientImpl supports setting request method for empty payload requests #### Improvements diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/extended/leaderelection/LeaderElector.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/extended/leaderelection/LeaderElector.java index d3f7e62ed1f..71f1b4fb49d 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/extended/leaderelection/LeaderElector.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/extended/leaderelection/LeaderElector.java @@ -23,6 +23,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.net.HttpURLConnection; import java.time.Duration; import java.time.LocalDateTime; import java.time.ZoneOffset; @@ -121,36 +122,44 @@ private synchronized void stopLeading() { return; // not leading } if (leaderElectionConfig.isReleaseOnCancel()) { - release(); - } else { - leaderElectionConfig.getLeaderCallbacks().onStopLeading(); + try { + if (release()) { + return; + } + } catch (KubernetesClientException e) { + final String lockDescription = leaderElectionConfig.getLock().describe(); + if (e.getCode() != HttpURLConnection.HTTP_CONFLICT) { + LOGGER.error("Exception occurred while releasing lock '{}' on cancel", lockDescription, e); + } else { + LOGGER.debug("Leadership was likely already lost '{}'", lockDescription, e); + } + } } + leaderElectionConfig.getLeaderCallbacks().onStopLeading(); } /** * Release the leadership if currently held. If not cancelled, the elector will * continue to try and re-acquire the lock. + * + * @return true if the lock was successfully released. false if there is no lock, or this is not the leader */ - public synchronized void release() { + public synchronized boolean release() { LeaderElectionRecord current = leaderElectionConfig.getLock().get(kubernetesClient); if (current == null || !isLeader(current)) { - return; // lost leadership already + return false; // lost leadership already } - try { - ZonedDateTime now = now(); - final LeaderElectionRecord newLeaderElectionRecord = new LeaderElectionRecord( - "", - Duration.ofSeconds(1), - now, - now, - current.getLeaderTransitions()); + ZonedDateTime now = now(); + final LeaderElectionRecord newLeaderElectionRecord = new LeaderElectionRecord( + "", + Duration.ofSeconds(1), + now, + now, + current.getLeaderTransitions()); - leaderElectionConfig.getLock().update(kubernetesClient, newLeaderElectionRecord); - updateObserved(newLeaderElectionRecord); - } catch (KubernetesClientException e) { - final String lockDescription = leaderElectionConfig.getLock().describe(); - LOGGER.error("Exception occurred while releasing lock '{}'", lockDescription, e); - } + leaderElectionConfig.getLock().update(kubernetesClient, newLeaderElectionRecord); + updateObserved(newLeaderElectionRecord); + return true; } private CompletableFuture acquire() { diff --git a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/extended/leaderelection/LeaderElectorTest.java b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/extended/leaderelection/LeaderElectorTest.java index 0692bcd5619..ff0417b89f0 100644 --- a/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/extended/leaderelection/LeaderElectorTest.java +++ b/kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/extended/leaderelection/LeaderElectorTest.java @@ -15,6 +15,7 @@ */ package io.fabric8.kubernetes.client.extended.leaderelection; +import io.fabric8.kubernetes.api.model.StatusBuilder; import io.fabric8.kubernetes.client.KubernetesClientException; import io.fabric8.kubernetes.client.NamespacedKubernetesClient; import io.fabric8.kubernetes.client.extended.leaderelection.resourcelock.LeaderElectionRecord; @@ -26,6 +27,7 @@ import org.mockito.Answers; import org.mockito.Mockito; +import java.net.HttpURLConnection; import java.time.Duration; import java.time.Instant; import java.time.ZoneOffset; @@ -144,6 +146,36 @@ void shouldReleaseWhenCanceled() throws Exception { assertEquals(1, activeLer.get().getLeaderTransitions()); } + @Test + void shouldStopOnReleaseWhenCanceled() throws Exception { + // Given + AtomicReference activeLer = new AtomicReference<>(); + final LeaderElectionConfig lec = mockLeaderElectionConfiguration(activeLer); + final CountDownLatch signal = new CountDownLatch(1); + final Lock mockedLock = lec.getLock(); + when(lec.isReleaseOnCancel()).thenReturn(true); + AtomicInteger count = new AtomicInteger(); + doAnswer(invocation -> { + if (count.addAndGet(1) == 2) { + // simulate that we've already lost election + throw new KubernetesClientException(new StatusBuilder().withCode(HttpURLConnection.HTTP_CONFLICT).build()); + } + LeaderElectionRecord leaderRecord = invocation.getArgument(1, LeaderElectionRecord.class); + activeLer.set(leaderRecord); + signal.countDown(); + return null; + }).when(mockedLock).update(any(), any()); + + // When + LeaderElector leaderElector = new LeaderElector(mock(NamespacedKubernetesClient.class), lec, CommonThreadPool.get()); + CompletableFuture started = leaderElector.start(); + assertTrue(signal.await(10, TimeUnit.SECONDS)); + started.cancel(true); + + // Then + verify(lec.getLeaderCallbacks(), times(1)).onStopLeading(); + } + @Test void shouldRelease() throws Exception { // Given