Add checks for retirement of correct child service in bundle #284

d-m-u · 2018-04-19T16:03:32Z

We should only retire service bundle children that have resource_type of service and that have a parent.

d-m-u · 2018-04-19T16:03:47Z

@tinaafitz

d-m-u · 2018-04-19T16:05:08Z

@miq_bot add_label bug

tinaafitz

@d-m-u Looks good. We'll need to add tests in the ManageIQ PR for retire_service_resources.
@mkanoor Please review.

d-m-u · 2018-04-20T14:20:42Z

@miq-bot add_label enhancement

tinaafitz

@d-m-u Looks good.

tinaafitz · 2018-05-02T20:11:02Z

bump @mkanoor @gmcculloug

mkanoor · 2018-05-07T15:32:12Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

@@ -14,6 +14,7 @@

 service.service_resources.each do |sr|
  next if sr.resource.nil?
+  next if sr.resource_type == "Service" && $evm.vmdb(:service).find(sr[:resource_id]).has_parent


@d-m-u
Can we add a spec for this.

d-m-u · 2018-05-10T17:50:42Z

@tinaafitz could you take another look for me at this please?

tinaafitz · 2018-05-10T18:31:57Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

+              end
+
+              def main
+                service = $evm.root['service']


@d-m-u You might want to have a method for service, something like this:

def service @handle.root["service"].tap do |service| if service.nil? @handle.log(:error, 'Service is nil') raise 'Service not found' end end end

tinaafitz · 2018-05-10T18:59:36Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

+                if sr.resource.retired?
+                  @handle.log('info', "resource: #{sr.resource.name} is already retired.")
+                else
+                  result = 'retry'


@d-m-u You could put the log message first here, then return retry

@handle.log('info', "resource: #{sr.resource.name} is not retired, setting retry.") 'retry'

tinaafitz · 2018-05-10T19:01:47Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb


-$evm.log('info', "Checking if all service resources have been retired.")
+                @handle.log('info', "Service: #{service.name} Resource retirement check returned <#{result}>")
+                case result


@d-m-u You could put the result logic in a method instead of doing it inline here.

d-m-u · 2018-05-15T13:32:31Z

@tinaafitz uno mas, por favor

mkanoor · 2018-05-15T13:42:47Z

...eIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired_spec.rb

+  let(:retired_vm) { FactoryGirl.create(:vm, :retired => true) }
+  let(:root_object) do
+    Spec::Support::MiqAeMockObject.new('service'             => svc_service,
+                                       'service_retire_task' => task,


@d-m-u
Everything fed into Automate is a Service Object. service_retire_task should point to svc_task instead of task

mkanoor · 2018-05-15T13:45:17Z

...eIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired_spec.rb

+      expect { described_class.new(ae_service).main }.to raise_error('Service not found')
+    end
+  end
+end


@d-m-u What is the coverage on the method after you run the spec?

The only line I'm missing is the one at the end that I haven't had time to finish yet, yarps.

mkanoor · 2018-05-15T14:04:05Z

...eIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired_spec.rb

+    it "check" do
+      service.service_resources << FactoryGirl.create(:service_resource, :resource_type => "VmOrTemplate", :service_id => service.id, :resource_id => vm.id)
+      service.save
+      allow(svc_service).to receive(:check_retired)


@d-m-u
Should the check_retired be returning a true/false and exhibiting different behaviour based on some earlier setting.

d-m-u · 2018-05-17T11:42:39Z

@tinaafitz bump

tinaafitz · 2018-05-17T16:38:45Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

+              end
+
+              def main
+                service


@d-m-u Could we set a local service attribute here so we can pass it into the check_service_resource_retirement and result methods?

tinaafitz · 2018-05-17T16:39:15Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

+                  next if sr.resource.nil?
+                  next if sr.resource_type == "Service" && @handle.vmdb(:service).find(sr[:resource_id]).has_parent
+                  next unless sr.resource.respond_to?(:retired?)
+                  result = check_service_resource_retirement(sr)


@d-m-u Can we pass the service argument here?

tinaafitz · 2018-05-17T16:39:37Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

+                end
+
+                @handle.log('info', "Service: #{service.name} Resource retirement check returned <#{result}>")
+                result(result)


@d-m-u Can we pass the service argument to result?

tinaafitz · 2018-05-17T16:40:01Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

-  else
-    result = 'retry'
-    $evm.log('info', "resource: #{sr.resource.name} is not retired, setting retry.")
+              def result(param)


@d-m-u Can we add the service arg here?

tinaafitz · 2018-05-17T16:40:11Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb


-result = 'ok'
+              def check_service_resource_retirement(sr)


@d-m-u Can we add the service arg here?

mkanoor · 2018-05-21T14:00:14Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb


-$evm.log('info', "Checking if all service resources have been retired.")
+              def service_obj


@d-m-u

def service @service ||= @handle.root['service'].tap do |obj| if obj.nil? @handle.log(:error, 'Service object not provided') raise 'Service object has not been provided' end end end

If you did it this way it will cache the service object in an instance variable once after all the validation and for subsequent calls you can just use service every where.

You can remove the
service = service_obj line
Not pass the service into check_retired_state

mkanoor · 2018-05-21T14:01:49Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

+                  next if sr.resource.nil?
+                  next if sr.resource_type == "Service" && @handle.vmdb(:service).find(sr[:resource_id]).has_parent
+                  next unless sr.resource.respond_to?(:retired?)
+                  result = check_service_resource_retirement(sr, service)


@d-m-u
Since this is in a loop is the result changing for each iteration, what happens if the first one has an error and the second one has a success, we will report the success but not the first failure

mkanoor · 2018-05-22T13:55:50Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb


-result = 'ok'
+              def check_service_resource_retirement(sr)
+                @handle.log('info', "Checking if service resource for service: #{@service.name} resource ID: #{sr.id} is retired")


@d-m-u this should just be service instead of @service.

d-m-u · 2018-05-23T16:14:09Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

-result = 'ok'
+              def check_service_resource_retirement(sr)
+                @handle.log('info', "Checking if service resource for service: #{service.name} resource ID: #{sr.id} is retired")
+                if sr.resource.retired?


@tinaafitz do we need an error check here? Madhu seems to think so.

@mkanoor What type of checking would you want here? The max_retries would stop this eventually.

tinaafitz · 2018-05-23T16:36:27Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

+                  next unless sr.resource.respond_to?(:retired?)
+                  result = check_service_resource_retirement(sr)
+                  if result == 'retry'
+                    check_retired_state(result)


@d-m-u Since we know that the result == retry, we can set the retry attributes and log message here.
@handle.log('info', "Service: #{service.name} resource is not retired, setting retry.")
@handle.root['ae_result'] = 'retry'
@handle.root['ae_retry_interval'] = '1.minute'

tinaafitz · 2018-05-23T16:38:20Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

+                    return result
+                  else
+                    @handle.log('info', "Service: #{service.name} Resource retirement check returned <#{result}>")
+                    check_retired_state(result)


@d-m-u Since we know the result != retry, we can add the log message and set the result here:
@handle.log('info', "All resources are retired for service: #{service.name}. ")
@handle.root['ae_result'] = 'ok'

You had originally told me to break it out into separate methods.

tinaafitz · 2018-05-23T16:39:44Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

-result = 'ok'
+              def check_service_resource_retirement(sr)
+                @handle.log('info', "Checking if service resource for service: #{service.name} resource ID: #{sr.id} is retired")
+                if sr.resource.retired?


@mkanoor What type of checking would you want here? The max_retries would stop this eventually.

tinaafitz · 2018-05-23T16:43:19Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

+                  result = check_service_resource_retirement(sr)
+                  if result == 'retry'
+                    check_retired_state(result)
+                    return result


@d-m-u I don't think we need to return result here.

mkanoor · 2018-05-23T16:07:19Z

...eIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired_spec.rb

+                                       'service_action'      => 'Retirement')
+  end
+  let(:ae_service) { Spec::Support::MiqAeMockService.new(root_object) }
+


@d-m-u
Can you please add this test

it_behaves_like "automate_engine_call", domain_file

mkanoor · 2018-05-23T18:52:05Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb


-result = 'ok'
+              def check_all_service_resources


@d-m-u
Can we try something like this

def check_all_service_resources if all_service_resources_done? @handle.log('info', "Service: #{service.name} Resource retirement check returned <#{result}>") @handle.log('info', "All resources are retired for service: #{service.name}. ") @handle.root['ae_result'] = 'ok' else @handle.log('info', "Service: #{service.name} resource is not retired, setting retry.") @handle.root['ae_result'] = 'retry' @handle.root['ae_retry_interval'] = '1.minute' end end def check_service_resource_retirement(sr) return 'ignore' unless process?(sr) @handle.log('info', "Checking if service resource for service: #{service.name} resource ID: #{sr.id} is retired") if sr.resource.retired? @handle.log('info', "resource: #{sr.resource.name} is already retired.") 'ok' else @handle.log('info', "resource: #{sr.resource.name} is not retired, setting retry.") 'retry' end end def process?(sr) return false if sr.resource.nil? return false if sr.resource_type == "Service" && @handle.vmdb(:service).find(sr[:resource_id]).has_parent sr.resource.respond_to?(:retired?) end def all_service_resources_done? service.service_resources.all? { |sr| %w(ok ignore).include?(check_service_resource_retirement(sr)) } end

…ormat

mkanoor · 2018-05-29T14:30:24Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

+
+              def main
+                @handle.log('info', "Checking if all service resources have been retired.")
+                @result = 'ok'


@d-m-u
This instance variable is not getting used anymore, can we delete it.

tinaafitz · 2018-05-23T21:02:14Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

+
+              def main
+                @handle.log('info', "Checking if all service resources have been retired.")
+                @result = 'ok'


@d-m-u We can remove the @Result = 'ok'

tinaafitz · 2018-05-23T21:02:55Z

...ManageIQ/Service/Retirement/StateMachines/Methods.class/__methods__/check_service_retired.rb

+
+              def check_all_service_resources
+                if all_service_resources_done?
+                  @handle.log('info', "Service: #{service.name} Resource retirement check returned <#{@result}>")


@d-m-u Can we remove this log message? The next log message has all of the necessary information.

miq-bot · 2018-06-20T14:58:43Z

Checked commits d-m-u/manageiq-content@96ce7c6~...2ef160f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

tinaafitz approved these changes Apr 19, 2018

View reviewed changes

tinaafitz self-assigned this Apr 19, 2018

miq-bot added the enhancement label Apr 20, 2018

tinaafitz approved these changes May 2, 2018

View reviewed changes

mkanoor reviewed May 7, 2018

View reviewed changes

tinaafitz suggested changes May 10, 2018

View reviewed changes

mkanoor reviewed May 15, 2018

View reviewed changes

tinaafitz suggested changes May 17, 2018

View reviewed changes

mkanoor reviewed May 21, 2018

View reviewed changes

mkanoor reviewed May 22, 2018

View reviewed changes

d-m-u commented May 23, 2018

View reviewed changes

tinaafitz reviewed May 23, 2018

View reviewed changes

mkanoor reviewed May 23, 2018

View reviewed changes

Add checks for retirement of correct child service in bundle in new f…

96ce7c6

…ormat

mkanoor reviewed May 29, 2018

View reviewed changes

tinaafitz reviewed May 29, 2018

View reviewed changes

Test check_service_retired

2ef160f

mkanoor approved these changes Jul 9, 2018

View reviewed changes

tinaafitz approved these changes Jul 9, 2018

View reviewed changes

mkanoor merged commit d5d6329 into ManageIQ:master Jul 9, 2018

mkanoor added this to the Sprint 90 Ending Jul 16, 2018 milestone Jul 9, 2018


		$evm.log('info', "Checking if all service resources have been retired.")
		def service_obj

Add checks for retirement of correct child service in bundle #284

Add checks for retirement of correct child service in bundle #284

Conversation

d-m-u commented Apr 19, 2018

d-m-u commented Apr 19, 2018

d-m-u commented Apr 19, 2018

tinaafitz left a comment

Choose a reason for hiding this comment

d-m-u commented Apr 20, 2018

tinaafitz left a comment

Choose a reason for hiding this comment

tinaafitz commented May 2, 2018

Choose a reason for hiding this comment

d-m-u commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-m-u commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-m-u commented May 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jun 20, 2018