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

Write lists of minions targeted by syndic masters to job cache #31670

Merged
merged 29 commits into from Mar 7, 2016

Conversation

terminalmage
Copy link
Contributor

Summary

When a master-of-masters publishes a command, and a syndic master re-publishes it to its subscribed minions, it fires an event back to the master containing the list of minions matched by the syndic master for that job.

This pull request scans these events for lists of minions, and writes them to the job cache.

Fixes #30528.

How to test

  1. Use this documentation to check out this pull request into a new branch, and install from git.
  2. Recommended setup: 3 hosts (1 master-of-masters, 1 syndic master, 1 lower-level minion). Set order_masters: True on the master-of-masters and subscribe the lower-level minion to the syndic master, and the syndic master to the master-of-masters.
  3. From the master-of-masters, run salt -v '*' test.ping
  4. Take the jid obtained in step 3 and run salt-run jobs.list_job <jid>. The Minions list in the return data should contain the minion ID of the lower-level minion. Prior to this pull request, the lower-level minions would not be in this return data. After completing the rest of the test steps, installing Salt from the head of 2015.8 on the master-of-masters and repeating steps 3 and 4 should show no lower-level minions in the jobs.list_job return data.
  5. Stop the salt-minion service on the lower-level minion, and then repeat step 3.
  6. Take the jid obtained in step 5 and run salt-run jobs.lookup_jid <jid> missing=True. The lower-level minion should be included in the return data, and should show Minion did not return. Repeating the call to jobs.lookup_jid without the missing=True should exclude the lower-level minion from the return data.

Review notes

Of particular focus in the review of this pull request should be the following:

  • The couchbase_return returner. To maintain API compatibility I needed to move the writing of minion lists into a separate function, as I did for the local_cache returner, and I did not have an instance to test this.
  • The file locking code added in salt/utils/files.py.

This allows minions to be updated when the master receives events
containing lists of minions targeted by lower-level masters.
This allows the job cache to include minions matched by lower-level
masters for a given job.
@justinta
Copy link

justinta commented Mar 4, 2016

Go Go Jenkins!

2 similar comments
@justinta
Copy link

justinta commented Mar 4, 2016

Go Go Jenkins!

@justinta
Copy link

justinta commented Mar 4, 2016

Go Go Jenkins!

@rallytime
Copy link
Contributor

Go Go Jenkins!

'''
Included for API consistency
'''
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sort of inclined to raise exceptions for these and catch and log higher up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason that I did not is that we're not already saving the minion list for this returner.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. Good point.

@cachedout
Copy link
Contributor

(With apologies for being the person who brought up locking the first place.)

After looking at this, I'm wondering if instead of locking we consider splitting this write up into multiple files and then scanning those when we retrieve the minions list. (I'm talking about the local cache of course.) The locking approach is starting to worry me..

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Mar 4, 2016
@terminalmage
Copy link
Contributor Author

@cachedout Good idea, I just pushed some changes, let me know what you think.

Some events just contain a timestamp and a minion list, but no jid.
These can safely be ignored, lest they cause supurious log messages.

Also, pass the syndic's id so that it can be stored in a unique file in
the job cache (in the local_cache returner).
Also fix successive updates
This is used to provide a unique filename for the minion list for a
given syndic.
@cachedout
Copy link
Contributor

Yeah, I think this is better. Let's go with this.

cachedout pushed a commit that referenced this pull request Mar 7, 2016
Write lists of minions targeted by syndic masters to job cache
@cachedout cachedout merged commit a1f32b7 into saltstack:2015.8 Mar 7, 2016
@terminalmage terminalmage deleted the issue30528 branch April 6, 2016 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants