Page MenuHomePhabricator

Refactor host/target context management
ClosedPublic

Authored by dduvall on Dec 3 2015, 12:53 AM.

Details

Maniphest Tasks
T119643: create an environment object that centralizes the file and directory lookup logic for scap3
Reviewers
thcipriani
mmodell
Group Reviewers
Release-Engineering-Team
Commits
rMSCA55ee398f20b2: Refactor host/target context management
Patch without arc
git checkout -b D70 && curl -L https://phabricator.wikimedia.org/D70?download=true | git apply
Summary

Established a context module and set of classes for managing
deployment directory setup, path resolution, and command execution
context.

Fixes T119643

Test Plan

Run a deploy along with deploy-log. Run the unit tests.

Diff Detail

Repository
rMSCA Scap
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dduvall updated this revision to Diff 207.Dec 3 2015, 12:53 AM
dduvall retitled this revision from to WIP: Refactor host/target environment management.
dduvall updated this object.
dduvall edited the test plan for this revision. (Show Details)
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald TranscriptDec 3 2015, 12:53 AM
mmodell added a subscriber: mmodell.EditedDec 3 2015, 5:06 PM

I like the direction this is headed, however, there is a lot of duplication of effort going on between the three of us all working on similar things / areas of the code.

I've got some code to abstract the command execution in cluster_ssh, which I'll submit as a diff soon, but in case you are working on the same thing, maybe take a look?

1# -*- coding: utf-8 -*-
2"""
3 scap.env
4 ~~~~~~~~~~~
5 command execution
6
7"""
8import scap.config as config
9
10
11class Command(object):
12 def __init__(self, *args):
13 self.cmd = args
14
15 def __call__(self, *args, **kwargs):
16 return " ".join(self.args)
17
18
19class arg(Command):
20 def __init__(self, name, cmd):
21 self.name = name
22 self.cmd = cmd
23
24 def required(self, required=True):
25 self._required = required
26 return self
27
28 def __call__(self, **values):
29 if self.name in values and values[self.name]:
30 val = values[self.name]
31 return self.cmd.format(val)
32 else:
33 return ''
34
35
36class ComposableCommand(Command):
37 def __init__(self, *cmds):
38 self.cmds = cmds
39
40 def __call__(self, *args, **values):
41 result = []
42 parts = self.cmds + args
43 for cmd in parts:
44 rendered = cmd
45 if callable(cmd):
46 rendered = cmd(**values)
47 if rendered:
48 if type(rendered) is str:
49 result.append(rendered)
50 else:
51 result.extend(rendered)
52 return result
53
54
55class SubCommand(ComposableCommand):
56 pass
57
58
59env = config.Environment()
60SSH_USER = arg('user', '-oUser={}')
61SUDO_USER = arg('user', '-u {}')
62SSH = ComposableCommand(env['ssh_command'], SSH_USER)
63SUDO = ComposableCommand('sudo', SUDO_USER, '-n', '--')

Usage:

import scap.cmd as cmd

print cmd.SSH(cmd.SUDO('hi', 'hello', user=env['ssh_user']))
# > ['/usr/bin/ssh', '-oBatchMode=yes', '-oSetupTimeout=10', '-F/dev/null', 'sudo', '-u mwdeploy', '-n', '--', 'hi', 'hello']
In D70#1672, @mmodell wrote:

I like the direction this is headed, however, there is a lot of duplication of effort going on between the three of us all working on similar things / areas of the code.

I completely agree. Let's use tomorrow's sprint meeting to get consensus on what's in need of refactoring lest we end up introducing more cruft/debt.

I've got some code to abstract the command execution in cluster_ssh, which I'll submit as a diff soon, but in case you are working on the same thing, maybe take a look?

Looks sane to me and not something I was planning on addressing, though I could see possible interaction between these two modules.

In D70#1681, @dduvall wrote:

Looks sane to me and not something I was planning on addressing, though I could see possible interaction between these two modules.

Yeah quite a lot of interaction.

dduvall updated this revision to Diff 212.Dec 5 2015, 12:52 AM
dduvall edited edge metadata.

Renamed env/Environment module/classes to directory/Directory to avoid ambiguity over the term environment and the exact concerns of the module.

dduvall updated this revision to Diff 218.Dec 7 2015, 9:44 PM

Finished refactoring Deploy and implemented HostDirectory.env_specific_path for resolving config things.

looks good, and works on my scap-vagrant (now that I finally got it working. I had to completely nuke everything and start over...)

mmodell accepted this revision.Dec 9 2015, 5:19 AM
mmodell added a reviewer: mmodell.
This revision is now accepted and ready to land.Dec 9 2015, 5:19 AM
dduvall updated this revision to Diff 224.Dec 11 2015, 8:58 PM
dduvall edited edge metadata.

Renamed module and classes to context/Context/HostContext/TargetContext. Directory didn't really sense either as the classes also encapsulate the user that commands run as. Oh naming.

This revision now requires review to proceed.Dec 11 2015, 8:58 PM
dduvall updated this revision to Diff 225.Dec 11 2015, 8:59 PM
dduvall retitled this revision from WIP: Refactor host/target environment management to Refactor host/target context management.
dduvall updated this object.
dduvall edited the test plan for this revision. (Show Details)
dduvall edited edge metadata.

Updated title/summary using arc diff --edit --verbatim

There are unit test failures even running this locally after applying the patch.

   BROKEN  test_context.TargetContextTest.create_test_rev_dir
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
TypeError: create_test_rev_dir() takes exactly 2 arguments (1 given)

   BROKEN  test_context.TargetContextTest.create_test_rev_dirs
Traceback (most recent call last):
  File "/usr/lib/python2.7/unittest/case.py", line 329, in run
    testMethod()
TypeError: create_test_rev_dirs() takes exactly 2 arguments (1 given)

Strange. The tests only fail for me when running them through arc unit. Running them directly works fine.

thcipriani requested changes to this revision.Dec 19 2015, 1:27 AM
thcipriani added a reviewer: thcipriani.
thcipriani added a subscriber: thcipriani.

The context manager looks really awesome. I have a few inline questions, but overall looks great.

I spotted two bugs. One of the bugs I caused a long time ago, and one bug is in this revision (see line 141 of deploy.py), and you likely would have noticed it if not for my bug shadowing it :P

scap/context.py
50

Any reason not to do these things in __init__?

80

This is a much improved function :)

scap/deploy.py
31

Good call moving these.

136

I have just realized that this will never be true, unless both the config being deployed AND the sha1 of the deployed directory revisions have not changed. This whole deploy has to be a no-op in order to fall into this block.

This is because the config_file_tree dict that is being hashed to generate a sha1 for the config is keyed using source which uses source_basepath which uses the current rev.

This can be verified in practice. If you add a new commit to a repo and then deploy without changing the config, you'll notice that the prefix of the directory under [repo-cache]/revs/ has changed even though if you run an md5sum on the deployed config files between the two revisions, they stay the same.

This means any time we're running a deployment with anything new, this block is never fallen into. Which explains why the bug I noticed on line 141 doesn't show up :)

141

This else shouldn't be here.

Once the bug I noted on line 136 is fixed, this will become a bug—note the comments on line 142.

The problem here is that if the config hasn't changed, we still need to record the config hash in tmp/.config-digest so that the rev-directory (created during fetch) is called [config-digest-sha1]_[rev-sha1]. Otherwise it will be called [rev-sha1].

630

Could this function be removed and instead could the context object be initialized in self._setup_loggers?

Maybe a _setup_context stage for cli.Application?

This revision now requires changes to proceed.Dec 19 2015, 1:27 AM
dduvall added inline comments.Dec 21 2015, 11:50 PM
scap/context.py
50

I generally shy away from doing FS operations or other side-effect-y things in constructors, otherwise you can't maintain 100% idempotency for purely introspective methods. (i.e. I'm a zealot.)

dduvall updated this revision to Diff 237.Dec 21 2015, 11:54 PM
dduvall edited edge metadata.

Skipping unit tests that exercise ln commands on OS X.

dduvall updated this revision to Diff 256.Jan 11 2016, 11:05 PM
dduvall edited edge metadata.

Fixed else bug in config deploy and added test for use of Context.env_specific_paths without arguments.

thcipriani accepted this revision.Jan 11 2016, 11:06 PM
thcipriani edited edge metadata.
thcipriani added inline comments.
scap/context.py
51

I'm a zealot.

Makes sense.

This revision is now accepted and ready to land.Jan 11 2016, 11:06 PM
This revision was automatically updated to reflect the committed changes.