#43 Talking about whether we should clean up the files left for users
Closed: Fixed a year ago by frantisekz. Opened 7 years ago by somebody.

I was intending to sumbit a diff which not only can take care of ^c, but also all the exceptions happen before a VM is defined, like exceptions raised in _create_local_disk, _generate_seed_image, write_domain_xml, etc. But now I'm not sure if this is a good idea, or is there any need to add this diff. So I have taken kamil's advice and opened this ticket. This is not an urgent thing, and you can give feedback whenever you get a time.

As you can see, it will take some space for nothing, but not much.

This is what I got in ^c case

(env_taskotron) [root@dhcp-128-28 libtaskotron]# python runtask.py --libvirt -i squid-3.5.20-1.fc24 -t koji_build  -a x86_64 /home/somebody/task-rpmlint/runtask.yml 
[libtaskotron] 09:30:50 INFO    Execution started at: 2016-07-13 09:30:50 UTC
[libtaskotron] 09:30:50 DEBUG   Using libtaskotron 0.4.13
[libtaskotron] 09:30:50 DEBUG   Parsed arguments: Namespace(arch=['x86_64'], debug=False, item='squid-3.5.20-1.fc24', jobid='-1', libvirt=True, local=False, no_destroy=False, override=[], patch=None, ssh=None, ssh_privkey=None, task='/home/somebody/task-rpmlint/runtask.yml', type='koji_build', uuid='20160713_093050_970126')
[libtaskotron] 09:30:50 DEBUG   Using config file: /etc/taskotron/taskotron.yaml
[libtaskotron] 09:30:50 DEBUG   Using config profile: development
[libtaskotron] 09:30:50 INFO    Task artifacts will be saved in: /var/lib/taskotron/artifacts/20160713_093050_970126
[libtaskotron] 09:30:51 INFO    Running task on a disposable client using libvirt...
[libtaskotron] 09:30:51 INFO    Creating VM for task
^C[libtaskotron] 09:30:51 WARNING Task execution was interrupted by an error, doing emergency cleanup.
[libtaskotron] 09:30:51 INFO    Shutting down the minion...
libvirt: QEMU Driver error : Domain not found: no domain with matching name 'taskotron-20160713_093050_970126'
[libtaskotron] 09:30:52 ERROR   Domain not found: no domain with matching name 'taskotron-20160713_093050_970126'
Traceback (most recent call last):
  File "/test/libtaskotron/libtaskotron/minion.py", line 240, in execute
    task_vm.teardown()
  File "/test/libtaskotron/libtaskotron/ext/disposable/vm.py", line 201, in teardown
    self._stop_instance(tc_instance)
  File "/test/libtaskotron/libtaskotron/ext/disposable/vm.py", line 174, in _stop_instance
    tc_instance.stop()
  File "/test/testcloud/testcloud/instance.py", line 420, in stop
    self._get_domain().destroy()
  File "/test/testcloud/testcloud/instance.py", line 294, in _get_domain
    return conn.lookupByName(self.name)
  File "/usr/lib64/python2.7/site-packages/libvirt.py", line 4169, in lookupByName
    if ret is None:raise libvirtError('virDomainLookupByName() failed', conn=self)
libvirtError: Domain not found: no domain with matching name 'taskotron-20160713_093050_970126'
[libtaskotron] 09:30:52 CRITICAL Traceback (most recent call last):
  File "runtask.py", line 10, in <module>
    main.main()
  File "/test/libtaskotron/libtaskotron/main.py", line 162, in main
    overlord.start()
  File "/test/libtaskotron/libtaskotron/overlord.py", line 92, in start
    runner.execute()
  File "/test/libtaskotron/libtaskotron/minion.py", line 201, in execute
    task_vm.prepare(**env)
  File "/test/libtaskotron/libtaskotron/ext/disposable/vm.py", line 139, in prepare
    self._prepare_instance(tc_image)
  File "/test/libtaskotron/libtaskotron/ext/disposable/vm.py", line 99, in _prepare_instance
    tc_instance.prepare()
  File "/test/testcloud/testcloud/instance.py", line 170, in prepare
    self._generate_seed_image()
  File "/test/testcloud/testcloud/instance.py", line 234, in _generate_seed_image
    self.seed_path])
  File "/usr/lib64/python2.7/subprocess.py", line 522, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/usr/lib64/python2.7/subprocess.py", line 1384, in wait
    pid, sts = _eintr_retry_call(os.waitpid, self.pid, 0)
  File "/usr/lib64/python2.7/subprocess.py", line 476, in _eintr_retry_call
    return func(*args)
KeyboardInterrupt
(env_taskotron) [root@dhcp-128-28 libtaskotron]# ll -h  /var/lib/testcloud/instances/taskotron-20160713_093050_970126
total 4.0K
drwxr-xr-x. 2 root root 4.0K Jul 13 17:30 meta
-rw-r--r--. 1 root root 287K Jul 13 17:30 taskotron-20160713_093050_970126-seed.img
def prepare(self):
        """Create local directories and metadata needed to spawn the instance
        """
        # create the dirs needed for this instance
        self._create_dirs()

        # generate metadata
        self._create_user_data(config_data.PASSWORD)
        self._create_meta_data(self.hostname)

        # generate seed image
        self._generate_seed_image()

        # deal with backing store
        self._create_local_disk()
        raise Exception

the following is what I get with the above code in testcloud

python runtask.py --libvirt -i squid-3.5.20-1.fc24 -t koji_build  -a x86_64 /home/somebody/task-rpmlint/runtask.yml 
[libtaskotron] 09:45:28 INFO    Execution started at: 2016-07-13 09:45:28 UTC
[libtaskotron] 09:45:28 DEBUG   Using libtaskotron 0.4.13
[libtaskotron] 09:45:28 DEBUG   Parsed arguments: Namespace(arch=['x86_64'], debug=False, item='squid-3.5.20-1.fc24', jobid='-1', libvirt=True, local=False, no_destroy=False, override=[], patch=None, ssh=None, ssh_privkey=None, task='/home/somebody/task-rpmlint/runtask.yml', type='koji_build', uuid='20160713_094528_363086')
[libtaskotron] 09:45:28 DEBUG   Using config file: /etc/taskotron/taskotron.yaml
[libtaskotron] 09:45:28 DEBUG   Using config profile: development
[libtaskotron] 09:45:28 INFO    Task artifacts will be saved in: /var/lib/taskotron/artifacts/20160713_094528_363086
[libtaskotron] 09:45:28 INFO    Running task on a disposable client using libvirt...
[libtaskotron] 09:45:28 INFO    Creating VM for task
[testcloud.instance] 09:45:30 INFO    Seed image generated successfully
Formatting '/var/lib/testcloud/instances/taskotron-20160713_094528_363086/taskotron-20160713_094528_363086-local.qcow2', fmt=qcow2 size=3221225472 backing_file=/var/lib/testcloud/backingstores/160312_1030-fedora-24-taskotron_cloud-x86_64.raw encryption=off cluster_size=65536 lazy_refcounts=off refcount_bits=16
[libtaskotron] 09:45:31 WARNING Task execution was interrupted by an error, doing emergency cleanup.
[libtaskotron] 09:45:31 INFO    Shutting down the minion...
libvirt: QEMU Driver error : Domain not found: no domain with matching name 'taskotron-20160713_094528_363086'
[libtaskotron] 09:45:31 ERROR   Domain not found: no domain with matching name 'taskotron-20160713_094528_363086'
Traceback (most recent call last):
  File "/test/libtaskotron/libtaskotron/minion.py", line 240, in execute
    task_vm.teardown()
  File "/test/libtaskotron/libtaskotron/ext/disposable/vm.py", line 201, in teardown
    self._stop_instance(tc_instance)
  File "/test/libtaskotron/libtaskotron/ext/disposable/vm.py", line 174, in _stop_instance
    tc_instance.stop()
  File "/test/testcloud/testcloud/instance.py", line 420, in stop
    self._get_domain().destroy()
  File "/test/testcloud/testcloud/instance.py", line 294, in _get_domain
    return conn.lookupByName(self.name)
  File "/usr/lib64/python2.7/site-packages/libvirt.py", line 4169, in lookupByName
    if ret is None:raise libvirtError('virDomainLookupByName() failed', conn=self)
libvirtError: Domain not found: no domain with matching name 'taskotron-20160713_094528_363086'
[libtaskotron] 09:45:31 CRITICAL Traceback (most recent call last):
  File "runtask.py", line 10, in <module>
    main.main()
  File "/test/libtaskotron/libtaskotron/main.py", line 162, in main
    overlord.start()
  File "/test/libtaskotron/libtaskotron/overlord.py", line 92, in start
    runner.execute()
  File "/test/libtaskotron/libtaskotron/minion.py", line 201, in execute
    task_vm.prepare(**env)
  File "/test/libtaskotron/libtaskotron/ext/disposable/vm.py", line 139, in prepare
    self._prepare_instance(tc_image)
  File "/test/libtaskotron/libtaskotron/ext/disposable/vm.py", line 99, in _prepare_instance
    tc_instance.prepare()
  File "/test/testcloud/testcloud/instance.py", line 174, in prepare
    raise Exception
Exception
(env_taskotron) [root@dhcp-128-28 libtaskotron]# ll -h /var/lib/testcloud/instances/taskotron-20160713_094528_363086
total 224K
drwxr-xr-x. 2 root root 4.0K Jul 13 17:45 meta
-rw-r--r--. 1 root root 193K Jul 13 17:45 taskotron-20160713_094528_363086-local.qcow2
-rw-r--r--. 1 root root 287K Jul 13 17:45 taskotron-20160713_094528_363086-seed.img

This ticket had assigned some Differential requests:
D933
D936

Setting proper project. Lili, can you please fix the formatting in your ticket? (everything that is code or command output set as monospaced text). It's easier to read. Thanks.

! In #816#11372, @kparal wrote:
Setting proper project. Lili, can you please fix the formatting in your ticket? (everything that is code or command output set as monospaced text). It's easier to read. Thanks.

Sure,done.

@kparal ,thanks a lot for your patient demonstration teaching,looks much more beautiful now: )

The first problem mentioned in description (hitting Ctrl+c during minion creation) should be much improved once D933 is pushed - the VM should not stay defined in libvirt. It will not be 100%, there are still some very narrow periods when you can hit Ctrl+c and something will be left over, but you would have to be quite lucky to do that. (Next time, please provide a debug log, i.e. running with --debug). However, that patch does not do anything about leftover files in /var/lib/testcloud/instances/ - which is your major gripe, IIUIC. That needs to be handled by testcloud.

For testcloud, I can imagine to be a reasonable request if you want temporary files (seed, etc) and/or the whole VM to be deleted if you hit Ctrl+c during VM creation. On the other hand, if an arbitrary exception happens, I'm not convinced. The leftover files might be essential to debug the problem. Also, you can't just place raise Exception at a random place in the code, there has be to some justification why this particular exception would be raised in this particular place.

Overall, I see this as quite a lot of trouble for little gain (the leftover files are totally negligible in size). If you want to work on this, I'd recommend you to work with testcloud directly (not through libtaskotron) and focus just on proper cleanup after Ctrl+c events (not arbitrary errors).

`>>! In #816#11381, @kparal wrote:

The first problem mentioned in description (hitting Ctrl+c during minion creation) should be much improved once D933 is pushed - the VM should not stay >defined in libvirt. It will not be 100%, there are still some very narrow periods when you can hit Ctrl+c and something will be left over, but you would have to be >quite lucky to do that. (Next time, please provide a debug log, i.e. running with --debug). However, that patch does not do anything about leftover files in >/var/lib/testcloud/instances/ - which is your major gripe, IIUIC.
Yeah,you need '^c' almost immediately after you type the command.Actually,the '^c' situation is only a subordinate reason why I submitted that diff.
That needs to be handled by testcloud.
If I modify the testcloud code instead,in my reach area,the only solution I can pick is to change destroy to something like below ( kind of grabbing stop's work),and only put _destroy_instance(no _stop_instance) to libtaskotron's teardown function.I wasn't sure this is a good idea,and it's a huge change,compared to only add try-except-finally in teardown.

def destroy(self):

        system_domains = _list_system_domains("qemu:///system")
        domain_exists = self.name in system_domains
        if domain_exists:
                if system_domains[self.name] == 'running':
                        self._get_domain().destroy()
                self._get_domain().undefine()
        shutil.rmtree(self.path)

For testcloud, I can imagine to be a reasonable request if you want temporary files (seed, etc) and/or the whole VM to be deleted if you hit Ctrl+c during VM >creation. On the other hand, if an arbitrary exception happens, I'm not convinced. The leftover files might be essential to debug the >problem.
I don't think the leftover files are so important,considering deleting them is the default action when libtaskotron hits an error.yeah, the libtaskotron has the '-no-destroy' option,which will let you debug the problem,I can add a same option to testcloud right now,if that's your main concern. Actually,The leftover-files thing only happens when the libtaskotron try to 'stop and destroy' the instance,in testcloud this won't hurt: if you find 'stop and destroy' will raise the libvirt error saying no specific VM ,then you can just do destroy ,and the leftover files will be gone,so there is no need,at least for now,to handle the stop-and-destroy thing for testcloud.
Also, you can't just >place raise Exception at a random place in the code, there has be to some justification why this particular exception would be raised in this particular place.
yeah,in a working well environment we won't see errors in generate_seed_image,create_local_disk,actually, we won't see any error.But errors will never happen during that steps?To be honest,I'm not 100% convinced: )
Overall, I see this as quite a lot of trouble for little gain (the leftover files are totally negligible in size).
I'm afraid I have to say that I can't figure out the reason why you call this as "quite a lot of trouble for little gain".
Little gain,yes(if the leftover files are large,I would suggest add the diff)but only 4 more lines with no side-affect ,at least for me,I can't take it as 'a lot of trouble'
If you want to work on this, I'd recommend you to work >with testcloud directly (not through libtaskotron) and focus just on proper cleanup after Ctrl+c events >(not arbitrary errors).
Actually,result of '^c' and exceptions happen before libvirt define a vm are the same :when libtaskotron try to stop a specific instance,libvirt error is raised,as the instance is not defined , then the program exit without executing '_destroy_instance',leaving the files there.
Will I work on this?Definitely NOT! I don't want to waste your time on this trivial any more,for the sake of fedora,then you can have more time to make more important contributions: )
Then why I have explained so much?Cause I'm imitating you,you are the idol(workaholic,patient,helpful,etc). I want to show the whole thing, make it clear for you,as you always do : )

When you see those leftover files you don't like, have you tried to run

testcloud instance list --all

and

testcloud image list

whether they show up in that list? Because if they do, you can remove them using

testcloud instance/image remove <name>

(using current testcloud git HEAD plus D936 applied -- we made some CLI syntax changes lately)

! In #816#11392, @kparal wrote:
(using current testcloud git HEAD plus D936 applied -- we made some CLI syntax changes lately)
Thanks for your useful information: )

Metadata Update from @frantisekz:
- Issue close_status updated to: Fixed
- Issue status updated to: Closed (was: Open)

a year ago

Login to comment on this ticket.

Metadata