From 0afed7462dfd962ce7061b662ace72132b475c4d Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 26 Sep 2019 13:57:18 +0200 Subject: [PATCH] Stop requiring root size for whole disk images This requirement has been fixed in newer versions of ironic. Change-Id: I4460755ee66b4aa0b8a651b6bd142c769d669ee2 (cherry picked from commit 2d801e2526f4d1dd9968a4a3e4ab6a8603e1416d) --- metalsmith/_provisioner.py | 5 +- metalsmith/_utils.py | 6 +- metalsmith/sources.py | 26 +++++++-- metalsmith/test/test_sources.py | 57 ++++++++++++++++++- playbooks/integration/run.yaml | 1 + .../whole-disk-root-gb-bd8ee3600de9ec8d.yaml | 5 ++ 6 files changed, 89 insertions(+), 11 deletions(-) create mode 100644 releasenotes/notes/whole-disk-root-gb-bd8ee3600de9ec8d.yaml diff --git a/metalsmith/_provisioner.py b/metalsmith/_provisioner.py index a00a059..1d389fe 100644 --- a/metalsmith/_provisioner.py +++ b/metalsmith/_provisioner.py @@ -388,7 +388,7 @@ class Provisioner(object): try: root_size_gb = _utils.get_root_disk(root_size_gb, node) - image._validate(self.connection) + image._validate(self.connection, root_size_gb) nics.validate() @@ -405,7 +405,8 @@ class Provisioner(object): capabilities['boot_option'] = 'netboot' if netboot else 'local' instance_info = self._clean_instance_info(node.instance_info) - instance_info['root_gb'] = root_size_gb + if root_size_gb is not None: + instance_info['root_gb'] = root_size_gb instance_info['capabilities'] = capabilities if hostname: instance_info['display_name'] = hostname diff --git a/metalsmith/_utils.py b/metalsmith/_utils.py index db930c3..5de9417 100644 --- a/metalsmith/_utils.py +++ b/metalsmith/_utils.py @@ -56,9 +56,9 @@ def get_root_disk(root_size_gb, node): try: assert int(node.properties['local_gb']) > 0 except KeyError: - raise exceptions.UnknownRootDiskSize( - 'No local_gb for node %s and no root partition size ' - 'specified' % log_res(node)) + LOG.debug('No local_gb for node %s and no root partition size ' + 'specified', log_res(node)) + return except (TypeError, ValueError, AssertionError): raise exceptions.UnknownRootDiskSize( 'The local_gb for node %(node)s is invalid: ' diff --git a/metalsmith/sources.py b/metalsmith/sources.py index c33b43d..94b630f 100644 --- a/metalsmith/sources.py +++ b/metalsmith/sources.py @@ -34,7 +34,7 @@ LOG = logging.getLogger(__name__) @six.add_metaclass(abc.ABCMeta) class _Source(object): - def _validate(self, connection): + def _validate(self, connection, root_size_gb): """Validate the source.""" @abc.abstractmethod @@ -53,7 +53,7 @@ class GlanceImage(_Source): self.image = image self._image_obj = None - def _validate(self, connection): + def _validate(self, connection, root_size_gb): if self._image_obj is not None: return try: @@ -64,8 +64,13 @@ class GlanceImage(_Source): 'Cannot find image %(image)s: %(error)s' % {'image': self.image, 'error': exc}) + if (root_size_gb is None and + any(getattr(self._image_obj, x, None) is not None + for x in ('kernel_id', 'ramdisk_id'))): + raise exceptions.UnknownRootDiskSize( + 'Partition images require root partition size') + def _node_updates(self, connection): - self._validate(connection) LOG.debug('Image: %s', self._image_obj) updates = { @@ -108,7 +113,7 @@ class HttpWholeDiskImage(_Source): self.checksum = checksum self.checksum_url = checksum_url - def _validate(self, connection): + def _validate(self, connection, root_size_gb): # TODO(dtantsur): should we validate image URLs here? Ironic will do it # as well, and images do not have to be accessible from where # metalsmith is running. @@ -140,7 +145,6 @@ class HttpWholeDiskImage(_Source): {'fname': fname, 'url': self.checksum_url}) def _node_updates(self, connection): - self._validate(connection) LOG.debug('Image: %(image)s, checksum %(checksum)s', {'image': self.url, 'checksum': self.checksum}) return { @@ -170,6 +174,12 @@ class HttpPartitionImage(HttpWholeDiskImage): self.kernel_url = kernel_url self.ramdisk_url = ramdisk_url + def _validate(self, connection, root_size_gb): + super(HttpPartitionImage, self)._validate(connection, root_size_gb) + if root_size_gb is None: + raise exceptions.UnknownRootDiskSize( + 'Partition images require root partition size') + def _node_updates(self, connection): updates = super(HttpPartitionImage, self)._node_updates(connection) updates['kernel'] = self.kernel_url @@ -237,6 +247,12 @@ class FilePartitionImage(FileWholeDiskImage): self.kernel_location = kernel_location self.ramdisk_location = ramdisk_location + def _validate(self, connection, root_size_gb): + super(FilePartitionImage, self)._validate(connection, root_size_gb) + if root_size_gb is None: + raise exceptions.UnknownRootDiskSize( + 'Partition images require root partition size') + def _node_updates(self, connection): updates = super(FilePartitionImage, self)._node_updates(connection) updates['kernel'] = self.kernel_location diff --git a/metalsmith/test/test_sources.py b/metalsmith/test/test_sources.py index a358c8b..14d3788 100644 --- a/metalsmith/test/test_sources.py +++ b/metalsmith/test/test_sources.py @@ -13,18 +13,49 @@ # See the License for the specific language governing permissions and # limitations under the License. +import mock import testtools +from metalsmith import exceptions from metalsmith import sources class TestDetect(testtools.TestCase): - def test_glance(self): + def test_glance_whole_disk(self): source = sources.detect('foobar') self.assertIsInstance(source, sources.GlanceImage) self.assertEqual(source.image, 'foobar') + conn = mock.Mock(spec=['image']) + conn.image.find_image.return_value = mock.Mock( + id=42, kernel_id=None, ramdisk_id=None) + source._validate(conn, None) + self.assertEqual({'image_source': 42}, source._node_updates(conn)) + + def test_glance_partition(self): + source = sources.detect('foobar') + self.assertIsInstance(source, sources.GlanceImage) + self.assertEqual(source.image, 'foobar') + + conn = mock.Mock(spec=['image']) + conn.image.find_image.return_value = mock.Mock( + id=42, kernel_id=1, ramdisk_id=2) + source._validate(conn, 9) + self.assertEqual({'image_source': 42, 'kernel': 1, 'ramdisk': 2}, + source._node_updates(conn)) + + def test_glance_partition_missing_root(self): + source = sources.detect('foobar') + self.assertIsInstance(source, sources.GlanceImage) + self.assertEqual(source.image, 'foobar') + + conn = mock.Mock(spec=['image']) + conn.image.find_image.return_value = mock.Mock( + id=42, kernel_id=1, ramdisk_id=2) + self.assertRaises(exceptions.UnknownRootDiskSize, + source._validate, conn, None) + def test_glance_invalid_arguments(self): for kwargs in [{'kernel': 'foo'}, {'ramdisk': 'foo'}, @@ -43,6 +74,8 @@ class TestDetect(testtools.TestCase): self.assertEqual(source.location, 'file:///image') self.assertEqual(source.checksum, 'abcd') + source._validate(mock.Mock(), None) + def test_file_partition_disk(self): source = sources.detect('file:///image', checksum='abcd', kernel='file:///kernel', @@ -53,6 +86,15 @@ class TestDetect(testtools.TestCase): self.assertEqual(source.kernel_location, 'file:///kernel') self.assertEqual(source.ramdisk_location, 'file:///ramdisk') + source._validate(mock.Mock(), 9) + + def test_file_partition_disk_missing_root(self): + source = sources.detect('file:///image', checksum='abcd', + kernel='file:///kernel', + ramdisk='file:///ramdisk') + self.assertRaises(exceptions.UnknownRootDiskSize, + source._validate, mock.Mock(), None) + def test_file_partition_inconsistency(self): for kwargs in [{'kernel': 'foo'}, {'ramdisk': 'foo'}, @@ -69,12 +111,16 @@ class TestDetect(testtools.TestCase): self.assertEqual(source.url, 'http:///image') self.assertEqual(source.checksum, 'abcd') + source._validate(mock.Mock(), None) + def test_https_whole_disk(self): source = sources.detect('https:///image', checksum='abcd') self.assertIs(source.__class__, sources.HttpWholeDiskImage) self.assertEqual(source.url, 'https:///image') self.assertEqual(source.checksum, 'abcd') + source._validate(mock.Mock(), None) + def test_https_whole_disk_checksum(self): source = sources.detect('https:///image', checksum='https://checksum') @@ -92,6 +138,15 @@ class TestDetect(testtools.TestCase): self.assertEqual(source.kernel_url, 'http:///kernel') self.assertEqual(source.ramdisk_url, 'http:///ramdisk') + source._validate(mock.Mock(), 9) + + def test_http_partition_disk_missing_root(self): + source = sources.detect('http:///image', checksum='abcd', + kernel='http:///kernel', + ramdisk='http:///ramdisk') + self.assertRaises(exceptions.UnknownRootDiskSize, + source._validate, mock.Mock(), None) + def test_https_partition_disk(self): source = sources.detect('https:///image', checksum='abcd', # Can mix HTTP and HTTPs diff --git a/playbooks/integration/run.yaml b/playbooks/integration/run.yaml index 9efea80..252f15b 100644 --- a/playbooks/integration/run.yaml +++ b/playbooks/integration/run.yaml @@ -14,6 +14,7 @@ vars: metalsmith_image: "{{ metalsmith_whole_disk_image }}" metalsmith_image_checksum: "{{ metalsmith_whole_disk_checksum | default('') }}" + metalsmith_root_size: # NOTE(dtantsur): cannot specify swap with whole disk images metalsmith_swap_size: diff --git a/releasenotes/notes/whole-disk-root-gb-bd8ee3600de9ec8d.yaml b/releasenotes/notes/whole-disk-root-gb-bd8ee3600de9ec8d.yaml new file mode 100644 index 0000000..5910b96 --- /dev/null +++ b/releasenotes/notes/whole-disk-root-gb-bd8ee3600de9ec8d.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + No longer requires root size for whole disk images. This requirement has + been removed from ironic.