Merge lp:~nuclearbob/utah/vm-teardown-fix into lp:utah

[Max Brustkern]

This branch wraps checks if self.vm is None in virtual machine classes to handle things correctly when self.vm has not yet been setup. Also, self.disks in destroy is similarly wrapped in a try/except block to workaround problems.

[Javier Collado (javier.collado)]

I'm not sure about how all this works, but I think that `vm` attribute
should be initialized to None in the __init_ method and that all the
try/except blocks should be transformed to checks to ensure that `vm`
is not `None`.

[Max Brustkern (nuclearbob)]

That makes sense. I'll make that change and resubmit later.

[Javier Collado (javier.collado)]

@Max

Thanks for the update.

Looking at the code, something I find a little bit weird is that activecheck
and _start call themselves recursively and there isn't a clear base case defined
in the code to stop the recursion.

What I mean is that if lv.lookupByName misbehaves and returns None, then _load
will be useless and activecheck/_start will call themselves indefinitely until
a stack overflow exception is raised.

Aside from that, I've seen that lv.lookupByName might raise libvirtError and
indeed there's a try/except block to detect that when calling _load from _provision.
Shouldn't activecheck and _start take that into account as well?

review: Needs Information

[lp:~nuclearbob/utah/vm-teardown-fix]

722. By Max Brustkern on 2012-11-08

Merged latest changes and rewrote behavior of _start and activecheck
* Merged initial arm support
* Pushing version to 0.5 to create a new stable version before UDS
* phoenix - Fix comment for fetch_method.
* phoenix - Adjust default ts_control to be more informative

723. By Max Brustkern on 2012-11-08

Merged latest changes
Dashboard integration merge

[Max Brustkern (nuclearbob)]

activecheck and _start shouldn't really be concerned with provisioning. activecheck runs provisioncheck, so if the machine isn't provisioned properly, something else should raise an exception. Just in case we somehow get past that, activecheck and start will now raise exceptions if self.vm isn't defined. Other than that, their behavior should be somewhat simpler, mostly assuming that provisioncheck either does its job or raises an exception.

This is some of the first code I wrote for UTAH, and I think I've learned a lot since then. Let me know what you think of the new changes. I've also merged the latest stuff from the dev branch.

review: Needs Resubmitting

[lp:~nuclearbob/utah/vm-teardown-fix]

724. By Max Brustkern on 2012-11-08

Changed CustomVM _start to be more like LibvirtVM _start

725. By Max Brustkern on 2012-11-08

Updated destroy method

[Javier Collado (javier.collado)]

It's been a while since I reviewed this branch, but I no longer see the issues in my previous comment, so I'm approving the merge request.

review: Approve

=== modified file 'utah/provisioning/vm/libvirtvm.py'
--- utah/provisioning/vm/libvirtvm.py	2012-10-19 21:57:01 +0000
+++ utah/provisioning/vm/libvirtvm.py	2012-11-08 18:18:27 +0000
@@ -91,17 +91,24 @@
         """
         self.logger.debug('Checking if VM is active')
         self.provisioncheck()
-        if self.vm.isActive() == 0:
-            self._start()
+        if self.vm is not None:
+            if self.vm.isActive() == 0:
+                self._start()
+            else:
+                self.active = True
         else:
-            self.active = True
+            raise UTAHVMProvisioningException('Failed to provision VM')
 
     def _start(self):
         """
-        Verify the machine is provisioned, then start it.
+        Start the VM.
         """
         self.logger.info('Starting VM')
-        self.vm.create()
+        if self.vm is not None:
+            if self.vm.isActive() == 0:
+                self.vm.create()
+        else:
+            raise UTAHVMProvisioningException('Failed to provision VM')
         self.active = True
 
     def stop(self, force=False):
@@ -111,14 +118,17 @@
         one.
         """
         self.logger.info('Stopping VM')
-        if self.vm.isActive() == 0:
-            self.logger.info('VM is already stopped')
+        if self.vm is not None:
+            if self.vm.isActive() == 0:
+                self.logger.info('VM is already stopped')
+            else:
+                if force:
+                    self.logger.info('Forced shutdown requested')
+                    self.vm.destroy()
+                else:
+                    self.vm.shutdown()
         else:
-            if force:
-                self.logger.info('Forced shutdown requested')
-                self.vm.destroy()
-            else:
-                self.vm.shutdown()
+            self.logger.info('VM not yet created')
         self.active = False
 
     def libvirterrorhandler(self, _context, err):
@@ -367,6 +377,7 @@
             self.disksizes = [8]
         else:
             self.disksizes = disksizes
+        self.disks = []
         if name is None:
             autoname = True
             name = '-'.join([str(prefix), str(machineid)])
@@ -423,7 +434,6 @@
         self.logger.info('Creating disks')
         if disksizes is None:
             disksizes = self.disksizes
-        self.disks = []
         for index, size in enumerate(disksizes):
             disksize = '{}G'.format(size)
             basename = 'disk{}.qcow2'.format(index)
@@ -689,10 +699,14 @@
 
     def _start(self):
         """
-        Start the machine.
+        Start the VM.
         """
         self.logger.info('Starting CustomVM')
-        self.vm.create()
+        if self.vm is not None:
+            if self.vm.isActive() == 0:
+                self.vm.create()
+        else:
+            raise UTAHVMProvisioningException('Failed to provision VM')
         self.logger.info('Waiting ' + str(self.boottimeout) +
                          ' seconds to allow machine to boot')
         self.sshpoll(self.boottimeout)
@@ -702,9 +716,13 @@
         """
         Remove the machine from libvirt and remove all the disk files.
         """
+        # TODO: make this use standard cleanup
         super(CustomVM, self).destroy(*args, **kw)
         self.stop(force=True)
-        self.vm.undefine()
+        if self.vm is not None:
+            self.vm.undefine()
+        else:
+            self.logger.info('VM not created')
         for disk in self.disks:
             os.unlink(disk['file'])
         shutil.rmtree(self.directory)

=== modified file 'utah/provisioning/vm/vm.py'
--- utah/provisioning/vm/vm.py	2012-08-22 15:20:25 +0000
+++ utah/provisioning/vm/vm.py	2012-11-08 18:18:27 +0000
@@ -12,4 +12,5 @@
     """
     def __init__(self, *args, **kw):
         super(VM, self).__init__(*args, **kw)
+        self.vm = None
         self.logger.debug('VM init finished')