Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions nova/compute/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
from nova.compute import vm_states
import nova.conf
from nova import exception
from nova.network import model as network_model
from nova import notifications
from nova.notifications.objects import aggregate as aggregate_notification
from nova.notifications.objects import base as notification_base
Expand Down Expand Up @@ -301,6 +302,64 @@ def heal_reqspec_is_bfv(ctxt, request_spec, instance):
request_spec.save()


def sanitize_image_props_for_kvm(request_spec):
"""Sanitize VMware-specific image properties for KVM scheduling.

Mutates request_spec.image.properties in place to remove or replace
VMware-pinning values so the scheduler can select KVM/CH hosts.

Returns a dict of original values that were overwritten or removed,
suitable for storage in MigrationContext.old_image_properties as a
rollback journal.

Precondition: only valid for VMware-to-KVM cross-hypervisor resize.

:param request_spec: nova.objects.RequestSpec to sanitize in place
:returns: dict of {field_name: original_value} for rollback, or
empty dict if nothing was changed
"""
if (not request_spec.obj_attr_is_set('image') or
request_spec.image is None):
return {}

image = request_spec.image
if not image.obj_attr_is_set('properties'):
return {}

props = image.properties
old = {}

if props.obj_attr_is_set('img_hv_type'):
old['img_hv_type'] = props.img_hv_type
delattr(props, 'img_hv_type')

if props.obj_attr_is_set('hw_disk_bus'):
old['hw_disk_bus'] = props.hw_disk_bus
props.hw_disk_bus = fields.DiskBus.VIRTIO

if props.obj_attr_is_set('hw_cdrom_bus'):
old['hw_cdrom_bus'] = props.hw_cdrom_bus
props.hw_cdrom_bus = fields.DiskBus.VIRTIO

if props.obj_attr_is_set('hw_scsi_model'):
old['hw_scsi_model'] = props.hw_scsi_model
delattr(props, 'hw_scsi_model')

if props.obj_attr_is_set('hw_vif_model'):
old['hw_vif_model'] = props.hw_vif_model
props.hw_vif_model = network_model.VIF_MODEL_VIRTIO

if props.obj_attr_is_set('hw_video_model'):
old['hw_video_model'] = props.hw_video_model
props.hw_video_model = fields.VideoModel.VIRTIO

if props.obj_attr_is_set('img_hv_requested_version'):
old['img_hv_requested_version'] = props.img_hv_requested_version
delattr(props, 'img_hv_requested_version')

return old


def convert_mb_to_ceil_gb(mb_value):
gb_int = 0
if mb_value:
Expand Down
31 changes: 30 additions & 1 deletion nova/conductor/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,9 @@ def reset(self):
exception.HypervisorUnavailable,
exception.InstanceInvalidState,
exception.MigrationPreCheckError,
exception.UnsupportedPolicyException)
exception.UnsupportedPolicyException,
exception.InvalidCrossHvResize,
exception.InvalidCrossHvResizePrecondition)
@targets_cell
@wrap_instance_event(prefix='conductor')
def migrate_server(self, context, instance, scheduler_hint, live, rebuild,
Expand Down Expand Up @@ -427,6 +429,15 @@ def _cold_migrate(self, context, instance, flavor, filter_properties,
self._set_vm_state_and_notify(context, instance.uuid,
'migrate_server',
updates, ex, request_spec)
except (exception.InvalidCrossHvResize,
exception.InvalidCrossHvResizePrecondition) as ex:
vm_state = instance.vm_state
if not vm_state:
vm_state = vm_states.ACTIVE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a test for this branch would also be nice: Verify the instance state is properly restored (not set to ERROR) when the exception fires and resets to the ACTIVE fallback.

updates = {'vm_state': vm_state, 'task_state': None}
self._set_vm_state_and_notify(context, instance.uuid,
'resize_server',
updates, ex, request_spec)
except Exception as ex:
with excutils.save_and_reraise_exception():
# Refresh the instance so we don't overwrite vm_state changes
Expand All @@ -449,6 +460,24 @@ def _cold_migrate(self, context, instance, flavor, filter_properties,
# NOTE(sbauza): Make sure we persist the new flavor in case we had
# a successful scheduler call if and only if nothing bad happened
if request_spec.obj_what_changed():
# Persist the rollback journal for image properties BEFORE
# saving the modified request_spec, so that revert can always
# find the originals even if a crash occurs between these two
# writes.
old_img_props = getattr(task, '_old_image_properties', None)
if old_img_props and isinstance(old_img_props, dict):
if ('migration_context' in instance and
instance.migration_context is not None):
mig_context = instance.migration_context
else:
mig_context = objects.MigrationContext(
context=context,
instance_uuid=instance.uuid,
migration_id=task._migration.id)
mig_context.old_image_properties = old_img_props
instance.migration_context = mig_context
instance.save()

# NOTE(jkulik): Make sure we do not store the "volume_sizes"
# scheduler_hint as that can change at any time and saving it in
# the DB thus is not useful.
Expand Down
34 changes: 34 additions & 0 deletions nova/conductor/tasks/migrate.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@

from nova import availability_zones
from nova.compute import utils as compute_utils
from nova.compute import power_state
from nova.conductor.tasks import base
from nova.conductor.tasks import cross_cell_migrate
from nova import exception
from nova.i18n import _
from nova import objects
from nova.scheduler.client import report
from nova.scheduler import utils as scheduler_utils
from nova.scheduler.filters import extra_specs_ops

LOG = logging.getLogger(__name__)

Expand Down Expand Up @@ -113,6 +115,7 @@ def revert_allocation_for_migration(context, source_cn, instance, migration):


class MigrationTask(base.TaskBase):

def __init__(self, context, instance, flavor,
request_spec, clean_shutdown, compute_rpcapi,
query_client, report_client, host_list, network_api):
Expand Down Expand Up @@ -283,6 +286,18 @@ def _execute(self):
self.request_spec.ensure_network_information(self.instance)
compute_utils.heal_reqspec_is_bfv(
self.context, self.request_spec, self.instance)
# For cross-hypervisor resize (VMware to KVM), sanitize the image
# properties directly on the canonical request_spec so the scheduler
# can select KVM/CH hosts. The returned dict of original values is
# stashed for the conductor to persist into MigrationContext before
# request_spec.save().
self._old_image_properties = {}
src_hv = self._source_cn.hypervisor_type

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could src_hv somehow also be None? It's properly handled for dest_hv below, just feels like we could do the same for src_hv?

dest_hv = self.flavor.extra_specs.get('capabilities:hypervisor_type')
if dest_hv and not extra_specs_ops.match(src_hv, dest_hv):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no test where dest_hv is set but is identical to src_hv (so both are VMware or both CH). It think it would be nice to have a test that we don't break the existing same-HV resize flow: It should confirm that _prep_cross_hv_resize is not called when source and destination are same-HV.

LOG.debug('Cross-hypervisor resize detected')
self._prep_cross_hv_resize(src_hv, dest_hv)

# On an initial call to migrate, 'self.host_list' will be None, so we
# have to call the scheduler to get a list of acceptable hosts to
# migrate to. That list will consist of a selected host, along with
Expand Down Expand Up @@ -338,6 +353,25 @@ def _execute(self):
node=node, clean_shutdown=self.clean_shutdown,
host_list=self.host_list)

def _prep_cross_hv_resize(self, src_hv: str, dest_hv: str):
if extra_specs_ops.match("VMware vCenter Server", src_hv) \
and extra_specs_ops.match("CH", dest_hv):
if not self.request_spec.is_bfv:
raise exception.InvalidCrossHvResizePrecondition(
'Must be BFV instance')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Must be BFV instance')
reason='Must be BFV instance')

if self.instance.power_state != power_state.RUNNING:
raise exception.InvalidCrossHvResizePrecondition(
'Instance must be running for cross-hypervisor resize. '

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Instance must be running for cross-hypervisor resize. '
reason='Instance must be running for cross-hypervisor resize. '

'The compute service will take care of stopping the instance.')

self._old_image_properties = (
compute_utils.sanitize_image_props_for_kvm(
self.request_spec))

else:
raise exception.InvalidCrossHvResize(
src_hv_type=src_hv, dest_hv_type=dest_hv)

def _schedule(self):
selection_lists = self.query_client.select_destinations(
self.context, self.request_spec, [self.instance.uuid],
Expand Down
8 changes: 8 additions & 0 deletions nova/exception.py
Original file line number Diff line number Diff line change
Expand Up @@ -2547,3 +2547,11 @@ class VolumeMigrationError(NovaException):
class InvalidGuestCpuNumaConfig(NovaException):
msg_fmt = _('The guest is configured with a cell with id %(cell_id)s '
'which the host does not provide.')


class InvalidCrossHvResize(NovaException):
msg_fmt = _('Resize from hypervisor type %(src_hv_type)s to '
'%(dest_hv_type)s is not allowed')

class InvalidCrossHvResizePrecondition(NovaException):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class InvalidCrossHvResizePrecondition(NovaException):
class InvalidCrossHvResizePrecondition(NovaException):

to make PEP8 happy I guess.

msg_fmt = _('Cross-hypervisor resize precondition not met: %(reason)s')
6 changes: 5 additions & 1 deletion nova/objects/migration_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ class MigrationContext(base.NovaPersistentObject, base.NovaObject):
# Version 1.0: Initial version
# Version 1.1: Add old/new pci_devices and pci_requests
# Version 1.2: Add old/new resources
VERSION = '1.2'
# Version 1.3: Add old_image_properties
VERSION = '1.3'

fields = {
'instance_uuid': fields.UUIDField(),
Expand All @@ -61,11 +62,14 @@ class MigrationContext(base.NovaPersistentObject, base.NovaObject):
nullable=True),
'old_resources': fields.ObjectField('ResourceList',
nullable=True),
'old_image_properties': fields.DictOfStringsField(nullable=True),
}

@classmethod
def obj_make_compatible(cls, primitive, target_version):
target_version = versionutils.convert_version_to_tuple(target_version)
if target_version < (1, 3):
primitive.pop('old_image_properties', None)
if target_version < (1, 2):
primitive.pop('old_resources', None)
primitive.pop('new_resources', None)
Expand Down
131 changes: 131 additions & 0 deletions nova/tests/unit/compute/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2074,3 +2074,134 @@ def test_delete_with_arq_uuid_and_dp(self, mock_del_inst, mock_del_uuid):
compute_utils.delete_arqs_if_needed(self.context, instance, arq_uuids)
mock_del_inst.assert_called_once_with(instance.uuid)
mock_del_uuid.assert_called_once_with(arq_uuids)


class TestSanitizeImagePropsForKvm(test.NoDBTestCase):
"""Tests for sanitize_image_props_for_kvm (early-commit approach)."""

def _make_request_spec(self, **image_props):
props = objects.ImageMetaProps(**image_props)
image = objects.ImageMeta(properties=props)
return objects.RequestSpec(image=image)

def test_img_hv_type_removed_and_journaled(self):
reqspec = self._make_request_spec(img_hv_type='vmware')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertFalse(
reqspec.image.properties.obj_attr_is_set('img_hv_type'))
self.assertEqual('vmware', old['img_hv_type'])

def test_img_hv_type_any_value_removed(self):
reqspec = self._make_request_spec(img_hv_type='kvm')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertFalse(
reqspec.image.properties.obj_attr_is_set('img_hv_type'))
self.assertEqual('kvm', old['img_hv_type'])

def test_hw_disk_bus_replaced_with_virtio(self):
reqspec = self._make_request_spec(hw_disk_bus='scsi')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertEqual('virtio', reqspec.image.properties.hw_disk_bus)
self.assertEqual('scsi', old['hw_disk_bus'])

def test_hw_disk_bus_already_virtio_still_journaled(self):
reqspec = self._make_request_spec(hw_disk_bus='virtio')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertEqual('virtio', reqspec.image.properties.hw_disk_bus)
self.assertEqual('virtio', old['hw_disk_bus'])

def test_hw_scsi_model_removed_and_journaled(self):
reqspec = self._make_request_spec(hw_scsi_model='virtio-scsi')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertFalse(
reqspec.image.properties.obj_attr_is_set('hw_scsi_model'))
self.assertEqual('virtio-scsi', old['hw_scsi_model'])

def test_hw_scsi_model_absent_no_error(self):
reqspec = self._make_request_spec(hw_disk_bus='scsi')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertNotIn('hw_scsi_model', old)

def test_hw_vif_model_replaced_with_virtio(self):
reqspec = self._make_request_spec(hw_vif_model='vmxnet3')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertEqual('virtio', reqspec.image.properties.hw_vif_model)
self.assertEqual('vmxnet3', old['hw_vif_model'])

def test_hw_video_model_replaced_with_virtio(self):
reqspec = self._make_request_spec(hw_video_model='vmvga')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertEqual('virtio',
reqspec.image.properties.hw_video_model)
self.assertEqual('vmvga', old['hw_video_model'])

def test_mutates_in_place(self):
reqspec = self._make_request_spec(
img_hv_type='vmware', hw_disk_bus='scsi')
props_before = reqspec.image.properties
compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertIs(props_before, reqspec.image.properties)

def test_returns_complete_journal(self):
reqspec = self._make_request_spec(
img_hv_type='vmware', hw_disk_bus='scsi',
hw_cdrom_bus='ide', hw_scsi_model='virtio-scsi',
hw_vif_model='vmxnet3', hw_video_model='vmvga',
img_hv_requested_version='>=6.0')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertEqual({
'img_hv_type': 'vmware',
'hw_disk_bus': 'scsi',
'hw_cdrom_bus': 'ide',
'hw_scsi_model': 'virtio-scsi',
'hw_vif_model': 'vmxnet3',
'hw_video_model': 'vmvga',
'img_hv_requested_version': '>=6.0',
}, old)

def test_no_image_returns_empty_dict(self):
reqspec = objects.RequestSpec()
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertEqual({}, old)

def test_image_none_returns_empty_dict(self):
reqspec = objects.RequestSpec(image=None)
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertEqual({}, old)

def test_hw_cdrom_bus_replaced_with_virtio(self):
reqspec = self._make_request_spec(hw_cdrom_bus='ide')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertEqual('virtio', reqspec.image.properties.hw_cdrom_bus)
self.assertEqual('ide', old['hw_cdrom_bus'])

def test_hw_cdrom_bus_sata_replaced_with_virtio(self):
reqspec = self._make_request_spec(hw_cdrom_bus='sata')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertEqual('virtio', reqspec.image.properties.hw_cdrom_bus)
self.assertEqual('sata', old['hw_cdrom_bus'])

def test_hw_cdrom_bus_already_virtio_still_journaled(self):
reqspec = self._make_request_spec(hw_cdrom_bus='virtio')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertEqual('virtio', reqspec.image.properties.hw_cdrom_bus)
self.assertEqual('virtio', old['hw_cdrom_bus'])

def test_img_hv_requested_version_removed_and_journaled(self):
reqspec = self._make_request_spec(
img_hv_requested_version='>=6.0,<7.0')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertFalse(
reqspec.image.properties.obj_attr_is_set(
'img_hv_requested_version'))
self.assertEqual('>=6.0,<7.0', old['img_hv_requested_version'])

def test_img_hv_requested_version_absent_no_error(self):
reqspec = self._make_request_spec(img_hv_type='vmware')
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertNotIn('img_hv_requested_version', old)

def test_no_props_set_returns_empty_dict(self):
reqspec = self._make_request_spec()
old = compute_utils.sanitize_image_props_for_kvm(reqspec)
self.assertEqual({}, old)
Loading