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

ansiblegate: take care of failed, skipped and unreachable tasks and propagate "retcode" #58214

Merged
merged 15 commits into from
Aug 24, 2020

Conversation

meaksh
Copy link
Contributor

@meaksh meaksh commented Aug 17, 2020

What does this PR do?

This PR fixes few issues on the ansiblegate execution and state modules when running playbooks:

  • Take care of "failed", "skipped" and "unreachable" tasks during playbook execution.
  • Propagate "retcode" from "ansible-playbook" CLI execution when calling ansible.playbooks to allow proper success/failed detection.
  • Only execute ansible-playbook --check in test mode (test=True) to allow running playbooks that are using modules which does not allow --check.
  • Add new unit tests for execution and state modules.
  • Add changes suggested by "pre-commit".

What issues does this PR fix or reference?

Fixes: #57854
Fixes: #57855

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@meaksh meaksh requested a review from a team as a code owner August 17, 2020 09:47
@ghost ghost requested review from DmitryKuzmenko and removed request for a team August 17, 2020 09:47
NO_PYTEST = not bool(pytest)


@skipIf(NO_PYTEST, False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just cosmetic request to drop the redundant NO_PYTEST constant. Other tests are using pytest directly in skipIf

@skipIf(pytest is None, "PyTest is missing")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@DmitryKuzmenko
Copy link
Contributor

@meaksh thank you for contribution. Could you please follow up just one cosmetic change request?

@meaksh
Copy link
Contributor Author

meaksh commented Aug 20, 2020

@DmitryKuzmenko I've pushed the changes you requested. Thanks for reviewing! 👍

@meaksh meaksh requested a review from DmitryKuzmenko August 20, 2020 14:38
assert ret == {"completed": True, "timeout": 1200}


@patch("salt.utils.path.which", MagicMock(return_value=True))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please move this mock object into the function level scope. Having mocks in the module level scope could produce issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, good to know, thanks! I've pushed the requested changes 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wait, I missed few of them. 1 sec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, it should be ready now.

@DmitryKuzmenko
Copy link
Contributor

Looks good now. Thank you for update.
Let's wait for CI results.

@dwoz dwoz merged commit 01e8c0a into saltstack:master Aug 24, 2020
@meaksh meaksh deleted the master-ansiblegate-fixes-new branch August 25, 2020 07:42
@sagetherage sagetherage added the Magnesium Mg release after Na prior to Al label Sep 14, 2020
@denis-khalturin-incountry

@meaksh why are you marking all skipped tasks as outstanding? this behavior terminates the application of the state with an error

@meaksh
Copy link
Contributor Author

meaksh commented Nov 25, 2020

Hey @denis-khalturin-incountry, I don't really get what you mean. Could you please elaborate that and provide some examples?

Thanks in advance!

@denis-khalturin-incountry
Copy link

denis-khalturin-incountry commented Nov 25, 2020

@meaksh yes of course.

i am applying playbook:

ansible-playbook playbooks/test.yml --diff --extra-vars='{"ansible_connection": "local", "inventory_hostname": "_my_host_"}' --inventory=inventory --forks = 5

One of the tasks is skipped according to the conditions:

PLAY RECAP *********************************************** ************************************************* ************************************************* ************************************************* *************************
_my_host_: ok = 7 changed = 0 unreachable = 0 failed = 0 skipped = 1 rescued = 0 ignored = 0

But if I start playbook by applying state, I get an error when executing playbook:

salt-call -l debug state.apply
....
[INFO    ] Executing command 'ansible-playbook playbooks/test.yml --diff --extra-vars='{"ansible_connection": "local", "inventory_hostname": "_my_host_"}' --inventory=inventory --forks=5' in directory 'ansible'
....
[ERROR   ] {'all:!without_hardening': {'_my_role_ : _task_that_is_skipped_':
....
[INFO    ] Completed state [playbooks/test.yml] at time 11:51:25.176235 (duration_in_ms=6288.308)
....
----------
          ID: ansible playbook
    Function: ansible.playbooks
        Name: playbooks/test.yml
      Result: False
     Comment: There were some issues running the playbook playbooks/test.yml
     Started: 11:51:18.887927
    Duration: 6288.308 ms
     Changes:
....
Succeeded: 6 (changed=2)
Failed:    1

I think this is the wrong handling of skipped tasks

@meaksh
Copy link
Contributor Author

meaksh commented Nov 25, 2020

Thanks for clarifying @denis-khalturin-incountry ! You're totally right 👍

I did some checks here and can confirm that ansible-playbook CLI does not set "retcode != 0" when there are skipped tasks. Therefore we should not force the Salt state to fail in such cases.

Could you please create a new issue so we can hopefully produce a fix soon?

Thanks in advance!

@denis-khalturin-incountry

Yes, I will create an issue. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Magnesium Mg release after Na prior to Al
Projects
None yet
5 participants