Volume attach sets mountpoint as /dev/na in Cinder attachment

I'm guessing that's why this was in the nova code:

https://review.openstack.org/#/c/525787/3/nova/virt/block_device.py@448

But we didn't know why it was there....

I'm adding Cinder to this bug report since this is arguably a bug in the PUT /attachments/{id} API since the connector dict is per compute host, so shoving the mountpoint down in the host connector dict is confusing and wrong IMO - the mountpoint is per volume attachment, so it should be a top-level field on the volume attachment itself.

Making that change to the PUT /attachments/{id} request format would be a microversion change though so I'm just documenting it here for a future enhancement.

We'll also have to fix this for live migration with volumes attached because we create the attachment with the host connector at that point:

https://github.com/openstack/nova/blob/32c8ac6b7dfe4ca0c211cbce7c5a67d88558126f/nova/compute/manager.py#L5786-L5790

Fix proposed to branch: master
Review: https://review.openstack.org/527468

Per comment 3, the POST /attachments API in Cinder should also take an optional mountpoint so it doesn't have to be passed through the connector parameter.

Fix proposed to branch: master
Review: https://review.openstack.org/527479

@Matt I'm trying to understand why you view the behavior of passing the mountpoint in the connector as incorrect?

Other than "because it used to be different" I'm not sure I agree that this is a bug. I could be missing an important point here, but IMO mountpoint should in fact be a part of the connector, until a connection is made it's pretty much useless. It would be different if specifying a mountpoint in nova volume-attach was actually honored but it's not and I'm not aware that it ever will be deterministic like that. Maybe there's some work going on there again that I'm unaware of.

I either case, given we provide an equivalent mechanism, it's just in a different place I'm unclear why this should be changed?

From a client perspective, the connector that nova passes to cinder is from brick and is specific to the host that we're going to connect to, always has been. The mountpoint is per-attachment, and as such it seems odd to make clients shove it into the connector dict that brick provides just to get the attachments API to show the mountpoint. It makes more sense to me from a client perspective to make the mountpoint a top-level field on the attachment resource in the API provided along with the connector, similar to how the old os-initialize_connection + os-attach volume action combination would work.

This isn't high priority by any means, but it does mean we have to have client-side workaround code in nova for this (and any other code that's going to use Cinder standalone).

Reviewed: https://review.openstack.org/527468
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=16d0ad344e22aa5aa5d0c42a872f079587b62c6d
Submitter: Zuul
Branch: master

commit 16d0ad344e22aa5aa5d0c42a872f079587b62c6d
Author: Matt Riedemann <email address hidden>
Date: Tue Dec 12 12:09:50 2017 -0500

    Pass mountpoint to volume attachment_update

    The old volume attach flow would pass the mountpoint,
    which is the BlockDeviceMapping.device_name, to the
    os-attach volume action API. With the new flow, apparently
    the mountpoint is expected to be passed to the update
    attachment API via the connector dict, which is not obvious
    and if not provided, causes Cinder to default the mountpoint
    to "/dev/na" which is wrong.

    We work around this in Nova by providing the mountpoint in a
    copy of the connector dict and pass that to attachment_update,
    and update the two places in the code that are updating an
    attachment (the normal driver block device attach code and
    swap volume). Long-term this should be fixed in the Cinder
    attachments update API, but that requires a microversion so
    we need to handle it client-side for now.

    Change-Id: I11ba269c3f7a2e7707b2b7e27d26eb7a2c948a82
    Partial-Bug: #1737779

Reviewed: https://review.openstack.org/527479
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=d550fe883fab160bc098864e732cccdfaf756c40
Submitter: Zuul
Branch: master

commit d550fe883fab160bc098864e732cccdfaf756c40
Author: Matt Riedemann <email address hidden>
Date: Tue Dec 12 12:43:53 2017 -0500

    Pass mountpoint to volume attachment_create with connector

    Similar to I11ba269c3f7a2e7707b2b7e27d26eb7a2c948a82, when
    we create a volume attachment with a connector, we need to
    also provide the mountpoint via the connector to Cinder,
    because internally within Cinder the attachment_create code
    is calling attachment_update if a connector is provided.

    Change-Id: If3afe8d8bd6b8c327ccc7d1140053bccaf7e1ad7
    Closes-Bug: #1737779

This issue was fixed in the openstack/nova 17.0.0.0b3 development milestone.