[Cinder] Restore volume snaptshots while volume is in use
Hi, for providing a more enterprise like (VMware, HyperV, Proxmox) snapshot feature, it would be great to be able to revert snapshots in use. With the "revert-to-snapshot" functions everything is there to support this feature. IMO if the storage backend (driver) does not support the operation, it should raise an error (ex. resetting a ceph rbd volume while a vm is running is not possible). But if it supports the operation, we should not stop the user doing this, even if the volume is "in-use". One might introduce a force switch, to prevent triggering it unintended. A frontend component like skyline can also show additional information like the status of an attached instance. Since the "Detach and attach boot volumes" spec [1] was never fully implemented, there is no way to revert an instance to a snapshot state without completely rebuilding it. Like described in the spec draft, this is not always suitable and at least always cumbersome. -- Jan [1] https://specs.openstack.org/openstack/nova-specs/specs/stein/approved/detach... --- suggested patch ----- https://github.com/jmussmann/cinder/commit/4f971d77a94806e9ebdccab45392058c2... From 4f971d77a94806e9ebdccab45392058c25005761 Mon Sep 17 00:00:00 2001 From: jmussmann <jmussmann+github@posteo.de> Date: Thu, 31 Oct 2024 09:13:45 +0100 Subject: [PATCH] add: allow reverting snapshots of volumes in use --- cinder/volume/api.py | 5 +++-- cinder/volume/manager.py | 4 ++-- cinder/volume/rpcapi.py | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 0e20baff0..9aa160740 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -390,8 +390,9 @@ class API(base.Base): """revert a volume to a snapshot""" context.authorize(vol_action_policy.REVERT_POLICY, target_obj=volume) + volume_state = volume.status v_res = volume.update_single_status_where( - 'reverting', 'available') + 'reverting', ['available', 'in-use']) if not v_res: msg = (_("Can't revert volume %(vol_id)s to its latest snapshot " "%(snap_id)s. Volume's status must be 'available'.") @@ -408,7 +409,7 @@ class API(base.Base): "snap_id": snapshot.id}) raise exception.InvalidSnapshot(reason=msg) - self.volume_rpcapi.revert_to_snapshot(context, volume, snapshot) + self.volume_rpcapi.revert_to_snapshot(context, volume, snapshot, volume_state) def delete(self, context: context.RequestContext, diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 30de5d8c7..18914c073 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -1150,7 +1150,7 @@ class VolumeManager(manager.CleanableManager, self.create_snapshot(context, snapshot) return snapshot - def revert_to_snapshot(self, context, volume, snapshot) -> None: + def revert_to_snapshot(self, context, volume, snapshot, original_volume_state) -> None: """Revert a volume to a snapshot. The process of reverting to snapshot consists of several steps: @@ -1207,7 +1207,7 @@ class VolumeManager(manager.CleanableManager, LOG.exception(msg, msg_args) v_res = volume.update_single_status_where( - 'available', 'reverting') + original_volume_state, 'reverting') if not v_res: msg_args = {"id": volume.id, "status": 'available'} diff --git a/cinder/volume/rpcapi.py b/cinder/volume/rpcapi.py index 58de69c1c..0b36cebdd 100644 --- a/cinder/volume/rpcapi.py +++ b/cinder/volume/rpcapi.py @@ -180,11 +180,11 @@ class VolumeAPI(rpc.RPCAPI): volume=volume) @rpc.assert_min_rpc_version('3.15') - def revert_to_snapshot(self, ctxt, volume, snapshot): + def revert_to_snapshot(self, ctxt, volume, snapshot, volume_state): version = self._compat_ver('3.15') cctxt = self._get_cctxt(volume.service_topic_queue, version) cctxt.cast(ctxt, 'revert_to_snapshot', volume=volume, - snapshot=snapshot) + snapshot=snapshot, original_volume_state=volume_state) def delete_volume(self, ctxt: context.RequestContext, -- 2.47.0
Hi Jan, thank you for the suggestion. If you'd like create a bug [1] and submit a patch through our gerrit interface [2], that would be a big step towards helping to get this reviewed and merged. Let me know if you have any questions. 1: https://bugs.launchpad.net/cinder/+filebug 2: https://review.opendev.org/q/project:openstack/cinder -- Jon On Nov 05, Jan <jan@uponu.com> wrote:
Hi,
for providing a more enterprise like (VMware, HyperV, Proxmox) snapshot feature, it would be great to be able to revert snapshots in use.
With the "revert-to-snapshot" functions everything is there to support this feature.
IMO if the storage backend (driver) does not support the operation, it should raise an error (ex. resetting a ceph rbd volume while a vm is running is not possible). But if it supports the operation, we should not stop the user doing this, even if the volume is "in-use".
One might introduce a force switch, to prevent triggering it unintended. A frontend component like skyline can also show additional information like the status of an attached instance.
Since the "Detach and attach boot volumes" spec [1] was never fully implemented, there is no way to revert an instance to a snapshot state without completely rebuilding it. Like described in the spec draft, this is not always suitable and at least always cumbersome.
-- Jan
[1] https://specs.openstack.org/openstack/nova-specs/specs/stein/approved/detach...
--- suggested patch ----- https://github.com/jmussmann/cinder/commit/4f971d77a94806e9ebdccab45392058c2...
From 4f971d77a94806e9ebdccab45392058c25005761 Mon Sep 17 00:00:00 2001 From: jmussmann <jmussmann+github@posteo.de> Date: Thu, 31 Oct 2024 09:13:45 +0100 Subject: [PATCH] add: allow reverting snapshots of volumes in use
--- cinder/volume/api.py | 5 +++-- cinder/volume/manager.py | 4 ++-- cinder/volume/rpcapi.py | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 0e20baff0..9aa160740 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -390,8 +390,9 @@ class API(base.Base): """revert a volume to a snapshot""" context.authorize(vol_action_policy.REVERT_POLICY, target_obj=volume) + volume_state = volume.status v_res = volume.update_single_status_where( - 'reverting', 'available') + 'reverting', ['available', 'in-use']) if not v_res: msg = (_("Can't revert volume %(vol_id)s to its latest snapshot " "%(snap_id)s. Volume's status must be 'available'.") @@ -408,7 +409,7 @@ class API(base.Base): "snap_id": snapshot.id}) raise exception.InvalidSnapshot(reason=msg)
- self.volume_rpcapi.revert_to_snapshot(context, volume, snapshot) + self.volume_rpcapi.revert_to_snapshot(context, volume, snapshot, volume_state)
def delete(self, context: context.RequestContext, diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 30de5d8c7..18914c073 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -1150,7 +1150,7 @@ class VolumeManager(manager.CleanableManager, self.create_snapshot(context, snapshot) return snapshot
- def revert_to_snapshot(self, context, volume, snapshot) -> None: + def revert_to_snapshot(self, context, volume, snapshot, original_volume_state) -> None: """Revert a volume to a snapshot.
The process of reverting to snapshot consists of several steps: @@ -1207,7 +1207,7 @@ class VolumeManager(manager.CleanableManager, LOG.exception(msg, msg_args)
v_res = volume.update_single_status_where( - 'available', 'reverting') + original_volume_state, 'reverting') if not v_res: msg_args = {"id": volume.id, "status": 'available'} diff --git a/cinder/volume/rpcapi.py b/cinder/volume/rpcapi.py index 58de69c1c..0b36cebdd 100644 --- a/cinder/volume/rpcapi.py +++ b/cinder/volume/rpcapi.py @@ -180,11 +180,11 @@ class VolumeAPI(rpc.RPCAPI): volume=volume)
@rpc.assert_min_rpc_version('3.15') - def revert_to_snapshot(self, ctxt, volume, snapshot): + def revert_to_snapshot(self, ctxt, volume, snapshot, volume_state): version = self._compat_ver('3.15') cctxt = self._get_cctxt(volume.service_topic_queue, version) cctxt.cast(ctxt, 'revert_to_snapshot', volume=volume, - snapshot=snapshot) + snapshot=snapshot, original_volume_state=volume_state)
def delete_volume(self, ctxt: context.RequestContext, -- 2.47.0
Just a tip - a spec you're mentioning is for Nova. And I think it's actually correct from cinder perspective, that used volume can't be reverted or resized. So I'd suggest addressing the nova team regarding this topic. From what I recall from discussions on the topic, there was an impression that all use cases where you might need to detach root volume were covered by fixing rebuild and rescue functionality. It could be that volume recovery/revert simply slipped minds back then. On Tue, 19 Nov 2024, 19:58 Jon Bernard, <jobernar@redhat.com> wrote:
Hi Jan, thank you for the suggestion. If you'd like create a bug [1] and submit a patch through our gerrit interface [2], that would be a big step towards helping to get this reviewed and merged. Let me know if you have any questions.
1: https://bugs.launchpad.net/cinder/+filebug 2: https://review.opendev.org/q/project:openstack/cinder
-- Jon
Hi,
for providing a more enterprise like (VMware, HyperV, Proxmox) snapshot feature, it would be great to be able to revert snapshots in use.
With the "revert-to-snapshot" functions everything is there to support
feature.
IMO if the storage backend (driver) does not support the operation, it should raise an error (ex. resetting a ceph rbd volume while a vm is running is not possible). But if it supports the operation, we should not stop
user doing this, even if the volume is "in-use".
One might introduce a force switch, to prevent triggering it unintended. A frontend component like skyline can also show additional information
On Nov 05, Jan <jan@uponu.com> wrote: this the like
the status of an attached instance.
Since the "Detach and attach boot volumes" spec [1] was never fully implemented, there is no way to revert an instance to a snapshot state without completely rebuilding it. Like described in the spec draft, this is not always suitable and at least always cumbersome.
-- Jan
[1] https://specs.openstack.org/openstack/nova-specs/specs/stein/approved/detach...
--- suggested patch -----
https://github.com/jmussmann/cinder/commit/4f971d77a94806e9ebdccab45392058c2...
From 4f971d77a94806e9ebdccab45392058c25005761 Mon Sep 17 00:00:00 2001 From: jmussmann <jmussmann+github@posteo.de> Date: Thu, 31 Oct 2024 09:13:45 +0100 Subject: [PATCH] add: allow reverting snapshots of volumes in use
--- cinder/volume/api.py | 5 +++-- cinder/volume/manager.py | 4 ++-- cinder/volume/rpcapi.py | 4 ++-- 3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/cinder/volume/api.py b/cinder/volume/api.py index 0e20baff0..9aa160740 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -390,8 +390,9 @@ class API(base.Base): """revert a volume to a snapshot""" context.authorize(vol_action_policy.REVERT_POLICY, target_obj=volume) + volume_state = volume.status v_res = volume.update_single_status_where( - 'reverting', 'available') + 'reverting', ['available', 'in-use']) if not v_res: msg = (_("Can't revert volume %(vol_id)s to its latest
snapshot
" "%(snap_id)s. Volume's status must be 'available'.") @@ -408,7 +409,7 @@ class API(base.Base): "snap_id": snapshot.id}) raise exception.InvalidSnapshot(reason=msg)
- self.volume_rpcapi.revert_to_snapshot(context, volume, snapshot) + self.volume_rpcapi.revert_to_snapshot(context, volume, snapshot, volume_state)
def delete(self, context: context.RequestContext, diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 30de5d8c7..18914c073 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -1150,7 +1150,7 @@ class VolumeManager(manager.CleanableManager, self.create_snapshot(context, snapshot) return snapshot
- def revert_to_snapshot(self, context, volume, snapshot) -> None: + def revert_to_snapshot(self, context, volume, snapshot, original_volume_state) -> None: """Revert a volume to a snapshot.
The process of reverting to snapshot consists of several steps: @@ -1207,7 +1207,7 @@ class VolumeManager(manager.CleanableManager, LOG.exception(msg, msg_args)
v_res = volume.update_single_status_where( - 'available', 'reverting') + original_volume_state, 'reverting') if not v_res: msg_args = {"id": volume.id, "status": 'available'} diff --git a/cinder/volume/rpcapi.py b/cinder/volume/rpcapi.py index 58de69c1c..0b36cebdd 100644 --- a/cinder/volume/rpcapi.py +++ b/cinder/volume/rpcapi.py @@ -180,11 +180,11 @@ class VolumeAPI(rpc.RPCAPI): volume=volume)
@rpc.assert_min_rpc_version('3.15') - def revert_to_snapshot(self, ctxt, volume, snapshot): + def revert_to_snapshot(self, ctxt, volume, snapshot, volume_state): version = self._compat_ver('3.15') cctxt = self._get_cctxt(volume.service_topic_queue, version) cctxt.cast(ctxt, 'revert_to_snapshot', volume=volume, - snapshot=snapshot) + snapshot=snapshot, original_volume_state=volume_state)
def delete_volume(self, ctxt: context.RequestContext, -- 2.47.0
participants (3)
-
Dmitriy Rabotyagov
-
Jan
-
Jon Bernard