Evaluate 'task_state' in resource (de)allocation

There are two types of VM states associated with shelving. The first,
'shelved' indicates that the VM has been powered off but the resources
remain allocated on the hypervisor. The second, 'shelved_offloaded',
indicates that the VM has been powered off and the resources freed.
When "unshelving" VMs in the latter state, the VM state does not change
from 'shelved_offloaded' until some time after the VM has been
"unshelved".

Change I83a5f06 introduced a change that allowed for deallocation of
resources when they were set to the 'shelved_offloaded' state. However,
the resource (de)allocation code path assumes any VM with a state of
'shelved_offloaded' should have resources deallocated from it, rather
than allocated to it. As the VM state has not changed when this code
path is executed, resources are incorrectly deallocated from the
instance twice.

Enhance the aformentioned check to account for task state in addition to
VM state. This ensures a VM that's still in 'shelved_offloaded' state,
but is in fact being unshelved, does not trigger deallocation.

Change-Id: Ie2e7b91937fc3d61bb1197fffc3549bebc65e8aa
Signed-off-by: Stephen Finucane <stephen.finucane@intel.com>
Resolves-bug: #1587386
Related-bug: #1545675

nova/compute/resource_tracker.py

@@ -844,9 +844,10 @@ class ResourceTracker(object):
 
         uuid = instance['uuid']
         is_new_instance = uuid not in self.tracked_instances
-        is_removed_instance = (
+        # NOTE(sfinucan): Both brand new instances as well as instances that
-                is_removed or
+        # are being unshelved will have is_new_instance == True
-                instance['vm_state'] in vm_states.ALLOW_RESOURCE_REMOVAL)
+        is_removed_instance = not is_new_instance and (is_removed or
+            instance['vm_state'] in vm_states.ALLOW_RESOURCE_REMOVAL)
 
         if is_new_instance:
             self.tracked_instances[uuid] = obj_base.obj_to_primitive(instance)

nova/tests/unit/compute/test_resource_tracker.py

@@ -698,6 +698,51 @@ class ComputeMonitorTestCase(BaseTestCase):
         self.assertEqual(metrics, expected_metrics)
 
 
+class UpdateUsageFromInstanceTestCase(BaseTrackerTestCase):
+
+    @mock.patch.object(resource_tracker.ResourceTracker,
+                       '_update_usage')
+    def test_building(self, mock_update_usage):
+        instance = self._fake_instance_obj()
+        instance.vm_state = vm_states.BUILDING
+        self.tracker._update_usage_from_instance(self.context, instance)
+
+        mock_update_usage.assert_called_once_with(instance, sign=1)
+
+    @mock.patch.object(resource_tracker.ResourceTracker,
+                       '_update_usage')
+    def test_shelve_offloading(self, mock_update_usage):
+        instance = self._fake_instance_obj()
+        instance.vm_state = vm_states.SHELVED_OFFLOADED
+        self.tracker.tracked_instances = {}
+        self.tracker.tracked_instances[
+            instance.uuid] = obj_base.obj_to_primitive(instance)
+        self.tracker._update_usage_from_instance(self.context, instance)
+
+        mock_update_usage.assert_called_once_with(instance, sign=-1)
+
+    @mock.patch.object(resource_tracker.ResourceTracker,
+                       '_update_usage')
+    def test_unshelving(self, mock_update_usage):
+        instance = self._fake_instance_obj()
+        instance.vm_state = vm_states.SHELVED_OFFLOADED
+        self.tracker._update_usage_from_instance(self.context, instance)
+
+        mock_update_usage.assert_called_once_with(instance, sign=1)
+
+    @mock.patch.object(resource_tracker.ResourceTracker,
+                       '_update_usage')
+    def test_deleted(self, mock_update_usage):
+        instance = self._fake_instance_obj()
+        instance.vm_state = vm_states.DELETED
+        self.tracker.tracked_instances = {}
+        self.tracker.tracked_instances[
+            instance.uuid] = obj_base.obj_to_primitive(instance)
+        self.tracker._update_usage_from_instance(self.context, instance, True)
+
+        mock_update_usage.assert_called_once_with(instance, sign=-1)
+
+
 class UpdateUsageFromMigrationsTestCase(BaseTrackerTestCase):
 
     @mock.patch.object(resource_tracker.ResourceTracker,