openstack/nova
Code Issues Proposed changes

Fix Instance object assumptions about joins

Browse Source 
The Instance object was assuming that security_groups and info_cache
were always joined for single instance_get() type queries.  This is a poor
assumption to make.  The object should explicitly ask for info_cache and
security_groups to be included in the DB object when necessary.

Also, we should actually fix sqlalchemy api to not join these always, but
that's probably too large of a change right now.  Instead, make sure we
don't ask to join them twice in the same query.

Fixes bug 1227251

Change-Id: Ib39a3d041904fb5ecebbe152f2fede135a0cdb6f
This commit is contained in:
Chris Behrens
2013-09-19 17:51:16 +00:00
parent 057534f2ba
commit 19953cee68
6 changed files with 112 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files

100
nova/objects/instance.py
View File

@@ -34,19 +34,27 @@ CONF = cfg.CONF
LOG = logging.getLogger(__name__)
# These are fields that can be specified as expected_attrs but are
# also joined often by default.
INSTANCE_OPTIONAL_COMMON_FIELDS = ['metadata', 'system_metadata']
# These are fields that can be specified as expected_attrs
INSTANCE_OPTIONAL_FIELDS = (INSTANCE_OPTIONAL_COMMON_FIELDS +
['fault', 'pci_devices'])
# These are fields that are always joined by the db right now
INSTANCE_IMPLIED_FIELDS = ['info_cache', 'security_groups']
# List of fields that can be joined in DB layer.
_INSTANCE_OPTIONAL_JOINED_FIELDS = ['metadata', 'system_metadata',
'info_cache', 'security_groups',
'pci_devices']
# These are fields that are optional but don't translate to db columns
INSTANCE_OPTIONAL_NON_COLUMNS = ['fault']
# These are all fields that most query calls load by default
INSTANCE_DEFAULT_FIELDS = (INSTANCE_OPTIONAL_COMMON_FIELDS +
INSTANCE_IMPLIED_FIELDS)
_INSTANCE_OPTIONAL_NON_COLUMN_FIELDS = ['fault']
# These are fields that can be specified as expected_attrs
INSTANCE_OPTIONAL_ATTRS = (_INSTANCE_OPTIONAL_JOINED_FIELDS +
_INSTANCE_OPTIONAL_NON_COLUMN_FIELDS)
# These are fields that most query calls load by default
INSTANCE_DEFAULT_FIELDS = ['metadata', 'system_metadata',
'info_cache', 'security_groups']
def _expected_cols(expected_attrs):
"""Return expected_attrs that are columns needing joining."""
if not expected_attrs:
return expected_attrs
return [attr for attr in expected_attrs
if attr in _INSTANCE_OPTIONAL_JOINED_FIELDS]
class Instance(base.NovaPersistentObject, base.NovaObject):
@@ -244,7 +252,7 @@ class Instance(base.NovaPersistentObject, base.NovaObject):
expected_attrs = []
# Most of the field names match right now, so be quick
for field in instance.fields:
if field in INSTANCE_OPTIONAL_FIELDS + INSTANCE_IMPLIED_FIELDS:
if field in INSTANCE_OPTIONAL_ATTRS:
continue
elif field == 'deleted':
instance.deleted = db_inst['deleted'] == db_inst['id']
@@ -285,26 +293,11 @@ class Instance(base.NovaPersistentObject, base.NovaObject):
instance.obj_reset_changes()
return instance
@staticmethod
def _attrs_to_columns(attrs):
"""Translate instance attributes into columns needing joining."""
columns_to_join = []
if 'metadata' in attrs:
columns_to_join.append('metadata')
if 'system_metadata' in attrs:
columns_to_join.append('system_metadata')
if 'pci_devices' in attrs:
columns_to_join.append('pci_devices')
# NOTE(danms): The DB API currently always joins info_cache and
# security_groups for get operations, so don't add them to the
# list of columns
return columns_to_join
@base.remotable_classmethod
def get_by_uuid(cls, context, uuid, expected_attrs=None):
if expected_attrs is None:
expected_attrs = ['info_cache', 'security_groups']
columns_to_join = cls._attrs_to_columns(expected_attrs)
columns_to_join = _expected_cols(expected_attrs)
db_inst = db.instance_get_by_uuid(context, uuid,
columns_to_join=columns_to_join)
return cls._from_db_object(context, cls(), db_inst,
@@ -314,7 +307,7 @@ class Instance(base.NovaPersistentObject, base.NovaObject):
def get_by_id(cls, context, inst_id, expected_attrs=None):
if expected_attrs is None:
expected_attrs = ['info_cache', 'security_groups']
columns_to_join = cls._attrs_to_columns(expected_attrs)
columns_to_join = _expected_cols(expected_attrs)
db_inst = db.instance_get(context, inst_id,
columns_to_join=columns_to_join)
return cls._from_db_object(context, cls(), db_inst,
@@ -447,14 +440,11 @@ class Instance(base.NovaPersistentObject, base.NovaObject):
if expected_vm_state is not None:
updates['expected_vm_state'] = expected_vm_state
expected_attrs = []
for attr in INSTANCE_OPTIONAL_FIELDS + INSTANCE_IMPLIED_FIELDS:
if self.obj_attr_is_set(attr):
expected_attrs.append(attr)
expected_attrs = [attr for attr in _INSTANCE_OPTIONAL_JOINED_FIELDS
if self.obj_attr_is_set(attr)]
old_ref, inst_ref = db.instance_update_and_get_original(
context, self.uuid, updates, update_cells=False,
columns_to_join=self._attrs_to_columns(expected_attrs))
columns_to_join=_expected_cols(expected_attrs))
if stale_instance:
_handle_cell_update_from_api()
@@ -468,10 +458,8 @@ class Instance(base.NovaPersistentObject, base.NovaObject):
@base.remotable
def refresh(self, context):
extra = []
for field in INSTANCE_OPTIONAL_FIELDS + INSTANCE_IMPLIED_FIELDS:
if self.obj_attr_is_set(field):
extra.append(field)
extra = [field for field in INSTANCE_OPTIONAL_ATTRS
if self.obj_attr_is_set(field)]
current = self.__class__.get_by_uuid(context, uuid=self.uuid,
expected_attrs=extra)
for field in self.fields:
@@ -480,29 +468,15 @@ class Instance(base.NovaPersistentObject, base.NovaObject):
self.obj_reset_changes()
def obj_load_attr(self, attrname):
extra = []
if attrname == 'system_metadata':
extra.append('system_metadata')
elif attrname == 'metadata':
extra.append('metadata')
elif attrname == 'info_cache':
extra.append('info_cache')
elif attrname == 'security_groups':
extra.append('security_groups')
elif attrname == 'pci_devices':
extra.append('pci_devices')
elif attrname == 'fault':
extra.append('fault')
if not extra:
if attrname not in INSTANCE_OPTIONAL_ATTRS:
raise exception.ObjectActionError(
action='obj_load_attr',
reason='attribute %s not lazy-loadable' % attrname)
# NOTE(danms): This could be optimized to just load the bits we need
# FIXME(comstud): This should be optimized to only load the attr.
instance = self.__class__.get_by_uuid(self._context,
uuid=self.uuid,
expected_attrs=extra)
expected_attrs=[attrname])
# NOTE(danms): Never allow us to recursively-load
if instance.obj_attr_is_set(attrname):
@@ -537,14 +511,6 @@ def _make_instance_list(context, inst_list, db_inst_list, expected_attrs):
return inst_list
def expected_cols(expected_attrs):
"""Return expected_attrs that are columns needing joining."""
if expected_attrs:
return list(set(expected_attrs) - set(INSTANCE_OPTIONAL_NON_COLUMNS))
else:
return expected_attrs
class InstanceList(base.ObjectListBase, base.NovaObject):
@base.remotable_classmethod
def get_by_filters(cls, context, filters,
@@ -552,14 +518,14 @@ class InstanceList(base.ObjectListBase, base.NovaObject):
marker=None, expected_attrs=None):
db_inst_list = db.instance_get_all_by_filters(
context, filters, sort_key, sort_dir, limit=limit, marker=marker,
columns_to_join=expected_cols(expected_attrs))
columns_to_join=_expected_cols(expected_attrs))
return _make_instance_list(context, cls(), db_inst_list,
expected_attrs)
@base.remotable_classmethod
def get_by_host(cls, context, host, expected_attrs=None):
db_inst_list = db.instance_get_all_by_host(
context, host, columns_to_join=expected_cols(expected_attrs))
context, host, columns_to_join=_expected_cols(expected_attrs))
return _make_instance_list(context, cls(), db_inst_list,
expected_attrs)