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: Fixes Boot From Volume #3225

Closed
wants to merge 2 commits into from
Closed

provider/openstack: Fixes Boot From Volume #3225

wants to merge 2 commits into from

Conversation

jtopjian
Copy link
Contributor

This commit fixes the previously broken "boot from volume" feature. It also
adds an acceptance test to ensure the feature continues to work.

The "delete_on_termination" option was also added.

Partly Fixes #3206

@jtopjian
Copy link
Contributor Author

The root cause of this was the following:

if blockDeviceRaw, ok := d.Get("block_device").(map[string]interface{}); ok && blockDeviceRaw != nil {

It should have been

if blockDeviceRaw, ok := d.Get("block_device").([]interface{}); ok && blockDeviceRaw != nil {

or something similar.

Because of this, the block of code never worked and so it won't exist in anyone's currently deployed state files. I took this opportunity to convert the block_device type to TypeSet and used the same implementation as the AWS root_block_device parameter.

This commit fixes the previously broken "boot from volume" feature. It also
adds an acceptance test to ensure the feature continues to work.

The "delete_on_termination" option was also added.
@jtopjian
Copy link
Contributor Author

Maybe I've just been looking at this for too long, but it's starting to feel awkward to me. I don't like how I've had to make volume_id optional in order to accurately report the root/boot volume.

As an alternative, I have made a new parameter called attached_volumes that is entirely computed. This way, the user specifies the boot volume in block_device, supplemental volumes in volume, and all will be reported in attached_volumes.

The downside is that anyone using the current volume parameter for Computed information will need to make changes.

This variation can be seem here: jtopjian@1555964

What's the best thing to do in this situation?

@jtopjian
Copy link
Contributor Author

Ignore the above. After reviewing the AWS implementation, I think the method used in this PR is correct. Leaving the above comment in case anyone would want to discuss, though.

I'm adding a second commit to this PR that does some supplemental volume cleanup and adds some extra tests. While the commit isn't required to fix the issue at hand, it does make the implementation cleaner and tries to prevents regression.

This commit cleans up the volume and block device handling in the instance
resource. It also adds more acceptance tests to deal with different workflows
of attaching and detaching a volume through the instance's lifecycle.

No new functionality has been added.
@jtopjian
Copy link
Contributor Author

Closing in favor of #3232

@jtopjian jtopjian closed this Sep 14, 2015
@ghost
Copy link

ghost commented May 1, 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 May 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

openstack_compute_instance_v2 block_device - boot from volume
2 participants