From c8f814569d07544734f10f134e146ce981639e07 Mon Sep 17 00:00:00 2001 From: John Griffith Date: Tue, 18 Jun 2013 23:22:48 -0600 Subject: [PATCH] Replace glance_metadata check with bootable column. This patch adds a column to indicate if a volume is bootable or not. Cinder API V1 was using get.volume_glance_metadata to determine if a volume was bootable for translate_view. This is fine until you put a heavy load on the system and things can fall apart, particularly the backref of the glance meta is no longer available and the call blows up. Also this results in a bit of extra unnecessary data being passed around with every create and get call. This is especially an issue when creating hundreds or thousands of volumes. Even better we can now reliably create thousands of volumes. Fixes bug: 1192390 Change-Id: Idd47a0a8069ee905b81c7aae562b82767ad91930 --- cinder/api/v1/volumes.py | 7 +- .../versions/011_add_bootable_column.py | 46 +++++++++++++ .../versions/011_sqlite_downgrade.sql | 66 +++++++++++++++++++ cinder/db/sqlalchemy/models.py | 2 + cinder/tests/api/v1/test_volumes.py | 18 ++--- cinder/tests/api/v2/stubs.py | 5 ++ cinder/tests/fake_flags.py | 1 - cinder/tests/test_migrations.py | 44 +++++++++++++ cinder/volume/api.py | 8 --- cinder/volume/manager.py | 18 ++++- 10 files changed, 191 insertions(+), 24 deletions(-) create mode 100644 cinder/db/sqlalchemy/migrate_repo/versions/011_add_bootable_column.py create mode 100644 cinder/db/sqlalchemy/migrate_repo/versions/011_sqlite_downgrade.sql diff --git a/cinder/api/v1/volumes.py b/cinder/api/v1/volumes.py index b9467a43a..e18d3d7ed 100644 --- a/cinder/api/v1/volumes.py +++ b/cinder/api/v1/volumes.py @@ -78,6 +78,7 @@ def _translate_volume_summary_view(context, vol, image_id=None): d['size'] = vol['size'] d['availability_zone'] = vol['availability_zone'] d['created_at'] = vol['created_at'] + d['bootable'] = vol['bootable'] d['attachments'] = [] if vol['attach_status'] == 'attached': @@ -110,11 +111,6 @@ def _translate_volume_summary_view(context, vol, image_id=None): else: d['metadata'] = {} - if vol.get('volume_glance_metadata'): - d['bootable'] = 'true' - else: - d['bootable'] = 'false' - return d @@ -339,6 +335,7 @@ class VolumeController(wsgi.Controller): image_href = None image_uuid = None if self.ext_mgr.is_loaded('os-image-create'): + # NOTE(jdg): misleading name "imageRef" as it's an image-id image_href = volume.get('imageRef') if image_href: image_uuid = self._image_uuid_from_href(image_href) diff --git a/cinder/db/sqlalchemy/migrate_repo/versions/011_add_bootable_column.py b/cinder/db/sqlalchemy/migrate_repo/versions/011_add_bootable_column.py new file mode 100644 index 000000000..c4a5095b9 --- /dev/null +++ b/cinder/db/sqlalchemy/migrate_repo/versions/011_add_bootable_column.py @@ -0,0 +1,46 @@ +# vim: tabstop=4 shiftwidth=4 softtabstop=4 + +# 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 Boolean, Column, MetaData, Table + + +def upgrade(migrate_engine): + """Add bootable column to volumes.""" + meta = MetaData() + meta.bind = migrate_engine + + volumes = Table('volumes', meta, autoload=True) + bootable = Column('bootable', Boolean) + + volumes.create_column(bootable) + volumes.update().values(bootable=False).execute() + + glance_metadata = Table('volume_glance_metadata', meta, autoload=True) + glance_items = list(glance_metadata.select().execute()) + for item in glance_items: + volumes.update().\ + where(volumes.c.id == item['volume_id']).\ + values(bootable=True).execute() + + +def downgrade(migrate_engine): + """Remove bootable column to volumes.""" + meta = MetaData() + meta.bind = migrate_engine + + volumes = Table('volumes', meta, autoload=True) + bootable = volumes.columns.bootable + #bootable = Column('bootable', Boolean) + volumes.drop_column(bootable) diff --git a/cinder/db/sqlalchemy/migrate_repo/versions/011_sqlite_downgrade.sql b/cinder/db/sqlalchemy/migrate_repo/versions/011_sqlite_downgrade.sql new file mode 100644 index 000000000..22a1f13af --- /dev/null +++ b/cinder/db/sqlalchemy/migrate_repo/versions/011_sqlite_downgrade.sql @@ -0,0 +1,66 @@ +BEGIN TRANSACTION; + + CREATE TABLE volumes_v10 ( + created_at DATETIME, + updated_at DATETIME, + deleted_at DATETIME, + deleted BOOLEAN, + id VARCHAR(36) NOT NULL, + ec2_id INTEGER, + user_id VARCHAR(255), + project_id VARCHAR(255), + snapshot_id VARCHAR(36), + host VARCHAR(255), + size INTEGER, + availability_zone VARCHAR(255), + instance_uuid VARCHAR(36), + mountpoint VARCHAR(255), + attach_time VARCHAR(255), + status VARCHAR(255), + attach_status VARCHAR(255), + scheduled_at DATETIME, + launched_at DATETIME, + terminated_at DATETIME, + display_name VARCHAR(255), + display_description VARCHAR(255), + provider_location VARCHAR(255), + provider_auth VARCHAR(255), + volume_type_id VARCHAR(36), + source_volid VARCHAR(36), + PRIMARY KEY (id) + ); + + INSERT INTO volumes_v10 + SELECT created_at, + updated_at, + deleted_at, + deleted, + id, + ec2_id, + user_id, + project_id, + snapshot_id, + host, + size, + availability_zone, + instance_uuid, + mountpoint, + attach_time, + status, + attach_status, + scheduled_at, + launched_at, + terminated_at, + display_name, + display_description, + provider_location, + provider_auth, + volume_type_id, + source_volid + FROM volumes; + + DROP TABLE volumes; + ALTER TABLE volumes_v10 RENAME TO volumes; + COMMIT; + + diff --git a/cinder/db/sqlalchemy/models.py b/cinder/db/sqlalchemy/models.py index 7adac5b88..bb45369fd 100644 --- a/cinder/db/sqlalchemy/models.py +++ b/cinder/db/sqlalchemy/models.py @@ -113,6 +113,8 @@ class Volume(BASE, CinderBase): volume_type_id = Column(String(36)) source_volid = Column(String(36)) + deleted = Column(Boolean, default=False) + bootable = Column(Boolean, default=False) class VolumeMetadata(BASE, CinderBase): diff --git a/cinder/tests/api/v1/test_volumes.py b/cinder/tests/api/v1/test_volumes.py index e9f7548ee..c0f2d7c30 100644 --- a/cinder/tests/api/v1/test_volumes.py +++ b/cinder/tests/api/v1/test_volumes.py @@ -85,7 +85,7 @@ class VolumeApiTest(test.TestCase): 'server_id': 'fakeuuid', 'id': '1', 'volume_id': '1'}], - 'bootable': 'false', + 'bootable': False, 'volume_type': 'vol_type_name', 'snapshot_id': None, 'source_volid': None, @@ -154,7 +154,7 @@ class VolumeApiTest(test.TestCase): 'server_id': 'fakeuuid', 'id': '1', 'volume_id': '1'}], - 'bootable': 'false', + 'bootable': False, 'volume_type': 'vol_type_name', 'image_id': test_id, 'snapshot_id': None, @@ -218,7 +218,7 @@ class VolumeApiTest(test.TestCase): 'server_id': 'fakeuuid', 'device': '/', }], - 'bootable': 'false', + 'bootable': False, 'volume_type': 'vol_type_name', 'snapshot_id': None, 'source_volid': None, @@ -248,7 +248,7 @@ class VolumeApiTest(test.TestCase): 'server_id': 'fakeuuid', 'device': '/', }], - 'bootable': 'false', + 'bootable': False, 'volume_type': 'vol_type_name', 'snapshot_id': None, 'source_volid': None, @@ -298,7 +298,7 @@ class VolumeApiTest(test.TestCase): 'server_id': 'fakeuuid', 'id': '1', 'volume_id': '1'}], - 'bootable': 'false', + 'bootable': False, 'volume_type': 'vol_type_name', 'snapshot_id': None, 'source_volid': None, @@ -322,7 +322,7 @@ class VolumeApiTest(test.TestCase): 'server_id': 'fakeuuid', 'id': '1', 'volume_id': '1'}], - 'bootable': 'false', + 'bootable': False, 'volume_type': 'vol_type_name', 'snapshot_id': None, 'source_volid': None, @@ -407,7 +407,7 @@ class VolumeApiTest(test.TestCase): 'server_id': 'fakeuuid', 'id': '1', 'volume_id': '1'}], - 'bootable': 'false', + 'bootable': False, 'volume_type': 'vol_type_name', 'snapshot_id': None, 'source_volid': None, @@ -431,7 +431,7 @@ class VolumeApiTest(test.TestCase): 'availability_zone': 'fakeaz', 'display_name': 'displayname', 'attachments': [], - 'bootable': 'false', + 'bootable': False, 'volume_type': 'vol_type_name', 'snapshot_id': None, 'source_volid': None, @@ -459,7 +459,7 @@ class VolumeApiTest(test.TestCase): 'server_id': 'fakeuuid', 'id': '1', 'volume_id': '1'}], - 'bootable': 'true', + 'bootable': True, 'volume_type': 'vol_type_name', 'snapshot_id': None, 'source_volid': None, diff --git a/cinder/tests/api/v2/stubs.py b/cinder/tests/api/v2/stubs.py index d3242c7a5..a27202fe3 100644 --- a/cinder/tests/api/v2/stubs.py +++ b/cinder/tests/api/v2/stubs.py @@ -44,9 +44,12 @@ def stub_volume(id, **kwargs): 'source_volid': None, 'volume_type_id': '3e196c20-3c06-11e2-81c1-0800200c9a66', 'volume_metadata': [], + 'bootable': False, 'volume_type': {'name': 'vol_type_name'}} volume.update(kwargs) + if kwargs.get('volume_glance_metadata', None): + volume['bootable'] = True return volume @@ -57,6 +60,7 @@ def stub_volume_create(self, context, size, name, description, snapshot, vol['display_name'] = name vol['display_description'] = description vol['source_volid'] = None + vol['bootable'] = False try: vol['snapshot_id'] = snapshot['id'] except (KeyError, TypeError): @@ -74,6 +78,7 @@ def stub_volume_create_from_image(self, context, size, name, description, vol['display_name'] = name vol['display_description'] = description vol['availability_zone'] = 'cinder' + vol['bootable'] = False return vol diff --git a/cinder/tests/fake_flags.py b/cinder/tests/fake_flags.py index ef0b1a827..36922abac 100644 --- a/cinder/tests/fake_flags.py +++ b/cinder/tests/fake_flags.py @@ -25,7 +25,6 @@ flags.DECLARE('policy_file', 'cinder.policy') flags.DECLARE('volume_driver', 'cinder.volume.manager') flags.DECLARE('xiv_proxy', 'cinder.volume.drivers.xiv') flags.DECLARE('backup_service', 'cinder.backup.manager') - def_vol_type = 'fake_vol_type' diff --git a/cinder/tests/test_migrations.py b/cinder/tests/test_migrations.py index edb0fed9f..a92252eba 100644 --- a/cinder/tests/test_migrations.py +++ b/cinder/tests/test_migrations.py @@ -677,3 +677,47 @@ class TestMigrations(test.TestCase): self.assertFalse(engine.dialect.has_table(engine.connect(), "transfers")) + + def test_migration_011(self): + """Test adding transfers table works correctly.""" + for (key, engine) in self.engines.items(): + migration_api.version_control(engine, + TestMigrations.REPOSITORY, + migration.INIT_VERSION) + migration_api.upgrade(engine, TestMigrations.REPOSITORY, 10) + metadata = sqlalchemy.schema.MetaData() + metadata.bind = engine + + volumes_v10 = sqlalchemy.Table('volumes', + metadata, + autoload=True) + + migration_api.upgrade(engine, TestMigrations.REPOSITORY, 11) + metadata = sqlalchemy.schema.MetaData() + metadata.bind = engine + + self.assertTrue(engine.dialect.has_table(engine.connect(), + "volumes")) + volumes = sqlalchemy.Table('volumes', + metadata, + autoload=True) + + # Make sure we didn't miss any columns in the upgrade + for column in volumes_v10.c: + self.assertTrue(volumes.c.__contains__(column.name)) + + self.assertTrue(isinstance(volumes.c.bootable.type, + sqlalchemy.types.BOOLEAN)) + + migration_api.downgrade(engine, TestMigrations.REPOSITORY, 10) + metadata = sqlalchemy.schema.MetaData() + metadata.bind = engine + + volumes = sqlalchemy.Table('volumes', + metadata, + autoload=True) + self.assertTrue('bootable' not in volumes.c) + + # Make sure we put all the columns back + for column in volumes_v10.c: + self.assertTrue(volumes.c.__contains__(column.name)) diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 5b3ca2f14..0ff722f30 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -366,16 +366,8 @@ class API(base.Base): def get(self, context, volume_id): rv = self.db.volume_get(context, volume_id) - glance_meta = rv.get('volume_glance_metadata', None) volume = dict(rv.iteritems()) check_policy(context, 'get', volume) - - # NOTE(jdg): As per bug 1115629 iteritems doesn't pick - # up the glance_meta dependency, add it explicitly if - # it exists in the rv - if glance_meta: - volume['volume_glance_metadata'] = glance_meta - return volume def get_all(self, context, marker=None, limit=None, sort_key='created_at', diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 2430495a5..08af03412 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -169,9 +169,20 @@ class VolumeManager(manager.SchedulerDependentManager): model_update = self.driver.create_volume_from_snapshot( volume_ref, snapshot_ref) + + originating_vref = self.db.volume_get(context, + snapshot_ref['volume_id']) + if originating_vref.bootable: + self.db.volume_update(context, + volume_ref['id'], + {'bootable': True}) elif srcvol_ref is not None: model_update = self.driver.create_cloned_volume(volume_ref, srcvol_ref) + if srcvol_ref.bootable: + self.db.volume_update(context, + volume_ref['id'], + {'bootable': True}) else: # create the volume from an image cloned = self.driver.clone_image(volume_ref, image_location) @@ -182,11 +193,16 @@ class VolumeManager(manager.SchedulerDependentManager): volume_ref = self.db.volume_update(context, volume_ref['id'], updates) + + # TODO(jdg): Wrap this in a try block and update status + # appropriately if the download image fails self._copy_image_to_volume(context, volume_ref, image_service, image_id) - + self.db.volume_update(context, + volume_ref['id'], + {'bootable': True}) return model_update, cloned def create_volume(self, context, volume_id, request_spec=None, -- 2.45.2