From ad4a9d090d6818747323a3d6ac14055769492abb Mon Sep 17 00:00:00 2001 From: Gytis Trikleris Date: Tue, 5 Jun 2018 11:02:08 +0200 Subject: [PATCH] Update LeadershipController#revoke logic --- .../leader/LeadershipController.java | 38 +++++++++++++++++-- .../leader/LeadershipControllerTest.java | 38 +++++++++++-------- 2 files changed, 57 insertions(+), 19 deletions(-) diff --git a/spring-cloud-kubernetes-leader/src/main/java/org/springframework/cloud/kubernetes/leader/LeadershipController.java b/spring-cloud-kubernetes-leader/src/main/java/org/springframework/cloud/kubernetes/leader/LeadershipController.java index 01552eda..4b2143dd 100644 --- a/spring-cloud-kubernetes-leader/src/main/java/org/springframework/cloud/kubernetes/leader/LeadershipController.java +++ b/spring-cloud-kubernetes-leader/src/main/java/org/springframework/cloud/kubernetes/leader/LeadershipController.java @@ -47,6 +47,21 @@ public class LeadershipController { this.leaderEventPublisher = leaderEventPublisher; } + /** + * Acquire leadership for the requested candidate, if there is no existing leader or existing leader is not valid. + *

+ * If leadership is successfully acquired {@code true} will be returned, {@link Candidate#onGranted(Context)} + * invoked and {@link LeaderEventPublisher#publishOnGranted(Object, Context, String)} event emitted. + *

+ * If requested candidate is already a leader, simply {@code true} will be returned. + *

+ * If for some reason leadership cannot be acquired (communication failure or there is another leader), + * {@code false} will be returned and {@link LeaderEventPublisher#publishOnFailedToAcquire(Object, Context, String)} + * event will be emitted. + * + * @param candidate + * @return {@code true} if at the end of execution candidate is a leader and {@code false} if it isn't. + */ public boolean acquire(Candidate candidate) { try { ConfigMap configMap = kubernetesHelper.getConfigMap(); @@ -75,29 +90,44 @@ public class LeadershipController { return false; } + /** + * Revoke leadership for the requested candidate. + *

+ * If candidate's leadership is successfully revoked, {@code true} will be returned, + * {@link Candidate#onRevoked(Context)} invoked and + * {@link LeaderEventPublisher#publishOnRevoked(Object, Context, String)} event emitted. + *

+ * If requested candidate is already not a leader, simply {@code true} will be returned. + *

+ * If leadership cannot be revoked for a communication error or a concurrent ConfigMap modification, {@code false} + * will be returned. + * + * @param candidate + * @return {@code true} if at the end of execution candidate is not a leader. {@code false} revoke operation failed. + */ public boolean revoke(Candidate candidate) { try { ConfigMap configMap = kubernetesHelper.getConfigMap(); if (configMap == null) { - return false; + return true; } Leader leader = getLeader(candidate.getRole(), configMap); if (leader == null) { - return false; + return true; } if (candidate.getId().equals(leader.getId())) { removeLeaderFromConfigMap(candidate, configMap); handleOnRevoked(candidate); - return true; } } catch (KubernetesClientException e) { LOGGER.warn("Failed to revoke leadership with role='{}' for candidate='{}': {}", candidate.getRole(), candidate.getId(), e.getMessage()); + return false; } - return false; + return true; } public Leader getLeader(String role) { diff --git a/spring-cloud-kubernetes-leader/src/test/java/org/springframework/cloud/kubernetes/leader/LeadershipControllerTest.java b/spring-cloud-kubernetes-leader/src/test/java/org/springframework/cloud/kubernetes/leader/LeadershipControllerTest.java index 101df4e7..aa5f2f7d 100644 --- a/spring-cloud-kubernetes-leader/src/test/java/org/springframework/cloud/kubernetes/leader/LeadershipControllerTest.java +++ b/spring-cloud-kubernetes-leader/src/test/java/org/springframework/cloud/kubernetes/leader/LeadershipControllerTest.java @@ -64,7 +64,7 @@ public class LeadershipControllerTest { } @Test - public void shouldAcquireWithoutExistingConfigMap() { + public void shouldAcquireWithoutExistingConfigMap() throws InterruptedException { boolean result = leadershipController.acquire(mockCandidate); assertThat(result).isTrue(); @@ -73,7 +73,7 @@ public class LeadershipControllerTest { } @Test - public void shouldAcquireWithExistingConfigMap() { + public void shouldAcquireWithExistingConfigMap() throws InterruptedException { given(mockKubernetesHelper.getConfigMap()).willReturn(mockConfigMap); boolean result = leadershipController.acquire(mockCandidate); @@ -84,7 +84,7 @@ public class LeadershipControllerTest { } @Test - public void shouldNotAcquireIfAlreadyLeader() { + public void shouldAcquireWithoutEventsIfAlreadyLeader() throws InterruptedException { given(mockKubernetesHelper.getConfigMap()).willReturn(mockConfigMap); given(mockKubernetesHelper.isPodAlive(ID)).willReturn(true); given(mockConfigMap.getData()).willReturn(leaderData); @@ -92,13 +92,14 @@ public class LeadershipControllerTest { boolean result = leadershipController.acquire(mockCandidate); assertThat(result).isTrue(); + verify(mockCandidate, times(0)).onGranted(any()); verify(mockKubernetesHelper, times(0)).createConfigMap(any()); verify(mockKubernetesHelper, times(0)).updateConfigMapEntry(any(), any()); verify(mockLeaderEventPublisher, times(0)).publishOnGranted(any(), any(), any()); } @Test - public void shouldTakeOverLeadershipFromInvalidLeader() { + public void shouldTakeOverLeadershipFromInvalidLeader() throws InterruptedException { String anotherId = "another-test-id"; given(mockKubernetesHelper.getConfigMap()).willReturn(mockConfigMap); given(mockKubernetesHelper.isPodAlive(ID)).willReturn(false); @@ -108,13 +109,13 @@ public class LeadershipControllerTest { boolean result = leadershipController.acquire(mockCandidate); assertThat(result).isTrue(); - verify(mockKubernetesHelper).updateConfigMapEntry(mockConfigMap, - Collections.singletonMap(PREFIX + ROLE, anotherId)); + Map anotherLeaderData = Collections.singletonMap(PREFIX + ROLE, anotherId); + verify(mockKubernetesHelper).updateConfigMapEntry(mockConfigMap, anotherLeaderData); verifyPublishOnGranted(); } @Test - public void shouldFailToAcquireBecauseOfExistingLeader() { + public void shouldFailToAcquireIfThereIsAnotherLeader() { given(mockKubernetesHelper.getConfigMap()).willReturn(mockConfigMap); given(mockKubernetesHelper.isPodAlive(ID)).willReturn(true); given(mockConfigMap.getData()).willReturn(leaderData); @@ -151,34 +152,37 @@ public class LeadershipControllerTest { } @Test - public void shouldNotRevokeLeadershipIfThereIsNoConfigMap() { + public void shouldRevokeLeadershipWithoutEventsIfThereIsNoConfigMap() { boolean result = leadershipController.revoke(mockCandidate); - assertThat(result).isFalse(); + assertThat(result).isTrue(); + verify(mockCandidate, times(0)).onRevoked(any()); verify(mockKubernetesHelper, times(0)).removeConfigMapEntry(any(), any()); verify(mockLeaderEventPublisher, times(0)).publishOnRevoked(any(), any(), any()); } @Test - public void shouldNotRevokeLeadershipIfThereIsNoLeader() { + public void shouldRevokeLeadershipWithoutEventsIfThereIsNoLeader() { given(mockKubernetesHelper.getConfigMap()).willReturn(mockConfigMap); boolean result = leadershipController.revoke(mockCandidate); - assertThat(result).isFalse(); + assertThat(result).isTrue(); + verify(mockCandidate, times(0)).onRevoked(any()); verify(mockKubernetesHelper, times(0)).removeConfigMapEntry(any(), any()); verify(mockLeaderEventPublisher, times(0)).publishOnRevoked(any(), any(), any()); } @Test - public void shouldNotRevokeLeadershipIfThereIsAnotherLeader() { + public void shouldRevokeLeadershipWithoutEventsIfThereIsAnotherLeader() { given(mockKubernetesHelper.getConfigMap()).willReturn(mockConfigMap); given(mockConfigMap.getData()).willReturn(leaderData); given(mockCandidate.getId()).willReturn("another-test-id"); boolean result = leadershipController.revoke(mockCandidate); - assertThat(result).isFalse(); + assertThat(result).isTrue(); + verify(mockCandidate, times(0)).onRevoked(any()); verify(mockKubernetesHelper, times(0)).removeConfigMapEntry(any(), any()); verify(mockLeaderEventPublisher, times(0)).publishOnRevoked(any(), any(), any()); } @@ -193,7 +197,7 @@ public class LeadershipControllerTest { boolean result = leadershipController.revoke(mockCandidate); assertThat(result).isFalse(); - verify(mockKubernetesHelper).removeConfigMapEntry(mockConfigMap, PREFIX + ROLE); + verify(mockCandidate, times(0)).onRevoked(any()); verify(mockLeaderEventPublisher, times(0)).publishOnRevoked(any(), any(), any()); } @@ -242,12 +246,14 @@ public class LeadershipControllerTest { assertThat(leader).isNull(); } - private void verifyPublishOnGranted() { + private void verifyPublishOnGranted() throws InterruptedException { ArgumentCaptor leaderContextCaptor = ArgumentCaptor.forClass(LeaderContext.class); verify(mockLeaderEventPublisher).publishOnGranted(eq(leadershipController), leaderContextCaptor.capture(), eq(ROLE)); LeaderContext expectedLeaderContext = new LeaderContext(mockCandidate, leadershipController); assertThat(leaderContextCaptor.getValue()).isEqualToComparingFieldByField(expectedLeaderContext); + + verify(mockCandidate).onGranted(leaderContextCaptor.getValue()); } private void verifyPublishOnRevoked() { @@ -256,6 +262,8 @@ public class LeadershipControllerTest { leaderContextCaptor.capture(), eq(ROLE)); LeaderContext expectedLeaderContext = new LeaderContext(mockCandidate, leadershipController); assertThat(leaderContextCaptor.getValue()).isEqualToComparingFieldByField(expectedLeaderContext); + + verify(mockCandidate).onRevoked(leaderContextCaptor.getValue()); } public void verifyPublishOnFailedToAcquire() {