Skip to content

fix zmq hang#58364

Merged
dwoz merged 13 commits intosaltstack:masterfrom
cmcmarrow:fix_zmq_hang
Sep 30, 2020
Merged

fix zmq hang#58364
dwoz merged 13 commits intosaltstack:masterfrom
cmcmarrow:fix_zmq_hang

Conversation

@cmcmarrow
Copy link
Contributor

@cmcmarrow cmcmarrow commented Sep 2, 2020

What does this PR do?

stops zmq from deconstructing it self. zmq can deconstruct out of order

What issues does this PR fix or reference?

Fixes:
#57856
#57574

Merge requirements satisfied?

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

Commits signed with GPG?

Yes

@cmcmarrow cmcmarrow requested a review from a team as a code owner September 2, 2020 18:34
@ghost ghost requested review from waynew and removed request for a team September 2, 2020 18:34
@cmcmarrow cmcmarrow added Aluminium Release Post Mg and Pre Si Magnesium Mg release after Na prior to Al and removed Aluminium Release Post Mg and Pre Si labels Sep 2, 2020
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Definitely want a changelog entry or two, and I'll probably want to give this a bit deeper dive, but mostly it looks good 👍

@cmcmarrow cmcmarrow changed the title fix zmq hang Wip fix zmq hang Sep 24, 2020
@cmcmarrow cmcmarrow changed the title Wip fix zmq hang fix zmq hang Sep 25, 2020
@cmcmarrow
Copy link
Contributor Author

re-run full pre-commit

@cmcmarrow cmcmarrow requested a review from waynew September 25, 2020 19:06
waynew
waynew previously approved these changes Sep 25, 2020
@dwoz dwoz merged commit 300aefb into saltstack:master Sep 30, 2020
@eliasp
Copy link
Contributor

eliasp commented Sep 30, 2020

Only noticed this PR after it's been merged, but having huge commits like a6ac184 or generic commit messages like "hot fix" or "fix pre" is not good for the general repository hygiene. IMHO those should be either handled properly or be rebased as long as a PR is still WIP.
This huge commit (a6ac184), containing the actual fix also does a lot of other completely unrelated things. This makes it really hard at a later point to pin-point a possible regression to a specific change, e.g. when using git bisect ....

As an outsider I can't really dictate any policies here, but as someone debugging those kind of issues quite often I can surely tell that this makes it really hard in being helpful providing valuable information in bug reports, e.g. pointing out a specific line or so which introduced a certain bug or finding commits by scanning over their messages.

@takeda
Copy link
Contributor

takeda commented Oct 2, 2020

I agree with @eliasp, Saltstack is a great product, but it should have stronger discipline with changes. The biggest issue with saltstack is that every release is like a lottery, it might fix your bug (like currently this one is the biggest one for me), but it can introduce another bug. It makes people hesitant to recommend it, because while it solves a lot of issue and in better ways than other CMs its weak part is stability.

This should be actually 2 PRs:

  1. only the fix with the issue (and have it backported to 3001.2 and 3000.4)
  2. removing py2.7 support and only merging it to master (I know there is a lot of code and it's not feasible to do that separately and it's best when making other changes, but it's so easy to make a mistake in these steps)

@fzipi
Copy link
Contributor

fzipi commented Oct 2, 2020

Agreed. I've cleaned it up to look like this for us: https://gist.github.com/fzipi/af135b5e1f78128ffdafff3f8240caa9

@thatITguy2000
Copy link

thatITguy2000 commented Nov 3, 2020

I see this issue has been closed and changes merged but I'm still seeing this behavior with a RHEL7.9 Workstation OS acting as Salt Master with a Ubuntu 20.04 minion using the Latest salt-master-3002.1-1.el7.noarch.rpm installation, my installs on the RHEL7 system are performed using the SaltStack repo (https://repo.saltstack.com/py3/redhat/7Workstation/x86_64/latest/) via a local Satellite mirror that syncs weekly and my Ubuntu installs are performed using a local mirror that syncs to the latest Ubuntu SaltStack repo.
I'm not certain what I'm missing but I'm hoping someone can help me along as a newbie to github and in regard to this issue specifically.
My Master and Minion are both using the latest packages from the SaltStack repos for their installations.

@jakehilton
Copy link

I recently upgraded to 2019.2.7 from 2019.2.4 for the latest CVE fixes. I am now seeing this hang issue as well. I'm on Ubuntu 18.04.

@ocfmatt
Copy link

ocfmatt commented Nov 24, 2020

Agreed. I've cleaned it up to look like this for us: https://gist.github.com/fzipi/af135b5e1f78128ffdafff3f8240caa9

Applying this patch has resolved the hang issue for me. Thank you @fzipi for committing it!

@jakehilton - applying the patch has made my 2019.2.7 install consistently work without having to disable zmq transport.

Regards,
Matt.

agraul added a commit to openSUSE/salt that referenced this pull request Mar 29, 2021
agraul added a commit to openSUSE/salt that referenced this pull request Mar 29, 2021
agraul added a commit to openSUSE/salt that referenced this pull request Apr 6, 2021
agraul added a commit to openSUSE/salt that referenced this pull request Apr 6, 2021
Fixes bsc#1181368

(cherry picked from commit 4abaaee)
agraul added a commit to openSUSE/salt that referenced this pull request Apr 6, 2021
morgana2313 added a commit to morgana2313/salt that referenced this pull request Aug 18, 2022
morgana2313 added a commit to morgana2313/salt that referenced this pull request Aug 18, 2022
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

9 participants