controller_configuration icon indicating copy to clipboard operation
controller_configuration copied to clipboard

Error handling for controller objects

Open przemkalit opened this issue 1 year ago • 3 comments

What does this PR do?

This PR is related to #984.

I've prepared the changes for all controller objects. What is different that the default behaviour stay mostly the same, unless variable collect_logs is set to true.

Known issue:

  • role name is not available in ['invocation']['module_args'], I believe it is minor bug in the module itself.

How should this be tested?

Manually

Is there a relevant Issue open for this?

resolves #984

Other Relevant info, PRs, etc

N/A

przemkalit avatar Dec 18 '24 13:12 przemkalit

There are a couple errors in the markdown linter and also sanity checks are failing, can you try and fix those. I think if we were to accept this we would also want it to be more then just controller related objects, but I will wait to hear feedback from others to make sure they are happy with the current implementation before asking you to continue the work to the rest of the roles. @Tompage1994 @sean-m-sullivan

Sure if I could, but currently we don't have AAP 2.5 so I only worked on objects that I could test, and we still don't use EDA.

przemkalit avatar Dec 18 '24 14:12 przemkalit

looks like there are some markdown lint errors

djdanielsson avatar Jan 17 '25 14:01 djdanielsson

I've added support for other objects, but I didn't tested, because I don't have access to AAP 2.5.

przemkalit avatar Apr 22 '25 11:04 przemkalit

I am kinda interested in the idea of finishing this up and merging it, @przemkalit have you gotten a 2.5 instance yet to test on? @sean-m-sullivan and @Tompage1994 outside of testing this what other thoughts do you both have on it?

djdanielsson avatar Jul 21 '25 18:07 djdanielsson

It would be great for this to be merged. I was just looking at doing something similar for the infra.controller_configuration collection. So if this gets merged I use this a base for enabling the same functionality in that collection. (IHAC that is looking for this type of error handling, but is on AAP 2.4).

branic avatar Jul 21 '25 19:07 branic

Generally I'm happy enough with this.

At a glance it seems all the new async_* files are very similar and could perhaps be condensed into one meta role which performs the actions instead (with the correct vars passed in). That way we reduce duplication and make it easier to adapt in the future

Tompage1994 avatar Jul 22 '25 08:07 Tompage1994

I am kinda interested in the idea of finishing this up and merging it, @przemkalit have you gotten a 2.5 instance yet to test on? @sean-m-sullivan and @Tompage1994 outside of testing this what other thoughts do you both have on it?

Hey, sorry I don't have it, and I don't have any way to test it properly, so it would be on you guys.

przemkalit avatar Jul 22 '25 09:07 przemkalit

To explain more what I mean by my above comment, I have probably done all of what would be needed. It is untested though, just message app based coding...

So, I think each role now has all the same other than the variables they set essentially. So what I'd do is create a role and it would likely just have the single task file with something like this:

---
- name: async | await the results
  block:
    - name: async | Managing Controller Object | Wait for management to complete
      ansible.builtin.async_status:
        jid: "{{ __generic_job_async_results_item.ansible_job_id }}"
      register: __generic_job_async_result
      until: __generic_job_async_result.finished
      retries: "{{ __async_retries }}"
      delay: "{{ __async_delay }}"
  rescue:
    - name: async | Load error details
      ansible.builtin.include_vars:
        file: "{{ __generic_job_async_result.results_file }}"
        name: __error_data

    - name: async | Show error and stop execution
      when: not collect_logs
      ansible.builtin.fail:
        msg: "error: {{ __error_data['msg'] }}, response: {{ __error_data['response'] | default ('N/A') }}"

    - name: async | Building list of not processed objects
      ansible.builtin.set_fact:
        {{ error_list_var_name }}: "{{ lookup('ansible.builtin.vars', error_list_var_name) | default([]) + [
                                                                                {
                                                                                 'name': __error_data['invocation']['module_args']['name'] | default(''),
                                                                                 'error': __error_data['msg'] | default('')
                                                                                }
                                                                               ]
                                  }}"
...

Then when you would call that role you would do so with:

- name: Managing Controller Credentials | Wait for finish the credential management
  ansible.builtin.include_role: infra.aap_configuration.meta_error_handling
  loop: "{{ __credentials_job_async.results }}"
  loop_control:
    loop_var: __credentials_job_async_results_item
  vars:
    __generic_job_async_results_item: "{{ __credentials_job_async_results_item }}"
    error_list_var_name: __credentials_error_list

As I say, I've not tested but I'm pretty certain that would work the same and would significantly reduce the amount of code and files to maintain

Tompage1994 avatar Jul 24 '25 08:07 Tompage1994