clean up the wrapper left in artifactsdir when libtaskotron is interrupted by an error
AbandonedPublic
Actions

Authored by lnie on Jun 30 2016, 3:57 AM.

Details

Reviewers

lbrabec
kparal
jskladan

Summary

As no useful log will be generated when libtaskotron is interrupted by an error,I'd like suggest we clean the wrapper for users,
so that they won't take the space for nothing.

Test Plan

Just do the common --libtaskotron runtask,and feed an exception to libtaskotron,then check if there is unwanted wrapper left.

Diff Detail

Repository

rLTRN libtaskotron

Branch

brownie

Lint

Lint OK

Unit

Unit Tests OK

Build Status

Buildable 693
Build 693: arc lint + arc unit

lnie retitled this revision from to clean up the wrapper left in artifactsdir when libtaskotron is interrupted by an error.Jun 30 2016, 3:57 AM

lnie updated this object.

lnie edited the test plan for this revision. (Show Details)

lnie added reviewers: jskladan, lbrabec, kparal.

Herald added a subscriber: tflink. · View Herald TranscriptJun 30 2016, 3:57 AM

I'm not in favor of this change. For cleanup we have conf/tmpfiles.d/taskotron.conf. If you copy that file to /etc/tmpfiles.d/, the artifactsdir gets cleaned every week (you can change the period). No need to clean the directory here.
Moreover, if self._get_output() raises in the middle of copying (network error?), this code would delete even the little that managed to copy out of the disposable client.

In D913#17158, @lbrabec wrote:

I'm not in favor of this change.

+1 I don't see any benefit of it, and it seems like it's likely to produce heisenbugs.

This revision now requires changes to proceed.Jul 7 2016, 1:44 PM

A good idea disk-wise, Lili, but we need to keep the logs if the execution fails, for the very reason that it failed, and therefore we need to debug what went wrong. For that reason, we need all logs that we can get from that minion. They are not "useless".

You can select "Abandon revision" in the Action box to close this diff, I don't think this is going to get accepted, sorry.

In D913#17499, @kparal wrote:

A good idea disk-wise, Lili, but we need to keep the logs if the execution fails, for the very reason that it failed, and therefore we need to debug what went wrong. For that reason, we need all logs that we can get from that minion. They are not "useless".
You can select "Abandon revision" in the Action box to close this diff, I don't think this is going to get accepted, sorry.

Gonna to do.I was in favor of this because I saw lots of wrappers ( no log in it) ,which will be there , if exceptions happen before libtaskotron do "dnf --cacheonly repolist || dnf makecache" ,but now I think it's better to abandon this diff: )

lnie abandoned this revision.Jul 13 2016, 5:00 AM

In D913#17516, @lnie wrote:

Gonna to do.I was in favor of this because I saw lots of wrappers ( no log in it) ,which will be there , if exceptions happen before libtaskotron do "dnf --cacheonly repolist || dnf makecache" ,but now I think it's better to abandon this diff: )

Out of curiosity, what do you mean by a "wrapper"? I can't figure that out.

By the way, if you format the text, it will be more readable. Use the help button in the editor to see available markup (or use the graphical buttons for basic operations). For example like this: dnf --cacheonly repolist || dnf makecache

This comment was removed by lnie.

Out of curiosity, what do you mean by a "wrapper"? I can't figure that out.

Er,empty file folder,another improper used word? Speaking of that,I want to say that I really feel sorry for having taken so much time just because I have
mistakenly understand the meaning when you are talking zombie VMs,and I shouldn't take for sure that the zombie VMs and zombie VM files are the same
thing. I know you handsome workaholic are very busy,as you are making contributions in lots of things,and I will try to use proper words and describe things in a more clear way :)

By the way, if you format the text, it will be more readable. Use the help button in the editor to see available markup (or use the graphical buttons for basic operations). For example like this: dnf --cacheonly repolist || dnf makecache

Thanks for your useful information,and I will keep it on mind

Er,empty file folder,another improper used word?

Never heard of using it like that. Empty folder/directory is just... empty folder/directory :-) A wrapper in an IT sense is usually a code which tightly wraps around another code, and offers some extra functionality or maybe simpler access. So let's say libvirt is a wrapper around kvm/xen low-level virtualization. Testcloud is a wrapper around libvirt. In taskotron, we often have python wrappers which serve to execute the task written in a different language (e.g. bash) and then return parsed output back to taskotron.

Never heard of using it like that. Empty folder/directory is just... empty folder/directory :-) A wrapper in an IT sense is usually a code which tightly wraps around another code, and offers some extra functionality or maybe simpler access. So let's say libvirt is a wrapper around kvm/xen low-level virtualization. Testcloud is a wrapper around libvirt. In taskotron, we often have python wrappers which serve to execute the task written in a different language (e.g. bash) and then return parsed output back to taskotron.

Thanks for your patient explanation,much clearer for me now,I shouldn't use words so casually .

Revision Contents

			Path	Coverage (All)	Coverage (Touched)	Packages
M			libtaskotron/ext/disposable/vm.py (6 lines)	64%	50%
M			libtaskotron/minion.py (2 lines)	35%	0%

Commit	Tree	Parents	Author	Summary	Date
a6fa511ac2b4	93c2add8151b	40afbdc8195d	lnie	clean up the wrapper left in artifactsdir when libtaskotron is interrupted by… (Show More…)	Jun 30 2016, 3:34 AM

Diff 2325

View Options

libtaskotron/ext/disposable/vm.py

1	# -- coding: utf-8 --	1		# -- coding: utf-8 --
2	# Copyright 2009-2015, Red Hat, Inc.	2		# Copyright 2009-2015, 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		6
7	"""Interface to locally spawned virtual machines that are used as disposable	7		"""Interface to locally spawned virtual machines that are used as disposable
8	clients for executing Taskotron tasks."""	8		clients for executing Taskotron tasks."""
9		9
10	from time import sleep, time	10		from time import sleep, time
11	import socket	11		import socket
12	import getpass	12		import getpass
13	import re	13		import re
14	import os	14		import os
15		15		import shutil
16	from libtaskotron.logger import log	16		from libtaskotron.logger import log
17	from libtaskotron import config	17		from libtaskotron import config
18	import libtaskotron.exceptions as exc	18		import libtaskotron.exceptions as exc
19		19
20	from testcloud import instance, image, cli	20		from testcloud import instance, image, cli
21	from testcloud.exceptions import TestcloudInstanceError, TestcloudImageError	21		from testcloud.exceptions import TestcloudInstanceError, TestcloudImageError
22		22
23		23
▲ Show 20 Lines • Show All 160 Lines • ▼ Show 20 Line(s)		180	def _destroy_instance(self, tc_instance):
184		184
185	log.debug("destroying instance {}".format(self.instancename))	185		log.debug("destroying instance {}".format(self.instancename))
186	try:	186		try:
187	tc_instance.destroy()	187		tc_instance.destroy()
188	except TestcloudInstanceError, e:	188		except TestcloudInstanceError, e:
189	log.error("Hit error while destroying instance {}".format(self.instancename), e)	189		log.error("Hit error while destroying instance {}".format(self.instancename), e)
190	raise exc.TaskotronRemoteError(e)	190		raise exc.TaskotronRemoteError(e)
191		191
192	def teardown(self):	192		def teardown(self, artifactsdir, execution_ok):
193	'''Tear down the virtual machine by shutting it down and removing any	193		'''Tear down the virtual machine by shutting it down and removing any
194	trace of it from the host machine.	194		trace of it from the host machine.
195		195
196	:raises TaskotronRemoteError: if there is a failure while stopping or	196		:raises TaskotronRemoteError: if there is a failure while stopping or
197	removing the virtual machine instance	197		removing the virtual machine instance
198	'''	198		'''
199		199
200	tc_instance = self._check_existing_instance(should_exist=True)	200		tc_instance = self._check_existing_instance(should_exist=True)
201	try:	201		try:
202	self._stop_instance(tc_instance)	202		self._stop_instance(tc_instance)
203		203
204	# wait for testcloud to actually stop the instance, currently	204		# wait for testcloud to actually stop the instance, currently
205	# instance.stop() returns before the instance is actually stopped	205		# instance.stop() returns before the instance is actually stopped
206	sleep(2)	206		sleep(2)
207	except Exception, e:	207		except Exception, e:
208	raise TestcloudInstanceError(e)	208		raise TestcloudInstanceError(e)
209	finally:	209		finally:
210	self._destroy_instance(tc_instance)	210		self._destroy_instance(tc_instance)
		211		if not execution_ok:
		212		shutil.rmtree(artifactsdir)
211		213
212		214
213	class ImageFinder(object):	215		class ImageFinder(object):
214	'''Retrieve images from either local or remote sources'''	216		'''Retrieve images from either local or remote sources'''
215		217
216	@classmethod	218		@classmethod
217	def get_all_filenames(cls, imagesdir=None):	219		def get_all_filenames(cls, imagesdir=None):
218	"""Get list of images present on the system.	220		"""Get list of images present on the system.
▲ Show 20 Lines • Show All 82 Lines • Show Last 20 Lines