Open Stack

On 6 June 2018 at 13:55, Jay Pipes <jaypipes at gmail.com> wrote:
> 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.

I can't comment on how it was originally documented, but I'm confident
in the use case. Certainly I know this is how our customers use it.
It's the Nova-side implementation of a cinder retype operation. There
are a bunch of potential reasons to want to do this, but a specific
one that I recall from a customer was that they'd originally stood up
their cloud with a single storage array. After a while they realised
that their cloud was getting way more use than they'd anticipated, and
the original array wasn't up to the job either in performance or
capacity. They needed to move a bunch of data from the old array to
the new one. They did this with a cinder retype. Without refreshing my
memory (so please don't sweat the details!), cinder does:

Create volume b with the new type (where 'new type' means 'in the new location')
{
  if volume a is attached:
    nova.swap_volume(volume a, volume b) >>> because cinder can't
safely copy itself while we're still writing to it
  else:
    copy it some other way presumably
} >>> The result of this is that volume b contains a copy of the data
of volume a
Update volume b to have the same uuid as volume a
Delete volume a

So the copy operation is fundamental to the purpose of the call. Any
other hypervisor driver which implemented it would also have to
implement the copy, there's nothing libvirt-specific about it. The
contract of the call is:

Copy the contents of volume a to volume b and swap volume a for volume
b without losing data.

We could, as you say, pause the VM during this operation. However,
note that this wouldn't solve the problem with multiattach because
you'd need to pause *all* attached VMs, so the libvirt implementation
which allows you to do it without downtime has no bearing. It would
also require 'data downtime' during the copy, which may well take many
hours. From discussions with customers, I don't believe this would be
well received.

We could change the flow in cinder to:

Create new volume b
for each volumea.attachment:
  nova.pause(attached vm)
cinder does the copy
Update volume b to have same uuid as volume a
for each volumea.attachment:
  nova.just_swap_no_copy(volume a, volume b)
  nova.unpause(attached vm)
Delete volume a

However, to be safe we'd have to create a mechanism for cinder to
'lock' those pauses, otherwise another operation during the
potentially very long period in which our data is unavailable could
unlock it and cause the data corruption we're trying to avoid here.

In short:
* swap_volume only exists because Cinder can't do this
* the contract of the call isn't tied to the libvirt driver
* pause vs live-copy is an optimisation which doesn't impact the
contract of the call
  * if we removed it anyway, users would be seriously unhappy with the downtime

As for swap_volume with multiattach, the use case is exactly the same.
The operator finds that their existing storage array is almost full,
can't be expanded further, occasionally emits blue smoke, and they're
fed up with that vendor anyway because the support contract increases
in price by 50% annually. They need to move the data off. Some of the
volumes on the array are multiattach. It needs to be possible to move
this data *somehow*, even if it's less convenient than with
non-multiattach.

Incidentally, I believe some users do it for load balancing, so it's routine.

>> 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.

As above, Cinder can't do this. The only way to achieve this
exclusively in Cinder would be to deprecate retype of an attached
volume. Then it would be down to the user to detach the volume,
retype, and reattach. As noted, this would be very much user-visible,
and the associated downtime would often be large. Red Hat, at least,
would have some very unhappy customers.

> 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.

If it was simple and pretty I wouldn't be bothering you with it ;)

Thanks,

Matt
-- 
Matthew Booth
Red Hat OpenStack Engineer, Compute DFG

Phone: +442070094448 (UK)