Merge lp:~doanac/uci-engine/pub-pause into lp:uci-engine

[Andy Doan]

lander: allow a worker to request a 'retry'

Workers like the publisher need to mark a ticket step as needing
to be retried. This adds a function, amqp_utils.progress_retry. When
called by a worker, the lander will pause the ticket and wait until
the ticket is updated to "resume". At this point the lander will resume
execution at this step and see if it can progress

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:589
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/878/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/878/rebuild

review: Approve (continuous-integration)

[Ursula Junque (ursinha)]

Hi Andy,

I've asked some questions on IRC a while ago but I think they got lost -- I should've asked them here, sorry about that.

[Joe Talbott (joetalbott)]

This looks okay to me code-wise. I'll leave this MP to you and Ursula to sort out as far as her comments are concerned.

[Andy Doan (doanac)]

I've responded to Ursula. I'd say we keep it the way I have it, but I'll let you guys decide.

[Francis Ginther (fginther)]

I think this does what it's intended to do (and there are unit tests to match). The progress_retry() isn't being used anywhere, so this shouldn't regress anything.

Only question is if the name 'progress_retry' was meant to be named 'progress_pause'?

I'll approve as we really can't do much more with it until something else is in place to exercise it.

review: Approve

[Andy Doan (doanac)]

On Thu, Jul 17, 2014 at 10:32 PM, Francis Ginther <
<email address hidden>> wrote:

> > +def progress_retry(progress_trigger, data=None):
> > + '''Let the engine know the ticket should be paused and retried
> after some
> > + manual acknowledgement has taken place
> > + '''
> > + if data is None:
> > + data = {}
> > + data['state'] = 'RETRY'
>
> It sounds a bit odd to have this method called 'progress_retry' when the
> outcome is to pause the workflow. I think this is what Ursula is pointing
> out.
>

Ah - i get it now. So "retry" in this since is still a manual thing that
takes advantage of our internal "pause" support. ie - the way the step
would get repeated would be for some interaction that puts the ticket
status into "continue"

Unmerged revisions

589. By Andy Doan on 2014-06-16

lander: allow a worker to request a 'retry'

Workers like the publisher need to mark a ticket step as needing
to be retried. This adds a function, amqp_utils.progress_retry. When
called by a worker, the lander will pause the ticket and wait until
the ticket is updated to "resume". At this point the lander will resume
execution at this step and see if it can progress

=== modified file 'ci-utils/ci_utils/amqp_utils.py'
--- ci-utils/ci_utils/amqp_utils.py	2014-06-16 13:18:53 +0000
+++ ci-utils/ci_utils/amqp_utils.py	2014-06-17 14:51:54 +0000
@@ -199,3 +199,16 @@
     data['result'] = 'FAILED'
     data['exit'] = True
     return send(progress_trigger, json.dumps(data))
+
+
+def progress_retry(progress_trigger, data=None):
+    '''Let the engine know the ticket should be paused and retried after some
+    manual acknowledgement has taken place
+    '''
+    if data is None:
+        data = {}
+    data['state'] = 'RETRY'
+    data['result'] = 'PASSED'
+    data['pause'] = {}
+    data['exit'] = True
+    return send(progress_trigger, json.dumps(data))

=== modified file 'lander/bin/lander_process_ticket.py'
--- lander/bin/lander_process_ticket.py	2014-06-16 13:18:53 +0000
+++ lander/bin/lander_process_ticket.py	2014-06-17 14:51:54 +0000
@@ -34,6 +34,11 @@
     rc = subprocess.call(
         [cmd, '--service', step['name'], '--results-file', results_file])
     logger.debug('Step completed with rc=%d', rc)
+
+    # load the result to see if the step requested a "retry", which means
+    # we are basically pausing this step
+    wflow = workflow.load(results_file)
+    _handle_pause(results_file, wflow, wflow.get_step(step['name']))
     return rc
 
 

=== modified file 'lander/bin/lander_service_wrapper.py'
--- lander/bin/lander_service_wrapper.py	2014-06-11 19:16:37 +0000
+++ lander/bin/lander_service_wrapper.py	2014-06-17 14:51:54 +0000
@@ -68,7 +68,6 @@
         if state == 'STATUS':
             self.started_cb()
         elif state == 'COMPLETED':
-            self.ticket.process_artifacts(data)
             if data['result'] == 'PASSED':
                 self.completed_cb(True)
             else:
@@ -76,6 +75,7 @@
 
         msg.channel.basic_ack(msg.delivery_tag)
         if data.get('exit'):
+            self.ticket.process_artifacts(data)
             self.data = data
             # this will kick us out of the run-forever worker loop
             msg.channel.basic_cancel(msg.consumer_tag)

=== modified file 'lander/lander/tests/test_process_ticket.py'
--- lander/lander/tests/test_process_ticket.py	2014-06-10 16:21:15 +0000
+++ lander/lander/tests/test_process_ticket.py	2014-06-17 14:51:54 +0000
@@ -116,3 +116,22 @@
             lander_process_ticket.main(args)
         self.assertEqual(1, do_step.call_count)
         self.assertEqual(1, ticket_api.TicketApi().pause_ticket.call_count)
+
+    @mock.patch('lander.ticket_api')
+    @mock.patch('subprocess.call')
+    def test_run_retry(self, subprocess, ticket_api):
+        subprocess.return_value = 0
+        data = {
+            'ticket_id': 10,
+            'ts_url': 'foo',
+            'steps': [
+                {'name': 'step1', 'pause': {}},
+            ],
+        }
+        wflow = workflow._Workflow(data)
+        with open(self.tmpfile, 'w') as f:
+            wflow.save(f)
+        with self.assertRaises(SystemExit):
+            step = wflow.get_step('step1')
+            lander_process_ticket._do_step(step, self.tmpfile)
+        self.assertEqual(1, ticket_api.TicketApi().pause_ticket.call_count)

=== modified file 'lander/lander/tests/test_service_wrapper.py'
--- lander/lander/tests/test_service_wrapper.py	2014-06-10 16:21:15 +0000
+++ lander/lander/tests/test_service_wrapper.py	2014-06-17 14:51:54 +0000
@@ -432,6 +432,38 @@
         self.assertEqual('FAILED', data['result'])
         self.assertEqual(err_msg, data['error'])
 
+    @mock.patch('lander.ticket_api.TicketApi')
+    @mock.patch('ci_utils.amqp_utils.send')
+    @mock.patch('ci_utils.amqp_utils.connection')
+    def testRunnerRetry(self, connection, send, api):
+        '''Ensure we generate the proper response if we get a retry msg.'''
+        api().get_full_ticket.return_value = {}
+        # mock the run-forever logic to ensure we return what we expect
+        state = {'state': 'RETRY', 'result': 'PASSED',
+                 'foo': 'bar', 'exit': True, 'name': 'test_runner'}
+
+        def amqp():
+            def run_forever(channel, queue, callback):
+                m = mock.Mock()
+                m.body = json.dumps({'state': 'WAITING'})
+                callback(m)
+                m.body = json.dumps(state)
+                callback(m)
+            return run_forever
+
+        args = self.base_args + ['--service', 'test_runner']
+        args = lander_service_wrapper._get_parser().parse_args(args)
+
+        with mock.patch('ci_utils.amqp_utils._run_forever', new_callable=amqp):
+            rc = lander_service_wrapper.main(args)
+            self.assertEqual(0, rc)
+
+        api().set_testing_waiting.assert_called_once()
+        self.assertEqual(0, api().set_testing_complete.call_count)
+
+        data = lander.workflow.load(self.results).get_step('test_runner')
+        self.assertEqual(state, data)
+
     @mock.patch('lander_service_wrapper._patch')
     def testPPAFreeSucceeds(self, patch):
         '''ensure we can free a PPA'''