Add retireable check to service ansible playbooks

[d-m-u]

We are currently creating the service retire tasks before we send them to automate, which means that if the parent service is an ansible playbook service with config options specifying the resources to not be retired, we will have already created tasks for the retirement of said resources, and will not honor the playbook config options hash. This cuts off the task creation for APS's early if the config option to not retire resources is present.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1692941

[d-m-u]

@miq-bot add_label bug
@miq-bot add_label hammer/yes

[d-m-u]

@miq-bot assign @gmcculloug

[d-m-u]

@miq-bot add_reviewer @tinaafitz
@miq_bot add_reviewer @mkanoor

[gmcculloug]

Two thoughts:

1. Compare the class object instead of the class name string: parent_svc.kind_of?(ServiceAnsiblePlaybook)
2. Instead of using a complex unless statement, which we try to avoid, move this into a separate private method retire_subtasks? which can be tested independently.

Will make this line easy to read:
create_retire_subtasks(parent_svc, self) if retire_subtasks?

[gmcculloug]

@tinaafitz Is this correct? The Service is considered non-retireable if we are retaining resources? I thought we would still run the playbook if it was configured and this feels like we would not do that.

[tinaafitz]

@gmcculloug The service_resources for ansible services are considered non-retireable if we are retaining resources. The service_retire_task has already been created, so it's only preventing the service_resources from potentially creating retire tasks.

[gmcculloug]

So discussed more with @tinaafitz and this does not feel correct. If you ask the service instance if it can be retired the answer should not be solely used to drive if child resources are retired, it should be answering the question for it self. Another way of looking at it would be if the UI was using this method to determine if the service itself could be retired. I think we want the response to be true regardless of the child-items.

Seems like the retain_resources_on_retirement is doing more what we want and this method is not needed.

[d-m-u]

I was under the impression that at the end of the conversation I and you and Madhu had in the kitchen that the proposed solution then was to use this retireable? check from the CiFeatureMixin to determine whether the service was retireable. If we're not using this approach I think we should have the discussion that Jason kinda started on last week about how checking to see if something is retireable requires both a general check like those used by the SupportsFeatureMixin and something more specific because since this issue is related it'd be easier to have that discussion sooner.

[gmcculloug]

Realized I already commented on this line, but looking at the options it seems like it is just:
options.fetch_path(:config_info, :retirement, :remove_resources).to_s.starts_with("no_")

Not a required change, but seems simpler and maybe more future proof.

[gmcculloug]

I think this method should call retain_resources_on_retirement? based on comments about retireable?. It will need to handle if the parent_service exposes this method.

[gmcculloug]

I suggest trying to make these method names more generic. Also the currently naming is misleading, you are not retaining subtasks, you are retaining resources of the service by skipping the subtasks that would retire them.

def create_subtasks?(parent_svc)
  !parent_svc.try(:retain_resources_on_retirement?)
end

This makes the caller above easier to read as it would change to:
if create_subtasks?(parent_svc)

[gmcculloug]

With the suggested change to the "create_subtasks?" method this would be easier to read as:
if create_subtasks?(parent_svc)

[gmcculloug]

@tinaafitz @lfu Please review

[lfu]

The logic here seems not right.

describe '#retain_resources_on_retirement?' do
    context "no_with_playbook" do
      let(:remove_resources) { 'no_with_playbook' }
      let(:can_children_be_retired?) { false }
      it_behaves_like "#retain_resources_on_retirement"

[lfu]

When resource is to be retained, then children can not be retired.

expect(service.retain_resources_on_retirement?).to eq(!can_children_be_retired?)

[lfu]

Shared example can be used for above two cases.

[lfu]

Same here.

[lfu]

Same here.

[lfu]

Same here.

[lfu]

Same here.

[miq-bot]

Checked commit d-m-u@0cf67b4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 👍

[d-m-u]

@lfu please re-review

[simaishi]

@d-m-u There is a conflict backporting this to hammer branch due to refactoring that happened in #18057. Can you please create a separate PR for hammer?

File: app/models/service_ansible_playbook.rb

 +  def manageiq_connection_env
      {
++<<<<<<< HEAD
 +      'url'         => api_url,
 +      'token'       => api_token,
 +      'X_MIQ_Group' => evm_owner.current_group.description
 +    }
 +  end
 +
 +  def api_token
 +    @api_token ||= Api::UserTokenService.new.generate_token(evm_owner.userid, 'api')
 +  end
 +
 +  def api_url
 +    @api_url ||= MiqRegion.my_region.remote_ws_url
++=======
+       'service' => href_slug,
+       'action'  => action
+     }.merge(manageiq_env(evm_owner, miq_group, miq_request_task))
+       .merge(request_options_extra_vars)
++>>>>>>> ff41ba27ab... Merge pull request #18609 from d-m-u/adding_remove_resource_method_to_service_template_ansible_playbook

[d-m-u]

sure @simaishi right away ma'am

[simaishi]

Backported to Hammer via #18620