Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compute v2: Enable Multiattach Volumes #442

Merged

Conversation

jtopjian
Copy link
Contributor

@jtopjian jtopjian commented Oct 13, 2018

This commit allows instances to attach multiattach volumes
by adding a new argument called "multiattach". When set, the
compute client will use Microversion 2.60, which enables multiattach
support in the Compute/Nova API.

For #432

@jamesmmccarthy I'm unable to test multiattach volumes at the moment, so I'm not sure if this works. Would you be able to give it a try? I've updated the documentation to include an example implementation, though I'm not sure if it's correct.

@ghost ghost added the size/S label Oct 13, 2018
@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 13, 2018

Build succeeded.

@jamesmmccarthy
Copy link
Contributor

Initial attempts at testing this PR seem to show it's not having the desired effect - will look into this further. (Also small typo in the doc section)

@jamesmmccarthy
Copy link
Contributor

jamesmmccarthy commented Oct 15, 2018

@jtopjian I have re-tested and found that this PR actually does work - it requires adding multiattach = true property to the attachment definition, looks like the doc typo is the only outstanding issue. Probably with terraform/IaC this is a normal requirement.

@jamesmmccarthy
Copy link
Contributor

jamesmmccarthy commented Oct 15, 2018

For completeness I also did a test where a single volume is attached to two instances. It required the use of depends_on for sanity, otherwise it worked well, like so:

$ openstack server list
+--------------------------------------+-------------+--------+----------------------+--------------+--------+
| ID | Name | Status | Networks | Image | Flavor |
+--------------------------------------+-------------+--------+----------------------+--------------+--------+
| 60c521a6-837c-4e03-a81e-01d11662ca2e | myinstance2 | ACTIVE | public=10.196.223.11 | cirros-0.4.0 | |
| 7deb75a3-14a1-4ac6-943b-931f6c8f1161 | myinstance | ACTIVE | public=10.196.223.23 | cirros-0.4.0 | |
+--------------------------------------+-------------+--------+----------------------+--------------+--------+
$ openstack volume list
+--------------------------------------+-------+--------+------+-------------------------------------------------------------------------+
| ID | Name | Status | Size | Attached to |
+--------------------------------------+-------+--------+------+-------------------------------------------------------------------------+
| b35f8871-2392-4dc4-af78-0f86ddcebd41 | myvol | in-use | 1 | Attached to myinstance on /dev/vdb Attached to myinstance2 on /dev/vdb |
+--------------------------------------+-------+--------+------+-------------------------------------------------------------------------+

@jtopjian
Copy link
Contributor Author

it requires adding multiattach = true property to the attachment definition, looks like the doc typo is the only outstanding issue

Where is the doc typo? The example in this PR has the following:

resource "openstack_compute_volume_attach_v2" "va_1" {
  instance_id = "${openstack_compute_instance_v2.instance_1.id}"
  volume_id   = "${openstack_blockstorage_volume_v2.volume_1.id}"
  multiattach = true
}

resource "openstack_compute_volume_attach_v2" "va_2" {
  instance_id = "${openstack_compute_instance_v2.instance_2.id}"
  volume_id   = "${openstack_blockstorage_volume_v2.volume_1.id}"
  multiattach = true
}

It required the use of depends_on for sanity

This makes sense. I'll update the PR to include this.

Thank you for testing this 😄

@jamesmmccarthy
Copy link
Contributor

jamesmmccarthy commented Oct 15, 2018

Typo: Line 151 * multiattach - (Optional) Enable attachment of multiattach-capabile volumes.
capabile ?

@jamesmmccarthy
Copy link
Contributor

jamesmmccarthy commented Oct 15, 2018

@jtopjian re: depends_on: the "va_2" resource would want something like:
depends_on = ["openstack_compute_volume_attach_v2.va_1"]

For reference, the depends_on is to avoid timing issues, and to prevent complaints such as:

  • openstack_compute_volume_attach_v2.attached: Bad request with: [POST ... error message: {"badRequest": {"message": "Invalid input received: Invalid volume: Volume eec1ba0f-3900-425c-bf7e-490b5ad326c7 status must be available or in-use or downloading (HTTP 400) (Request-ID: req-2381cdd0-e013-49b7-b8a0-75a3ff3875bd)", "code": 400}}

Probably there are other terraform techniques to achieve similar, this was what I used for this testing.

Thanks for the updates ! 😄

@jtopjian
Copy link
Contributor Author

Probably there are other terraform techniques to achieve similar, this was what I used for this testing.

I think for this situation, depends_on is the most appropriate solution. depends_on is the cleanest way of "serializing" Terraform.

This commit allows instances to attach multiattach volumes
by adding a new argument called "multiattach". When set, the
compute client will use Microversion 2.60, which enables multiattach
support in the Compute/Nova API.
@jtopjian jtopjian changed the title [WIP] Compute v2: Enable Multiattach Volumes Compute v2: Enable Multiattach Volumes Oct 16, 2018
@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 16, 2018

Build failed.

  • terraform-provider-openstack-unittest : RETRY_LIMIT in 1m 00s
  • terraform-provider-openstack-acceptance-test : RETRY_LIMIT in 1m 00s

@jtopjian
Copy link
Contributor Author

recheck

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 16, 2018

Build failed.

@jtopjian
Copy link
Contributor Author

recheck

@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 16, 2018

Build succeeded.

Copy link
Member

@ozerovandrei ozerovandrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great

@jtopjian jtopjian merged commit f9b32da into terraform-provider-openstack:master Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants