Merge lp:~justin-fathomdb/nova/where-has-my-instance-gone into lp:~hudson-openstack/nova/trunk

[justinsb]

Keep guest instances when libvirt host restarts

[justinsb (justin-fathomdb)]

I haven't been able to test this in production as much as I'd like, but it's a fairly critical bug, so proposing for merge.

[Vish Ishaya (vishvananda)]

Really like to see this in soon.

review: Approve

[Devin Carlen (devcamcar)]

It's generally our policy to not leave commented code blocks in. We have history in bzr so we can always go find if we need.

14 + # NOTE(justinsb): We no longer auto-remove SHUTOFF instances
15 + # It's quite hard to get them back when we do.
16 + #if vm_state == power_state.SHUTOFF:
17 + # # TODO(soren): This is what the compute manager does when you
18 + # # terminate an instance. At some point I figure we'll have a
19 + # # "terminated" state and some sort of cleanup job that runs
20 + # # occasionally, cleaning them out.
21 + # self.db.instance_destroy(context, db_instance['id'])

59 + # NOTE(justinsb): We no longer delete these instances,
60 + # the user may want to power them back on
61 + #if state == power_state.SHUTOFF:
62 + # # TODO(soren): This is what the compute manager does when you
63 + # # terminate # an instance. At some point I figure we'll have a
64 + # # "terminated" state and some sort of cleanup job that runs
65 + # # occasionally, cleaning them out.
66 + # db.instance_destroy(ctxt, instance['id'])

This looks good, and I'll approve once this is cleaned up.

[justinsb (justin-fathomdb)]

Removed commented code and fixed up with correct libvirt exit code

[Devin Carlen (devcamcar)]

lgtm

review: Approve

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-03-30 05:59:13 +0000
+++ nova/compute/manager.py	2011-04-07 22:54:36 +0000
@@ -1097,12 +1097,8 @@
                                            db_instance['id'],
                                            vm_state)
 
-            if vm_state == power_state.SHUTOFF:
-                # TODO(soren): This is what the compute manager does when you
-                # terminate an instance. At some point I figure we'll have a
-                # "terminated" state and some sort of cleanup job that runs
-                # occasionally, cleaning them out.
-                self.db.instance_destroy(context, db_instance['id'])
+            # NOTE(justinsb): We no longer auto-remove SHUTOFF instances
+            # It's quite hard to get them back when we do.
 
         # Are there VMs not in the DB?
         for vm_not_found_in_db in vms_not_found_in_db:

=== modified file 'nova/tests/test_compute.py'
--- nova/tests/test_compute.py	2011-04-04 20:17:04 +0000
+++ nova/tests/test_compute.py	2011-04-07 22:54:36 +0000
@@ -666,4 +666,5 @@
 
         instances = db.instance_get_all(context.get_admin_context())
         LOG.info(_("After force-killing instances: %s"), instances)
-        self.assertEqual(len(instances), 0)
+        self.assertEqual(len(instances), 1)
+        self.assertEqual(power_state.SHUTOFF, instances[0]['state'])

=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2011-04-07 21:48:29 +0000
+++ nova/virt/libvirt_conn.py	2011-04-07 22:54:36 +0000
@@ -117,6 +117,8 @@
                     'Define live migration behavior')
 flags.DEFINE_string('qemu_img', 'qemu-img',
                     'binary to use for qemu-img commands')
+flags.DEFINE_bool('start_guests_on_host_boot', False,
+                  'Whether to restart guests when the host reboots')
 
 
 def get_connection(read_only):
@@ -231,12 +233,8 @@
                           {'name': instance['name'], 'state': state})
             db.instance_set_state(ctxt, instance['id'], state)
 
-            if state == power_state.SHUTOFF:
-                # TODO(soren): This is what the compute manager does when you
-                # terminate # an instance. At some point I figure we'll have a
-                # "terminated" state and some sort of cleanup job that runs
-                # occasionally, cleaning them out.
-                db.instance_destroy(ctxt, instance['id'])
+            # NOTE(justinsb): We no longer delete SHUTOFF instances,
+            # the user may want to power them back on
 
             if state != power_state.RUNNING:
                 continue
@@ -475,7 +473,7 @@
         xml = self.to_xml(instance)
         self.firewall_driver.setup_basic_filtering(instance)
         self.firewall_driver.prepare_instance_filter(instance)
-        self._conn.createXML(xml, 0)
+        self._create_new_domain(xml)
         self.firewall_driver.apply_instance_filter(instance)
 
         timer = utils.LoopingCall(f=None)
@@ -523,7 +521,7 @@
                          'kernel_id': FLAGS.rescue_kernel_id,
                          'ramdisk_id': FLAGS.rescue_ramdisk_id}
         self._create_image(instance, xml, '.rescue', rescue_images)
-        self._conn.createXML(xml, 0)
+        self._create_new_domain(xml)
 
         timer = utils.LoopingCall(f=None)
 
@@ -566,10 +564,15 @@
         self.firewall_driver.setup_basic_filtering(instance, network_info)
         self.firewall_driver.prepare_instance_filter(instance, network_info)
         self._create_image(instance, xml, network_info)
-        self._conn.createXML(xml, 0)
+        domain = self._create_new_domain(xml)
         LOG.debug(_("instance %s: is running"), instance['name'])
         self.firewall_driver.apply_instance_filter(instance)
 
+        if FLAGS.start_guests_on_host_boot:
+            LOG.debug(_("instance %s: setting autostart ON") %
+                      instance['name'])
+            domain.setAutostart(1)
+
         timer = utils.LoopingCall(f=None)
 
         def _wait_for_boot():
@@ -989,11 +992,22 @@
         return xml
 
     def get_info(self, instance_name):
+        # NOTE(justinsb): When libvirt isn't running / can't connect, we get:
+        # libvir: Remote error : unable to connect to
+        #  '/var/run/libvirt/libvirt-sock', libvirtd may need to be started:
+        #  No such file or directory
         try:
             virt_dom = self._conn.lookupByName(instance_name)
-        except:
-            raise exception.NotFound(_("Instance %s not found")
-                                     % instance_name)
+        except libvirt.libvirtError as e:
+            errcode = e.get_error_code()
+            if errcode == libvirt.VIR_ERR_NO_DOMAIN:
+                raise exception.NotFound(_("Instance %s not found")
+                                         % instance_name)
+            LOG.warning(_("Error from libvirt during lookup. "
+                          "Code=%(errcode)s Error=%(e)s") %
+                        locals())
+            raise
+
         (state, max_mem, mem, num_cpu, cpu_time) = virt_dom.info()
         return {'state': state,
                 'max_mem': max_mem,
@@ -1001,6 +1015,24 @@
                 'num_cpu': num_cpu,
                 'cpu_time': cpu_time}
 
+    def _create_new_domain(self, xml, persistent=True, launch_flags=0):
+        # NOTE(justinsb): libvirt has two types of domain:
+        # * a transient domain disappears when the guest is shutdown
+        # or the host is rebooted.
+        # * a permanent domain is not automatically deleted
+        # NOTE(justinsb): Even for ephemeral instances, transient seems risky
+
+        if persistent:
+            # To create a persistent domain, first define it, then launch it.
+            domain = self._conn.defineXML(xml)
+
+            domain.createWithFlags(launch_flags)
+        else:
+            # createXML call creates a transient domain
+            domain = self._conn.createXML(xml, launch_flags)
+
+        return domain
+
     def get_diagnostics(self, instance_name):
         raise exception.ApiError(_("diagnostics are not supported "
                                    "for libvirt"))