Skip to content

Commit 0aaaac8

Browse files
gberche-orangegberche-orange
committed
Handle race conditions (including for K8S dups): handle dupl sync update
 * [ ] Test update https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#response-5 * [x] Implement update error recovery * Check si, if an update operation is in progress, then return 202 accepted, * otherwise 500 bad request if update request was previously accepted * otherwise 400 bad request otherwise * OSB api v2.16 will be supporting error details such as "usable", see cloudfoundry/servicebroker#661 * But not yet implemented in CF API v3 http://v3-apidocs.cloudfoundry.org/version/3.83.0/index.html#create-a-service-instance * Nor in SC-OSB * [x] Refine USI sync success test: sync update will trigger a new backing update and not enter error recovery branch, just perform the update twise * [x] OSB provision dupl same SI: check same dupl receives right status * [x] 200 Ok as backing service was completed
1 parent 87f0b80 commit 0aaaac8

File tree

3 files changed

+114
-24
lines changed

3 files changed

+114
-24
lines changed

‎osb-cmdb/docs/impl-notes/TODO-cmdb-native.md‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,16 @@
2626
* [ ] Refactor race condition support
2727
* [ ] extract concurrent exception handler in its collaborator object to unit test it
2828
* [ ] Test update https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#response-5
29-
* [ ] Implement update error recovery
30-
* [ ] Refine USI sync success test
31-
* [ ] OSB provision dupl same SI: check same dupl receives right status
32-
* [ ] 200 Ok as backing service was completed
29+
* [x] Implement update error recovery
30+
* Check si, if an update operation is in progress, then return 202 accepted,
31+
* otherwise 500 bad request if update request was previously accepted
32+
* otherwise 400 bad request otherwise
33+
* OSB api v2.16 will be supporting error details such as "usable", see https://github.com/openservicebrokerapi/servicebroker/pull/661
34+
* But not yet implemented in CF API v3 http://v3-apidocs.cloudfoundry.org/version/3.83.0/index.html#create-a-service-instance
35+
* Nor in SC-OSB
36+
* [x] Refine USI sync success test: sync update will trigger a new backing update and not enter error recovery branch, just perform the update twise
37+
* [x] OSB provision dupl same SI: check same dupl receives right status
38+
* [x] 200 Ok as backing service was completed
3339
* [ ] Check UpdateAsyncInstanceWithBackingServiceAcceptanceTest does
3440
* [ ] USI
3541
* [ ] OSB provision dupl same SI: check same dupl receives right status

‎osb-cmdb/src/main/java/com/orange/oss/osbcmdb/serviceinstance/OsbCmdbServiceInstance.java‎

Lines changed: 80 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -362,6 +362,82 @@ private Mono<DeleteServiceInstanceResponse> handleDeleteException(Exception orig
362362
throw new ServiceInstanceDoesNotExistException(request.getServiceInstanceId());
363363
}
364364

365+
/**
366+
* Handle exceptions by checking current backing service instance in order to return the appropriate response
367+
* as expected in the OSB specifications.
368+
* @param originalException The exception occuring during processing. Note that Subclasses of
369+
* ServiceBrokerException are considered already qualified, and returned as-is
370+
*/
371+
private Mono<UpdateServiceInstanceResponse> handleUpdateException(Exception originalException,
372+
String backingServiceInstanceName,
373+
CloudFoundryOperations spacedTargetedOperations,
374+
UpdateServiceInstanceRequest request) {
375+
LOG.info("Inspecting exception caught {} for possible concurrent dupl while handling request {} ", originalException, request);
376+
377+
if (originalException instanceof ServiceBrokerException && ! originalException.getClass().equals(ServiceBrokerException.class)) {
378+
LOG.info("Exception was thrown by ourselves, and already qualified, rethrowing it unmodified");
379+
throw (ServiceBrokerException) originalException;
380+
}
381+
382+
ServiceInstance updatedSi = getCfServiceInstance(spacedTargetedOperations, backingServiceInstanceName);
383+
String spaceId = getSpacedIdFromTargettedOperationsInternals(spacedTargetedOperations);
384+
385+
if (updatedSi != null) {
386+
if (!"update".equals(updatedSi.getLastOperation())) {
387+
LOG.info("No instance in the inventory with id={} in space with id={}, and recently updated: " +
388+
"last_operation={}, flowing up original exception",
389+
request.getServiceInstanceId(), spaceId, updatedSi.getLastOperation() );
390+
//500 error
391+
throw new ServiceBrokerInvalidParametersException(originalException.getMessage());
392+
}
393+
switch (updatedSi.getStatus()) {
394+
case OsbApiConstants.LAST_OPERATION_STATE_INPROGRESS:
395+
LOG.info("Concurrent update request is still in progress. " +
396+
"Returning accepted: 202");
397+
String operation = toJson(new CmdbOperationState(updatedSi.getId(),
398+
OsbOperation.UPDATE));
399+
//202 Accepted
400+
return Mono.just(UpdateServiceInstanceResponse.builder()
401+
.operation(operation)
402+
.async(true)
403+
.build());
404+
405+
case OsbApiConstants.LAST_OPERATION_STATE_SUCCEEDED:
406+
if (updatedSi.getService().equals(request.getServiceDefinition().getName()) &&
407+
updatedSi.getPlan().equals(request.getPlan().getName())) {
408+
LOG.info("Concurrent update request has completed. " +
409+
"Returning 200 OK");
410+
//200 OK
411+
return Mono.just(UpdateServiceInstanceResponse.builder()
412+
.async(false)
413+
.build());
414+
}
415+
else {
416+
LOG.info("Plan update did not succeed while SI update completed, assuming invalid input. " +
417+
"Existing si service name={} and service plan={}", updatedSi.getService(), updatedSi.getPlan());
418+
throw new ServiceBrokerInvalidParametersException(originalException.getMessage());
419+
}
420+
421+
case OsbApiConstants.LAST_OPERATION_STATE_FAILED:
422+
LOG.info("Backing service failed to update with {}, flowing up the original error to the osb " +
423+
"client: ",
424+
updatedSi.getMessage(), originalException.getMessage());
425+
//500 error
426+
//In the future, return the usable field once CF supports it
427+
throw new ServiceBrokerException(originalException.getMessage());
428+
429+
default:
430+
LOG.error("Unexpected last operation state:" + updatedSi.getStatus());
431+
throw new ServiceBrokerException("Internal CF protocol error");
432+
}
433+
}
434+
LOG.info("No existing instance in the inventory with id={} in space with id={}, assuming invalid requestrace " +
435+
"condition " +
436+
"among concurrent update request. Returning 410", request.getServiceInstanceId(), spaceId);
437+
//410 Gone
438+
throw new ServiceInstanceDoesNotExistException(request.getServiceInstanceId());
439+
}
440+
365441
private String getRequestIncompatibilityWithExistingInstance(ServiceInstance existingServiceInstance,
366442
ServiceDefinition serviceDefinition, Plan plan) {
367443
if (! existingServiceInstance.getService().equals(serviceDefinition.getName())) {
@@ -615,11 +691,12 @@ public Mono<UpdateServiceInstanceResponse> updateServiceInstance(UpdateServiceIn
615691

616692
String backingServiceName = request.getServiceDefinition().getName();
617693
String backingServicePlanName = request.getPlan().getName();
694+
String backingServiceInstanceName = ServiceInstanceNameHelper.truncateNameToCfMaxSize(request.getServiceInstanceId());
618695

619696
//ignore race condition during space creation for K8S dupl requests
620697
CloudFoundryOperations spacedTargetedOperations = getSpaceScopedOperations(backingServiceName);
621698
ServiceInstance existingBackingServiceInstance = getCfServiceInstance(spacedTargetedOperations,
622-
ServiceInstanceNameHelper.truncateNameToCfMaxSize(request.getServiceInstanceId()));
699+
backingServiceInstanceName);
623700
//Lookup guids necessary for low level api usage, and that CloudFoundryOperations hides in its response
624701
String spaceId = getSpacedIdFromTargettedOperationsInternals(spacedTargetedOperations);
625702
String backingServicePlanId = fetchBackingServicePlanId(backingServiceName, backingServicePlanName, spaceId);
@@ -667,10 +744,8 @@ public Mono<UpdateServiceInstanceResponse> updateServiceInstance(UpdateServiceIn
667744
}
668745
responseBuilder.async(asyncProvisioning);
669746
}
670-
catch (ClientV2Exception e) {
671-
LOG.info("Unable to update service, caught:" + e, e);
672-
//wrap to avoid log polution from sc-osb catching unexpectidely unknown (cf-java-client's) exceptions
673-
throw new ServiceBrokerException(e.toString());
747+
catch (Exception e) {
748+
return handleUpdateException(e, backingServiceInstanceName, spacedTargetedOperations, request);
674749
}
675750
finally {
676751
//systematically try to update metadata (e.g. service instance rename) even if update failed

‎spring-cloud-app-broker-acceptance-tests/src/test/java/org/springframework/cloud/appbroker/acceptance/UpdateInstanceWithBackingServiceAcceptanceTest.java‎

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,16 @@
1919
import org.cloudfoundry.operations.services.ServiceInstance;
2020
import org.junit.jupiter.api.Tag;
2121
import org.junit.jupiter.api.Test;
22+
import org.slf4j.Logger;
23+
import org.slf4j.LoggerFactory;
2224

2325
import static org.assertj.core.api.Assertions.assertThat;
2426

2527
@Tag("cmdb")
2628
class UpdateInstanceWithBackingServiceAcceptanceTest extends CmdbCloudFoundryAcceptanceTest {
2729

30+
private static final Logger LOG = LoggerFactory.getLogger(UpdateInstanceWithBackingServiceAcceptanceTest.class);
31+
2832
private static final String SUFFIX = "update-instance";
2933

3034
@Override
@@ -56,24 +60,29 @@ protected String testSuffix() {
5660
void brokeredServiceUpdates() {
5761
// given a brokered service instance is created
5862
createServiceInstance(brokeredServiceInstanceName());
63+
String backingServiceName = null;
5964

60-
//given a backend service is configured to accept any update
61-
//when a brokered service update plan is requested
62-
updateServiceInstance(brokeredServiceInstanceName(), PLAN2_NAME);
65+
for (int i=0; i<2; i++) { //Performing the update twice, to ensure idempotency, and support for K8S
66+
// duplicated concurrent requests
67+
LOG.info("Update #{}", i);
68+
//given a backend service is configured to accept any update
69+
//when a brokered service update plan is requested
70+
updateServiceInstance(brokeredServiceInstanceName(), PLAN2_NAME);
6371

64-
//then a brokered service is updated
65-
ServiceInstance brokeredServiceInstance = getServiceInstance(brokeredServiceInstanceName());
66-
// then the brokered service instance once completes, is expected to be failed
67-
assertThat(brokeredServiceInstance.getLastOperation()).isEqualTo("update");
68-
assertThat(brokeredServiceInstance.getStatus()).isEqualTo("succeeded");
72+
//then a brokered service is updated
73+
ServiceInstance brokeredServiceInstance = getServiceInstance(brokeredServiceInstanceName());
74+
// then the brokered service instance once completes, is expected to be failed
75+
assertThat(brokeredServiceInstance.getLastOperation()).isEqualTo("update");
76+
assertThat(brokeredServiceInstance.getStatus()).isEqualTo("succeeded");
6977

70-
//and backing service was indeed updated
71-
String backingServiceName = brokeredServiceInstance.getId();
72-
ServiceInstance backingServiceInstance = getServiceInstance(backingServiceName, brokeredServiceName());
73-
//was indeed updated, and has still its last operation as failed
74-
assertThat(backingServiceInstance.getLastOperation()).isEqualTo("update");
75-
assertThat(backingServiceInstance.getStatus()).isEqualTo("succeeded");
76-
assertThat(backingServiceInstance.getPlan()).isEqualTo(PLAN2_NAME);
78+
//and backing service was indeed updated
79+
backingServiceName = brokeredServiceInstance.getId();
80+
ServiceInstance backingServiceInstance = getServiceInstance(backingServiceName, brokeredServiceName());
81+
//was indeed updated, and has still its last operation as failed
82+
assertThat(backingServiceInstance.getLastOperation()).isEqualTo("update");
83+
assertThat(backingServiceInstance.getStatus()).isEqualTo("succeeded");
84+
assertThat(backingServiceInstance.getPlan()).isEqualTo(PLAN2_NAME);
85+
}
7786

7887
// when the service instance is deleted
7988
deleteServiceInstance(brokeredServiceInstanceName());

0 commit comments

Comments
 (0)