Merge lp:~ricardokirkner/ols-jenkaas/land-approvals-without-container into lp:~ols-jenkaas-admins/ols-jenkaas/trunk

[Ricardo Kirkner]

support landing approved proposals by running landing tests in host (for docker projects)

if no container is specified, the land approval command will run the command in the host itself

[Daniel Manrique (roadmr)]

Looks OK in principle/spirit. I'm a bit scared about running stuff not-in-a-container, though launching a docker image should be somewhat equivalent.

See a comment below, and also, could you document this magic behavior somewhere?

Example, the README says

All test jobs occur inside an lxd container, only the push to launchpad
occur on the slaves themselves so that the needed credentials are never
exposed to test jobs.

this is potentially no longer accurate.

review: Needs Information

[lp:~ricardokirkner/ols-jenkaas/land-approvals-without-container]

516. By Ricardo Kirkner on 2017-09-13

flip condition for improved readability

517. By Ricardo Kirkner on 2017-09-13

updated README

[Daniel Manrique (roadmr)]

Looks OK, I'm still somewhat scared by this but it's worth a try.

review: Approve

=== modified file 'README.rst'
--- README.rst	2017-06-22 01:18:29 +0000
+++ README.rst	2017-09-13 15:29:22 +0000
@@ -6,7 +6,9 @@
 
 All test jobs occur inside an lxd container, only the push to launchpad
 occur on the slaves themselves so that the needed credentials are never
-exposed to test jobs.
+exposed to test jobs. The only exception is when jobs use docker, in which case
+the tests are run inside the docker containers but docker itself is run
+in the host.
 
 Another container is used to create and update the jobs from a version
 controlled branch.

=== modified file 'olsjenkaas/commands.py'
--- olsjenkaas/commands.py	2017-08-10 17:48:33 +0000
+++ olsjenkaas/commands.py	2017-09-13 15:29:22 +0000
@@ -304,6 +304,11 @@
     def run_in_container(self, command, with_creds=False):
         return self.worker.raw_shell(command, with_agent=with_creds)
 
+    def run_in_host(self, command):
+        out = None if self.out == sys.stdout else self.out
+        err = None if self.err == sys.stderr else self.err
+        return containers.raw_run(command, out=out, err=err, shell=True)
+
 
 class RunInWorker(CommandWithContainer):
 
@@ -483,22 +488,36 @@
                 raise Rejected('There was nothing to merge')
             mounts, remote_work_dir = self.get_mounts_and_remote(
                 local_work_dir)
-            logger.info('Creating worker...')
-            with olsjenkaas.working_directory(local_work_dir):
-                self.create_worker(self.options.container_name, mounts=mounts)
-            logger.info('{} setup on top of {}'.format(
-                self.worker.name, self.worker.backing_name))
-            logger.info('Running {}'.format(self.options.setup_cmd))
-            if self.run_in_container(
-                    ['cd', remote_work_dir, '&&', self.options.setup_cmd],
-                    with_creds=True):
-                logger.error('{} failed'.format(self.options.setup_cmd))
-                raise Rejected('Project setup failed')
-            logger.info('Running {}'.format(self.options.test_cmd))
-            if self.run_in_container(
-                    ['cd', remote_work_dir, '&&', self.options.test_cmd]):
-                logger.error('{} failed'.format(self.options.test_cmd))
-                raise Rejected('Running landing tests failed')
+            if self.options.container_name == '':
+                # no container name specified, run in host
+                logger.info('Running {}'.format(self.options.setup_cmd))
+                if self.run_in_host(
+                       ['cd', local_work_dir, '&&', self.options.setup_cmd]):
+                    logger.error('{} failed'.format(self.options.setup_cmd))
+                    raise Rejected('Project setup failed')
+                logger.info('Running {}'.format(self.options.test_cmd))
+                if self.run_in_host(
+                        ['cd', local_work_dir, '&&', self.options.test_cmd]):
+                    logger.error('{} failed'.format(self.options.test_cmd))
+                    raise Rejected('Running landing tests failed')
+            else:
+                # run in named container
+                logger.info('Creating worker...')
+                with olsjenkaas.working_directory(local_work_dir):
+                    self.create_worker(self.options.container_name, mounts=mounts)
+                logger.info('{} setup on top of {}'.format(
+                    self.worker.name, self.worker.backing_name))
+                logger.info('Running {}'.format(self.options.setup_cmd))
+                if self.run_in_container(
+                        ['cd', remote_work_dir, '&&', self.options.setup_cmd],
+                        with_creds=True):
+                    logger.error('{} failed'.format(self.options.setup_cmd))
+                    raise Rejected('Project setup failed')
+                logger.info('Running {}'.format(self.options.test_cmd))
+                if self.run_in_container(
+                        ['cd', remote_work_dir, '&&', self.options.test_cmd]):
+                    logger.error('{} failed'.format(self.options.test_cmd))
+                    raise Rejected('Running landing tests failed')
             self.commit_proposal(local_work_dir, proposal)
             self.push_merged_proposal(local_work_dir, target_url)
             # FIXME: Once webhooks are used, lp can be trusted to do that (with

=== modified file 'olsjenkaas/containers.py'
--- olsjenkaas/containers.py	2017-08-30 16:20:36 +0000
+++ olsjenkaas/containers.py	2017-09-13 15:29:22 +0000
@@ -43,7 +43,7 @@
 
 
 # FIXME: Maybe worth backporting to ols-vms with tests -- vila 2017-01-25
-def raw_run(args, out=None, err=None):
+def raw_run(args, out=None, err=None, shell=False):
     """Invoke an ols-vms command, possibly capturing the output.
 
     The command line tool (and its API) are used to reduce the coupling.
@@ -52,7 +52,7 @@
     stdout = subprocess.PIPE if out is not None else None
     stderr = subprocess.PIPE if err is not None else None
     logger.debug('Running {}'.format(' '.join(args)))
-    proc = subprocess.Popen(args, stdout=stdout, stderr=stderr)
+    proc = subprocess.Popen(args, stdout=stdout, stderr=stderr, shell=shell)
     # If the output/error are not captured, communicate() will return empty
     # strings (i.e. either output and errors pass through or they are captured
     # by 'out' and 'err').

=== modified file 'olsjenkaas/tests/test_commands.py'
--- olsjenkaas/tests/test_commands.py	2017-08-10 17:48:33 +0000
+++ olsjenkaas/tests/test_commands.py	2017-09-13 15:29:22 +0000
@@ -525,6 +525,46 @@
             self.err.getvalue())
 
 
+class TestCommandInHost(unittest.TestCase):
+
+    def setUp(self):
+        super(TestCommandInHost, self).setUp()
+        fixtures.isolate_from_disk(self)
+        self.out = io.StringIO()
+        self.err = io.StringIO()
+        self.log_stream = io.StringIO()
+        fixtures.override_logging(self, self.log_stream)
+        self.command = commands.CommandWithContainer(
+            out=self.out, err=self.err)
+        fixtures.define_uniq_container(self)
+
+    def test_command_success(self):
+        ret = self.command.run_in_host(['/bin/true'])
+        self.assertEqual(0, ret)
+        self.assertEqual('', self.out.getvalue())
+        self.assertEqual('', self.err.getvalue())
+
+    def test_command_fails(self):
+        ret = self.command.run_in_host(['/bin/false'])
+        self.assertEqual(1, ret)
+        self.assertEqual('', self.out.getvalue())
+        self.assertEqual('', self.err.getvalue())
+
+    def test_command_output(self):
+        ret = self.command.run_in_host(['whoami'])
+        self.assertEqual(0, ret)
+        # This got captured by the command.out stream
+        self.assertEqual('{}\n'.format(os.getlogin()), self.out.getvalue())
+        self.assertEqual('', self.err.getvalue())
+
+    def test_command_with_shell_commands(self):
+        ret = self.command.run_in_host(['cd /tmp && pwd'])
+        self.assertEqual(0, ret)
+        # This got captured by the command.out stream
+        self.assertEqual('/tmp\n', self.out.getvalue())
+        self.assertEqual('', self.err.getvalue())
+
+
 class TestSetupVcs(unittest.TestCase):
 
     def setUp(self):
@@ -674,3 +714,21 @@
         last_comment = list(proposal.all_comments)[-1]
         self.assertEqual('Merging failed\n{}'.format(self.fake_url),
                          last_comment.message_body)
+
+    def test_landing_without_container_succeeds(self):
+        # ensure no container is defined
+        self.container_name = ''
+        proposal = fixtures.create_proposal(self, 'approved', approved=True)
+        ret = self.run_command([self.target_url, './work', '',
+                                '/bin/true', '/bin/true'])
+        self.assertEqual(0, ret)
+        self.assertLogEndsWith('{} has landed\n'.format(proposal.web_link))
+        log = self.log_stream.getvalue()
+        # ensure container was not involved
+        self.assertNotIn('Creating worker...', log)
+        self.assertNotIn('setup on top of', log)
+        proposal.lp_refresh()
+        # FIXME: Restore the following when webhooks are used and launchpad
+        # trusted to mark the MP as merged (and decorate commits)
+        # -- vila 2017-03-30
+        self.assertEqual('Merged', proposal.queue_status)