Differential D904

exc: adjust TaskotronRemoteTimeoutError hierarchy
ClosedPublic
Actions

Authored by kparal on Jun 24 2016, 1:45 PM.

Details

Reviewers
mkrizek
Commits
rLTRNcfbebdcf65a8: exc: adjust TaskotronRemoteTimeoutError hierarchy
Summary

Make TaskotronRemoteTimeoutError a descendant of
TaskotronRemoteError (which sounds logical). Ajust minion code to
reflect that change. Add tests.

This is a followup of D826.

Test Plan

test suite works, tested rpmlint execution locally and with a disposable client

Diff Detail

Repository
rLTRN libtaskotron
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kparal retitled this revision from to exc: adjust TaskotronRemoteTimeoutError hierarchy.Jun 24 2016, 1:45 PM
kparal updated this object.
kparal edited the test plan for this revision. (Show Details)
kparal added a reviewer: mkrizek.
mkrizek accepted this revision.Jun 27 2016, 5:22 AM

LGTM

This revision is now accepted and ready to land.Jun 27 2016, 5:22 AM
Closed by commit rLTRNcfbebdcf65a8: exc: adjust TaskotronRemoteTimeoutError hierarchy (authored by kparal). · Explain WhyJun 27 2016, 7:58 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathPackages
Mlibtaskotron/exceptions.py (2 lines)
Mlibtaskotron/minion.py (32 lines)
Mtesting/test_minion.py (61 lines)

Diff 2303

View Options

libtaskotron/exceptions.py

Show All 32 Lines31class TaskotronDirectiveError(TaskotronError):
33 pass 33​ pass
34 34​
35 35​
36class TaskotronRemoteError(TaskotronError): 36​class TaskotronRemoteError(TaskotronError):
37 '''All network and remote-server related errors''' 37​ '''All network and remote-server related errors'''
38 pass 38​ pass
39 39​
40 40​
41class TaskotronRemoteTimeoutError(TaskotronError): 41​class TaskotronRemoteTimeoutError(TaskotronRemoteError):
42 '''A remote-server did not send any output in a given timeout''' 42​ '''A remote-server did not send any output in a given timeout'''
43 pass 43​ pass
44 44​
45 45​
46class TaskotronRemoteProcessError(TaskotronRemoteError): 46​class TaskotronRemoteProcessError(TaskotronRemoteError):
47 '''A process/command executed remotely returned a non-zero exit code, crashed or failed in a 47​ '''A process/command executed remotely returned a non-zero exit code, crashed or failed in a
48 different way''' 48​ different way'''
49 pass 49​ pass
Show All 29 Lines
View Options

libtaskotron/minion.py

Show First 20 Lines • Show All 157 Lines • ▼ Show 20 Line(s)151 def execute(self):
158 158​
159 log.info("Running task over SSH on machine '%s'...", self.machine) 159​ log.info("Running task over SSH on machine '%s'...", self.machine)
160 160​
161 with remote_exec.ParamikoWrapper(self.machine, self.port, self.user, 161​ with remote_exec.ParamikoWrapper(self.machine, self.port, self.user,
162 self.ssh_privkey, stdio_filename) as self.ssh: 162​ self.ssh_privkey, stdio_filename) as self.ssh:
163 try: 163​ try:
164 self._prepare_task() 164​ self._prepare_task()
165 self._run() 165​ self._run()
166​ except exc.TaskotronRemoteTimeoutError:
167​ # do not try to get logs - the minion is likely dead and we would hang on
168​ # getting the logs, see https://phab.qadevel.cloud.fedoraproject.org/T665
169​ raise
166 except exc.TaskotronRemoteError: 170​ except exc.TaskotronRemoteError:
167 # This is the case when remote execution failed but it should be possible to 171​ # this is the case when remote execution failed but it should be possible to
168 # get logs from the minion, so try it. We don't try getting logs in case when 172​ # get logs from the minion, so try it
169 # exc.TaskotronRemoteTimeoutError is thrown which means that the minion is ​
170 # likely dead and we would hang on getting the logs. ​
171 # See https://phab.qadevel.cloud.fedoraproject.org/T665 ​
172 self._get_output() 173​ self._get_output()
173 raise 174​ raise
174 175​ else:
175 self._get_output() 176​ self._get_output()
176 ​
177 177​
178 178​
179class DisposableMinion(BaseMinion): 179​class DisposableMinion(BaseMinion):
180 '''Minion class that creates a disposable client, connects to it over SSH and runs the task 180​ '''Minion class that creates a disposable client, connects to it over SSH and runs the task
181 there. 181​ there.
182 ''' 182​ '''
183 183​
184 def execute(self): 184​ def execute(self):
Show All 23 Lines193 try:
208 208​
209 task_vm.wait_for_port(self.port) 209​ task_vm.wait_for_port(self.port)
210 210​
211 with remote_exec.ParamikoWrapper(self.machine, self.port, self.user, 211​ with remote_exec.ParamikoWrapper(self.machine, self.port, self.user,
212 self.ssh_privkey, stdio_filename) as self.ssh: 212​ self.ssh_privkey, stdio_filename) as self.ssh:
213 try: 213​ try:
214 self._prepare_task() 214​ self._prepare_task()
215 self._run() 215​ self._run()
216​ except exc.TaskotronRemoteTimeoutError:
217​ # do not try to get logs - the minion is likely dead and we would hang on
218​ # getting the logs, see https://phab.qadevel.cloud.fedoraproject.org/T665
219​ raise
216 except exc.TaskotronRemoteError: 220​ except exc.TaskotronRemoteError:
217 # This is the case when remote execution failed but it should be possible to 221​ # this is the case when remote execution failed but it should be possible to
218 # get logs from the minion, so try it. We don't try getting logs in case when 222​ # get logs from the minion, so try it
219 # exc.TaskotronRemoteTimeoutError is thrown which means that the minion is ​
220 # likely dead and we would hang on getting the logs. ​
221 # See https://phab.qadevel.cloud.fedoraproject.org/T665 ​
222 self._get_output() 223​ self._get_output()
223 # propagate the exception into teardown ​
224 raise 224​ raise
225 225​ else:
226 self._get_output() 226​ self._get_output()
227 227​
228 execution_ok = True 228​ execution_ok = True
229 finally: 229​ finally:
230 if not execution_ok: 230​ if not execution_ok:
231 log.warn('Task execution was interrupted by an error, doing emergency cleanup.') 231​ log.warn('Task execution was interrupted by an error, doing emergency cleanup.')
232 232​
233 if self.arg_data['no_destroy']: 233​ if self.arg_data['no_destroy']:
234 log.info('--no-destroy mode was set from command line, not tearing VM ' 234​ log.info('--no-destroy mode was set from command line, not tearing VM '
Show All 17 Lines
View Options

testing/test_minion.py

1# -*- coding: utf-8 -*- 1​# -*- coding: utf-8 -*-
2# Copyright 2016, Red Hat, Inc. 2​# Copyright 2016, Red Hat, Inc.
3# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+> 3​# License: GPL-2.0+ <http://spdx.org/licenses/GPL-2.0+>
4# See the LICENSE file for more details on Licensing 4​# See the LICENSE file for more details on Licensing
5 5​
6'''Unit tests for libtaskotron/minion.py''' 6​'''Unit tests for libtaskotron/minion.py'''
7 7​
8import argparse 8​import argparse
9import mock 9​import mock
10import pytest 10​import pytest
11import shlex 11​import shlex
12 12​
13from libtaskotron import minion 13​from libtaskotron import minion
14from libtaskotron import main 14​from libtaskotron import main
15​import libtaskotron.exceptions as exc
15 16​
16 17​
17@pytest.mark.usefixtures('setup') 18​@pytest.mark.usefixtures('setup')
18class TestBaseMinion(object): 19​class TestBaseMinion(object):
19 20​
20 @pytest.fixture 21​ @pytest.fixture
21 def setup(self, monkeypatch): 22​ def setup(self):
22 self.ref_arg_data = {'artifactsdir': '/somedir/', 23​ self.ref_arg_data = {'artifactsdir': '/somedir/',
23 'task': '/path/mytask.yaml', 24​ 'task': '/path/mytask.yaml',
24 'ssh_privkey': None, 25​ 'ssh_privkey': None,
25 '_orig_args': {'item': 'foo', 26​ '_orig_args': {'item': 'foo',
26 'type': 'koji_tag'}} 27​ 'type': 'koji_tag'}}
27 self.bm = minion.BaseMinion(None, self.ref_arg_data) 28​ self.bm = minion.BaseMinion(None, self.ref_arg_data)
28 self.bm.taskdir = '/minion/path/' 29​ self.bm.taskdir = '/minion/path/'
29 self.mock_ssh = mock.Mock() 30​ self.mock_ssh = mock.Mock()
▲ Show 20 Lines • Show All 63 Lines • ▼ Show 20 Line(s)92 def test_run_exclude_args(self):
93 self.bm._run() 94​ self.bm._run()
94 cmd_calls = [call for call in self.mock_ssh.method_calls if call[0] == 'cmd'] 95​ cmd_calls = [call for call in self.mock_ssh.method_calls if call[0] == 'cmd']
95 pos_args = cmd_calls[0][1] 96​ pos_args = cmd_calls[0][1]
96 cmdline = pos_args[0] 97​ cmdline = pos_args[0]
97 elems = shlex.split(cmdline) 98​ elems = shlex.split(cmdline)
98 assert '--item' not in elems 99​ assert '--item' not in elems
99 assert 'foo' not in elems 100​ assert 'foo' not in elems
100 assert '--type' in elems 101​ assert '--type' in elems
102​
103​
104​@pytest.mark.usefixtures('setup')
105​class TestRemoteMinion(object):
106​ '''A common class for running tests shared between all remote minions'''
107​
108​ remote_minions = (minion.PersistentMinion, minion.DisposableMinion)
109​
110​ # everything calls setup, and setup is parametrized, therefore everything gets run for all
111​ # remote minion classes
112​ @pytest.fixture(params=remote_minions)
113​ def setup(self, monkeypatch, request):
114​ minion_cls = request.param
115​ self.ref_arg_data = {'artifactsdir': '/somedir/',
116​ 'task': '/path/mytask.yaml',
117​ 'ssh_privkey': None,
118​ '_orig_args': {'item': 'foo',
119​ 'type': 'koji_tag'},
120​ 'no_destroy': False,
121​ 'uuid': 1,
122​ }
123​
124​ monkeypatch.setattr(minion, 'logger', mock.Mock())
125​ monkeypatch.setattr(minion, 'log', mock.Mock())
126​ monkeypatch.setattr(minion, 'remote_exec', mock.MagicMock())
127​ monkeypatch.setattr(minion, 'taskformula', mock.MagicMock())
128​ monkeypatch.setattr(minion, 'vm', mock.Mock())
129​
130​ self.mock_prepare = mock.Mock()
131​ self.mock_run = mock.Mock()
132​ self.mock_output = mock.Mock()
133​ self.mini = minion_cls(None, self.ref_arg_data)
134​ monkeypatch.setattr(self.mini, '_prepare_task', self.mock_prepare)
135​ monkeypatch.setattr(self.mini, '_run', self.mock_run)
136​ monkeypatch.setattr(self.mini, '_get_output', self.mock_output)
137​
138​ def test_timeout_no_logs(self):
139​ '''don't gather logs if timeout is reached during execution'''
140​ self.mock_run.side_effect = exc.TaskotronRemoteTimeoutError()
141​
142​ with pytest.raises(exc.TaskotronRemoteTimeoutError):
143​ self.mini.execute()
144​
145​ assert self.mock_output.called is False
146​
147​ def test_crash_get_logs(self):
148​ '''logs should be gathered after execution crash'''
149​ self.mock_run.side_effect = exc.TaskotronRemoteProcessError()
150​
151​ with pytest.raises(exc.TaskotronRemoteProcessError):
152​ self.mini.execute()
153​
154​ assert self.mock_output.call_count == 1
155​
156​ def test_no_crash_get_logs(self):
157​ '''logs should be gathered if execution doesn't crash'''
158​ self.mini.execute()
159​ assert self.mock_output.call_count == 1