From 25eb0a7d76922e3a1a289d26c36b96a91c4059db Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 6 Jul 2021 13:35:44 +0200 Subject: [PATCH] NFS: Update connection info on online snap create The NFS snapshot creation for an attached volume requires interaction between Nova and Cinder, and a new qcow2 file is used after the attachment completes. This means that the connection properties stored in the Attachment is no longer valid, as it is pointing to the old qcow2 file, so if Nova tries to use that attachment it will start writing on the old qcow2 file. A flow showing this issue is: - Attach NFS volume - Create snapshot - Hard reboot After that the VM will start using the base image, breaking the qcow2 chain. If we delete the snapshot in the meantime, then the VM will fail to reboot. This patch fixes this inconsistency by updating the connection info field inside the remotefs driver. We usually prefer that drivers don't to touch the DB, directly or indirectly (using OVOs), but in this case we are using OVOs methods instead of the usual model update on the volume manager because there are cases in the driver where a snapshot is created (cloning via snapshot) and we have to update the attachment without the manager, as it is unaware that a temporary snapshot is being created. Besides that main reason there are other less critical reasons to do the attachment update in the driver: - Smaller code change - Easier to backport - Limit change impact on other areas (better for backport) - The snapshot_create model update code in the manager does not support updating attachments. - There are cases in the cinder volume manager where the model update values returned by snapshot_create are not being applied. Snapshot deletion belonging to in-use volumes are not affected by this issue because we only do block commit when the snapshot file we are deleting is not the active file. In _delete_snapshot_online: if utils.paths_normcase_equal(info['active_file'], info['snapshot_file']): Closes-Bug: #1860913 Change-Id: I62fcef3169dcb9f4363a5344af4b2711edfef632 --- .../unit/volume/drivers/test_remotefs.py | 19 ++++++++++++++++++- cinder/volume/drivers/remotefs.py | 11 +++++++++-- .../nfs-online-snapshot-c05e6c8113bbded6.yaml | 6 ++++++ 3 files changed, 33 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/nfs-online-snapshot-c05e6c8113bbded6.yaml diff --git a/cinder/tests/unit/volume/drivers/test_remotefs.py b/cinder/tests/unit/volume/drivers/test_remotefs.py index 1bf38961820..cbb2ca1e600 100644 --- a/cinder/tests/unit/volume/drivers/test_remotefs.py +++ b/cinder/tests/unit/volume/drivers/test_remotefs.py @@ -74,7 +74,7 @@ class RemoteFsSnapDriverTestCase(test.TestCase): self._fake_snap_c = fake_snapshot.fake_snapshot_obj(self.context) self._fake_snap_c.volume = self.volume_c self.volume_c_path = os.path.join(self._FAKE_MNT_POINT, - self._fake_snap_c.name) + self.volume_c.name) self._fake_snap_c_path = (self.volume_c_path + '.' + self._fake_snap_c.id) @@ -356,6 +356,18 @@ class RemoteFsSnapDriverTestCase(test.TestCase): snapshot.volume.status = 'backing-up' snapshot.volume.attach_status = 'attached' expected_method_called = '_create_snapshot_online' + conn_info = ('{"driver_volume_type": "nfs",' + '"export": "localhost:/srv/nfs1",' + '"name": "old_name"}') + attachment = fake_volume.volume_attachment_ovo( + self.context, connection_info=conn_info) + snapshot.volume.volume_attachment.objects.append(attachment) + mock_save = self.mock_object(attachment, 'save') + + # After the snapshot the connection info should change the name of + # the file + expected = copy.deepcopy(attachment.connection_info) + expected['name'] = snapshot.volume.name + '.' + snapshot.id else: expected_method_called = '_do_create_snapshot' @@ -372,6 +384,11 @@ class RemoteFsSnapDriverTestCase(test.TestCase): mock.sentinel.fake_info_path, expected_snapshot_info) + if volume_in_use: + mock_save.assert_called_once() + changed_fields = attachment.cinder_obj_get_changes() + self.assertEqual(expected, changed_fields['connection_info']) + @ddt.data({'encryption': True}, {'encryption': False}) def test_create_snapshot_volume_available(self, encryption): self._test_create_snapshot(encryption=encryption) diff --git a/cinder/volume/drivers/remotefs.py b/cinder/volume/drivers/remotefs.py index d786f86ef44..6c3cb44a067 100644 --- a/cinder/volume/drivers/remotefs.py +++ b/cinder/volume/drivers/remotefs.py @@ -1653,18 +1653,25 @@ class RemoteFSSnapDriverBase(RemoteFSDriver): backing_filename = self.get_active_image_from_info( snapshot.volume) new_snap_path = self._get_new_snap_path(snapshot) + active = os.path.basename(new_snap_path) if self._is_volume_attached(snapshot.volume): self._create_snapshot_online(snapshot, backing_filename, new_snap_path) + # Update reference in the only attachment (no multi-attach support) + attachment = snapshot.volume.volume_attachment[0] + attachment.connection_info['name'] = active + # Let OVO know it has been updated + attachment.connection_info = attachment.connection_info + attachment.save() else: self._do_create_snapshot(snapshot, backing_filename, new_snap_path) - snap_info['active'] = os.path.basename(new_snap_path) - snap_info[snapshot.id] = os.path.basename(new_snap_path) + snap_info['active'] = active + snap_info[snapshot.id] = active self._write_info_file(info_path, snap_info) def _create_snapshot_online(self, snapshot, backing_filename, diff --git a/releasenotes/notes/nfs-online-snapshot-c05e6c8113bbded6.yaml b/releasenotes/notes/nfs-online-snapshot-c05e6c8113bbded6.yaml new file mode 100644 index 00000000000..1aab2005164 --- /dev/null +++ b/releasenotes/notes/nfs-online-snapshot-c05e6c8113bbded6.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + NFS driver `bug #1860913 + `_: Fixed instance uses + base image file when it is rebooted after online snapshot creation.