Page MenuHomePhabricator
Create Task
Maniphest T67549

Trebuchet doesn't like when a deployer server is also a minion, a edge case for scap
Closed, ResolvedPublic
Actions

Description

From bug 65542:

Directory permissions were probably broken initially because in the current
setup tin is acting as both the deploy server and a minion for the scap deploy.
When the minion gets the fetch/checkout commands from salt it runs as root. I
sort of wondered if this would cause problems and I think now I have my answer.

We can either try to rearrange the way that classes are defined in
operations/puppet.git to eliminate this problem or figure out how to make
trebuchet aware of the edge case and avoid updating the deployment server if it
is also in the minions list.

Version: wmf-deployment
Severity: normal

Details

Reference
bz65549
SubjectRepoBranchLines +/-
git deploy: don't fetch/checkout/restart on the deployment serveroperations/puppetproduction+17 -7
Customize query in gerrit

Related Objects

Event Timeline

• bzimport raised the priority of this task from to High.Nov 22 2014, 3:08 AM
• bzimport added a project: Deployments.
• bzimport set Reference to bz65549.
• bzimport added a subscriber: Unknown Object (MLST).
greg created this task.May 20 2014, 8:08 PM
bd808 added a comment.May 20 2014, 8:22 PM

[13:15] < bd808> Tin gets the deployment::target define for scap via mediawiki::sync which is how all of the other nodes get it too.
[13:16] Ryan_Lane nods
[13:19] < bd808> Off the top of my head I can't think of a clean way to keep Deployment::Target['scap'] from applying on tin.
[13:19] < bd808> How horrible would it be to handle this edge case in trebuchet itself?
[13:20] <Ryan_Lane> hm. it's not a simple edge case to workaround
[13:20] <Ryan_Lane> it's doable using compound matching for targeting
[13:20] <Ryan_Lane> you could use "scap and not deployment server" I believe
[13:21] < bd808> Ryan_Lane: Where would that go?
[13:22] <Ryan_Lane> inside of the runner code
[13:22] <Ryan_Lane> the runner code looks up the grain, then does a salt call via the salt client api
[13:23] < bd808> Ryan_Lane: Ok. grain = "deployment_target:" + grain .... in deploy.py
[13:24] <Ryan_Lane> yep
[13:24] <Ryan_Lane> and client.cmd(grain, cmd, expr_form='grain', arg=arg, ...
[13:24] <Ryan_Lane> http://docs.saltstack.com/en/latest/ref/clients/index.html#salt.client.LocalClient.cmd
[13:25] <Ryan_Lane> expr_form= <-- that would need to be changed to compound
[13:25] < bd808> yup.
[13:25] <Ryan_Lane> http://docs.saltstack.com/en/latest/topics/targeting/compound.html

bd808 added a comment.May 20 2014, 8:23 PM

The runner being discussed in comment #1 is modules/deployment/files/runners/deploy.py in the operations/puppet.git repo.

bd808 added a comment.May 28 2014, 7:54 PM

This caused problems for /srv/deployment/scap/scap on tin again today. There seems to be really bad corruption of the file permissions on the .git directory whenever git deploy is run.

greg moved this task from To Triage to Next: Maintenance on the Deployments board.Nov 25 2014, 11:27 PM
hashar unsubscribed.Nov 25 2014, 11:28 PM
greg moved this task from Next: Maintenance to Backlog (Tech) on the Deployments board.Dec 11 2014, 7:33 PM
ArielGlenn subscribed.May 20 2015, 2:49 PM

https://gerrit.wikimedia.org/r/#/c/212291/ this?

bd808 merged a task: T94754: Trebuchet can mess up .git permissions on deployment server.May 20 2015, 4:04 PM
bd808 added subscribers: gerritbot, yuvipanda, • mmodell, Aklapper.
gerritbot added a comment.May 20 2015, 4:05 PM

Change 212291 had a related patch set uploaded (by BryanDavis):
git deploy: don't fetch/checkout/restart on the deployment server

https://gerrit.wikimedia.org/r/212291

gerritbot added a project: Patch-For-Review.May 20 2015, 4:05 PM
thcipriani moved this task from Backlog (Tech) to In-progress on the Deployments board.Jun 15 2015, 6:26 PM
thcipriani added a project: acl*sre-team.Jul 6 2015, 6:14 PM
thcipriani set Security to None.
Restricted Application added a subscriber: Matanya. · View Herald TranscriptJul 6 2015, 6:14 PM
thcipriani subscribed.Jul 6 2015, 6:15 PM

Are there any updates that need to happen on this patch? Could pull to deployment-prep for a sanity check.

fgiunchedi subscribed.Jul 20 2015, 10:35 AM

@bd808 @ArielGlenn thoughts on this?

ArielGlenn added a comment.Jul 20 2015, 1:45 PM

fine to test on deployment-prep; if it works there it's good for prod.

fgiunchedi lowered the priority of this task from High to Medium.Jul 20 2015, 2:45 PM

doubtful this is high priority, @thcipriani looks good to test on deployment-prep and we can push to prod then

thcipriani added a comment.Jul 22 2015, 12:56 AM

@fgiunchedi works as expected on deployment-prep deploying the test repo. LGTM.

gerritbot added a comment.Jul 28 2015, 1:49 PM

Change 212291 merged by Filippo Giunchedi:
git deploy: don't fetch/checkout/restart on the deployment server

https://gerrit.wikimedia.org/r/212291

fgiunchedi mentioned this in rOPUPcbd32f4220e2: git deploy: don't fetch/checkout/restart on the deployment server.Jul 28 2015, 1:49 PM
fgiunchedi closed this task as Resolved.Jul 28 2015, 1:50 PM
fgiunchedi claimed this task.
In T67549#1469568, @thcipriani wrote:

@fgiunchedi works as expected on deployment-prep deploying the test repo. LGTM.

cool! merged

• Phabricator_maintenance removed a subscriber: yuvipanda.Jun 7 2017, 7:00 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL