openstack
/
nova
Code Issues Proposed changes

Add Instance.hidden field 

During a cross-cell move operation, an instance can
exist in multiple cell databases but can be mapped to
only one cell in the instances_mappings table in the API DB.
When listing servers out of the API, we need a way to
filter out hidden instances to make sure we return the
appropriate record. In other words, the cell with the
hidden instance should not be the one in the instance
mapping and should not be returned when listing instances.
Note that the API already filters out duplicates (see
_get_unique_filter_method in API.get_all) but this gives
us a way to control which is shown rather than get lucky.
The DB API method to list instances will filter out hidden
instances by default.

Quotas
======

Counting quota for instances, cores and ram has to be updated
to filter out hidden instances as well so we don't double
count usage for a hidden instance since we should count it
for the non-hidden copy of the instance in another cell DB.

Similarly, we filter hidden instances when counting server
group members (this required the change in the DB API
instance_get_all_by_filters_sort method to make that filter
work).

Functional tests are added for both quota scenarios.

REST API Filters and Sorting
============================

Note that this change does not add the ability to filter or
sort instances by the new hidden field in the REST API since
it's only used internally. There is no explicit blacklist
for this, but trying to sort on hidden will result in a 400
error because the field is not in VALID_SORT_KEYS. Filtering
on hidden will not result in an error because of the
additionalProperties=True in query_params_v21 but it will not
be passed through to the DB API either.

Part of blueprint cross-cell-resize

Change-Id: Iaffb27bd8c562ba120047c04bb62619c0864f594
This commit is contained in:
Matt Riedemann 2019-01-15 18:32:44 -05:00
parent b92276d4ed
commit 2d2cfc4aa5
15 changed files with 139 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
nova/compute/api.py
View File

@ -2737,6 +2737,23 @@ class API(base.Base):
seen_uuids = set()
def _filter(instance):
# During a cross-cell move operation we could have the instance
# in more than one cell database so we not only have to filter
# duplicates but we want to make sure we only return the
# "current" one which should also be the one that the instance
# mapping points to, but we don't want to do that expensive
# lookup here. The DB API will filter out hidden instances by
# default but there is a small window where two copies of an
# instance could be hidden=False in separate cell DBs.
# NOTE(mriedem): We could make this better in the case that we
# have duplicate instances that are both hidden=False by
# showing the one with the newer updated_at value, but that
# could be tricky if the user is filtering on
# changes-since/before or updated_at, or sorting on updated_at,
# but technically that was already potentially broken with this
# _filter method if we return an older BuildRequest.instance,
# and given the window should be very small where we have
# duplicates, it's probably not worth the complexity.
if instance.uuid in seen_uuids:
return False
seen_uuids.add(instance.uuid)

10
nova/db/sqlalchemy/api.py
View File

@ -2048,8 +2048,10 @@ def instance_get_all_by_filters_sort(context, filters, limit=None, marker=None,
| ['project_id', 'user_id', 'image_ref',
| 'vm_state', 'instance_type_id', 'uuid',
| 'metadata', 'host', 'system_metadata', 'locked']
| 'metadata', 'host', 'system_metadata', 'locked', 'hidden']
Hidden instances will *not* be returned by default, unless there's a
filter that says otherwise.
A third type of filter (also using exact matching), filters
based on instance metadata tags when supplied under a special
@ -2211,12 +2213,16 @@ def instance_get_all_by_filters_sort(context, filters, limit=None, marker=None,
else:
filters['user_id'] = context.user_id
if 'hidden' not in filters:
# Filter out hidden instances by default.
filters['hidden'] = False
# Filters for exact matches that we can do along with the SQL query...
# For other filters that don't match this, we will do regexp matching
exact_match_filter_names = ['project_id', 'user_id', 'image_ref',
'vm_state', 'instance_type_id', 'uuid',
'metadata', 'host', 'task_state',
'system_metadata', 'locked']
'system_metadata', 'locked', 'hidden']
# Filter the query
query_prefix = _exact_instance_filter(query_prefix,

24
nova/db/sqlalchemy/migrate_repo/versions/399_add_instances_hidden.py Normal file
View File

@ -0,0 +1,24 @@
# 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 MetaData, Column, Table
from sqlalchemy import Boolean
def upgrade(migrate_engine):
meta = MetaData(bind=migrate_engine)
for prefix in ('', 'shadow_'):
instances = Table('%sinstances' % prefix, meta, autoload=True)
if not hasattr(instances.c, 'hidden'):
hidden = Column('hidden', Boolean, default=False)
instances.create_column(hidden)

2
nova/db/sqlalchemy/models.py
View File

@ -352,6 +352,8 @@ class Instance(BASE, NovaBase, models.SoftDeleteMixin):
# Records whether an instance has been deleted from disk
cleaned = Column(Integer, default=0)
hidden = Column(Boolean, default=False)
class InstanceInfoCache(BASE, NovaBase, models.SoftDeleteMixin):
"""Represents a cache of information about an instance

3
nova/objects/build_request.py
View File

@ -87,7 +87,8 @@ class BuildRequest(base.NovaObject):
# field being set, so when hydrating it back here, we should get the
# right value but in case we don't have it, let's suppose that the
# instance is not deleted, which is the default value for that field.
self.instance.obj_set_defaults('deleted')
# NOTE(mriedem): Same for the "hidden" field.
self.instance.obj_set_defaults('deleted', 'hidden')
# NOTE(alaski): Set some fields on instance that are needed by the api,
# not lazy-loadable, and don't change.
self.instance.disable_terminate = False

11
nova/objects/instance.py
View File

@ -21,6 +21,7 @@ from oslo_serialization import jsonutils
from oslo_utils import timeutils
from oslo_utils import versionutils
from sqlalchemy import or_
from sqlalchemy.sql import false
from sqlalchemy.sql import func
from sqlalchemy.sql import null
@ -112,7 +113,8 @@ class Instance(base.NovaPersistentObject, base.NovaObject,
# Version 2.3: Added device_metadata
# Version 2.4: Added trusted_certs
# Version 2.5: Added hard_delete kwarg in destroy
VERSION = '2.5'
# Version 2.6: Added hidden
VERSION = '2.6'
fields = {
'id': fields.IntegerField(),
@ -214,6 +216,7 @@ class Instance(base.NovaPersistentObject, base.NovaObject,
nullable=True),
'keypairs': fields.ObjectField('KeyPairList'),
'trusted_certs': fields.ObjectField('TrustedCerts', nullable=True),
'hidden': fields.BooleanField(default=False),
}
obj_extra_fields = ['name']
@ -221,6 +224,8 @@ class Instance(base.NovaPersistentObject, base.NovaObject,
def obj_make_compatible(self, primitive, target_version):
super(Instance, self).obj_make_compatible(primitive, target_version)
target_version = versionutils.convert_version_to_tuple(target_version)
if target_version < (2, 6) and 'hidden' in primitive:
del primitive['hidden']
if target_version < (2, 4) and 'trusted_certs' in primitive:
del primitive['trusted_certs']
if target_version < (2, 3) and 'device_metadata' in primitive:
@ -1458,6 +1463,10 @@ class InstanceList(base.ObjectListBase, base.NovaObject):
filter_by(deleted=0).\
filter(not_soft_deleted).\
filter_by(project_id=project_id)
# NOTE(mriedem): Filter out hidden instances since there should be a
# non-hidden version of the instance in another cell database and the
# API will only show one of them, so we don't count the hidden copy.
project_query = project_query.filter_by(hidden=false())
project_result = project_query.first()
fields = ('instances', 'cores', 'ram')

18
nova/tests/functional/db/test_instance.py
View File

@ -136,3 +136,21 @@ class InstanceObjectTestCase(test.TestCase):
count = objects.InstanceList.get_count_by_hosts(
self.context, hosts=['fake_host1', 'fake_host2'])
self.assertEqual(3, count)
def test_hidden_instance_not_counted(self):
"""Tests that a hidden instance is not counted against quota usage."""
# Create an instance that is not hidden and count usage.
instance = self._create_instance(vcpus=1, memory_mb=2048)
counts = objects.InstanceList.get_counts(
self.context, instance.project_id)['project']
self.assertEqual(1, counts['instances'])
self.assertEqual(instance.vcpus, counts['cores'])
self.assertEqual(instance.memory_mb, counts['ram'])
# Now hide the instance and count usage again, everything should be 0.
instance.hidden = True
instance.save()
counts = objects.InstanceList.get_counts(
self.context, instance.project_id)['project']
self.assertEqual(0, counts['instances'])
self.assertEqual(0, counts['cores'])
self.assertEqual(0, counts['ram'])

13
nova/tests/functional/db/test_quota.py
View File

@ -126,6 +126,19 @@ class QuotaTestCase(test.NoDBTestCase):
# Logged a warning about falling back to legacy count.
mock_warn_log.assert_called_once()
# Create a duplicate of the cell1 instance in cell2 except hidden.
with context.target_cell(ctxt, mapping2) as cctxt:
instance = objects.Instance(context=cctxt,
project_id='fake-project',
user_id='fake-user',
uuid=instance_uuids[0],
hidden=True)
instance.create()
# The duplicate hidden instance should not be counted.
count = quota._server_group_count_members_by_user(
ctxt, group, instance.user_id)
self.assertEqual(2, count['user']['server_group_members'])
def test_instances_cores_ram_count(self):
ctxt = context.RequestContext('fake-user', 'fake-project')
mapping1 = objects.CellMapping(context=ctxt,

8
nova/tests/unit/api/openstack/compute/test_serversV21.py
View File

@ -906,7 +906,8 @@ class ServersControllerTest(ControllerTest):
self.controller.detail, req)
def test_get_servers_invalid_sort_key(self):
req = self.req('/fake/servers?sort_key=foo&sort_dir=desc')
# "hidden" is a real field for instances but not exposed in the API.
req = self.req('/fake/servers?sort_key=hidden&sort_dir=desc')
self.assertRaises(exception.ValidationError,
self.controller.index, req)
@ -1482,6 +1483,9 @@ class ServersControllerTest(ControllerTest):
# Allowed only by admins with admin API on
self.assertIn('ip', search_opts)
self.assertNotIn('unknown_option', search_opts)
# "hidden" is ignored as a filter parameter since it is only used
# internally
self.assertNotIn('hidden', search_opts)
return objects.InstanceList(
objects=[fakes.stub_instance_obj(100, uuid=server_uuid)])
@ -1494,7 +1498,7 @@ class ServersControllerTest(ControllerTest):
self.mock_get_all.side_effect = fake_get_all
query_str = ("name=foo&ip=10.*&status=active&unknown_option=meow&"
"terminated_at=^2016-02-01.*")
"terminated_at=^2016-02-01.*&hidden=true")
req = self.req('/fake/servers?%s' % query_str)
servers = self.controller.index(req)['servers']

3
nova/tests/unit/api/openstack/fakes.py
View File

@ -448,7 +448,7 @@ def stub_instance(id=1, user_id=None, project_id=None, host=None,
memory_mb=0, vcpus=0, root_gb=0, ephemeral_gb=0,
instance_type=None, launch_index=0, kernel_id="",
ramdisk_id="", user_data=None, system_metadata=None,
services=None, trusted_certs=None):
services=None, trusted_certs=None, hidden=False):
if user_id is None:
user_id = 'fake_user'
if project_id is None:
@ -561,6 +561,7 @@ def stub_instance(id=1, user_id=None, project_id=None, host=None,
"cleaned": cleaned,
"services": services,
"tags": [],
"hidden": hidden,
}
instance.update(info_cache)

13
nova/tests/unit/db/test_db_api.py
View File

@ -962,6 +962,19 @@ class SqlAlchemyDbApiTestCase(DbTestCase):
filters={},
sort_keys=keys)
def test_instance_get_all_by_filters_sort_hidden(self):
"""Tests the default filtering behavior of the hidden column."""
# Create a hidden instance record.
self.create_instance_with_args(hidden=True)
# Get instances which by default will filter out the hidden instance.
instances = sqlalchemy_api.instance_get_all_by_filters_sort(
self.context, filters={}, limit=10)
self.assertEqual(0, len(instances))
# Now explicitly filter for hidden instances.
instances = sqlalchemy_api.instance_get_all_by_filters_sort(
self.context, filters={'hidden': True}, limit=10)
self.assertEqual(1, len(instances))
class ProcessSortParamTestCase(test.TestCase):

5
nova/tests/unit/db/test_migrations.py
View File

@ -1031,6 +1031,11 @@ class NovaMigrationsCheckers(test_migrations.ModelsMigrationsSync,
self.assertColumnExists(engine, 'instance_extra', 'vpmems')
self.assertColumnExists(engine, 'shadow_instance_extra', 'vpmems')
def _check_399(self, engine, data):
for prefix in ('', 'shadow_'):
self.assertColumnExists(
engine, '%sinstances' % prefix, 'hidden')
class TestNovaMigrationsSQLite(NovaMigrationsCheckers,
test_fixtures.OpportunisticDBTestMixin,

7
nova/tests/unit/objects/test_build_request.py
View File

@ -152,16 +152,17 @@ class _TestBuildRequestObject(object):
self.assertEqual(getattr(build_request.instance, field),
getattr(instance, field))
def test_from_db_object_set_deleted(self):
def test_from_db_object_set_deleted_hidden(self):
# Assert that if we persisted an instance not yet having the deleted
# field being set, we still return a value for that field.
# or hidden field being set, we still return a value for that field.
fake_req = fake_build_request.fake_db_req()
with mock.patch.object(o_vo_base.VersionedObject,
'obj_set_defaults') as mock_obj_set_defaults:
build_request = objects.BuildRequest._from_db_object(
self.context, objects.BuildRequest(), fake_req)
mock_obj_set_defaults.assert_called_once_with('deleted')
mock_obj_set_defaults.assert_called_once_with('deleted', 'hidden')
self.assertFalse(build_request.instance.deleted)
self.assertFalse(build_request.instance.hidden)
def test_obj_make_compatible_pre_1_3(self):
obj = fake_build_request.fake_req_obj(self.context)

14
nova/tests/unit/objects/test_instance.py
View File

@ -21,6 +21,7 @@ from oslo_db import exception as db_exc
from oslo_serialization import jsonutils
from oslo_utils.fixture import uuidsentinel as uuids
from oslo_utils import timeutils
from oslo_versionedobjects import base as ovo_base
from nova.compute import task_states
from nova.compute import vm_states
@ -1521,6 +1522,19 @@ class _TestInstanceObject(object):
inst1 = inst1.obj_clone()
self.assertEqual(len(inst1.obj_what_changed()), 0)
def test_obj_make_compatible(self):
inst_obj = objects.Instance(
# trusted_certs were added in 2.4
trusted_certs=objects.TrustedCerts(ids=[uuids.cert1]),
# hidden was added in 2.6
hidden=True)
versions = ovo_base.obj_tree_get_versions('Instance')
data = lambda x: x['nova_object.data']
primitive = data(inst_obj.obj_to_primitive(
target_version='2.5', version_manifest=versions))
self.assertIn('trusted_certs', primitive)
self.assertNotIn('hidden', primitive)
class TestInstanceObject(test_objects._LocalTest,
_TestInstanceObject):

2
nova/tests/unit/objects/test_objects.py
View File

@ -1070,7 +1070,7 @@ object_data = {
'IDEDeviceBus': '1.0-29d4c9f27ac44197f01b6ac1b7e16502',
'ImageMeta': '1.8-642d1b2eb3e880a367f37d72dd76162d',
'ImageMetaProps': '1.22-b1c9ea78c43e8d7989a7d1f0f34d149a',
'Instance': '2.5-94e3881f0b9a06c2c3640e44816b51de',
'Instance': '2.6-5fefbcb483703c85e4d328b887c8af33',
'InstanceAction': '1.2-9a5abc87fdd3af46f45731960651efb5',
'InstanceActionEvent': '1.3-c749e1b3589e7117c81cb2aa6ac438d5',
'InstanceActionEventList': '1.1-13d92fb953030cdbfee56481756e02be',