Merge lp:~johannes.erdfelt/nova/vm-mode into lp:~hudson-openstack/nova/trunk

[Johannes Erdfelt]

The Xen driver supports running instances in PV or HVM modes, but the method it uses to determine which to use is complicated and doesn't work in all cases. The result is that images that need to use HVM mode (such as FreeBSD 64-bit) end up setting a property named 'os' set to 'windows'.

This branch adds checks for a new property called 'vm_mode'. This can be set to 'pv' or 'hvm' and select the appropriate mode. If the property doesn't exist, the existing logic is still used to determine which mode to use.

[Rick Harris (rconradharris)]

Looks good, nice work Johannes.

I remember at one point we had talked about including virtualization mode as
a separate property. I can't remember exactly why we decided to derive it
from the os_type. It may have had something to do with not exposing the
`vm_mode` directly for some reason or another.

You might want to ping Ant or Cory Wright just to be sure...

> 31 +# Copyright 2010 OpenStack LLC.

s/2010/2011/

> 106 + vm_mode = use_pv_kernel and 'pv' or 'hvm'

Just as a point of comparison, this could also be written using the ternary:

  vm_mode = 'pv' if use_pv_kernel else 'hvm'

review: Approve

[Johannes Erdfelt (johannes.erdfelt)]

I've pushed up a fix for the copyright year.

Is it documented what the minimum version of Python that is required for nova? I avoided the ternary because it's a relatively recent feature. I found an unanswered question on the mailing list asking a similar question.

[Johannes Erdfelt (johannes.erdfelt)]

As far as using os_type for determining what VM mode, I think it's a big mistake to hard code policy in nova.

If Windows ever supports PV then a new os_type will be needed. If FreeBSD 64-bit ever supports PV, then a new os_type will be needed. Along with the requisite code updates.

I think it's a mistake to hard code policy like this and that's the intent of this merge prop. It's intended to make it easy to migrate towards an explicit means of identifying VM mode.

[Rick Harris (rconradharris)]

> Is it documented what the minimum version of Python that is required for nova?

Hmm, good question. I *think* we're targeting 2.6. That said, I know we're using 2.5 features (`with` without from __future__, for example), so I think the ternary is safe if you want to use it.

> I think it's a big mistake to hard code policy in nova.

++

[Trey Morris (tr3buchet)]

Looks good. I agree with you against hard coding policy.

One thing I'd like to see more of:
49 +instances = Table('instances', meta,
50 + Column('id', Integer(), primary_key=True, nullable=False),
51 + )

can be replaced with:

instances = Table('instances', meta, autoload=True)

inside your upgrade and downgrade methods. Makes things cleaner to just load the needed preexisting tables instead of having to basically stub something in place so you can refer to them. This won't work in some cases with sqlite, such as when adding a column with a foreign key.

I'll approve either way, let me know what you decide.

[Johannes Erdfelt (johannes.erdfelt)]

I've pushed a fix to auto load the table instead of stubbing it out.

Thanks Trey.

[Ed Leafe (ed-leafe)]

Lines 8-10: if it is likely that these conditions will be true, it is preferable to use a try/except construct instead of multiple tests. E.g.:

try:
    vm_mode = image['properties']['vm_mode']
except KeyError:
    vm_mode = None

Other than that, lgtm.

[Ed Leafe (ed-leafe)]

OK, after discussing on IRC, let's stick with the current format, as it is more or less consistent with the existing code.

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~johannes.erdfelt/nova/vm-mode into lp:nova failed. Below is the output from the failed tests.

ERROR

======================================================================
ERROR: <nose.suite.ContextSuite context=nova.tests>
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/pymodules/python2.6/nose/suite.py", line 183, in run
    self.setUp()
  File "/usr/lib/pymodules/python2.6/nose/suite.py", line 264, in setUp
    self.setupContext(ancestor)
  File "/usr/lib/pymodules/python2.6/nose/suite.py", line 287, in setupContext
    try_run(context, names)
  File "/usr/lib/pymodules/python2.6/nose/util.py", line 487, in try_run
    return func()
  File "/tmp/tmpREmfSf/nova/tests/__init__.py", line 54, in setup
    migration.db_sync()
  File "/tmp/tmpREmfSf/nova/db/migration.py", line 35, in db_sync
    return IMPL.db_sync(version=version)
  File "/tmp/tmpREmfSf/nova/db/sqlalchemy/migration.py", line 43, in db_sync
    return versioning_api.upgrade(FLAGS.sql_connection, repo_path, version)
  File "/usr/lib/pymodules/python2.6/migrate/versioning/api.py", line 185, in upgrade
    return _migrate(url, repository, version, upgrade=True, err=err, **opts)
  File "<string>", line 2, in _migrate
  File "/usr/lib/pymodules/python2.6/migrate/versioning/util/__init__.py", line 160, in with_engine
    return f(*a, **kw)
  File "/usr/lib/pymodules/python2.6/migrate/versioning/api.py", line 364, in _migrate
    schema.runchange(ver, change, changeset.step)
  File "/usr/lib/pymodules/python2.6/migrate/versioning/schema.py", line 83, in runchange
    change.run(self.engine, step)
  File "/usr/lib/pymodules/python2.6/migrate/versioning/script/py.py", line 137, in run
    script_func = self._func(funcname)
  File "/usr/lib/pymodules/python2.6/migrate/versioning/script/py.py", line 156, in _func
    if not hasattr(self.module, funcname):
  File "/usr/lib/pymodules/python2.6/migrate/versioning/script/py.py", line 152, in module
    self._module = self.verify_module(self.path)
  File "/usr/lib/pymodules/python2.6/migrate/versioning/script/py.py", line 95, in verify_module
    module = import_path(path)
  File "/usr/lib/pymodules/python2.6/migrate/versioning/util/importpath.py", line 12, in import_path
    module = __import__(filename)
  File "/tmp/tmpREmfSf/nova/db/sqlalchemy/migrate_repo/versions/023_add_vm_mode_to_instances.py", line 21, in <module>
    instances = Table('instances', meta, autoload=True)
  File "/usr/lib/python2.6/dist-packages/sqlalchemy/schema.py", line 209, in __new__
    table._init(name, metadata, *args, **kw)
  File "/usr/lib/python2.6/dist-packages/sqlalchemy/schema.py", line 260, in _init
    msg="No engine is bound to this Table's MetaData. "
  File "/usr/lib/python2.6/dist-packages/sqlalchemy/schema.py", line 2464, in _bind_or_error
    raise exc.UnboundExecutionError(msg)
UnboundExecutionError: No engine is bound to this Table's MetaData. Pass an engine to the Table via autoload_with=<someengine>, or associate the MetaData with an engine via metadata.bind=<someengine>
-------------------- >> begin captured logging << --------------------
2011-06...

[Johannes Erdfelt (johannes.erdfelt)]

Serves me right for missing a test run after making changes in response to code reviews.

I've fixed the migrate test failure.

[Ed Leafe (ed-leafe)]

Since this was already approved and the only issue was the test fail, I'm marking this as approved again.

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~johannes.erdfelt/nova/vm-mode into lp:nova failed. Below is the output from the failed tests.

AccountsTest
    test_account_create OK
    test_account_delete OK
    test_account_update OK
    test_get_account OK
AdminAPITest
    test_admin_disabled OK
    test_admin_enabled OK
APITest
    test_exceptions_are_converted_to_faults OK
Test
    test_authorize_project OK
    test_authorize_token OK
    test_authorize_user OK
    test_bad_project OK
    test_bad_token OK
    test_bad_user_bad_key OK
    test_bad_user_good_key OK
    test_no_user OK
    test_not_existing_project OK
    test_token_expiry OK
TestFunctional
    test_token_doesnotexist OK
    test_token_expiry OK
TestLimiter
    test_authorize_token OK
LimiterTest
    test_limiter_custom_max_limit OK
    test_limiter_limit_and_offset OK
    test_limiter_limit_medium OK
    test_limiter_limit_over_max OK
    test_limiter_limit_zero OK
    test_limiter_negative_limit OK
    test_limiter_negative_offset OK
    test_limiter_nothing OK
    test_limiter_offset_bad OK
    test_limiter_offset_blank OK
    test_limiter_offset_medium OK
    test_limiter_offset_over_max OK
    test_limiter_offset_zero OK
PaginationParamsTest
    test_invalid_limit OK
    test_invalid_marker OK
    test_no_params OK
    test_valid_limit OK
    test_valid_marker OK
ActionExtensionTest
    test_extended_action OK
    test_invalid_action OK
    test_invalid_action_body OK
ExtensionControllerTest
    test_get_by_alias OK
    test_index OK
ExtensionManagerTest
    test_get_resources ...

[lp:~johannes.erdfelt/nova/vm-mode]

1173. By Johannes Erdfelt on 2011-06-14

Merge with trunk

[Brian Waldon (bcwaldon)]

One more try :)

review: Approve

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-06-14 21:11:25 +0000
+++ nova/compute/api.py	2011-06-14 21:29:25 +0000
@@ -165,6 +165,9 @@
         os_type = None
         if 'properties' in image and 'os_type' in image['properties']:
             os_type = image['properties']['os_type']
+        vm_mode = None
+        if 'properties' in image and 'vm_mode' in image['properties']:
+            vm_mode = image['properties']['vm_mode']
 
         if kernel_id is None:
             kernel_id = image['properties'].get('kernel_id', None)
@@ -226,7 +229,8 @@
             'locked': False,
             'metadata': metadata,
             'availability_zone': availability_zone,
-            'os_type': os_type}
+            'os_type': os_type,
+            'vm_mode': vm_mode}
 
         return (num_instances, base_options, security_groups)
 

=== added file 'nova/db/sqlalchemy/migrate_repo/versions/023_add_vm_mode_to_instances.py'
--- nova/db/sqlalchemy/migrate_repo/versions/023_add_vm_mode_to_instances.py	1970-01-01 00:00:00 +0000
+++ nova/db/sqlalchemy/migrate_repo/versions/023_add_vm_mode_to_instances.py	2011-06-14 21:29:25 +0000
@@ -0,0 +1,45 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 OpenStack LLC.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from sqlalchemy import Column, Integer, MetaData, String, Table
+
+meta = MetaData()
+
+instances_vm_mode = Column('vm_mode',
+                           String(length=255, convert_unicode=False,
+                                  assert_unicode=None, unicode_error=None,
+                                  _warn_on_bytestring=False),
+                           nullable=True)
+
+
+def upgrade(migrate_engine):
+    # Upgrade operations go here. Don't create your own engine;
+    # bind migrate_engine to your metadata
+    meta.bind = migrate_engine
+
+    instances = Table('instances', meta, autoload=True,
+                      autoload_with=migrate_engine)
+
+    instances.create_column(instances_vm_mode)
+
+
+def downgrade(migrate_engine):
+    meta.bind = migrate_engine
+
+    instances = Table('instances', meta, autoload=True,
+                      autoload_with=migrate_engine)
+
+    instances.drop_column('vm_mode')

=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2011-06-06 15:41:04 +0000
+++ nova/db/sqlalchemy/models.py	2011-06-14 21:29:25 +0000
@@ -232,6 +232,7 @@
     locked = Column(Boolean)
 
     os_type = Column(String(255))
+    vm_mode = Column(String(255))
 
     # TODO(vish): see Ewan's email about state improvements, probably
     #             should be in a driver base class or some such

=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-06-07 13:56:51 +0000
+++ nova/virt/xenapi/vmops.py	2011-06-14 21:29:25 +0000
@@ -160,9 +160,24 @@
         # Create the VM ref and attach the first disk
         first_vdi_ref = self._session.call_xenapi('VDI.get_by_uuid',
                 vdis[0]['vdi_uuid'])
-        use_pv_kernel = VMHelper.determine_is_pv(self._session,
-                instance.id, first_vdi_ref, disk_image_type,
-                instance.os_type)
+
+        vm_mode = instance.vm_mode and instance.vm_mode.lower()
+        if vm_mode == 'pv':
+            use_pv_kernel = True
+        elif vm_mode in ('hv', 'hvm'):
+            use_pv_kernel = False
+            vm_mode = 'hvm'  # Normalize
+        else:
+            use_pv_kernel = VMHelper.determine_is_pv(self._session,
+                    instance.id, first_vdi_ref, disk_image_type,
+                    instance.os_type)
+            vm_mode = use_pv_kernel and 'pv' or 'hvm'
+
+        if instance.vm_mode != vm_mode:
+            # Update database with normalized (or determined) value
+            db.instance_update(context.get_admin_context(),
+                               instance['id'], {'vm_mode': vm_mode})
+
         vm_ref = VMHelper.create_vm(self._session, instance,
                 kernel, ramdisk, use_pv_kernel)
         VMHelper.create_vbd(session=self._session, vm_ref=vm_ref,