v3(services): attempt to delete SIs being created

[#173149474](https://www.pivotaltracker.com/story/show/173149474)

Author: Derik Evangelista

app/actions/v3/service_instance_delete.rb

@@ -17,12 +17,12 @@ def delete(service_instance)
         cannot_delete_shared_instances! if service_instance.shared?
 
         lock = DeleterLock.new(service_instance)
-        lock.lock!
 
         job = case service_instance
               when ManagedServiceInstance
                 asynchronous_destroy(service_instance)
               when UserProvidedServiceInstance
+                lock.lock!
                 synchronous_destroy(service_instance, lock)
               end
 

app/jobs/v3/delete_service_instance_job.rb

@@ -19,6 +19,10 @@ def send_broker_request(client)
       rescue VCAP::Services::ServiceBrokers::V2::Errors::ServiceBrokerBadResponse => err
         @request_failed = true
         raise DeprovisionBadResponse.new(err.message)
+      rescue CloudController::Errors::ApiError => err
+        raise OperationAborted.new('The service broker rejected the request') if err.name == 'AsyncServiceInstanceOperationInProgress'
+
+        raise err
       end
 
       def operation_succeeded

app/jobs/v3/service_instance_async_job.rb

@@ -5,6 +5,9 @@ module V3
     class LastOperationStateFailed < StandardError
     end
 
+    class OperationAborted < StandardError
+    end
+
     class ServiceInstanceAsyncJob < VCAP::CloudController::Jobs::ReoccurringJob
       MAX_RETRIES = 3
       attr_reader :warnings
@@ -48,6 +51,8 @@ def perform
           fail_and_raise!(err.message) unless restart_on_failure?
 
           restart_job(err.message || 'no error description returned by the broker')
+        rescue OperationAborted
+          aborted!(service_instance.last_operation&.type)
         rescue => err
           fail!(err)
         end
@@ -102,6 +107,8 @@ def execute_request(client)
       def raise_if_cannot_proceed!
         last_operation_type = service_instance.last_operation&.type
 
+        return if operation_type == 'delete' && last_operation_type == 'create'
+
         if service_instance.operation_in_progress? && last_operation_type != operation_type
           aborted!(last_operation_type)
         end

spec/request/service_instances_spec.rb

@@ -2528,17 +2528,6 @@ def check_filtered_instances(*instances)
           to_return(status: broker_status_code, body: broker_response.to_json, headers: {})
       }
 
-      it 'sets the service instance last operation to delete in progress' do
-        api_call.call(admin_headers)
-        expect(last_response).to have_status_code(HTTP::Status::ACCEPTED)
-
-        instance.reload
-
-        expect(instance.last_operation).to_not be_nil
-        expect(instance.last_operation.type).to eq('delete')
-        expect(instance.last_operation.state).to eq('in progress')
-      end
-
       it 'responds with job resource' do
         api_call.call(admin_headers)
         expect(last_response).to have_status_code(202)
@@ -2655,6 +2644,17 @@ def check_filtered_instances(*instances)
             expect(Delayed::Job.count).to eq(1)
           end
 
+          it 'sets the service instance last operation to delete in progress' do
+            api_call.call(admin_headers)
+            execute_all_jobs(expected_successes: 1, expected_failures: 0)
+
+            instance.reload
+
+            expect(instance.last_operation).to_not be_nil
+            expect(instance.last_operation.type).to eq('delete')
+            expect(instance.last_operation.state).to eq('in progress')
+          end
+
           context 'when last operation eventually returns `delete succeeded`' do
             before do
               stub_request(:get, "#{instance.service_broker.broker_url}/v2/service_instances/#{instance.guid}/last_operation").
@@ -2983,6 +2983,86 @@ def check_filtered_instances(*instances)
           expect(response).to include('detail' => include('Service instances must be unshared before they can be deleted.'))
         end
       end
+
+      context 'when the creation is still in progress' do
+        before do
+          instance.save_with_new_operation({}, {
+            type: 'create',
+            state: 'in progress',
+            broker_provided_operation: 'some create operation'
+          })
+        end
+
+        context 'and the broker confirms the deletion' do
+          it 'deletes the service instance' do
+            api_call.call(admin_headers)
+            execute_all_jobs(expected_successes: 1, expected_failures: 0)
+
+            expect(VCAP::CloudController::ServiceInstance.first(guid: instance.guid)).to be_nil
+          end
+        end
+
+        context 'and the broker accepts the delete' do
+          let(:broker_status_code) { 202 }
+          let(:broker_response) { { operation: 'some delete operation' } }
+          let(:last_operation_response) { { state: 'in progress', description: 'deleting si' } }
+          let(:last_operation_status_code) { 200 }
+
+          before do
+            stub_request(:get, "#{instance.service_broker.broker_url}/v2/service_instances/#{instance.guid}/last_operation").
+              with(
+                query: {
+                  operation: 'some delete operation',
+                  service_id: instance.service.broker_provided_id,
+                  plan_id: instance.service_plan.broker_provided_id
+                }).
+              to_return(status: last_operation_status_code, body: last_operation_response.to_json, headers: {})
+          end
+
+          it 'triggers the delete process' do
+            api_call.call(admin_headers)
+            expect(last_response).to have_status_code(HTTP::Status::ACCEPTED)
+            execute_all_jobs(expected_successes: 1, expected_failures: 0)
+
+            instance.reload
+
+            expect(instance.last_operation).to_not be_nil
+            expect(instance.last_operation.type).to eq('delete')
+            expect(instance.last_operation.state).to eq('in progress')
+            expect(instance.last_operation.broker_provided_operation).to eq('some delete operation')
+          end
+        end
+
+        context 'but the broker rejects the delete' do
+          let(:broker_status_code) { 422 }
+          let(:broker_response) { { error: 'ConcurrencyError', description: 'Cannot delete right now' } }
+
+          it 'responds with an error' do
+            api_call.call(admin_headers)
+            expect(last_response).to have_status_code(HTTP::Status::ACCEPTED)
+            execute_all_jobs(expected_successes: 0, expected_failures: 1)
+
+            job = VCAP::CloudController::PollableJobModel.last
+            expect(job.state).to eq(VCAP::CloudController::PollableJobModel::FAILED_STATE)
+
+            expect(job.cf_api_error).to_not be_nil
+            api_error = YAML.safe_load(job.cf_api_error)['errors'].first
+            expect(api_error['title']).to eql('CF-UnableToPerform')
+            expect(api_error['detail']).to eql('delete could not be completed: create in progress')
+          end
+
+          it 'does not change the operation in progress' do
+            api_call.call(admin_headers)
+            expect(last_response).to have_status_code(HTTP::Status::ACCEPTED)
+            execute_all_jobs(expected_successes: 0, expected_failures: 1)
+
+            instance.reload
+
+            expect("#{instance.last_operation.type} #{instance.last_operation.state}").to eq('create in progress')
+            expect(instance.last_operation.broker_provided_operation).to eq('some create operation')
+          end
+        end
+      end
     end
 
     context 'when associations are not empty' do

spec/unit/actions/v3/service_instance_delete_spec.rb

@@ -63,15 +63,6 @@ module CloudController
         context 'managed service instances' do
           let!(:service_instance) { VCAP::CloudController::ManagedServiceInstance.make }
 
-          it 'locks the service instance' do
-            action.delete(service_instance)
-
-            lo = service_instance.reload.last_operation
-            expect(lo).to be
-            expect(lo.type).to eq('delete')
-            expect(lo.state).to eq('in progress')
-          end
-
           it 'enqueues a job and returns the job guid' do
             job_guid = subject.delete(service_instance)
             job = VCAP::CloudController::PollableJobModel.last

spec/unit/jobs/v3/delete_service_instance_job_spec.rb

@@ -22,7 +22,7 @@ module V3
       end
 
       describe '#perform' do
-        let(:client) { double('BrokerClient', deprovision: 'some response') }
+        let(:client) { double('BrokerClient', deprovision: { instance: {}, last_operation: {} }) }
 
         before do
           allow(VCAP::Services::ServiceClientProvider).to receive(:provide).and_return(client)
@@ -44,6 +44,16 @@ module V3
           end
         end
 
+        context 'when there is a create operation in progress' do
+          before do
+            service_instance.save_with_new_operation({}, { type: 'create', state: 'in progress', description: 'barz' })
+          end
+
+          it 'attempts to delete anyways' do
+            expect { subject.perform }.to_not raise_error
+          end
+        end
+
         context 'when the client raises a ServiceBrokerBadResponse' do
           let(:r) do
             VCAP::Services::ServiceBrokers::V2::HttpResponse.new(code: '204', body: 'unexpected failure!')
@@ -100,6 +110,48 @@ module V3
           end
         end
 
+        context 'when the client raises an API Error' do
+          before do
+            allow(client).to receive(:deprovision).and_raise(err)
+            service_instance.save_with_new_operation({}, {
+              type: 'create',
+              state: 'in progress',
+              broker_provided_operation: 'some create operation'
+            })
+          end
+
+          let(:err) do
+            CloudController::Errors::ApiError.new_from_details('NotFound')
+          end
+
+          it 'fails the job and update the service instance last operation' do
+            expect { subject.perform }.to raise_error(CloudController::Errors::ApiError, /Unknown request/)
+            expect(subject.instance_variable_get(:@attempts)).to eq(0)
+
+            service_instance.reload
+
+            expect(service_instance.last_operation.type).to eq('delete')
+            expect(service_instance.last_operation.state).to eq('failed')
+          end
+
+          context 'and the error name is AsyncServiceInstanceOperationInProgress' do
+            let(:err) do
+              CloudController::Errors::ApiError.new_from_details('AsyncServiceInstanceOperationInProgress', 'some name')
+            end
+
+            it 'fails the job but do not update the service instance last operation' do
+              expect { subject.perform }.to raise_error(CloudController::Errors::ApiError, /create in progress/)
+              expect(subject.instance_variable_get(:@attempts)).to eq(0)
+
+              service_instance.reload
+
+              expect(service_instance.last_operation.type).to eq('create')
+              expect(service_instance.last_operation.state).to eq('in progress')
+              expect(service_instance.last_operation.broker_provided_operation).to eq('some create operation')
+            end
+          end
+        end
+
         context 'when the client raises a general error' do
           let(:err) { StandardError.new('random error') }
 
@@ -163,6 +215,15 @@ module V3
           end
         end
 
+        context 'when the client raises a AsyncServiceInstanceOperationInProgress' do
+          it 'raises a DeprovisionBadResponse error' do
+            err = CloudController::Errors::ApiError.new_from_details('AsyncServiceInstanceOperationInProgress', 'some instance name')
+            allow(client).to receive(:deprovision).and_raise(err)
+
+            expect { subject.send_broker_request(client) }.to raise_error(OperationAborted, /rejected the request/)
+          end
+        end
+
         context 'when the client raises an unknown error' do
           it 'raises the error' do
             allow(client).to receive(:deprovision).and_raise(RuntimeError.new('oh boy'))