Update LeadershipController#revoke logic

Author: Gytis Trikleris

spring-cloud-kubernetes-leader/src/main/java/org/springframework/cloud/kubernetes/leader/LeadershipController.java

@@ -47,6 +47,21 @@ public LeadershipController(LeaderProperties leaderProperties, KubernetesHelper
 		this.leaderEventPublisher = leaderEventPublisher;
 	}
 
+	/**
+	 * Acquire leadership for the requested candidate, if there is no existing leader or existing leader is not valid.
+	 * <p>
+	 * If leadership is successfully acquired {@code true} will be returned, {@link Candidate#onGranted(Context)}
+	 * invoked and {@link LeaderEventPublisher#publishOnGranted(Object, Context, String)} event emitted.
+	 * <p>
+	 * If requested candidate is already a leader, simply {@code true} will be returned.
+	 * <p>
+	 * 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 boolean acquire(Candidate candidate) {
 		return false;
 	}
 
+	/**
+	 * Revoke leadership for the requested candidate.
+	 * <p>
+	 * 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.
+	 * <p>
+	 * If requested candidate is already not a leader, simply {@code true} will be returned.
+	 * <p>
+	 * 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) {

spring-cloud-kubernetes-leader/src/test/java/org/springframework/cloud/kubernetes/leader/LeadershipControllerTest.java

@@ -64,7 +64,7 @@ public void before() {
 	}
 
 	@Test
-	public void shouldAcquireWithoutExistingConfigMap() {
+	public void shouldAcquireWithoutExistingConfigMap() throws InterruptedException {
 		boolean result = leadershipController.acquire(mockCandidate);
 
 		assertThat(result).isTrue();
@@ -73,7 +73,7 @@ public void shouldAcquireWithoutExistingConfigMap() {
 	}
 
 	@Test
-	public void shouldAcquireWithExistingConfigMap() {
+	public void shouldAcquireWithExistingConfigMap() throws InterruptedException {
 		given(mockKubernetesHelper.getConfigMap()).willReturn(mockConfigMap);
 
 		boolean result = leadershipController.acquire(mockCandidate);
@@ -84,21 +84,22 @@ public void shouldAcquireWithExistingConfigMap() {
 	}
 
 	@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);
 
 		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 void shouldTakeOverLeadershipFromInvalidLeader() {
 		boolean result = leadershipController.acquire(mockCandidate);
 
 		assertThat(result).isTrue();
-		verify(mockKubernetesHelper).updateConfigMapEntry(mockConfigMap,
-			Collections.singletonMap(PREFIX + ROLE, anotherId));
+		Map<String, String> 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 void shouldRevokeLeadership() {
 	}
 
 	@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 void shouldFailToRevokeLeadership() {
 		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 void shouldHandleFailureWhenGettingLeader() {
 		assertThat(leader).isNull();
 	}
 
-	private void verifyPublishOnGranted() {
+	private void verifyPublishOnGranted() throws InterruptedException {
 		ArgumentCaptor<LeaderContext> 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 @@ private void verifyPublishOnRevoked() {
 			leaderContextCaptor.capture(), eq(ROLE));
 		LeaderContext expectedLeaderContext = new LeaderContext(mockCandidate, leadershipController);
 		assertThat(leaderContextCaptor.getValue()).isEqualToComparingFieldByField(expectedLeaderContext);
+
+		verify(mockCandidate).onRevoked(leaderContextCaptor.getValue());
 	}
 
 	public void verifyPublishOnFailedToAcquire() {