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.

Sign up for GitHub

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

Jump to bottom

ZeroMQ no longer required when transport is TCP #29294

Merged
merged 1 commit into from Dec 8, 2015
Merged

ZeroMQ no longer required when transport is TCP #29294

merged 1 commit into from Dec 8, 2015

Conversation

skizunov
Copy link
Contributor

@skizunov skizunov commented Dec 1, 2015

salt/master.py:

  • If ZMQ is not importable, use tornado.ioloop.IOLoop as the loop class.
  • Check HAS_ZMQ before using any zmq.* content.
  • Did not touch FloMWorker usage of zmq.*. This is only used when the
    transport is RAET.

salt/minion.py:

  • If ZMQ is not importable, use tornado.ioloop.IOLoop as the loop class.
  • Check HAS_ZMQ before using any zmq.* content.
  • Syndic and MultiSyndic classes still require ZeroMQ.

salt/transport/ipc.py:

  • Added IPCMessagePublisher. This is intended to function much like
    ZMQ's zmq.PUB sockets. Used in implementing utils/event.py to function
    without ZMQ.
  • Added IPCMessageSubscriber. This is intended to function much like
    ZMQ's zmq.SUB sockets. Used in implementing utils/event.py to function
    without ZMQ. What makes this class a bit different is that the associated
    IO Loop is meant to not be running when it is used. Due to this, it is
    recommended that the caller create a new IO Loop for this purpose. The
    reason for this is that the get_event() API may be invoked from
    anywhere, whether or not there is a current IO Loop that is running in
    the thread of the invocation.

salt/utils/async.py:

  • If ZMQ is not importable, use tornado.ioloop.IOLoop as the loop class.

salt/utils/event.py:

  • Implemented using salt.transport.ipc instead of ZMQ.
  • zmq.PUB ==> IPCMessagePublisher
  • zmq.SUB ==> IPCMessageSubscriber
  • zmq.PUSH ==> IPCMessageClient
  • zmq.PULL ==> IPCMessageServer

Signed-off-by: Sergey Kizunov sergey.kizunov@ni.com

@skizunov skizunov force-pushed the develop2 branch 4 times, most recently from 27d1f48 to 12b2024 Compare December 1, 2015 09:05
@jfindlay jfindlay added Core relates to code central or existential to Salt Expert Review Needed Transport labels Dec 1, 2015
@cachedout
Copy link
Contributor

@skizunov This is great!! Thank you!

There are some minor lint errors and it doesn't look like the tests are running. I'll kick off another test run and also see if I can get them to run locally.

I am also going to cc: @jacksontj and @DmitryKuzmenko for additional review, since I'm sure they'll both want to see this. Thanks for your hard work here. This is really great!

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Dec 1, 2015
@cachedout
Copy link
Contributor

Go Go Jenkins!

@skizunov
Copy link
Contributor Author

skizunov commented Dec 1, 2015

@cachedout : It looks like there are some failures. Give me some time to fix them. I will let you know when I am done.

@cachedout
Copy link
Contributor

@skizunov Sounds great. Thank you very much.

@skizunov skizunov force-pushed the develop2 branch 2 times, most recently from b5b99f8 to 7b11ad7 Compare December 2, 2015 01:00
@DmitryKuzmenko
Copy link
Contributor

Woot! It's awesome! I'll review the code today.

@skizunov
Copy link
Contributor Author

skizunov commented Dec 2, 2015

@cachedout : I think the code is at a point where it has a chance of passing the tests. Are the current test failures due to a problem with the code or a problem with the testing framework?

Admittedly, I am relying on the test framework to test the parts of the code that I don't personally use, such as Syndic.

@DmitryKuzmenko
Copy link
Contributor

The code looks great! Thank you @skizunov !

@cachedout
Copy link
Contributor

@skizunov This appears to be a legit bug. It doesn't look like the daemons are starting up prior to the test suite beginning.

@skizunov skizunov force-pushed the develop2 branch 11 times, most recently from faf69a5 to 5bf1b49 Compare December 7, 2015 03:41
@skizunov
ZeroMQ no longer required when transport is TCP
a37a270 
salt/master.py:
- If ZMQ is not importable, use `tornado.ioloop.IOLoop` as the loop class.
- Check HAS_ZMQ before using any zmq.* content.
- Did not touch `FloMWorker` usage of zmq.*. This is only used when the
transport is RAET.

salt/minion.py:
- If ZMQ is not importable, use `tornado.ioloop.IOLoop` as the loop class.
- Check HAS_ZMQ before using any zmq.* content.

salt/transport/ipc.py:
- Added `IPCMessagePublisher`. This is intended to function much like
ZMQ's zmq.PUB sockets. Used in implementing utils/event.py to function
without ZMQ.
- Added `IPCMessageSubscriber`. This is intended to function much like
ZMQ's zmq.SUB sockets. Used in implementing utils/event.py to function
without ZMQ. What makes this class a bit different is that the associated
IO Loop is meant to not be running when it is used. Due to this, it is
recommended that the caller create a new IO Loop for this purpose. The
reason for this is that the `get_event()` API may be invoked from
anywhere, whether or not there is a current IO Loop that is running in
the thread of the invocation.

salt/utils/async.py:
- If ZMQ is not importable, use `tornado.ioloop.IOLoop` as the loop class.

salt/utils/event.py:
- Implemented using `salt.transport.ipc` instead of ZMQ.
- zmq.PUB ==> `IPCMessagePublisher`
- zmq.SUB ==> `IPCMessageSubscriber`
- zmq.PUSH ==> `IPCMessageClient`
- zmq.PULL ==> `IPCMessageServer`

Signed-off-by: Sergey Kizunov <sergey.kizunov@ni.com>
@skizunov
Copy link
Contributor Author

skizunov commented Dec 8, 2015

@cachedout : The tests seem to be passing now (except for CentOS 6, but I think that is expected). Can we continue the review process?

@cachedout
Copy link
Contributor

Great! I will look at this in a few minutes. Thanks, @skizunov

cachedout pushed a commit that referenced this pull request Dec 8, 2015
Merge pull request #29294 from skizunov/develop2
87fb8ba 
ZeroMQ no longer required when transport is TCP
@cachedout cachedout merged commit 87fb8ba into saltstack:develop Dec 8, 2015
This was referenced Feb 29, 2016
Closed
Merged
@Ch3LL Ch3LL mentioned this pull request Jan 11, 2017
Closed
rallytime pushed a commit to rallytime/salt that referenced this pull request Jan 13, 2017
Remove "event_publisher_pub_hwm" and "salt_event_pub_hwm" from config…
706c885 
…/__init__.py

These options were used back when the event system was based on ZMQ. As of PR
PR saltstack#29294, these options are no longer needed and are obsolete. PR saltstack#29294 changed
the event system transport from ZMQ to TCP/tornado.

There are no other mentions of these configuration options in the salt code base
except in the man pages. Once the man pages are regenerated, those references will
be removed.

Fixes saltstack#38674
@rallytime rallytime mentioned this pull request Jan 13, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Core relates to code central or existential to Salt Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged Transport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@skizunov @cachedout @DmitryKuzmenko @jfindlay