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

bugfix: trailing "...done" in rabbitmq output #24511

Merged
merged 1 commit into from Jun 11, 2015

Conversation

Projects
None yet
5 participants
@jquast
Contributor

jquast commented Jun 8, 2015

Problem

Some versions of rabbitmq (v3.4.1) do not output any final "...done" line in output of listings, but the given salt code unconditionally removed the final line of output, which may often result in inconsistent behavior of dependent state functions, where the final line has a value that is significant.

Solution

This bugfix changes this output manipulation to be conditional, matching only lines of "Listing ..." as the first, and "...done" as the last.

Details

In my environment::

[rabbitmq@db01 ~]$ rabbitmqctl list_vhosts
Listing vhosts ...
/

This causes issues in, for example, a vhost is attempted to be created for '/' that already exists, if that vhost happens to be the last one listed in command output, it is thought non-existant, resulting in state failure::

Failure: rabbitmq_vhost_|-rabbitmq-vhost_|-/_|-present: Creating vhost "/" ...
Error: vhost_already_exists: /
@joejulian

This comment has been minimized.

Contributor

joejulian commented Jun 8, 2015

👍

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jun 9, 2015

Thanks @jquast - tests too! :) It looks like you have some minor pylint errors. Can you grab those really quickly?

https://jenkins.saltstack.com/job/salt-pr-lint-n/6415/

Also, it looks like your changes are affecting some of the other tests. Can you take a look at those as well? If you get stuck let us know and we can help out. Thanks again!

@jquast

This comment has been minimized.

Contributor

jquast commented Jun 9, 2015

no problem

@jquast

This comment has been minimized.

Contributor

jquast commented Jun 9, 2015

I force pushed amendments and an additional negative test of an existing.

Awaiting your jenkins. Apologize if it fails, unable test locally by HACKING.rst instructions, calling python setup.py test results in complete system crash !

bugfix: trailing "...done" in rabbitmq output
Problem
-----------

Some versions of rabbitmq (v3.4.1) does not output any final
"...done" line in output of listings, but the given salt code
unconditionally removed the final line of output, which may
often result in inconsistent behavior of dependent state
functions, where the final line has a value that is significant.

Solution
-----------

This bugfix changes this output manipulation to be conditional,
matching only lines of ``"Listing ..."`` as the first, and
``"...done"`` as the last.

Details
---------

In my environment::

    [rabbitmq@db01 ~]$ rabbitmqctl list_vhosts
    Listing vhosts ...
    /

This causes issues in, for example, a vhost is attempted to
be created for '/' that already exists, if that vhost happens
to be the last one listed in command output, it is thought
non-existent, resulting in state failure::

    Failure: rabbitmq_vhost_|-rabbitmq-vhost_|-/_|-present: Creating vhost "/" ...
    Error: vhost_already_exists: /
@jquast

This comment has been minimized.

Contributor

jquast commented Jun 10, 2015

integration.states.pkgrepo.PkgrepoTest.test_pkgrepo_01_managed is the only remaining test failure, I don't believe it to be related.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jun 10, 2015

Awesome! Thanks very much @jquast!

thatch45 added a commit that referenced this pull request Jun 11, 2015

Merge pull request #24511 from jquast/rabbitmq-trailing-outp-fix
bugfix: trailing "...done" in rabbitmq output

@thatch45 thatch45 merged commit 94067fd into saltstack:develop Jun 11, 2015

3 of 5 checks passed

default Merged build finished.
Details
jenkins/salt-pr-linode-cent7-n Salt PR - Linode CentOS 7 #495 — FAILURE
Details
jenkins/salt-pr-clone Salt PR - Clone Repository #6684 — SUCCESS
Details
jenkins/salt-pr-lint-n Salt PR - Code Lint #6464 — SUCCESS
Details
jenkins/salt-pr-rs-ubuntu14.04-n Salt PR - RS Ubuntu 14 #6027 — SUCCESS
Details

@jquast jquast deleted the jquast:rabbitmq-trailing-outp-fix branch Jun 15, 2015

@jquast

This comment has been minimized.

Contributor

jquast commented Jul 14, 2015

Hello @rallytime,

Would you accept any further pull requests to put this in the 201x. release branches?

I feel everybody may benefit from this bugfix.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 14, 2015

@jquast Definitely. I'll mark if for backproting. I'll look at getting the backport in either later today or tomorrow. Thanks!

@jquast

This comment has been minimized.

Contributor

jquast commented Jul 14, 2015

Would it expedite if I prepared them for you? There are many conflicts: the tests do not exist in older versions, and at least 1 merge conflict in 2014.1 branch. I would be happy to perform the effort if it helps you.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 14, 2015

@jquast That would definitely be helpful! I'd recommend backporting this to the 2015.5 branch, as the 2014.1 and 2014.7 branches are retired and no longer on a release path.

@jquast

This comment has been minimized.

Contributor

jquast commented Jul 14, 2015

@rallytime nevermind regarding 2014.7, this bugfix present in 2015.8. I am submitting PR for 2015.5.

(i just helped bugfix the 2014.7 branch this past day due to a mis-backportted bugfix made the previous by your staff, which made me question why this bugfix is not included in the branch: if 2014.7 really is inactive, that's ok.)

@jquast

This comment has been minimized.

Contributor

jquast commented Jul 14, 2015

2015.8 only backport submitted as #25426

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 14, 2015

@jquast There are still a couple of people submitting changes to 2014.7 and we are still doing "merge-forwards" from that branch. If people choose to pull from the branch, then they can. The difference is that we just won't be releasing from that branch any more. Our current release branch is 2015.5. Anyway, thanks for taking care of that. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment