Open Stack

On 06/06/2018 07:46 AM, Matthew Booth wrote:
> TL;DR I think we need to entirely disable swap volume for multiattach
> volumes, and this will be an api breaking change with no immediate
> workaround.
> 
> I was looking through tempest and came across
> api.compute.admin.test_volume_swap.TestMultiAttachVolumeSwap.test_volume_swap_with_multiattach.
> This test does:
> 
> Create 2 multiattach volumes
> Create 2 servers
> Attach volume 1 to both servers
> ** Swap volume 1 for volume 2  on server 1 **
> Check all is attached as expected
> 
> The problem with this is that swap volume is a copy operation.

Is it, though? The original blueprint and implementation seem to suggest 
that the swap_volume operation was nothing more than changing the 
mountpoint for a volume to point to a different location (in a safe
manner that didn't lose any reads or writes).

https://blueprints.launchpad.net/nova/+spec/volume-swap

Nothing about the description of swap_volume() in the virt driver 
interface mentions swap_volume() being a "copy operation":

https://github.com/openstack/nova/blob/76ec078d3781fb55c96d7aaca4fb73a74ce94d96/nova/virt/driver.py#L476

> We don't just replace one volume with another, we copy the contents
> from one to the other and then do the swap. We do this with a qemu
> drive mirror operation, which is able to do this copy safely without
> needing to make the source read-only because it can also track writes
> to the source and ensure the target is updated again. Here's a link
> to the libvirt logs showing a drive mirror operation during the swap
> volume of an execution of the above test:
After checking the source code, the libvirt virt driver is the only virt 
driver that implements swap_volume(), so it looks to me like a public 
HTTP API method was added that was specific to libvirt's implementation 
of drive mirroring. Yay, more implementation leaking out through the API.

> http://logs.openstack.org/58/567258/5/check/nova-multiattach/d23fad8/logs/libvirt/libvirtd.txt.gz#_2018-06-04_10_57_05_201
> 
> The problem is that when the volume is attached to more than one VM,
> the hypervisor doing the drive mirror *doesn't* know about writes on
> the other attached VMs, so it can't do that copy safely, and the
> result is data corruption.

Would it be possible to swap the volume by doing what Vish originally 
described in the blueprint: pause the VM, swap the volume mountpoints 
(potentially after migrating the underlying volume), start the VM?

 >
  Note that swap volume isn't visible to the
> guest os, so this can't be addressed by the user. This is a data
> corrupter, and we shouldn't allow it. However, it is in released code
> and users might be doing it already, so disabling it would be a
> user-visible api change with no immediate workaround.

I'd love to know who is actually using the swap_volume() functionality, 
actually. I'd especially like to know who is using swap_volume() with 
multiattach.

> However, I think we're attempting to do the wrong thing here anyway,
> and the above tempest test is explicit testing behaviour that we don't
> want. The use case for swap volume is that a user needs to move volume
> data for attached volumes, e.g. to new faster/supported/maintained
> hardware.

Is that the use case?

As was typical, there's no mention of a use case on the original 
blueprint. It just says "This feature allows a user or administrator to 
transparently swap out a cinder volume that connected to an instance." 
Which is hardly a use case since it uses the feature name in a 
description of the feature itself. :(

The commit message (there was only a single commit for this 
functionality [1]) mentions overwriting data on the new volume:

   Adds support for transparently swapping an attached volume with
   another volume. Note that this overwrites all data on the new volume
   with data from the old volume.

Yes, that is the commit message in its entirety. Of course, the commit 
had no documentation at all in it, so there's no ability to understand 
what the original use case really was here.

https://review.openstack.org/#/c/28995/

If the use case was really "that a user needs to move volume data for 
attached volumes", why not just pause the VM, detach the volume, do a 
openstack volume migrate to the new destination, reattach the volume and 
start the VM? That would mean no libvirt/QEMU-specific implementation 
behaviour leaking out of the public HTTP API and allow the volume 
service (Cinder) to do its job properly.

> With single attach that's exactly what they get: the end
> user should never notice. With multi-attach they don't get that. We're
> basically forking the shared volume at a point in time, with the
> instance which did the swap writing to the new location while all
> others continue writing to the old location. Except that even the fork
> is broken, because they'll get a corrupt, inconsistent copy rather
> than point in time. I can't think of a use case for this behaviour,
> and it certainly doesn't meet the original design intent.
> 
> What they really want is for the multi-attached volume to be copied
> from location a to location b and for all attachments to be updated.
> Unfortunately I don't think we're going to be in a position to do that
> any time soon, but I also think users will be unhappy if they're no
> longer able to move data at all because it's multi-attach. We can
> compromise, though, if we allow a multiattach volume to be moved as
> long as it only has a single attachment. This means the operator can't
> move this data without disruption to users, but at least it's not
> fundamentally immovable.
> 
> This would require some cooperation with cinder to achieve, as we need
> to be able to temporarily prevent cinder from allowing new
> attachments. A natural way to achieve this would be to allow a
> multi-attach volume with only a single attachment to be redesignated
> not multiattach, but there might be others. The flow would then be:
> 
> Detach volume from server 2
> Set multiattach=False on volume
> Migrate volume on server 1
> Set multiattach=True on volume
> Attach volume to server 2
> 
> Combined with a patch to nova to disallow swap_volume on any
> multiattach volume, this would then be possible if inconvenient.
> 
> Regardless of any other changes, though, I think it's urgent that we
> disable the ability to swap_volume a multiattach volume because we
> don't want users to start using this relatively new, but broken,
> feature.

Or we could deprecate the swap_volume Compute API operation and use 
Cinder for all of this.

But sure, we could also add more cruft to the Compute API and add more 
conditional "it works but only when X" docs to the API reference.

Just my two cents,
-jay