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

More granular master job targeting #13285

Merged
merged 1 commit into from
Jul 9, 2014
Merged

Conversation

jacksontj
Copy link
Contributor

Please DO NOT MERGE IMMEDIATELY.

Salt targeting is done using the zmq pub/sub mechanism. No filtering is done so all requests go to all minions. This is an attempt to make the requests more target-able. ZeroMQ supports filters on these sockets which are processed publisher side. So the thought is to set it to the minion id and a shared "broadcast" one. Only requests of tgt_type == list will use this. The thought is if we like this we can use other data (mine etc) to determine a more specific target on the master (such as a list) from a glob match etc.

This is more or less backwards compatible, during my testing it still works but the old minion will throw an exception like:

[CRITICAL] An exception occurred while polling the minion
Traceback (most recent call last):
File "/home/thjackso/src/salt/salt/minion.py", line 1324, in tune_in
payload = self.serial.loads(self.socket.recv(zmq.NOBLOCK))
File "/home/thjackso/src/salt/salt/payload.py", line 95, in loads
return msgpack.loads(msg, use_list=True)
File "_unpacker.pyx", line 114, in msgpack._unpacker.unpackb (msgpack/_unpacker.cpp:114)
ExtraData: unpack(b) recieved extra data.

The new minion (in this PR) will work with both configurations-- although once a minion has enabled targeting it will no longer receive old-style messages.

This is another attempt of #10374.
And exactly the same as #13154

@ghost
Copy link

ghost commented Jun 6, 2014

Test Failed.

If the failures are unrelated to your code, don't stress, a core developer will know these apart.
In the future, if possible, please branch off a passing commit to avoid false positives.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/4928/

@thatch45
Copy link
Contributor

I never got the go ahead on this one, is it ready for merge/review?

@jacksontj
Copy link
Contributor Author

This is ready for review, but I need to remove the default config change. The plan is to make the master by default send the message with the new header. And then you can enable the actual filtering on the minion side. The plan would be to change the default for that after this major release. Would it be possible to get a couple of you guys to test this out as well? Everything passes on my box, but more people testing it is almost always better :)

I know that when this is disabled tests still pass, question is does it work when it is enabled. I've tested, but the CI stack has pinned master/minion versions which I can't change the configs of.

@thatch45
Copy link
Contributor

Sounds good, you can change the configs in the test suite in a pull req to run them on Jenkins.

@thatch45
Copy link
Contributor

thatch45 commented Jul 7, 2014

@jacksontj should we close this? it is badly conflicting.

@jacksontj
Copy link
Contributor Author

I'm going to try to get some time this week to fix it up, I can just fix
the pull req
On Jul 7, 2014 2:10 PM, "Thomas S Hatch" notifications@github.com wrote:

@jacksontj https://github.com/jacksontj should we close this? it is
badly conflicting.


Reply to this email directly or view it on GitHub
#13285 (comment).

Salt targeting is done using the zmq pub/sub mechanism. No filtering is done so all requests go to all minions. This is an attempt to make the requests more target-able. ZeroMQ supports filters on these sockets which are processed publisher side. So the thought is to set it to the minion id and a shared "broadcast" one. Only requests of tgt_type == list will use this. The thought is if we like this we can use other data (mine etc) to determine a more specific target on the master (such as a list) from a glob match etc.

This is more or less backwards compatible, during my testing it still works but the old minion will throw an exception like:

[CRITICAL] An exception occurred while polling the minion
Traceback (most recent call last):
  File "/home/thjackso/src/salt/salt/minion.py", line 1324, in tune_in
    payload = self.serial.loads(self.socket.recv(zmq.NOBLOCK))
  File "/home/thjackso/src/salt/salt/payload.py", line 95, in loads
    return msgpack.loads(msg, use_list=True)
  File "_unpacker.pyx", line 114, in msgpack._unpacker.unpackb (msgpack/_unpacker.cpp:114)
ExtraData: unpack(b) recieved extra data.

So if we decide this is a good idea we'll probably want to wrap the new master side logic in a config option, and just merge in the minion side changes for the first release.

This is another attempt of saltstack#10374.
@jacksontj
Copy link
Contributor Author

@thatch45 I just fixed the merge conflict, but I'm not sure how to change the configs for the jenkins run. I changed the default config values in this pull req, but if I remember right the Jenkins runs use a pinned version of the master and minion-- if thats the case I can't really get the CI stack to run all this code for me. If that is the case we can either manually run it somehow (tests are running locally right now, but in jenkins would be nice) or we can merge this in with the configs off then switch the pinned version to include this code so then i could make another branch which just has the config change to trigger test runs.

Apologies if I lost you there ;) Maybe we should involve @s0undt3ch since he is the test master :)

@jacksontj
Copy link
Contributor Author

FYI the tests just all passed locally.

@ghost
Copy link

ghost commented Jul 9, 2014

Test Failed.

If the failures are unrelated to your code, don't stress, a core developer will know these apart.
In the future, if possible, please branch off a passing commit to avoid false positives.
Refer to this link for build results: http://jenkins.saltstack.com/job/salt-pr-build/5888/

@jacksontj
Copy link
Contributor Author

Interesting, seems that all tests (besides some unrelated pylint tests) passed. With the config that I have (zmq_filtering on) this should fail the backwards compatibility tests, are we not running those?

@thatch45
Copy link
Contributor

thatch45 commented Jul 9, 2014

No, we don't have backward compat tests inline. But this does look ready to merge.
The backward compat is only with the config value correct?

thatch45 added a commit that referenced this pull request Jul 9, 2014
More granular master job targeting
@thatch45 thatch45 merged commit 1c062a7 into saltstack:develop Jul 9, 2014
@jacksontj
Copy link
Contributor Author

The defaults i set make it backwards incompatible. Just have to change the
defaults.
On Jul 9, 2014 11:48 AM, "Thomas S Hatch" notifications@github.com wrote:

Merged #13285 #13285.


Reply to this email directly or view it on GitHub
#13285 (comment).

jacksontj added a commit to jacksontj/salt that referenced this pull request Jul 9, 2014
thatch45 added a commit that referenced this pull request Jul 10, 2014
Change the defaults to be backwards compatible (fix #13285)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants