Skip to content

Commit 129b33c

Browse files
jandadavjandadav
authored
fix: Load balancer remote cache bugs (#1636)
* always store to cache after routing Signed-off-by: jandadav <janda.david@gmail.com> * Cache update even for conflict Signed-off-by: jandadav <janda.david@gmail.com> * Mocks for instanceId Signed-off-by: jandadav <janda.david@gmail.com>
1 parent 2daf227 commit 129b33c

File tree

8 files changed

+101
-43
lines changed

8 files changed

+101
-43
lines changed

‎gateway-service/src/main/java/org/zowe/apiml/gateway/cache/CachingServiceClient.java

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -125,14 +125,4 @@ public KeyValue() {
125125
}
126126
}
127127

128-
public class CachingServiceClientException extends Exception {
129-
130-
public CachingServiceClientException(String message, Throwable cause) {
131-
super(message, cause);
132-
}
133-
134-
public CachingServiceClientException(String message) {
135-
super(message);
136-
}
137-
}
138128
}

‎gateway-service/src/main/java/org/zowe/apiml/gateway/cache/CachingServiceClientException.java

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
/*
2+
* This program and the accompanying materials are made available under the terms of the
3+
* Eclipse Public License v2.0 which accompanies this distribution, and is available at
4+
* https://www.eclipse.org/legal/epl-v20.html
5+
*
6+
* SPDX-License-Identifier: EPL-2.0
7+
*
8+
* Copyright Contributors to the Zowe Project.
9+
*/
10+
11+
package org.zowe.apiml.gateway.cache;
12+
13+
public class CachingServiceClientException extends Exception {
14+
15+
public CachingServiceClientException(String message, Throwable cause) {
16+
super(message, cause);
17+
}
18+
19+
public CachingServiceClientException(String message) {
20+
super(message);
21+
}
22+
}

‎gateway-service/src/main/java/org/zowe/apiml/gateway/cache/LoadBalancerCache.java

Lines changed: 43 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
1515
import lombok.Getter;
1616
import lombok.extern.slf4j.Slf4j;
17+
import org.springframework.http.HttpStatus;
18+
import org.springframework.web.client.HttpClientErrorException;
1719
import org.zowe.apiml.gateway.ribbon.loadbalancer.model.LoadBalancerCacheRecord;
1820

1921
import java.util.Map;
@@ -43,6 +45,7 @@ public LoadBalancerCache(CachingServiceClient cachingServiceClient) {
4345

4446
/**
4547
* Store information about instance the user is balanced towards.
48+
* If there is already existing record, it will be updated
4649
*
4750
* @param user User being routed towards southbound service
4851
* @param service Service towards which is the user routed
@@ -51,20 +54,50 @@ public LoadBalancerCache(CachingServiceClient cachingServiceClient) {
5154
*/
5255
public boolean store(String user, String service, LoadBalancerCacheRecord loadBalancerCacheRecord) {
5356
if (remoteCache != null) {
54-
try {
55-
remoteCache.create(new CachingServiceClient.KeyValue(getKey(user, service), mapper.writeValueAsString(loadBalancerCacheRecord)));
56-
log.debug("Stored record to remote cache for user: {}, service: {}, record: {}", user, service, loadBalancerCacheRecord);
57-
} catch (CachingServiceClient.CachingServiceClientException e) {
58-
log.debug("Failed to store record for user: {}, service: {}, record {}, with exception: {}", user, service, loadBalancerCacheRecord, e);
59-
} catch (JsonProcessingException e) {
60-
log.debug("Failed to serialize record for user: {}, service: {}, record {}, with exception: {}", user, service, loadBalancerCacheRecord, e);
61-
}
57+
storeToRemoteCache(user, service, loadBalancerCacheRecord);
6258
}
6359
localCache.put(getKey(user, service), loadBalancerCacheRecord);
6460
log.debug("Stored record to local cache for user: {}, service: {}, record: {}", user, service, loadBalancerCacheRecord);
6561
return true;
6662
}
6763

64+
private void storeToRemoteCache(String user, String service, LoadBalancerCacheRecord loadBalancerCacheRecord) {
65+
try {
66+
String serializedRecord = mapper.writeValueAsString(loadBalancerCacheRecord);
67+
CachingServiceClient.KeyValue toStore = new CachingServiceClient.KeyValue(getKey(user, service), serializedRecord);
68+
createToRemoteCache(user, service, loadBalancerCacheRecord, toStore);
69+
} catch (JsonProcessingException e) {
70+
log.debug("Failed to serialize record for user: {}, service: {}, record {}, with exception: {}", user, service, loadBalancerCacheRecord, e);
71+
}
72+
}
73+
74+
private void createToRemoteCache(String user, String service, LoadBalancerCacheRecord loadBalancerCacheRecord, CachingServiceClient.KeyValue toStore) {
75+
try {
76+
remoteCache.create(toStore);
77+
log.debug("Created record to remote cache for user: {}, service: {}, record: {}", user, service, loadBalancerCacheRecord);
78+
} catch (CachingServiceClientException createException) {
79+
if (isCausedByCacheConflict(createException)) {
80+
updateToRemoteCache(user, service, loadBalancerCacheRecord, toStore);
81+
} else {
82+
log.debug("Failed to create record for user: {}, service: {}, record {}, with exception: {}", user, service, loadBalancerCacheRecord, createException);
83+
}
84+
}
85+
}
86+
87+
private void updateToRemoteCache(String user, String service, LoadBalancerCacheRecord loadBalancerCacheRecord, CachingServiceClient.KeyValue toStore) {
88+
try {
89+
remoteCache.update(toStore);
90+
log.debug("Updated record to remote cache for user: {}, service: {}, record: {}", user, service, loadBalancerCacheRecord);
91+
} catch (CachingServiceClientException updateException) {
92+
log.debug("Failed to update record for user: {}, service: {}, record {}, with exception: {}", user, service, loadBalancerCacheRecord, updateException);
93+
}
94+
}
95+
96+
private boolean isCausedByCacheConflict(CachingServiceClientException e) {
97+
return e.getCause() instanceof HttpClientErrorException &&
98+
((HttpClientErrorException) e.getCause()).getStatusCode().equals(HttpStatus.CONFLICT);
99+
}
100+
68101
/**
69102
* Retrieve information about selected instance for combination of User and Service.
70103
*
@@ -81,7 +114,7 @@ public LoadBalancerCacheRecord retrieve(String user, String service) {
81114
log.debug("Retrieved record from remote cache for user: {}, service: {}, record: {}", user, service, loadBalancerCacheRecord);
82115
return loadBalancerCacheRecord;
83116
}
84-
} catch (CachingServiceClient.CachingServiceClientException e) {
117+
} catch (CachingServiceClientException e) {
85118
log.debug("Failed to retrieve record for user: {}, service: {}, with exception: {}", user, service, e);
86119
} catch (JsonProcessingException e) {
87120
log.debug("Failed to deserialize record for user: {}, service: {}, with exception: {}", user, service, e);
@@ -103,7 +136,7 @@ public void delete(String user, String service) {
103136
try {
104137
remoteCache.delete(getKey(user, service));
105138
log.debug("Deleted record from remote cache for user: {}, service: {}", user, service);
106-
} catch (CachingServiceClient.CachingServiceClientException e) {
139+
} catch (CachingServiceClientException e) {
107140
log.debug("Failed to deleted record from remote cache for user: {}, service: {}, with exception: {}", user, service, e);
108141
}
109142
}

‎gateway-service/src/main/java/org/zowe/apiml/gateway/filters/post/PostStoreLoadBalancerCacheFilter.java

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,8 @@ public Object run() {
7474
RequestContext context = RequestContext.getCurrentContext();
7575
String currentServiceId = (String) context.get(SERVICE_ID_KEY);
7676
Optional<String> principal = authenticationService.getPrincipalFromRequest(context.getRequest());
77-
if (principal.isPresent() && !instanceIsCached(principal.get(), currentServiceId)) {
78-
// Dont store instance info when failed.
77+
if (principal.isPresent()) {
78+
// Dont store instance info when there is exception in request processing. This means failed request.
7979
if (context.get("throwable") != null) {
8080
return null;
8181
}
@@ -87,8 +87,4 @@ public Object run() {
8787

8888
return null;
8989
}
90-
91-
private boolean instanceIsCached(String user, String service) {
92-
return loadBalancerCache.retrieve(user, service) != null;
93-
}
9490
}

‎gateway-service/src/main/java/org/zowe/apiml/gateway/ribbon/loadbalancer/predicate/AuthenticationBasedPredicate.java

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ public class AuthenticationBasedPredicate extends RequestAwarePredicate {
4141
@Override
4242
public boolean apply(LoadBalancingContext context, DiscoveryEnabledServer server) {
4343
RequestContext requestContext = context.getRequestContext();
44+
String instanceId = context.getInstanceInfo().getInstanceId();
4445
String serviceId = (String) requestContext.get(SERVICE_ID_KEY);
4546
if (serviceId == null) {
4647
// This should never happen
@@ -50,20 +51,20 @@ public boolean apply(LoadBalancingContext context, DiscoveryEnabledServer server
5051
Optional<String> authenticatedUser = authenticationService.getPrincipalFromRequest(requestContext.getRequest());
5152

5253
if (!authenticatedUser.isPresent()) {
53-
log.debug("No authentication present on request, not filtering instance: {}", serviceId);
54+
log.debug("No authentication present on request, not filtering instance: {}", instanceId);
5455
return true;
5556
}
5657

5758
String username = authenticatedUser.get();
5859
LoadBalancerCacheRecord loadBalancerCacheRecord = cache.retrieve(username, serviceId);
5960
if (loadBalancerCacheRecord == null || loadBalancerCacheRecord.getInstanceId() == null) {
60-
log.debug("No preference exists, not filtering instance: {}", serviceId);
61+
log.debug("No preference exists, not filtering instance: {}", instanceId);
6162
return true;
6263
}
6364

6465
if (isTooOld(loadBalancerCacheRecord.getCreationTime())) {
6566
cache.delete(username, serviceId);
66-
log.debug("Expired preference exists and was deleted. not filtering instance: {}", serviceId);
67+
log.debug("Expired preference exists and was deleted. not filtering instance: {}", instanceId);
6768
return true;
6869
}
6970

‎gateway-service/src/test/java/org/zowe/apiml/gateway/cache/CachingServiceClientTest.java

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ void createWithoutProblem() {
4444
@Test
4545
void createWithExceptionFromRestTemplateThrowsDefined() {
4646
doThrow(new RestClientException("oops")).when(restTemplate).exchange(anyString(), any(HttpMethod.class), any(HttpEntity.class), eq(String.class));
47-
assertThrows(CachingServiceClient.CachingServiceClientException.class,() -> underTest.create(new CachingServiceClient.KeyValue("Britney", "Spears")));
47+
assertThrows(CachingServiceClientException.class,() -> underTest.create(new CachingServiceClient.KeyValue("Britney", "Spears")));
4848
}
4949
}
5050

@@ -60,7 +60,7 @@ void updateWithoutProblem() {
6060
@Test
6161
void updateWithExceptionFromRestTemplateThrowsDefined() {
6262
doThrow(new RestClientException("oops")).when(restTemplate).exchange(anyString(), any(HttpMethod.class), any(HttpEntity.class), eq(String.class));
63-
assertThrows(CachingServiceClient.CachingServiceClientException.class,() -> underTest.update(new CachingServiceClient.KeyValue("Britney", "Spears")));
63+
assertThrows(CachingServiceClientException.class,() -> underTest.update(new CachingServiceClient.KeyValue("Britney", "Spears")));
6464
}
6565
}
6666

@@ -71,16 +71,16 @@ class givenReadOperation {
7171

7272
@Test
7373
void readWithNullResponseOrNullBody() {
74-
assertThrows(CachingServiceClient.CachingServiceClientException.class, () -> underTest.read(keyToRead));
74+
assertThrows(CachingServiceClientException.class, () -> underTest.read(keyToRead));
7575
verify(restTemplate).exchange(eq(urlBase + "/" + keyToRead), eq(HttpMethod.GET), any(HttpEntity.class), eq(CachingServiceClient.KeyValue.class));
7676
ResponseEntity<CachingServiceClient.KeyValue> responseEntity = mock(ResponseEntity.class);
7777
doReturn(false).when(responseEntity).hasBody();
7878
doReturn(responseEntity).when(restTemplate).exchange(eq(urlBase + "/" + keyToRead), eq(HttpMethod.GET), any(HttpEntity.class), eq(CachingServiceClient.KeyValue.class));
79-
assertThrows(CachingServiceClient.CachingServiceClientException.class, () -> underTest.read(keyToRead));
79+
assertThrows(CachingServiceClientException.class, () -> underTest.read(keyToRead));
8080
}
8181

8282
@Test
83-
void readWithoutProblem() throws CachingServiceClient.CachingServiceClientException {
83+
void readWithoutProblem() throws CachingServiceClientException {
8484
ResponseEntity<CachingServiceClient.KeyValue> responseEntity = mock(ResponseEntity.class);
8585
doReturn(true).when(responseEntity).hasBody();
8686
doReturn(new CachingServiceClient.KeyValue(keyToRead, "Wonder")).when(responseEntity).getBody();
@@ -91,7 +91,7 @@ void readWithoutProblem() throws CachingServiceClient.CachingServiceClientExcept
9191
@Test
9292
void readWithExceptonFromRestTemplateThrowsDefined() {
9393
doThrow(new RestClientException("oops")).when(restTemplate).exchange(anyString(), any(HttpMethod.class), any(HttpEntity.class), eq(String.class));
94-
assertThrows(CachingServiceClient.CachingServiceClientException.class, () -> underTest.read(keyToRead));
94+
assertThrows(CachingServiceClientException.class, () -> underTest.read(keyToRead));
9595
}
9696
}
9797

@@ -107,7 +107,7 @@ void deleteWithoutProblem() {
107107
@Test
108108
void deleteWithExceptionFromRestTemplateThrowsDefined() {
109109
doThrow(new RestClientException("oops")).when(restTemplate).exchange(anyString(), any(HttpMethod.class), any(HttpEntity.class), eq(String.class));
110-
assertThrows(CachingServiceClient.CachingServiceClientException.class,() -> underTest.delete(keyToDelete));
110+
assertThrows(CachingServiceClientException.class,() -> underTest.delete(keyToDelete));
111111
}
112112
}
113113

‎gateway-service/src/test/java/org/zowe/apiml/gateway/cache/LoadBalancerCacheTest.java

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,9 @@
1313
import com.fasterxml.jackson.databind.ObjectMapper;
1414
import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
1515
import org.junit.jupiter.api.*;
16+
import org.springframework.http.HttpHeaders;
17+
import org.springframework.http.HttpStatus;
18+
import org.springframework.web.client.HttpClientErrorException;
1619
import org.zowe.apiml.gateway.ribbon.loadbalancer.model.LoadBalancerCacheRecord;
1720

1821
import static org.hamcrest.CoreMatchers.is;
@@ -72,17 +75,27 @@ void setUp() {
7275
class Storage {
7376

7477
@Test
75-
void storageHappensToLocalAndRemoteCache() throws CachingServiceClient.CachingServiceClientException, JsonProcessingException {
78+
void storageHappensToLocalAndRemoteCache() throws CachingServiceClientException, JsonProcessingException {
7679
underTest.store("user", "serviceid", record);
7780
String serializedRecord = mapper.writeValueAsString(record);
78-
// TODO should the keys be prefixed by loadbalancer denomination?
7981
verify(cachingServiceClient).create(new CachingServiceClient.KeyValue(keyPrefix + "user:serviceid", serializedRecord));
8082
assertThat(underTest.getLocalCache().containsKey(keyPrefix + "user:serviceid"), is(true));
8183
}
8284

8385
@Test
84-
void storageFailsToRemoteCacheAndStoresLocal() throws CachingServiceClient.CachingServiceClientException {
85-
doThrow(CachingServiceClient.CachingServiceClientException.class).when(cachingServiceClient).create(any());
86+
void storageHappensToRemoteEvenForConflict() throws CachingServiceClientException, JsonProcessingException {
87+
HttpClientErrorException clientErrorException = HttpClientErrorException.create(HttpStatus.CONFLICT, "", new HttpHeaders(), null, null);
88+
CachingServiceClientException e = new CachingServiceClientException("oops", clientErrorException);
89+
doThrow(e).when(cachingServiceClient).create(any());
90+
String serializedRecord = mapper.writeValueAsString(record);
91+
92+
underTest.store("user", "serviceid", record);
93+
verify(cachingServiceClient).update(new CachingServiceClient.KeyValue(keyPrefix + "user:serviceid", serializedRecord));
94+
}
95+
96+
@Test
97+
void storageFailsToRemoteCacheAndStoresLocal() throws CachingServiceClientException {
98+
doThrow(CachingServiceClientException.class).when(cachingServiceClient).create(any());
8699
underTest.store("user", "serviceid", record);
87100
assertThat(underTest.getLocalCache().containsKey(keyPrefix + "user:serviceid"), is(true));
88101
}
@@ -92,9 +105,9 @@ void storageFailsToRemoteCacheAndStoresLocal() throws CachingServiceClient.Cachi
92105
class Retrieval {
93106

94107
@Test
95-
void retrievalFromRemoteHasPriority() throws CachingServiceClient.CachingServiceClientException, JsonProcessingException {
108+
void retrievalFromRemoteHasPriority() throws CachingServiceClientException, JsonProcessingException {
96109
underTest.getLocalCache().put(keyPrefix + "user:serviceid", record);
97-
doThrow(CachingServiceClient.CachingServiceClientException.class).when(cachingServiceClient).read(any());
110+
doThrow(CachingServiceClientException.class).when(cachingServiceClient).read(any());
98111
LoadBalancerCacheRecord retrievedRecord = underTest.retrieve("user", "serviceid");
99112
assertThat(retrievedRecord.getInstanceId(), is("instanceid"));
100113

@@ -111,7 +124,7 @@ void retrievalFromRemoteHasPriority() throws CachingServiceClient.CachingService
111124
@Nested
112125
class Deletion {
113126
@Test
114-
void deleteRemovesAllEntriesLocalAndRemote() throws CachingServiceClient.CachingServiceClientException {
127+
void deleteRemovesAllEntriesLocalAndRemote() throws CachingServiceClientException {
115128
underTest.getLocalCache().put(keyPrefix + "user:serviceid", record);
116129
underTest.delete("user", "serviceid");
117130
verify(cachingServiceClient).delete(keyPrefix + "user:serviceid");

‎gateway-service/src/test/java/org/zowe/apiml/gateway/ribbon/loadbalancer/predicate/AuthenticationBasedPredicateTest.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ void setUp() {
4848
context = mock(LoadBalancingContext.class);
4949
requestContext = mock(RequestContext.class);
5050
when(context.getRequestContext()).thenReturn(requestContext);
51+
InstanceInfo info = mock(InstanceInfo.class);
52+
when(info.getInstanceId()).thenReturn("hostname:service:port");
53+
when(context.getInstanceInfo()).thenReturn(info);
5154

5255
underTest = new AuthenticationBasedPredicate(authenticationService, cache, 8);
5356
}

0 commit comments

Comments
 (0)