Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Idea of managing errors while creating the objects in controller #862

Merged
merged 14 commits into from
Aug 22, 2024

Conversation

przemkalit
Copy link
Contributor

What does this PR do?

This PR proposes a method to manage errors while creating objects in the controller.

Background: I was testing the collection in our environment and encountered an annoying issue. While restoring objects from files created with filetree_create, the dispatch role stopped every time it encountered a problem, such as when an object was missing something (e.g., a playbook could not be found in the project).

I came up with a block and rescue directive for the async task. The rescue block extracts errors from the result file and puts them into a list of dicts, which is then set as an artifact.

If you like this idea, I will introduce it to every object in the collection. However, if you have a different approach to this problem, I am open to suggestions.

How should this be tested?

  1. Create job_templates which playbook does not exist in the project in target controller.
  2. Export with filetree_create.
  3. Execute playbook:
- name: Restore objects
   hosts: all 
   tasks:
     - name: Include vars from path directory
       ansible.builtin.include_vars:
         dir: '/path'
         extensions:
           - "yml"
           - "yaml"
      
     - ansible.builtin.include_role: infra.controller_configuration.filetree_read
      
     - ansible.builtin.include_role: infra.controller_configuration.dispatch

Is there a relevant Issue open for this?

N/A

Other Relevant info, PRs, etc

N/A

@djdanielsson
Copy link
Collaborator

It's an interesting idea, we will have to do quite a bit of testing to ensure this doesn't break anything. When I have some time I will try it out and give more detailed feedback

roles/.DS_Store Outdated Show resolved Hide resolved
Copy link
Collaborator

@Tompage1994 Tompage1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think this is good and may be useful. I don't think it will slow down execution and I believe it to be scalable.

I haven't tested this on a live system.

It would be good though if you could have a go at adding into our CI testing. Get a JT which will fail and assert that the output is correct

roles/job_templates/tasks/async.yml Outdated Show resolved Hide resolved
roles/job_templates/tasks/async.yml Show resolved Hide resolved
@przemkalit
Copy link
Contributor Author

Overall I think this is good and may be useful. I don't think it will slow down execution and I believe it to be scalable.

I haven't tested this on a live system.

It would be good though if you could have a go at adding into our CI testing. Get a JT which will fail and assert that the output is correct

Ok I will work on JT.

@przemkalit
Copy link
Contributor Author

Please let me know if the test provided by me is sufficient or should I make some changes.

@Tompage1994
Copy link
Collaborator

Please let me know if the test provided by me is sufficient or should I make some changes.

The testing you've done looks good but is unfortunately in the wrong place. Could you add it to the following section:

https://github.com/redhat-cop/controller_configuration/blob/devel/tests/configure_controller.yml

Copy link
Collaborator

@Tompage1994 Tompage1994 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that looked better.

I've just done a quick refactor of the test and pushed it to this PR to simplify it a little bit. If you're happy with that then I'm happy to merge this

@przemkalit
Copy link
Contributor Author

Thanks for the refactor, so as you approved this kind of error handling, could I add this change to the workflows, or should I added with a new PR?

@Tompage1994
Copy link
Collaborator

@przemkalit Ok, I've made another change to the testing because we don't set the variables as we are setting stat rather than vars.

There is a bigger conversation to be had here about how we want to handle these errors, whether we want to fail the playbook by default (I think we may want to add in a variable here to set to fail on error. As it is this will continue on error)

In terms of creating the same on other roles, I would hold off for now. It may all go through this PR or could be separate but we need to decide how we want this to go for now. This will now sit here for a bit whilst we internally discuss and decide on direction.

Thanks for the contribution

@przemkalit przemkalit requested a review from a team as a code owner July 26, 2024 13:22
@przemkalit
Copy link
Contributor Author

Hi, I've added small change which is related to existence of variables in eg. extra_vars.

@djdanielsson
Copy link
Collaborator

update the branch and I will merge it

@djdanielsson
Copy link
Collaborator

@Tompage1994 ready to merge it?

@djdanielsson
Copy link
Collaborator

@Tompage1994 reminder, are you good with this now?

@Tompage1994
Copy link
Collaborator

@Tompage1994 reminder, are you good with this now?

Yes I'm good with it. We will want to look into replicating this across all the roles

@djdanielsson djdanielsson merged commit 244bed7 into redhat-cop:devel Aug 22, 2024
10 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants