Differential D612

Propagate errors during remote execution
ClosedPublic
Actions

Authored by mkrizek on Oct 8 2015, 2:18 PM.

Details

Reviewers
lbrabec
kparal
tflink
jskladan
Maniphest Tasks
T628: errors during remote execution are not propagated up
Commits
rLTRN30147c40f9ef: Propagate errors during remote execution
Summary

Until we have some kind of remote logging we need not to catch remote
execution exceptions and stop the execution so we know something went
wrong. Also, if remote execution fails, we don't try to get logs from
minion since there is a chance that the minion is dead and we'd hang
while trying to get the logs. If there is any error during remote
execution AND during teardown, raise the remote execution exception
and just log the teardown one since the former is more important to
us.

Test Plan

Ran tasks locally both in --ssh and --libvirt, seemed to worked. Tests pass as well.

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.
mkrizek retitled this revision from to Propagate errors during remote execution.Oct 8 2015, 2:18 PM
mkrizek updated this object.
mkrizek edited the test plan for this revision. (Show Details)
mkrizek added reviewers: tflink, kparal, jskladan, lbrabec.
tflink accepted this revision.Oct 9 2015, 6:02 AM

Tiny nit but otherwise LGTM

libtaskotron/minion.py
142

tiny nit but I'd like to see a longer var name here, took me a sec to figure out what the var was for

This revision is now accepted and ready to land.Oct 9 2015, 6:02 AM
kparal added inline comments.Oct 9 2015, 11:25 AM
libtaskotron/minion.py
142

If that name is confusing (and it's missing p;) ), I'd use something clearer like execution_ok. False by default, True at the end of the try block. Then we can leave out the except block completely (which also removed an edge case when a different exception is thrown and teardown fails as well).

mkrizek updated this revision to Diff 1608.Oct 9 2015, 11:48 AM
  • Prioritize execution exception even if not TaskotronRemoteError
kparal accepted this revision.Oct 9 2015, 12:01 PM
kparal added inline comments.
libtaskotron/minion.py
174

Maybe I would add something like:

# a different exception is already being raised, don't override it
Closed by commit rLTRN30147c40f9ef: Propagate errors during remote execution (authored by mkrizek). · Explain WhyOct 9 2015, 12:47 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathPackages
Mlibtaskotron/minion.py (24 lines)

Diff 1609

View Options

libtaskotron/minion.py

Show First 20 Lines • Show All 115 Lines • ▼ Show 20 Line(s)112 def execute(self):
116 logger.add_filehandler(filelog_path=filelog_path, remove_mem_handler=True) 116​ logger.add_filehandler(filelog_path=filelog_path, remove_mem_handler=True)
117 117​
118 stdio_filename = os.path.join(self.uuid_dir, 'taskotron-stdio.log') 118​ stdio_filename = os.path.join(self.uuid_dir, 'taskotron-stdio.log')
119 119​
120 log.info("Running task over SSH on machine '%s'...", self.machine) 120​ log.info("Running task over SSH on machine '%s'...", self.machine)
121 121​
122 with remote_exec.ParamikoWrapper(self.machine, self.port, self.user, 122​ with remote_exec.ParamikoWrapper(self.machine, self.port, self.user,
123 self.ssh_privkey, stdio_filename) as self.ssh: 123​ self.ssh_privkey, stdio_filename) as self.ssh:
124 try: 124​ self._prepare_task()
125 self._prepare_task() 125​ self._run()
126 self._run() ​
127 except exc.TaskotronRemoteError, e: ​
128 log.exception(e) ​
129 ​
130 self._get_output() 126​ self._get_output()
131 127​
132 128​
133class DisposableMinion(BaseMinion): 129​class DisposableMinion(BaseMinion):
134 '''Minion class that creates a disposable client, connects to it over SSH and runs the task 130​ '''Minion class that creates a disposable client, connects to it over SSH and runs the task
135 there. 131​ there.
136 ''' 132​ '''
137 133​
138 def execute(self): 134​ def execute(self):
139 '''Init environment and connect to disposable client for task execution.''' 135​ '''Init environment and connect to disposable client for task execution.'''
140 136​
141 filelog_path = os.path.join(self.uuid_dir, 'taskotron-initiator.log') 137​ filelog_path = os.path.join(self.uuid_dir, 'taskotron-initiator.log')
142 logger.add_filehandler(filelog_path=filelog_path, remove_mem_handler=True) 138​ logger.add_filehandler(filelog_path=filelog_path, remove_mem_handler=True)
143 139​
144 stdio_filename = os.path.join(self.uuid_dir, 'taskotron-stdio.log') 140​ stdio_filename = os.path.join(self.uuid_dir, 'taskotron-stdio.log')
145 141​
142​ execution_ok = False
tflinkUnsubmitted
Not Done

tiny nit but I'd like to see a longer var name here, took me a sec to figure out what the var was for

kparalUnsubmitted
Not Done

If that name is confusing (and it's missing p;) ), I'd use something clearer like execution_ok. False by default, True at the end of the try block. Then we can leave out the except block completely (which also removed an edge case when a different exception is thrown and teardown fails as well).

146 try: 143​ try:
147 log.info("Running task on a disposable client using libvirt...") 144​ log.info("Running task on a disposable client using libvirt...")
148 145​
149 log.info("Creating VM for task") 146​ log.info("Creating VM for task")
150 task_vm = vm.TestCloudMachine(self.arg_data['uuid']) 147​ task_vm = vm.TestCloudMachine(self.arg_data['uuid'])
151 task_vm.prepare() 148​ task_vm.prepare()
152 self.machine = task_vm.ipaddr 149​ self.machine = task_vm.ipaddr
153 self.port = 22 150​ self.port = 22
154 self.user = task_vm.username 151​ self.user = task_vm.username
155 152​
156 log.info("Running Task over SSH on machine %s" % task_vm.instancename) 153​ log.info("Running Task over SSH on machine %s" % task_vm.instancename)
157 154​
158 task_vm.wait_for_port(self.port) 155​ task_vm.wait_for_port(self.port)
159 156​
160 with remote_exec.ParamikoWrapper(self.machine, self.port, self.user, 157​ with remote_exec.ParamikoWrapper(self.machine, self.port, self.user,
161 self.ssh_privkey, stdio_filename) as self.ssh: 158​ self.ssh_privkey, stdio_filename) as self.ssh:
162 try: ​
163 self._prepare_task() 159​ self._prepare_task()
164 self._run() 160​ self._run()
165 except exc.TaskotronRemoteError, e: 161​ self._get_output()
166 log.exception(e) 162​ execution_ok = True
167 ​
168 self._get_output() ​
169 finally: 163​ finally:
170 if self.arg_data['no_destroy']: 164​ if self.arg_data['no_destroy']:
171 log.info("--no-destroy mode was set from command line, not tearing VM " 165​ log.info("--no-destroy mode was set from command line, not tearing VM "
172 "down for task %s" % self.arg_data['uuid']) 166​ "down for task %s" % self.arg_data['uuid'])
173 else: 167​ else:
174 try: 168​ try:
175 task_vm.teardown() 169​ task_vm.teardown()
176 except exc.TaskotronRemoteError, e: 170​ except exc.TaskotronRemoteError, e:
177 log.error("Hit problem during teardown of VM for task %s" % 171​ if execution_ok:
178 self.arg_data['uuid']) 172​ raise e
179 log.exception(e) 173​ else:
174​ # a different exception is already being raised, don't override it
kparalUnsubmitted
Not Done

Maybe I would add something like:

# a different exception is already being raised, don't override it
175​ log.exception(e)