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

provider/openstack: Volume Attachment and Detachment Fixes #8172

Merged
merged 1 commit into from
Aug 24, 2016

Conversation

jtopjian
Copy link
Contributor

@jtopjian jtopjian commented Aug 13, 2016

This commit is changing the volumes block from being computed to non-computed.
This change makes the Terraform configuration the source of truth about volumes
attached to the instance and therefore is able to correctly detect when a user
detaches a volume during an update.

One thing to be aware of is that if a user attached a volume out of band of an
instance controlled by Terraform, that volume will be detached upon the next
apply. The best thing to do is add a volume entry in the instance's
configuration of any volumes that were attached out of band.

This commit also explicitly detaches volumes from an instance before the
instance terminates. Most Block Storage volume drivers account for this
scenario internally, but there are a few that don't. This change is to support
those that don't.

In addition, when volumes are read by the instance, volumes configured in the
Terraform configuration are the source of truth. Previously, a call was being
made to OpenStack to provide the list of attached volumes.

It also adds a few new tests and fixes existing tests for various volume
attach-related scenarios.

Fixes #8040
Fixes #8042
Fixes #8045

@jtopjian
Copy link
Contributor Author

jtopjian commented Aug 13, 2016

@GiovanniPaoloGibilisco thank you very much for your bug reports. I believe this PR should fix all of them. Would you be able to test and verify?

A note about #8042: if you look at the test I wrote, you'll see I had to add an explicit depends_on between the instances. This is to avoid a race condition of both instances being updated in parallel and thus acting on the volume at the same time. depends_on is a bit of a workaround... a better solution would be something like a openstack_compute_volumeattach_v2 resource that is dedicated to the relationship between a volume and instance. That's on my list of things to look into.

Let me know if you have any questions!

@jtopjian jtopjian force-pushed the openstack-volumeattach-fix branch 2 times, most recently from 045afc6 to f116fb0 Compare August 24, 2016 14:46
This commit is changing the `volumes` block from being computed to non-computed.
This change makes the Terraform configuration the source of truth about volumes
attached to the instance and therefore is able to correctly detect when a user
detaches a volume during an update.

One thing to be aware of is that if a user attached a volume out of band of an
instance controlled by Terraform, that volume will be detached upon the next
apply. The best thing to do is add a `volume` entry in the instance's
configuration of any volumes that were attached out of band.

This commit also explicitly detaches volumes from an instance before the
instance terminates. Most Block Storage volume drivers account for this
scenario internally, but there are a few that don't. This change is to support
those that don't.

In addition, when volumes are read by the instance, volumes configured in the
Terraform configuration are the source of truth. Previously, a call was being
made to OpenStack to provide the list of attached volumes.

It also adds a few new tests and fixes existing tests for various volume
attach-related scenarios.
@jtopjian
Copy link
Contributor Author

Some notes about volume attachment in general (both as a brain dump for myself and in case this is helpful to anyone looking into the OpenStack provider):

For those not familiar with OpenStack, keep in mind that OpenStack is a collection of projects that provide APIs to different services. Sometimes a project provides "proxy" support to another project. For example, the OpenStack Block Storage API has two major versions in the wild: v1 and v2. OpenStack Compute provides an os-volumes_attach API that will attach a block storage volume to an instance by only using the Compute API. The developer only needs to work with the Compute API. It will handle the work of determining whether to use Block Storage v1 or v2.

Other cloud providers support the attachment of volumes during instance creation. OpenStack doesn't have that kind of feature, so to mimic it in Terraform, a separate call is made to attach volumes after the instance has been successfully created.

Because volume attachment is not native to instance creation, it's arguably best moved to a new resource like:

openstack_compute_volume_attach_v2 {
  instance_id = "${openstack_compute_instance_v2.name.id}"
  volume_id = "${openstack_blockstorage_volume_v1.name.id}"
}

As mentioned earlier, OpenStack Nova/Compute provides the os-volumes_attach API that can handle volume attachment... except that os-volumes_attach doesn't report the status of the attachment or the status of the volume. This means a call is made to attach a volume and if the volume is still in a CREATING state, an error will be returned. The best that can be done within the Compute API is to just retry when an error occurs and hopefully the errors eventually stop. I'm not a fan of that. The same goes for detaching as well.

The next option is to integrate the Block Storage API, so an attach is triggered by the Compute API and the attachment status is detected by the Block Storage API. This is what the openstack_compute_instance_v2 resource currently does. There's a problem with this, though: It's hard-coded to use the Block Storage v1 API. This needs resolved at some point in time.

A third option is to just make volume attachment entirely within the Block Storage API. That would work well for Block Storage v2 since it has an attachment API, but Block Storage v1 actually relies on the Compute API for attachment. 😵

So at the moment, this is what I'm thinking for a long-term goal:

  1. Deprecate the volumes block in openstack_compute_instance_v2.
  2. Create openstack_blockstorage_volume_attach_v1 that will use both the Compute API and Block Storage v1 API to attach Block Storage v1 volumes.
  3. Create openstack_blockstorage_volume_attach_v2 that will use the Block Storage v2 api to attach Block Storage v2 volumes.

The downside of breaking out volume attachment into their own resources is that if a user wants to wait for all volumes to finish attaching before provisioning, they will need to use a null resource or something similar. I think that's acceptable as that type of workflow is usually inevitable with more complex Terraform configurations.

@jtopjian jtopjian merged commit 49bda26 into hashicorp:master Aug 24, 2016
@jtopjian jtopjian deleted the openstack-volumeattach-fix branch March 23, 2017 16:09
@ghost
Copy link

ghost commented Apr 15, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 15, 2020
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
1 participant