Merge lp:~jk0/nova/diagnostics-per-instance into lp:~hudson-openstack/nova/trunk

[Josh Kearney]

Make InstanceActions and live diagnostics available through the Admin API.

The InstanceDiagnostics table was removed since storing diagnostics is beyond the scope of this blueprint and is a big enough project to have its own blueprint.

[Matt Dietz (cerberus)]

I'm not going to make you do this, but we need to more explicitly establish what "admin" API means, because right now it's simply whether or not a whole slew of extra routes are available. No vetting of any kind is being done. I know that the DB layer will ultimately reject the request, but I think that's a silly waste of time if the API 1) already knows what you are and 2) can do something about it

Also, all controller verbs returning data are expected to format it into a dictionary for proper formatting into the Accept'd type. It doesn't look like this would return anything we would want to the client. We should be consistent

review: Needs Fixing

[Matt Dietz (cerberus)]

Josh corrected me offline. I'm fine with the rest

review: Approve

[Eric Day (eday)]

70, 81: Does this need to be elevated?

For get_actions, why are you diving all the way down into the virt worker when you could just read the table from compute.api? I know we discussed storing this data elsewhere eventually since it's not critical for operation, but for now we are still central (and this call will probably happen in compuite.api. I imagine when libvirt support is added it will use the same reporting mechanism (which is currently the actions table), so I wouldn't worry about different implementations yet either.

review: Needs Fixing

[Cory Wright (corywright)]

I don't understand the changes to test_run_terminate. Why change

  self.assertEqual(len(instances), 1)

to

  self.assertNotEqual(instance_count, 0)

? The former is more specific and is a tighter test.

Also, the assertion should probably compare against a constant value that you are expecting, 0 or 1 in this case, rather than len(instances) + 1.

You may have a good reason for these changes and I'm just not aware of it. If so, let me know. :)

[Josh Kearney (jk0)]

> 70, 81: Does this need to be elevated?

I don't believe so -- good catch. I'll remove that and test again without it.

> For get_actions, why are you diving all the way down into the virt worker when
> you could just read the table from compute.api? I know we discussed storing
> this data elsewhere eventually since it's not critical for operation, but for
> now we are still central (and this call will probably happen in compuite.api.
> I imagine when libvirt support is added it will use the same reporting
> mechanism (which is currently the actions table), so I wouldn't worry about
> different implementations yet either.

Sure thing. I'll clean that up.

[Josh Kearney (jk0)]

> I don't understand the changes to test_run_terminate. Why change
>
> self.assertEqual(len(instances), 1)
>
> to
>
> self.assertNotEqual(instance_count, 0)
>
> ? The former is more specific and is a tighter test.

I had to change that because the test was failing when more than one instance was returned. By doing it this way, as long as the instance count is >0, we know there is one or more running. The restriction comes from Python 2.6's unittest. There just isn't much to work with - it's either Equal or Not Equal.

> Also, the assertion should probably compare against a constant value that you
> are expecting, 0 or 1 in this case, rather than len(instances) + 1.
>
> You may have a good reason for these changes and I'm just not aware of it. If
> so, let me know. :)

The thing is we don't know that it will *always* be 1 or 0. We could start off with 15 running instances. Terminate one. Assert that the original instance_count is equal to the current instance count + 1. If we were running Python 2.7's unittest, there is assertGreater, assertGreaterEqual, etc, but unfortunately those aren't available in 2.6.

[Eric Day (eday)]

But what actually changed in this patch that would have broken the tests? Is this unrelated to the changes you propose? I've not seen any issues with this test case failing as it was.

[Josh Kearney (jk0)]

> But what actually changed in this patch that would have broken the tests? Is
> this unrelated to the changes you propose? I've not seen any issues with this
> test case failing as it was.

Nothing changed in this branch that would break the tests -- I just merged trunk yesterday and ran the tests and had that problem.

[Eric Day (eday)]

get_actions is still going over an RPC call to the compute manager, where it could be answered directly in compute.api. Push it one more layer up :)

[Cory Wright (corywright)]

> > Also, the assertion should probably compare against a constant value that you
> > are expecting, 0 or 1 in this case, rather than len(instances) + 1.
> >
> The thing is we don't know that it will *always* be 1 or 0. We could start off
> with 15 running instances. Terminate one. Assert that the original
> instance_count is equal to the current instance count + 1.

Shouldn't those instances be torn down between each test case?

> If we were running
> Python 2.7's unittest, there is assertGreater, assertGreaterEqual, etc, but
> unfortunately those aren't available in 2.6.

Don't forget that you can always just use Python's built-in assert:

  assert len(instances) > 0

[Eric Day (eday)]

Sorry to be a bother, but compute.api methods should include a verb, such as get_actions, not just 'actions'. In the future I could see having other operations on actions. :)

[Eric Day (eday)]

lgtm!

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~jk0/nova/diagnostics-per-instance into lp:nova failed. Below is the output from the failed tests.

TrialTestCase
    runTest ok
AdminAPITest
    test_admin_disabled ok
    test_admin_enabled ok
APITest
    test_exceptions_are_converted_to_faults ok
Test
    test_authorize_token ok
    test_authorize_user ok
    test_bad_token ok
    test_bad_user ok
    test_no_user ok
    test_token_expiry ok
TestLimiter
    test_authorize_token ok
TestFaults
    test_fault_parts ok
    test_raise ok
    test_retry_header ok
FlavorsTest
    test_get_flavor_by_id ok
    test_get_flavor_list ok
GlanceImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
ImageControllerWithGlanceServiceTest
    test_get_image_details ok
    test_get_image_index ok
LocalImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
LimiterTest
    test_minute ok
    test_one_per_period ok
    test_second ok
    test_users_get_separate_buckets ok
    test_we_can_go_indefinitely_if_we_spread_out_requests ok
WSGIAppProxyTest
    test_200 ok
    test_403 ok
    test_failure ok
WSGIAppTest
    test_escaping ok
    test_good_urls ok
    test_invalid_methods ok
    test_invalid_urls ok
    test_response_to_delays ok
ServersTest
    test_create_backup_schedules ERROR
    test_create_instance ERROR
    test_delete_backup_schedules ERROR
    test_delete_server_instance ...

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~jk0/nova/diagnostics-per-instance into lp:nova failed. Below is the output from the failed tests.

TrialTestCase
    runTest ok
AdminAPITest
    test_admin_disabled ok
    test_admin_enabled ok
APITest
    test_exceptions_are_converted_to_faults ok
Test
    test_authorize_token ok
    test_authorize_user ok
    test_bad_token ok
    test_bad_user ok
    test_no_user ok
    test_token_expiry ok
TestLimiter
    test_authorize_token ok
TestFaults
    test_fault_parts ok
    test_raise ok
    test_retry_header ok
FlavorsTest
    test_get_flavor_by_id ok
    test_get_flavor_list ok
GlanceImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
ImageControllerWithGlanceServiceTest
    test_get_image_details ok
    test_get_image_index ok
LocalImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
LimiterTest
    test_minute ok
    test_one_per_period ok
    test_second ok
    test_users_get_separate_buckets ok
    test_we_can_go_indefinitely_if_we_spread_out_requests ok
WSGIAppProxyTest
    test_200 ok
    test_403 ok
    test_failure ok
WSGIAppTest
    test_escaping ok
    test_good_urls ok
    test_invalid_methods ok
    test_invalid_urls ok
    test_response_to_delays ok
ServersTest
    test_create_backup_schedules ok
    test_create_instance ok
    test_delete_backup_schedules ok
    test_delete_server_instance ...

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~jk0/nova/diagnostics-per-instance into lp:nova failed. Below is the output from the failed tests.

TrialTestCase
    runTest ok
AdminAPITest
    test_admin_disabled ok
    test_admin_enabled ok
APITest
    test_exceptions_are_converted_to_faults ok
Test
    test_authorize_token ok
    test_authorize_user ok
    test_bad_token ok
    test_bad_user ok
    test_no_user ok
    test_token_expiry ok
TestLimiter
    test_authorize_token ok
TestFaults
    test_fault_parts ok
    test_raise ok
    test_retry_header ok
FlavorsTest
    test_get_flavor_by_id ok
    test_get_flavor_list ok
GlanceImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
ImageControllerWithGlanceServiceTest
    test_get_image_details ok
    test_get_image_index ok
LocalImageServiceTest
    test_create ok
    test_create_and_show_non_existing_image ok
    test_delete ok
    test_update ok
LimiterTest
    test_minute ok
    test_one_per_period ok
    test_second ok
    test_users_get_separate_buckets ok
    test_we_can_go_indefinitely_if_we_spread_out_requests ok
WSGIAppProxyTest
    test_200 ok
    test_403 ok
    test_failure ok
WSGIAppTest
    test_escaping ok
    test_good_urls ok
    test_invalid_methods ok
    test_invalid_urls ok
    test_response_to_delays ok
ServersTest
    test_create_backup_schedules ok
    test_create_instance ok
    test_delete_backup_schedules ok
    test_delete_server_instance ...

[lp:~jk0/nova/diagnostics-per-instance]

501. By Josh Kearney on 2010-12-31

Removed problematic test

502. By Josh Kearney on 2010-12-31

Improved test

503. By Josh Kearney on 2010-12-31

Cleanup

=== modified file 'nova/api/openstack/__init__.py'
--- nova/api/openstack/__init__.py	2010-12-30 17:26:21 +0000
+++ nova/api/openstack/__init__.py	2010-12-31 01:09:13 +0000
@@ -93,6 +93,8 @@
             logging.debug("Including admin operations in API.")
             server_members['pause'] = 'POST'
             server_members['unpause'] = 'POST'
+            server_members["diagnostics"] = "GET"
+            server_members["actions"] = "GET"
             server_members['suspend'] = 'POST'
             server_members['resume'] = 'POST'
 

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2010-12-28 23:34:51 +0000
+++ nova/api/openstack/servers.py	2010-12-31 01:09:13 +0000
@@ -218,3 +218,13 @@
             logging.error(_("compute.api::resume %s"), readable)
             return faults.Fault(exc.HTTPUnprocessableEntity())
         return exc.HTTPAccepted()
+
+    def diagnostics(self, req, id):
+        """Permit Admins to retrieve server diagnostics."""
+        ctxt = req.environ["nova.context"]
+        return self.compute_api.get_diagnostics(ctxt, id)
+
+    def actions(self, req, id):
+        """Permit Admins to retrieve server actions."""
+        ctxt = req.environ["nova.context"]
+        return self.compute_api.get_actions(ctxt, id)

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2010-12-28 19:01:54 +0000
+++ nova/compute/api.py	2010-12-31 01:09:13 +0000
@@ -293,6 +293,20 @@
                  {"method": "unpause_instance",
                   "args": {"instance_id": instance['id']}})
 
+    def get_diagnostics(self, context, instance_id):
+        """Retrieve diagnostics for the given instance."""
+        instance = self.db.instance_get_by_internal_id(context, instance_id)
+        host = instance["host"]
+        return rpc.call(context,
+            self.db.queue_get_for(context, FLAGS.compute_topic, host),
+            {"method": "get_diagnostics",
+             "args": {"instance_id": instance["id"]}})
+
+    def get_actions(self, context, instance_id):
+        """Retrieve actions for the given instance."""
+        instance = self.db.instance_get_by_internal_id(context, instance_id)
+        return self.db.instance_get_actions(context, instance["id"])
+
     def suspend(self, context, instance_id):
         """suspend the instance with instance_id"""
         instance = self.db.instance_get_by_internal_id(context, instance_id)

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2010-12-28 19:01:54 +0000
+++ nova/compute/manager.py	2010-12-31 01:09:13 +0000
@@ -318,6 +318,15 @@
                                                        result))
 
     @exception.wrap_exception
+    def get_diagnostics(self, context, instance_id):
+        """Retrieve diagnostics for an instance on this server."""
+        instance_ref = self.db.instance_get(context, instance_id)
+
+        if instance_ref["state"] == power_state.RUNNING:
+            logging.debug(_("instance %s: retrieving diagnostics"),
+                instance_ref["internal_id"])
+            return self.driver.get_diagnostics(instance_ref)
+
     def suspend_instance(self, context, instance_id):
         """suspend the instance with instance_id"""
         context = context.elevated()

=== modified file 'nova/db/api.py'
--- nova/db/api.py	2010-12-22 21:17:30 +0000
+++ nova/db/api.py	2010-12-31 01:09:13 +0000
@@ -383,6 +383,11 @@
     return IMPL.instance_action_create(context, values)
 
 
+def instance_get_actions(context, instance_id):
+    """Get instance actions by instance id."""
+    return IMPL.instance_get_actions(context, instance_id)
+
+
 ###################
 
 

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2010-12-23 21:41:54 +0000
+++ nova/db/sqlalchemy/api.py	2010-12-31 01:09:13 +0000
@@ -856,6 +856,18 @@
     return action_ref
 
 
+@require_admin_context
+def instance_get_actions(context, instance_id):
+    """Return the actions associated to the given instance id"""
+    session = get_session()
+    actions = {}
+    for action in session.query(models.InstanceActions).\
+        filter_by(instance_id=instance_id).\
+        all():
+        actions[action.action] = action.error
+    return actions
+
+
 ###################
 
 

=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2010-12-22 21:17:30 +0000
+++ nova/db/sqlalchemy/models.py	2010-12-31 01:09:13 +0000
@@ -22,7 +22,7 @@
 import datetime
 
 from sqlalchemy.orm import relationship, backref, object_mapper
-from sqlalchemy import Column, Integer, Float, String, schema
+from sqlalchemy import Column, Integer, String, schema
 from sqlalchemy import ForeignKey, DateTime, Boolean, Text
 from sqlalchemy.exc import IntegrityError
 from sqlalchemy.ext.declarative import declarative_base
@@ -236,21 +236,6 @@
     #                     'shutdown', 'shutoff', 'crashed'])
 
 
-class InstanceDiagnostics(BASE, NovaBase):
-    """Represents a guest VM's diagnostics"""
-    __tablename__ = "instance_diagnostics"
-    id = Column(Integer, primary_key=True)
-    instance_id = Column(Integer, ForeignKey('instances.id'))
-
-    memory_available = Column(Float)
-    memory_free = Column(Float)
-    cpu_load = Column(Float)
-    disk_read = Column(Float)
-    disk_write = Column(Float)
-    net_tx = Column(Float)
-    net_rx = Column(Float)
-
-
 class InstanceActions(BASE, NovaBase):
     """Represents a guest VM's actions and results"""
     __tablename__ = "instance_actions"
@@ -561,7 +546,7 @@
     it will never need to be called explicitly elsewhere.
     """
     from sqlalchemy import create_engine
-    models = (Service, Instance, InstanceDiagnostics, InstanceActions,
+    models = (Service, Instance, InstanceActions,
               Volume, ExportDevice, IscsiTarget, FixedIp, FloatingIp,
               Network, SecurityGroup, SecurityGroupIngressRule,
               SecurityGroupInstanceAssociation, AuthToken, User,

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2010-12-23 21:33:16 +0000
+++ nova/tests/api/openstack/test_servers.py	2010-12-31 01:09:13 +0000
@@ -95,6 +95,10 @@
                        fake_compute_api)
         self.stubs.Set(nova.compute.api.ComputeAPI, 'resume',
                        fake_compute_api)
+        self.stubs.Set(nova.compute.api.ComputeAPI, "get_diagnostics",
+                       fake_compute_api)
+        self.stubs.Set(nova.compute.api.ComputeAPI, "get_actions",
+                       fake_compute_api)
         self.allow_admin = FLAGS.allow_admin_api
 
     def tearDown(self):
@@ -274,6 +278,18 @@
         res = req.get_response(nova.api.API('os'))
         self.assertEqual(res.status_int, 202)
 
+    def test_server_diagnostics(self):
+        req = webob.Request.blank("/v1.0/servers/1/diagnostics")
+        req.method = "GET"
+        res = req.get_response(nova.api.API("os"))
+        self.assertEqual(res.status_int, 404)
+
+    def test_server_actions(self):
+        req = webob.Request.blank("/v1.0/servers/1/actions")
+        req.method = "GET"
+        res = req.get_response(nova.api.API("os"))
+        self.assertEqual(res.status_int, 404)
+
     def test_server_reboot(self):
         body = dict(server=dict(
             name='server_test', imageId=2, flavorId=2, metadata={},

=== modified file 'nova/tests/test_xenapi.py'
--- nova/tests/test_xenapi.py	2010-12-28 18:53:32 +0000
+++ nova/tests/test_xenapi.py	2010-12-31 01:09:13 +0000
@@ -166,6 +166,10 @@
         instances = self.conn.list_instances()
         self.assertEquals(instances, [])
 
+    def test_get_diagnostics(self):
+        instance = self._create_instance()
+        self.conn.get_diagnostics(instance)
+
     def test_instance_snapshot(self):
         stubs.stubout_instance_snapshot(self.stubs)
         instance = self._create_instance()
@@ -253,7 +257,7 @@
                   'kernel_id': 2,
                   'ramdisk_id': 3,
                   'instance_type': 'm1.large',
-                  'mac_address': 'aa:bb:cc:dd:ee:ff',
+                  'mac_address': 'aa:bb:cc:dd:ee:ff'
                   }
         instance = db.instance_create(values)
         self.conn.spawn(instance)

=== modified file 'nova/virt/fake.py'
--- nova/virt/fake.py	2010-12-28 19:01:54 +0000
+++ nova/virt/fake.py	2010-12-31 01:09:13 +0000
@@ -216,6 +216,9 @@
                 'num_cpu': 2,
                 'cpu_time': 0}
 
+    def get_diagnostics(self, instance_name):
+        pass
+
     def list_disks(self, instance_name):
         """
         Return the IDs of all the virtual disks attached to the specified

=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2010-12-30 17:26:21 +0000
+++ nova/virt/libvirt_conn.py	2010-12-31 01:09:13 +0000
@@ -587,6 +587,9 @@
                 'num_cpu': num_cpu,
                 'cpu_time': cpu_time}
 
+    def get_diagnostics(self, instance_name):
+        raise exception.APIError("diagnostics are not supported for libvirt")
+
     def get_disks(self, instance_name):
         """
         Note that this function takes an instance name, not an Instance, so

=== modified file 'nova/virt/xenapi/vm_utils.py'
--- nova/virt/xenapi/vm_utils.py	2010-12-28 19:01:54 +0000
+++ nova/virt/xenapi/vm_utils.py	2010-12-31 01:09:13 +0000
@@ -347,6 +347,10 @@
         try:
             host = session.get_xenapi_host()
             host_ip = session.get_xenapi().host.get_record(host)["address"]
+        except (cls.XenAPI.Failure, KeyError) as e:
+            return {"Unable to retrieve diagnostics": e}
+
+        try:
             diags = {}
             xml = get_rrd(host_ip, record["uuid"])
             if xml:

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2010-12-28 19:01:54 +0000
+++ nova/virt/xenapi/vmops.py	2010-12-31 01:09:13 +0000
@@ -268,11 +268,12 @@
         rec = self._session.get_xenapi().VM.get_record(vm)
         return VMHelper.compile_info(rec)
 
-    def get_diagnostics(self, instance_id):
+    def get_diagnostics(self, instance):
         """Return data about VM diagnostics"""
-        vm = VMHelper.lookup(self._session, instance_id)
+        vm = VMHelper.lookup(self._session, instance.name)
         if vm is None:
-            raise exception.NotFound(_("Instance not found %s") % instance_id)
+            raise exception.NotFound(_("Instance not found %s") %
+                instance.name)
         rec = self._session.get_xenapi().VM.get_record(vm)
         return VMHelper.compile_diagnostics(self._session, rec)
 

=== modified file 'nova/virt/xenapi_conn.py'
--- nova/virt/xenapi_conn.py	2010-12-28 19:01:54 +0000
+++ nova/virt/xenapi_conn.py	2010-12-31 01:09:13 +0000
@@ -167,9 +167,9 @@
         """Return data about VM instance"""
         return self._vmops.get_info(instance_id)
 
-    def get_diagnostics(self, instance_id):
+    def get_diagnostics(self, instance):
         """Return data about VM diagnostics"""
-        return self._vmops.get_diagnostics(instance_id)
+        return self._vmops.get_diagnostics(instance)
 
     def get_console_output(self, instance):
         """Return snapshot of console"""
@@ -242,7 +242,7 @@
             status = self._session.xenapi.task.get_status(task)
             action = dict(
                 instance_id=int(id),
-                action=name,
+                action=name[0:255],  # Ensure action is never > 255
                 error=None)
             if status == "pending":
                 return