nova-manage: Always get BDMs using get_by_volume_and_instance

Previously the volume_attachments show command would incorrectly use the
nova.objects.BlockDeviceMapping.get_by_volume helper to fetch the
underlying BlockDeviceMapping object from the database that does not
support multiattach volumes.

This is corrected by switching to the get_by_volume_and_instance helper
that can pick out unique BlockDeviceMapping objects using both of the
supplied volume and instance UUIDs.

Change-Id: Ifab05abf3775efb0f29f80c9300297208f60d5d9
Closes-Bug: #1945452

nova/cmd/manage.py

@@ -2929,7 +2929,7 @@ class VolumeAttachmentCommands(object):
             im = objects.InstanceMapping.get_by_instance_uuid(
                 ctxt, instance_uuid)
             with context.target_cell(ctxt, im.cell_mapping) as cctxt:
-                bdm = objects.BlockDeviceMapping.get_by_volume(
+                bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
                     cctxt, volume_id, instance_uuid)
                 if connection_info and json:
                     print(bdm.connection_info)

nova/tests/functional/test_nova_manage.py

@@ -261,11 +261,16 @@ class TestNovaManageVolumeAttachmentRefresh(
 ):
     """Functional tests for 'nova-manage volume_attachment refresh'."""
 
+    # Required for any multiattach volume tests
+    microversion = '2.60'
+
     def setUp(self):
         super().setUp()
         self.tmpdir = self.useFixture(fixtures.TempDir()).path
         self.ctxt = context.get_admin_context()
         self.cli = manage.VolumeAttachmentCommands()
+        self.output = StringIO()
+        self.useFixture(fixtures.MonkeyPatch('sys.stdout', self.output))
         self.flags(my_ip='192.168.1.100')
         self.fake_connector = {
             'ip': '192.168.1.128',
@@ -286,7 +291,7 @@ class TestNovaManageVolumeAttachmentRefresh(
         self.assertEqual('create', actions[5]['action'])
 
     def test_refresh(self):
-        server = self._create_server()
+        server = self._create_server(networks='none')
         volume_id = self.cinder.IMAGE_BACKED_VOL
         self.api.post_server_volume(
             server['id'], {'volumeAttachment': {'volumeId': volume_id}})
@@ -344,7 +349,7 @@ class TestNovaManageVolumeAttachmentRefresh(
         self._assert_instance_actions(server)
 
     def test_refresh_rpcapi_remove_volume_connection_rollback(self):
-        server = self._create_server()
+        server = self._create_server(networks='none')
         volume_id = self.cinder.IMAGE_BACKED_VOL
         self.api.post_server_volume(
             server['id'], {'volumeAttachment': {'volumeId': volume_id}})
@@ -390,7 +395,7 @@ class TestNovaManageVolumeAttachmentRefresh(
         self._assert_instance_actions(server)
 
     def test_refresh_cinder_attachment_update_rollback(self):
-        server = self._create_server()
+        server = self._create_server(networks='none')
         volume_id = self.cinder.IMAGE_BACKED_VOL
         self.api.post_server_volume(
             server['id'], {'volumeAttachment': {'volumeId': volume_id}})
@@ -447,7 +452,7 @@ class TestNovaManageVolumeAttachmentRefresh(
     def test_refresh_pre_cinderv3_without_attachment_id(self):
         """Test the refresh command when the bdm has no attachment_id.
         """
-        server = self._create_server()
+        server = self._create_server(networks='none')
         volume_id = self.cinder.IMAGE_BACKED_VOL
         self.api.post_server_volume(
             server['id'], {'volumeAttachment': {'volumeId': volume_id}})
@@ -489,6 +494,29 @@ class TestNovaManageVolumeAttachmentRefresh(
         # Assert that we have actions we expect against the instance
         self._assert_instance_actions(server)
 
+    def test_show_multiattach_volume(self):
+        """Test that the show command doesn't fail for multiattach volumes
+        """
+        volume_id = self.cinder.MULTIATTACH_VOL
+
+        # Launch two instances and attach the same multiattach volume to both
+        server_1 = self._create_server(networks='none')
+        self.api.post_server_volume(
+            server_1['id'], {'volumeAttachment': {'volumeId': volume_id}})
+        self._wait_for_volume_attach(server_1['id'], volume_id)
+
+        server_2 = self._create_server(networks='none')
+        self.api.post_server_volume(
+            server_2['id'], {'volumeAttachment': {'volumeId': volume_id}})
+        self._wait_for_volume_attach(server_2['id'], volume_id)
+
+        result = self.cli.show(
+            volume_id=volume_id, instance_uuid=server_1['id'])
+
+        # Assert that the command completes successfully, this was previously
+        # broken and documented under bug #1945452
+        self.assertEqual(0, result)
+
 
 class TestNovaManagePlacementHealAllocations(
         integrated_helpers.ProviderUsageBaseTestCase):

nova/tests/unit/cmd/test_manage.py

@@ -3111,7 +3111,7 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase):
         }
 
     @mock.patch.object(manage, 'format_dict')
-    @mock.patch('nova.objects.BlockDeviceMapping.get_by_volume')
+    @mock.patch('nova.objects.BlockDeviceMapping.get_by_volume_and_instance')
     @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid')
     def test_show(self, mock_get_im, mock_get_bdm, mock_format_dict):
         """Test the 'show' command."""
@@ -3146,7 +3146,7 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase):
         mock_format_dict.assert_called_once_with(bdm)
 
     @mock.patch('oslo_serialization.jsonutils.dumps')
-    @mock.patch('nova.objects.BlockDeviceMapping.get_by_volume')
+    @mock.patch('nova.objects.BlockDeviceMapping.get_by_volume_and_instance')
     @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid')
     def test_show_json(self, mock_get_im, mock_get_bdm, mock_dump):
         """Test the 'show' command with --json."""
@@ -3181,7 +3181,7 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase):
         mock_dump.assert_called_once_with(bdm)
 
     @mock.patch.object(manage, 'format_dict')
-    @mock.patch('nova.objects.BlockDeviceMapping.get_by_volume')
+    @mock.patch('nova.objects.BlockDeviceMapping.get_by_volume_and_instance')
     @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid')
     def test_show_connection_info(
         self, mock_get_im, mock_get_bdm, mock_format_dict
@@ -3225,7 +3225,7 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase):
         mock_format_dict.assert_called_once_with(
             fake_connection_info)
 
-    @mock.patch('nova.objects.BlockDeviceMapping.get_by_volume')
+    @mock.patch('nova.objects.BlockDeviceMapping.get_by_volume_and_instance')
     @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid')
     def test_show_connection_info_json(self, mock_get_im, mock_get_bdm):
         """Test the 'show' command with --json and --connection_info."""
@@ -3300,7 +3300,7 @@ class VolumeAttachmentCommandsTestCase(test.NoDBTestCase):
             mock.sentinel.context, uuidsentinel.instance)
         self.assertEqual(2, ret)
 
-    @mock.patch('nova.objects.BlockDeviceMapping.get_by_volume')
+    @mock.patch('nova.objects.BlockDeviceMapping.get_by_volume_and_instance')
     @mock.patch('nova.objects.InstanceMapping.get_by_instance_uuid')
     def test_show_bdm_not_found(self, mock_get_im, mock_get_bdm):
         """Test the 'show' command with a missing bdm."""