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

Add any_errors_fatal to openstack playbooks #7225

Conversation

tzumainn
Copy link
Contributor

No description provided.

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 20, 2018
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 25, 2018
@tomassedovic tomassedovic self-assigned this Feb 26, 2018
@tomassedovic
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 26, 2018
@tomassedovic
Copy link
Contributor

Awesome, thanks! This is not necessary for the localhost jobs, only for the multihost plays, but there's no harm in leaving it there.

Anyway, it works great! Needs a rebase but we can merge it afterwards. Note that some of the plays have now moved under install and they still require any_errors_fatal, too.

@tzumainn tzumainn force-pushed the openstack-ansible-abort-on-failure branch from 2133eef to 712cb50 Compare February 26, 2018 17:02
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2018
@tzumainn
Copy link
Contributor Author

Thanks for the review! I've rebased and added any_errors_fatal to the install playbook.

@tomassedovic
Copy link
Contributor

So sadly, when I tested this, it didn't seem to work. But there's apparently a few ansible bugs about that, so I'm okay to merge this.

For the record, here's how I'm testing this. I've added the following to the provision playbook, right after the Populate DNS entries task:

  - os_server:
      name: app-node-0.openshift.example.com
      state: absent

It will delete an app server (the name is hardcoded so if your env is different, change this). The play should abort during the TASK [Detecting Operating System from ostree_booted]. In my testing, it continued way after that just skipping tasks related to the app node.

Apparently though, it's possible to enable this option globally in ansible.cfg as well:

any_errors_fatal = True

And that seem so be working fine. Would you mind adding that to the readme? Something along the lines of "It is recommended that you put any_errors_fatal = True which will abort the Ansible execution as soon as any error is encountered."

@tzumainn
Copy link
Contributor Author

I actually was going to just document the ansible.cfg change and leave it at that, but when I was testing - I did so by removing a nova server after the heat stack was created - the ansible.cfg change did nothing, while my changes worked.

any_errors_fatal only seems to work on the specific tasks I threw it on, so perhaps the reason your test didn't work is because you were expecting it to fail in the playbooks/init/basic_facts.yml playbook?

@tomassedovic
Copy link
Contributor

Interesting. Maybe our Ansible versions are different. The changes in this PR did nothing at all to me -- as if they weren't there. While the ansible.cfg version worked perfectly.

So let's do both.

@tzumainn
Copy link
Contributor Author

Fair enough! Out of curiosity, what's your ansible version?

@tomassedovic
Copy link
Contributor

@tzumainn: ansible 2.4.1.0

@tzumainn
Copy link
Contributor Author

@tomassedovic I'm using 2.4.2.0. Now I'm really confused...

@tomassedovic
Copy link
Contributor

Hm, so I updated to 2.4.2.0 but I keep seeing the same thing: ansible.cfg stops the play right after the failure, whereas without it, this patch keeps going for ages afterwards.

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 28, 2018
@tzumainn
Copy link
Contributor Author

@tomassedovic Okay, I've added a note to the README. I guess one of the billion options here will work, although it might be a different one for each person.

@tomassedovic
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2018
@tzumainn
Copy link
Contributor Author

/test gcp

2 similar comments
@tzumainn
Copy link
Contributor Author

/test gcp

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 1, 2018

/test gcp

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 1, 2018

/test install

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 1, 2018

/test containerized

@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 2, 2018

/test install

@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 2, 2018

/test gcp

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 5, 2018

/retest

1 similar comment
@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 5, 2018

/retest

@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 6, 2018

/retest

@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 6, 2018

/test install

4 similar comments
@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 6, 2018

/test install

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 7, 2018

/test install

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 7, 2018

/test install

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 7, 2018

/test install

@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 38a82a0 into openshift:master Mar 7, 2018
@openshift-ci-robot
Copy link

@tzumainn: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/system-containers bb20e09 link /test system-containers
ci/openshift-jenkins/containerized bb20e09 link /test containerized
ci/openshift-jenkins/extended_conformance_install_crio bb20e09 link /test crio
ci/openshift-jenkins/logging bb20e09 link /test logging

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants