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

Salt the Salt master #350

Merged
merged 5 commits into from Aug 17, 2016
Merged

Salt the Salt master #350

merged 5 commits into from Aug 17, 2016

Conversation

@aneeshusa
Copy link
Member

aneeshusa commented Apr 30, 2016

Use Salt to install the Salt master.
The Salt master will get restart on configuration changes,
but will need to be manually restarted on package changes to
allow for proper ordering of updating.
(Salt masters must be updated before Salt minions.)

Use Jinja to generate the config file YAML directly, preventing
typos in the master config file and providing access to master
configuration options in the SLS file (e.g. the file_roots locations).

Incorporates and thus fixes #298.

TODO:

  • Add gitfs support. Fixes #264.
  • Add _modules/ dir and backport launchctl.py module

TODO after landing:

  • Add master install instructions to the wiki
  • Update Salt-inside-Vagrant usage instructions on the wiki

After I finish the WIP tasks, landing this will unblock a bunch of other things that need the launchctl.py module (upgraded ssh, salt-minion salting, upgrading salt, etc.)


This change is Reviewable

@aneeshusa
Copy link
Member Author

aneeshusa commented May 2, 2016

OK, this is ready for review. See the commit messages for details; also, a few notes:

  • The Vagrantfile needed to be updated, so I cherry-picked #298 as well.
  • I tested out the new launchctl.py module and updates to the Buildbot slave state on servo-macpro1 and in Vagrant.
@aneeshusa aneeshusa force-pushed the aneeshusa:saltify-salt-master branch from 89a821b to bf7d46f May 2, 2016
@aneeshusa aneeshusa changed the title [WIP] Salt the Salt master Salt the Salt master May 2, 2016
@aneeshusa aneeshusa force-pushed the aneeshusa:saltify-salt-master branch from bf7d46f to a4d30b7 May 6, 2016
@aneeshusa
Copy link
Member Author

aneeshusa commented May 6, 2016

Rebased since #298 landed.

@bors-servo
Copy link
Contributor

bors-servo commented Jun 6, 2016

The latest upstream changes (presumably #374) made this pull request unmergeable. Please resolve the merge conflicts.

@edunham
Copy link
Contributor

edunham commented Jun 6, 2016

Reviewed 1 of 11 files at r6, 4 of 4 files at r7, 3 of 3 files at r8, 3 of 3 files at r9.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


_modules/launchctl.py, line 3 [r9] (raw file):

# -*- coding: utf-8 -*-
'''
Module for the management of MacOS systems that use launchd/launchctl

It looks like we're vendoring this in from the Salt builtin module (https://docs.saltstack.com/en/latest/ref/modules/all/salt.modules.launchctl.html). Do we need to worry about keeping this up to date with changes that Salt makes upstream, or will it be obsoleted relatively soon once we upgrade our Salt version?


Comments from Reviewable

@aneeshusa aneeshusa force-pushed the aneeshusa:saltify-salt-master branch from a4d30b7 to be72774 Jun 8, 2016
@aneeshusa
Copy link
Member Author

aneeshusa commented Jun 8, 2016

@edunham I backported that module temporarily; we shouldn't need to update it unless it breaks, and we can simply remove it once we've updated Salt.

I rebased the branch and made some tweaks, but haven't tested them in Vagrant yet because my home Internet connection is slower than at school. Do you know why Travis doesn't seem to be triggering?

@edunham
Copy link
Contributor

edunham commented Jun 20, 2016

Not sure what's up with Travis; I've manually restarted https://travis-ci.org/servo/saltfs/builds/128187498 .

Edit: Lars had some GitHub integration issues, https://twitter.com/larsberg_/status/743857405319077888, around the time that Travis failed to notice this PR. Possibly related.

@aneeshusa aneeshusa force-pushed the aneeshusa:saltify-salt-master branch from be72774 to 770cf2f Jun 22, 2016
@aneeshusa
Copy link
Member Author

aneeshusa commented Jun 22, 2016

Rebased, but I want to tweak this a little more before merging.

@aneeshusa aneeshusa force-pushed the aneeshusa:saltify-salt-master branch from 770cf2f to afba644 Jun 27, 2016
@aneeshusa
Copy link
Member Author

aneeshusa commented Jun 27, 2016

Rebased with tweaks and lightly tested in Vagrant. Ready for review.

@bors-servo
Copy link
Contributor

bors-servo commented Jul 12, 2016

The latest upstream changes (presumably #394) made this pull request unmergeable. Please resolve the merge conflicts.

This was referenced Jul 15, 2016
aneeshusa added 3 commits Apr 30, 2016
Use Salt to install the Salt master.
The Salt master will not get restarted automatically,
neither on configuration changes nor on package changes.
Instead, it will need to be manually restarted to avoid interrupting
the in-progress highstate as well as allow for proper update ordering.
(Salt masters must be updated before Salt minions.)

Use Jinja to generate the config file YAML directly, preventing
typos in the master config file and providing access to master
configuration options in the SLS file (e.g. the file_roots locations).
Previously, the minion config was used to specify that a local set
of file trees should be used. However, in order to test that the Salt
master states function, and to more closely match the actual deployment
of our minions, amend the minion config to point to a localhost master.
Additionally, amend our Travis config and Vagrantfile to explicitly
pass options to set the local file and pillar trees.
Only sync them via Vagrant shared folders for the Salt master, which
should have them available to allow debugging. Because using Salt is
the only way to learn which minions (ids) would have a Salt master
running on them, hardcode the salt-master\d+ regex into the Vagrantfile.

In the future, this will allow adding a smoke test that check if the
master can test.ping a local minion (on the same VM). However, this
requires the minion to be started, which requires that the Salt minion
also be salted, (which requires this PR to backport the launchctl.py
execution module).
Use gitfs backed by the saltfs repo (https://github.com/servo/saltfs/)
as the main source for the Salt file tree.

For local testing, additionally configure a local directory in /tmp as
a Salt file tree root, and give it precedence over the gitfs tree.
This allows temporarily cloning the saltfs repo into that directory
and makes local changes to iterate on them.
Add an ADMIN_README file with some ground rules about proper usage of
the directory; this file should be set as immutable, but Salt does not
yet have a module/state for this.
@aneeshusa aneeshusa force-pushed the aneeshusa:saltify-salt-master branch from afba644 to 037d31b Aug 4, 2016
@edunham
Copy link
Contributor

edunham commented Aug 10, 2016

Reviewed 3 of 4 files at r11, 4 of 4 files at r12, 3 of 3 files at r13.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@edunham
Copy link
Contributor

edunham commented Aug 10, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Aug 10, 2016

📌 Commit 037d31b has been approved by edunham

@bors-servo
Copy link
Contributor

bors-servo commented Aug 10, 2016

Testing commit 037d31b with merge d7f388d...

bors-servo added a commit that referenced this pull request Aug 10, 2016
Salt the Salt master

Use Salt to install the Salt master.
The Salt master will get restart on configuration changes,
but will need to be manually restarted on package changes to
allow for proper ordering of updating.
(Salt masters must be updated before Salt minions.)

Use Jinja to generate the config file YAML directly, preventing
typos in the master config file and providing access to master
configuration options in the SLS file (e.g. the file_roots locations).

Incorporates and thus fixes #298.

TODO:
  - [x] Add gitfs support. Fixes #264.
  - [x] Add _modules/ dir and backport `launchctl.py` module

TODO after landing:
  - [ ] Add master install instructions to the wiki
  - [ ] Update Salt-inside-Vagrant usage instructions on the wiki

After I finish the WIP tasks, landing this will unblock a bunch of other things that need the `launchctl.py` module (upgraded ssh, salt-minion salting, upgrading salt, etc.)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/350)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Aug 10, 2016

💔 Test failed - status-travis

@aneeshusa
Copy link
Member Author

aneeshusa commented Aug 10, 2016

I think I'll need to backport the pip_state.py state module as well for this to pass Travis - I'll try to do that this evening.

@aneeshusa
Copy link
Member Author

aneeshusa commented Aug 11, 2016

OK, I backported the pip execution and state modules. Something important I discovered is that custom modules with the same name don't work properly, so I prefixed the files with backported_.

Should be good to go now.

aneeshusa added 2 commits May 2, 2016
Also update the buildbot slave state to use the
now-working service state on OS X, and update the
name of the service from 'buildslave' to
'buildbot-slave' for better distinguishability.

Add a note to README.md about licensing of files
taken and/or modified from the main Salt repo.
The backported versions support old and new pips more robustly without
depending on pip internals.

Recently (in version 8.1.2), pip changed their internals in a
non-backwards compatible way, breaking the Salt pip modules.
Salt used to depend on these internals, but has since been updated
to no longer depend on them as they have no compatibility guarantees.
We have been encountering issues where Salt is finding a new pip
version, even though the installed pip is old (6.1.1). To work around
this issue, backport updated modules that handle both old and new pips.
@aneeshusa aneeshusa force-pushed the aneeshusa:saltify-salt-master branch from f1054aa to a713928 Aug 11, 2016
@aneeshusa
Copy link
Member Author

aneeshusa commented Aug 11, 2016

It turns out the file naming wasn't the issue, and that giving the files the same name is the right thing to do for overriding. However, our version of Salt is so old that the backported pip modules weren't working properly (looking for Salt utilities that don't exist in our version), so I've backported only the needed changes to pip_state.py.

Also, I added some headers to (try to) comply with the Apache 2.0 license of the backported modules.

@aneeshusa
Copy link
Member Author

aneeshusa commented Aug 11, 2016

All of the test failures are happening on the builders where we test the PR as an upgrade; they fail when applying the initial highstate (of the old code). All of the builders that test the PR from scratch are passing, though, so I think this is OK.

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Aug 17, 2016

@edunham Is there anything else left to review here? We're having to do all manual merges to saltfs until this lands.

@edunham
Copy link
Contributor

edunham commented Aug 17, 2016

Reviewed 2 of 2 files at r14, 1 of 1 files at r15.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from Reviewable

@edunham
Copy link
Contributor

edunham commented Aug 17, 2016

I think that the problems we're encountering from outdated Salt version are greater than any problems likely to remain in this PR. The Travis failures look like the standard Salt version issues we've been seeing.

@edunham edunham merged commit e69cbbc into servo:master Aug 17, 2016
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
code-review/reviewable 1 discussion left (aneeshusa, edunham)
Details
@aneeshusa
Copy link
Member Author

aneeshusa commented Aug 18, 2016

Just a clarification, this PR doesn't update the whole Salt version, only backports a few modules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.