From ce56ff9756b2d3847d06deadbe65d33b362e4637 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Fri, 18 Aug 2017 14:39:06 -0700 Subject: [PATCH] Add wrapper driver execution context We recently began altering the mount map used by the wrapper driver for each execution run (so that we can only include the current playbook). However, the setMountsMap method operates on the global driver object rather than an object more closely bound to the lifetime of the playbook run. The fact that this works at all is just luck (executing process is slow enough that hitting a race condition where the wrong directories are mounted is unlikely). To correct this, add a new layer which contains the context for the current playbook execution. Change-Id: I3a06f19e88435a49c7b9aea4e1221b812f5a43d0 --- tests/unit/test_bubblewrap.py | 12 +++--- zuul/driver/__init__.py | 24 +++++------- zuul/driver/bubblewrap/__init__.py | 61 ++++++++++++++++++------------ zuul/driver/nullwrap/__init__.py | 15 +++++--- zuul/execution_context/__init__.py | 40 ++++++++++++++++++++ zuul/executor/server.py | 6 +-- 6 files changed, 105 insertions(+), 53 deletions(-) create mode 100644 zuul/execution_context/__init__.py diff --git a/tests/unit/test_bubblewrap.py b/tests/unit/test_bubblewrap.py index d94b3f2227..bb1be733c7 100644 --- a/tests/unit/test_bubblewrap.py +++ b/tests/unit/test_bubblewrap.py @@ -32,12 +32,13 @@ class TestBubblewrap(testtools.TestCase): def test_bubblewrap_wraps(self): bwrap = bubblewrap.BubblewrapDriver() + context = bwrap.getExecutionContext() work_dir = tempfile.mkdtemp() ssh_agent = SshAgent() self.addCleanup(ssh_agent.stop) ssh_agent.start() - po = bwrap.getPopen(work_dir=work_dir, - ssh_auth_sock=ssh_agent.env['SSH_AUTH_SOCK']) + po = context.getPopen(work_dir=work_dir, + ssh_auth_sock=ssh_agent.env['SSH_AUTH_SOCK']) self.assertTrue(po.passwd_r > 2) self.assertTrue(po.group_r > 2) self.assertTrue(work_dir in po.command) @@ -55,14 +56,15 @@ class TestBubblewrap(testtools.TestCase): def test_bubblewrap_leak(self): bwrap = bubblewrap.BubblewrapDriver() + context = bwrap.getExecutionContext() work_dir = tempfile.mkdtemp() ansible_dir = tempfile.mkdtemp() ssh_agent = SshAgent() self.addCleanup(ssh_agent.stop) ssh_agent.start() - po = bwrap.getPopen(work_dir=work_dir, - ansible_dir=ansible_dir, - ssh_auth_sock=ssh_agent.env['SSH_AUTH_SOCK']) + po = context.getPopen(work_dir=work_dir, + ansible_dir=ansible_dir, + ssh_auth_sock=ssh_agent.env['SSH_AUTH_SOCK']) leak_time = 60 # Use hexadecimal notation to avoid false-positive true_proc = po(['bash', '-c', 'sleep 0x%X & disown' % leak_time]) diff --git a/zuul/driver/__init__.py b/zuul/driver/__init__.py index 6ac9197535..9096197f15 100644 --- a/zuul/driver/__init__.py +++ b/zuul/driver/__init__.py @@ -258,25 +258,19 @@ class WrapperInterface(object, metaclass=abc.ABCMeta): """ @abc.abstractmethod - def getPopen(self, **kwargs): - """Create and return a subprocess.Popen factory wrapped however the - driver sees fit. + def getExecutionContext(self, ro_paths=None, rw_paths=None): + """Create and return an execution context. + + The execution context is meant to be used for a single + invocation of a command. This method is required by the interface - :arg dict kwargs: key/values for use by driver as needed - - :returns: a callable that takes the same args as subprocess.Popen - :rtype: Callable - """ - pass - - @abc.abstractmethod - def setMountsMap(self, state_dir, ro_paths=None, rw_paths=None): - """Add additional mount point to the execution environment. - - :arg str state_dir: the state directory to be read write :arg list ro_paths: read only files or directories to bind mount :arg list rw_paths: read write files or directories to bind mount + + :returns: a new ExecutionContext object. + :rtype: BaseExecutionContext + """ pass diff --git a/zuul/driver/bubblewrap/__init__.py b/zuul/driver/bubblewrap/__init__.py index cbaa6099d9..d70329ed3f 100644 --- a/zuul/driver/bubblewrap/__init__.py +++ b/zuul/driver/bubblewrap/__init__.py @@ -27,6 +27,7 @@ import re from typing import Dict, List # flake8: noqa from zuul.driver import (Driver, WrapperInterface) +from zuul.execution_context import BaseExecutionContext class WrappedPopen(object): @@ -69,27 +70,11 @@ class WrappedPopen(object): self.group_r = None -class BubblewrapDriver(Driver, WrapperInterface): - name = 'bubblewrap' - log = logging.getLogger("zuul.BubblewrapDriver") +class BubblewrapExecutionContext(BaseExecutionContext): + log = logging.getLogger("zuul.BubblewrapExecutionContext") - mounts_map = {'rw': [], 'ro': []} # type: Dict[str, List] - release_file_re = re.compile('^\W+-release$') - - def __init__(self): - self.bwrap_command = self._bwrap_command() - - def reconfigure(self, tenant): - pass - - def stop(self): - pass - - def setMountsMap(self, ro_paths=None, rw_paths=None): - if not ro_paths: - ro_paths = [] - if not rw_paths: - rw_paths = [] + def __init__(self, bwrap_command, ro_paths, rw_paths): + self.bwrap_command = bwrap_command self.mounts_map = {'ro': ro_paths, 'rw': rw_paths} def getPopen(self, **kwargs): @@ -145,6 +130,22 @@ class BubblewrapDriver(Driver, WrapperInterface): return wrapped_popen + +class BubblewrapDriver(Driver, WrapperInterface): + log = logging.getLogger("zuul.BubblewrapDriver") + name = 'bubblewrap' + + release_file_re = re.compile('^\W+-release$') + + def __init__(self): + self.bwrap_command = self._bwrap_command() + + def reconfigure(self, tenant): + pass + + def stop(self): + pass + def _bwrap_command(self): bwrap_command = [ 'bwrap', @@ -185,6 +186,15 @@ class BubblewrapDriver(Driver, WrapperInterface): return bwrap_command + def getExecutionContext(self, ro_paths=None, rw_paths=None): + if not ro_paths: + ro_paths = [] + if not rw_paths: + rw_paths = [] + return BubblewrapExecutionContext( + self.bwrap_command, + ro_paths, rw_paths) + def main(args=None): logging.basicConfig(level=logging.DEBUG) @@ -192,18 +202,19 @@ def main(args=None): driver = BubblewrapDriver() parser = argparse.ArgumentParser() - parser.add_argument('--ro-bind', nargs='+') - parser.add_argument('--rw-bind', nargs='+') + parser.add_argument('--ro-paths', nargs='+') + parser.add_argument('--rw-paths', nargs='+') parser.add_argument('work_dir') parser.add_argument('run_args', nargs='+') cli_args = parser.parse_args() ssh_auth_sock = os.environ.get('SSH_AUTH_SOCK') - driver.setMountsMap(cli_args.ro_bind, cli_args.rw_bind) + context = driver.getExecutionContext( + cli_args.ro_paths, cli_args.rw_paths) - popen = driver.getPopen(work_dir=cli_args.work_dir, - ssh_auth_sock=ssh_auth_sock) + popen = context.getPopen(work_dir=cli_args.work_dir, + ssh_auth_sock=ssh_auth_sock) x = popen(cli_args.run_args) x.wait() diff --git a/zuul/driver/nullwrap/__init__.py b/zuul/driver/nullwrap/__init__.py index 50fea2759a..d807904951 100644 --- a/zuul/driver/nullwrap/__init__.py +++ b/zuul/driver/nullwrap/__init__.py @@ -18,14 +18,19 @@ import logging import subprocess from zuul.driver import (Driver, WrapperInterface) +from zuul.execution_context import BaseExecutionContext + + +class NullExecutionContext(BaseExecutionContext): + log = logging.getLogger("zuul.NullExecutionContext") + + def getPopen(self, **kwargs): + return subprocess.Popen class NullwrapDriver(Driver, WrapperInterface): name = 'nullwrap' log = logging.getLogger("zuul.NullwrapDriver") - def getPopen(self, **kwargs): - return subprocess.Popen - - def setMountsMap(self, **kwargs): - pass + def getExecutionContext(self, ro_paths=None, rw_paths=None): + return NullExecutionContext() diff --git a/zuul/execution_context/__init__.py b/zuul/execution_context/__init__.py new file mode 100644 index 0000000000..29b29122b4 --- /dev/null +++ b/zuul/execution_context/__init__.py @@ -0,0 +1,40 @@ +# Copyright 2016 Red Hat, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import abc + + +class BaseExecutionContext(object, metaclass=abc.ABCMeta): + """The execution interface returned by a wrapper. + + Wrapper drivers return instances which implement this interface. + + It is used to hold information and aid in the execution of a + single command. + + """ + + @abc.abstractmethod + def getPopen(self, **kwargs): + """Create and return a subprocess.Popen factory wrapped however the + driver sees fit. + + This method is required by the interface + + :arg dict kwargs: key/values for use by driver as needed + + :returns: a callable that takes the same args as subprocess.Popen + :rtype: Callable + """ + pass diff --git a/zuul/executor/server.py b/zuul/executor/server.py index 9340f093cf..55e47da7bb 100644 --- a/zuul/executor/server.py +++ b/zuul/executor/server.py @@ -1509,10 +1509,10 @@ class AnsibleJob(object): if self.executor_variables_file: ro_paths.append(self.executor_variables_file) - self.executor_server.execution_wrapper.setMountsMap(ro_paths, - rw_paths) + context = self.executor_server.execution_wrapper.getExecutionContext( + ro_paths, rw_paths) - popen = self.executor_server.execution_wrapper.getPopen( + popen = context.getPopen( work_dir=self.jobdir.work_root, ssh_auth_sock=env_copy.get('SSH_AUTH_SOCK'))