V3 handle last operation on unbind route service 174748133 (cloudfoundry#1913)

* v3(servise): Handling of last operation responses on unbind request

* v3(services): Set that Last operation to failed on unbind job timeout

* v3(services): Make sure to set LO to failed on unbind job failures

* v3(services): Delete job succeds when binding is not found.

Updated the result from binding delete action and

* v3(services) job completes when it should

* Add missing test setup

* Use if/else to imporve code readability

Author: FelisiaM

app/actions/service_route_binding_delete.rb

@@ -4,9 +4,14 @@ class ServiceRouteBindingDelete
       class UnprocessableDelete < StandardError; end
 
       RequiresAsync = Class.new.freeze
-      DeleteComplete = Class.new.freeze
-      DeleteStarted = Struct.new(:operation).freeze
-      DeleteInProgress = Struct.new(:retry_after).freeze
+
+      DeleteStatus = Struct.new(:finished, :operation).freeze
+      DeleteStarted = ->(operation) { DeleteStatus.new(false, operation) }
+      DeleteComplete = DeleteStatus.new(true, nil).freeze
+
+      PollingStatus = Struct.new(:finished, :retry_after).freeze
+      PollingFinished = PollingStatus.new(true, nil).freeze
+      ContinuePolling = ->(retry_after) { PollingStatus.new(false, retry_after) }
 
       def initialize(service_event_repository)
         @service_event_repository = service_event_repository
@@ -18,14 +23,13 @@ def delete(binding, async_allowed:)
         operation_in_progress! if binding.service_instance.operation_in_progress?
 
         result = send_unbind_to_broker(binding)
-        case result
-        when DeleteStarted
-          update_last_operation(binding, operation: result[:operation])
-        when DeleteComplete
+        if result[:finished]
           perform_delete_actions(binding)
+        else
+          update_last_operation(binding, operation: result[:operation])
         end
 
-        result
+        return result
       rescue => e
         update_last_operation(binding, state: 'failed', description: e.message)
 
@@ -34,22 +38,23 @@ def delete(binding, async_allowed:)
 
       def poll(binding)
         client = VCAP::Services::ServiceClientProvider.provide(instance: binding.service_instance)
-        details = client.fetch_service_binding_last_operation(binding)
+        details = client.fetch_and_handle_service_binding_last_operation(binding)
         case details[:last_operation][:state]
         when 'in progress'
           update_last_operation(binding, description: details[:last_operation][:description])
-          DeleteInProgress.new(details[:retry_after])
+          return ContinuePolling.call(details[:retry_after])
         when 'succeeded'
           perform_delete_actions(binding)
-          DeleteComplete.new
+          return PollingFinished
         when 'failed'
           update_last_operation(binding, state: 'failed', description: details[:last_operation][:description])
-          DeleteComplete.new
+          raise LastOperationFailedState
         end
+      rescue LastOperationFailedState => e
+        raise e
       rescue => e
-        update_last_operation(binding, description: e.message)
-
-        DeleteInProgress.new(nil)
+        update_last_operation(binding, state: 'failed', description: e.message)
+        raise e
       end
 
       private
@@ -59,7 +64,7 @@ def poll(binding)
       def send_unbind_to_broker(binding)
         client = VCAP::Services::ServiceClientProvider.provide(instance: binding.service_instance)
         details = client.unbind(binding, nil, true)
-        details[:async] ? DeleteStarted.new(details[:operation]) : DeleteComplete.new
+        details[:async] ? DeleteStarted.call(details[:operation]) : DeleteComplete
       rescue => err
         raise UnprocessableDelete.new("Service broker failed to delete service binding for instance #{binding.service_instance.name}: #{err.message}")
       end

app/jobs/v3/delete_route_binding_job.rb

@@ -38,7 +38,7 @@ def resource_type
 
       def perform
         binding = route_binding
-        gone! unless binding
+        return finish if binding.nil?
 
         service_event_repository = VCAP::CloudController::Repositories::ServiceEventRepository::WithUserActor.new(@user_audit_info)
         action = V3::ServiceRouteBindingDelete.new(service_event_repository)
@@ -47,24 +47,43 @@ def perform
         if @first_time
           @first_time = false
           delete_result = action.delete(binding, async_allowed: true)
-          return finish if delete_result.is_a? V3::ServiceRouteBindingDelete::DeleteComplete
+          if delete_result[:finished]
+            return finish
+          end
         end
 
         polling_status = action.poll(binding)
-        case polling_status
-        when V3::ServiceRouteBindingDelete::DeleteComplete
-          finish
-        when V3::ServiceRouteBindingDelete::DeleteInProgress
-          unless polling_status.retry_after.nil?
-            self.polling_interval_seconds = polling_status.retry_after.to_i
-          end
+        if polling_status[:finished]
+          return finish
+        end
+
+        if polling_status[:retry_after].present?
+          self.polling_interval_seconds = polling_status[:retry_after]
         end
       rescue => e
+        if route_binding.reload.last_operation.state != 'failed'
+          save_failure(e.message)
+        end
         raise CloudController::Errors::ApiError.new_from_details('UnableToPerform', 'unbind', e.message)
       end
 
+      def handle_timeout
+        save_failure("Service Broker failed to #{operation} within the required time.")
+      end
+
       private
 
+      def save_failure(description)
+        route_binding.save_with_attributes_and_new_operation(
+          {},
+          {
+            type: operation_type,
+            state: 'failed',
+            description: description,
+          }
+        )
+      end
+
       def route_binding
         RouteBinding.first(guid: resource_guid)
       end
@@ -73,10 +92,6 @@ def compute_maximum_duration
         max_poll_duration_on_plan = route_binding.service_instance.service_plan.try(:maximum_polling_duration)
         self.maximum_duration_seconds = max_poll_duration_on_plan
       end
-
-      def gone!
-        raise CloudController::Errors::ApiError.new_from_details('ResourceNotFound', "The binding could not be found: #{resource_guid}")
-      end
     end
   end
 end

spec/request/service_route_bindings_spec.rb

@@ -943,7 +943,7 @@
           let(:broker_base_url) { service_instance.service_broker.broker_url }
           let(:broker_unbind_url) { "#{broker_base_url}/v2/service_instances/#{service_instance.guid}/service_bindings/#{binding.guid}" }
           let(:route_service_url) { 'https://route_service_url.com' }
-          let(:broker_status_code) { 200 }
+          let(:broker_unbind_status_code) { 200 }
           let(:broker_response) { {} }
           let(:query) do
             {
@@ -959,7 +959,7 @@
 
             stub_request(:delete, broker_unbind_url).
               with(query: query).
-              to_return(status: broker_status_code, body: broker_response.to_json, headers: {})
+              to_return(status: broker_unbind_status_code, body: broker_response.to_json, headers: {})
           end
 
           it 'sends an unbind request with the right arguments to the service broker' do
@@ -988,7 +988,7 @@
           end
 
           context 'when the unbind responds asynchronously' do
-            let(:broker_status_code) { 202 }
+            let(:broker_unbind_status_code) { 202 }
             let(:operation) { Sham.guid }
             let(:broker_response) { { operation: operation } }
             let(:broker_binding_last_operation_url) { "#{broker_base_url}/v2/service_instances/#{service_instance.guid}/service_bindings/#{binding.guid}/last_operation" }
@@ -1073,11 +1073,11 @@
               end
             end
 
-            it_behaves_like 'unbinding last operation response handling', 'delete'
+            it_behaves_like 'binding last operation response handling', 'delete'
           end
 
           context 'when the broker returns a failure' do
-            let(:broker_status_code) { 418 }
+            let(:broker_unbind_status_code) { 418 }
             let(:broker_response) { 'nope' }
 
             it 'does not remove the binding' do

spec/support/shared_examples/request/last_operation_response_handling.rb

@@ -85,92 +85,3 @@
     end
   end
 end
-
-# TODO: Merge these two shared examples into one in https://www.pivotaltracker.com/story/show/174748133
-RSpec.shared_examples 'unbinding last operation response handling' do |operation_type|
-  context 'failure http codes' do
-    valid_responses = [
-      { code: 400, body: { description: 'helpful message' } },
-      { code: 200, body: { state: 'failed', description: 'something went wrong' } }
-    ]
-
-    valid_responses.each do |response|
-      context "last operation response is #{response[:code]}" do
-        let(:state) { response[:state] }
-        let(:last_operation_status_code) { response[:code] }
-        let(:last_operation_body) { response[:body] }
-
-        it 'updates the binding and job to failed' do
-          execute_all_jobs(expected_successes: 1, expected_failures: 0)
-
-          binding.reload
-          expect(binding.last_operation.type).to eq(operation_type)
-          expect(binding.last_operation.state).to eq('failed')
-          expect(binding.last_operation.description).to eq(response[:body][:description])
-
-          expect(job.state).to eq(VCAP::CloudController::PollableJobModel::COMPLETE_STATE)
-        end
-      end
-    end
-  end
-
-  context 'keep polling http codes' do
-    [404, 500].each do |code|
-      context "last operation response is #{code}" do
-        let(:last_operation_status_code) { code }
-        let(:last_operation_body) { 'something awful' }
-
-        it 'continues polling' do
-          execute_all_jobs(expected_successes: 1, expected_failures: 0)
-
-          binding.reload
-          expect(binding.last_operation.type).to eq(operation_type)
-          expect(binding.last_operation.state).to eq('in progress')
-          expect(binding.last_operation.description).to include("Status Code: #{code}")
-
-          expect(job.state).to eq(VCAP::CloudController::PollableJobModel::POLLING_STATE)
-        end
-      end
-    end
-  end
-
-  context 'timeout' do
-    before do
-      stub_request(:get, broker_binding_last_operation_url).
-        with(query: hash_including({
-          operation: operation
-        })).to_timeout
-    end
-
-    it 'continues polling' do
-      execute_all_jobs(expected_successes: 1, expected_failures: 0)
-
-      binding.reload
-      expect(binding.last_operation.type).to eq(operation_type)
-      expect(binding.last_operation.state).to eq('in progress')
-
-      expect(job.state).to eq(VCAP::CloudController::PollableJobModel::POLLING_STATE)
-    end
-  end
-
-  context 'connection errors' do
-    [SocketError, Errno::ECONNREFUSED, RuntimeError].each do |error|
-      before do
-        stub_request(:get, broker_binding_last_operation_url).
-          with(query: hash_including({
-            operation: operation
-          })).to_raise(error)
-      end
-
-      it 'continues polling' do
-        execute_all_jobs(expected_successes: 1, expected_failures: 0)
-
-        binding.reload
-        expect(binding.last_operation.type).to eq(operation_type)
-        expect(binding.last_operation.state).to eq('in progress')
-
-        expect(job.state).to eq(VCAP::CloudController::PollableJobModel::POLLING_STATE)
-      end
-    end
-  end
-end