From 52ce1e4bc0d5fd00251b02e3652fcf8ab6db94d6 Mon Sep 17 00:00:00 2001
From: Lukas Brabec <lbrabec@redhat.com>
Date: Mar 07 2018 10:37:48 +0000
Subject: [PATCH 1/2] Retry to spawn disposable client


---

diff --git a/conf/taskotron.yaml.example b/conf/taskotron.yaml.example
index b35bd2f..b0482a4 100644
--- a/conf/taskotron.yaml.example
+++ b/conf/taskotron.yaml.example
@@ -144,6 +144,9 @@
 #default_disposable_flavor: taskotron_cloud
 #default_disposable_arch: x86_64
 
+## Number of retries when disposable client fails to boot within timeout
+#spawn_vm_retries: 3
+
 ## Additional repos for minion to install packages from
 #minion_repos:
 #  - https://server1/repo/
diff --git a/libtaskotron/config_defaults.py b/libtaskotron/config_defaults.py
index 8202e00..ebd569c 100644
--- a/libtaskotron/config_defaults.py
+++ b/libtaskotron/config_defaults.py
@@ -90,6 +90,8 @@ class Config(object):
     default_disposable_flavor = 'taskotron_cloud'                           #:
     default_disposable_arch = 'x86_64'                                      #:
 
+    spawn_vm_retries = 3                                                    #:
+
     minion_repos = []                                                       #:
 
     log_level_stream = 'INFO'                                               #:
diff --git a/libtaskotron/executor.py b/libtaskotron/executor.py
index 45015f1..3dd4b57 100644
--- a/libtaskotron/executor.py
+++ b/libtaskotron/executor.py
@@ -55,14 +55,26 @@ class Executor(object):
 
         env = image_utils.devise_environment(self.arg_data)
         self.task_vm = vm.TestCloudMachine(uuid)
-        self.task_vm.prepare(**env)
 
-        self.task_vm.wait_for_port(22)
+        retries = config.get_config().spawn_vm_retries
 
-        log.debug('Disposable client (%s %s) ready', self.task_vm.instancename,
+        while retries > 0:
+            try:
+                self.task_vm.prepare(**env)
+                self.task_vm.wait_for_port(22)
+
+                log.debug('Disposable client (%s %s) ready', self.task_vm.instancename,
                   self.task_vm.ipaddr)
 
-        return self.task_vm.ipaddr
+                return self.task_vm.ipaddr
+            except vm.TestcloudInstanceError, e:
+                log.info('Disposable client failed to boot: %s, retrying.' % e)
+                self.task_vm.teardown()
+
+            retries -= 1
+
+        raise exc.TaskotronRemoteError('Disposable client failed to boot')
+
 
     def _get_client_ipaddr(self):
         '''Get an IP address of the machine the task is going to be executed
diff --git a/testing/test_executor.py b/testing/test_executor.py
index 60a4ede..135296e 100644
--- a/testing/test_executor.py
+++ b/testing/test_executor.py
@@ -16,6 +16,8 @@ from libtaskotron import executor
 import libtaskotron.exceptions as exc
 from libtaskotron.directives import resultsdb_directive
 from libtaskotron import os_utils
+from libtaskotron.ext.disposable import vm
+from libtaskotron import config
 
 PLAYBOOK='''
 - hosts: localhost
@@ -269,6 +271,21 @@ class TestExecutor():
         mock_report_results.assert_called_once()
         mock_task_vm.teardown.assert_called_once()
 
+    def test_execute_libvirt_retries(self, monkeypatch):
+        '''Execution using libvirt mode, vm boot retries'''
+
+        mock_vm_prepare = mock.Mock(side_effect=vm.TestcloudInstanceError)
+        mock_vm = mock.Mock(return_value=mock.Mock(prepare=mock_vm_prepare))
+        monkeypatch.setattr(vm, 'TestCloudMachine', mock_vm)
+
+        self.executor.arg_data['local'] = False
+        self.executor.arg_data['libvirt'] = True
+
+        with pytest.raises(exc.TaskotronRemoteError):
+            self.executor.execute()
+
+        assert mock_vm_prepare.call_count == config.get_config().spawn_vm_retries
+
     def test_execute_no_playbooks(self, monkeypatch):
         '''Should raise when there are no playbooks'''
         mock_run_playbook = mock.Mock()

From f97dc3e1ef93e7b5b53bdd1568f00023636e84e7 Mon Sep 17 00:00:00 2001
From: Kamil Páral <kparal@redhat.com>
Date: Mar 07 2018 12:11:33 +0000
Subject: [PATCH 2/2] small improvements during review


---

diff --git a/libtaskotron/exceptions.py b/libtaskotron/exceptions.py
index 020863a..f5dd479 100644
--- a/libtaskotron/exceptions.py
+++ b/libtaskotron/exceptions.py
@@ -93,3 +93,8 @@ class TaskotronInterruptError(TaskotronError):
 
     def __str__(self):
         return 'Received system signal %d (%s)' % (self.signum, self.signame)
+
+
+class TaskotronMinionError(TaskotronError):
+    '''All errors related to persistent or disposable minion handling'''
+    pass
diff --git a/libtaskotron/executor.py b/libtaskotron/executor.py
index 3dd4b57..7cc497c 100644
--- a/libtaskotron/executor.py
+++ b/libtaskotron/executor.py
@@ -59,22 +59,24 @@ class Executor(object):
         retries = config.get_config().spawn_vm_retries
 
         while retries > 0:
+            retries -= 1
             try:
                 self.task_vm.prepare(**env)
                 self.task_vm.wait_for_port(22)
 
-                log.debug('Disposable client (%s %s) ready', self.task_vm.instancename,
-                  self.task_vm.ipaddr)
+                log.debug('Disposable client (%s %s) ready',
+                    self.task_vm.instancename,
+                    self.task_vm.ipaddr)
 
                 return self.task_vm.ipaddr
             except vm.TestcloudInstanceError, e:
-                log.info('Disposable client failed to boot: %s, retrying.' % e)
-                self.task_vm.teardown()
-
-            retries -= 1
-
-        raise exc.TaskotronRemoteError('Disposable client failed to boot')
-
+                if retries <= 0:
+                    raise exc.TaskotronMinionError('Disposable client failed '
+                        'to boot: %s', e)
+                else:
+                    log.warning('Disposable client failed to boot, retrying: '
+                        '%s', e)
+                    self.task_vm.teardown()
 
     def _get_client_ipaddr(self):
         '''Get an IP address of the machine the task is going to be executed
diff --git a/testing/test_executor.py b/testing/test_executor.py
index 135296e..6b11e29 100644
--- a/testing/test_executor.py
+++ b/testing/test_executor.py
@@ -18,6 +18,7 @@ from libtaskotron.directives import resultsdb_directive
 from libtaskotron import os_utils
 from libtaskotron.ext.disposable import vm
 from libtaskotron import config
+import testcloud.exceptions
 
 PLAYBOOK='''
 - hosts: localhost
@@ -35,7 +36,7 @@ PLAYBOOK='''
 class TestExecutor():
 
     @pytest.fixture
-    def setup(self, tmpdir):
+    def setup(self, tmpdir, monkeypatch):
         '''Run this before every test invocation'''
         self.artifactsdir = tmpdir.mkdir('artifacts')
         self.taskdir = tmpdir.mkdir('taskdir')
@@ -58,6 +59,10 @@ class TestExecutor():
         self.ipaddr = '127.0.0.1'
         self.executor = executor.Executor(self.arg_data)
 
+        monkeypatch.setattr(config, '_config', None)
+        self.conf = config.get_config()
+        self.conf.spawn_vm_retries = 5
+
     def test_interrupt_handler(self):
         '''Raise correct exception for an interrupt'''
 
@@ -271,21 +276,6 @@ class TestExecutor():
         mock_report_results.assert_called_once()
         mock_task_vm.teardown.assert_called_once()
 
-    def test_execute_libvirt_retries(self, monkeypatch):
-        '''Execution using libvirt mode, vm boot retries'''
-
-        mock_vm_prepare = mock.Mock(side_effect=vm.TestcloudInstanceError)
-        mock_vm = mock.Mock(return_value=mock.Mock(prepare=mock_vm_prepare))
-        monkeypatch.setattr(vm, 'TestCloudMachine', mock_vm)
-
-        self.executor.arg_data['local'] = False
-        self.executor.arg_data['libvirt'] = True
-
-        with pytest.raises(exc.TaskotronRemoteError):
-            self.executor.execute()
-
-        assert mock_vm_prepare.call_count == config.get_config().spawn_vm_retries
-
     def test_execute_no_playbooks(self, monkeypatch):
         '''Should raise when there are no playbooks'''
         mock_run_playbook = mock.Mock()
@@ -361,3 +351,30 @@ class TestExecutor():
         assert finished == False
         mock_run_playbook.assert_called_once()
         mock_report_results.assert_not_called()
+
+    def test_spawn_vm_retries(self, monkeypatch):
+        '''Spawning VM should be attempted several times on failure'''
+        mock_vm_prepare = mock.Mock(
+            side_effect=[testcloud.exceptions.TestcloudInstanceError,
+                mock.DEFAULT])
+        mock_vm = mock.Mock()
+        vm_instance = mock_vm.return_value
+        vm_instance.prepare = mock_vm_prepare
+        vm_instance.ipaddr = '10.11.12.13'
+        monkeypatch.setattr(vm, 'TestCloudMachine', mock_vm)
+
+        ipaddr = self.executor._spawn_vm(None)
+        assert ipaddr == '10.11.12.13'
+        assert mock_vm_prepare.call_count == 2
+
+    def test_spawn_vm_complete_fail(self, monkeypatch):
+        '''Exception should be raised when VM can't be spawned using retries'''
+        mock_vm_prepare = mock.Mock(
+            side_effect=testcloud.exceptions.TestcloudInstanceError)
+        mock_vm = mock.Mock(return_value=mock.Mock(prepare=mock_vm_prepare))
+        monkeypatch.setattr(vm, 'TestCloudMachine', mock_vm)
+
+        with pytest.raises(exc.TaskotronMinionError):
+            self.executor._spawn_vm(None)
+
+        assert mock_vm_prepare.call_count == self.conf.spawn_vm_retries

