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 stop before destroy #7184

Merged
merged 2 commits into from
Jun 23, 2016

Conversation

alkersan
Copy link
Contributor

@alkersan alkersan commented Jun 15, 2016

@jtopjian Please review this patch for #7129 behavior. It includes 1 added dependency to gophercloud package and basic opt-in implementation of stop_before_destroy flag.

Need advice on acceptance test for it. Currently it was tested manually within corporate openstack setup.

First scenario was a mini consul cluster, where clients were configured with leave_on_terminate option. When I scaled down consul machines - they correctly left cluster and disappeared from node catatlog, thus it subjectively proves that Nova sent ACPI shutdown signal to guests OS. Without stop_before_destroy consul shows all nodes in failing state.

Another check is to peek at instance action history API. With stop_before_destroy enabled, action history should include stop:

+--------+------------------------------------------+---------+----------------------------+
| Action | Request_ID                               | Message | Start_Time                 |
+--------+------------------------------------------+---------+----------------------------+
| create | req-e0d5c543-161a-43a0-9fb5-527b802faa03 | -       | 2016-06-15T20:01:39.000000 |
| stop   | req-b86d88e3-8916-47e3-a139-8b0cf5cfb35a | -       | 2016-06-15T20:03:47.000000 |
+--------+------------------------------------------+---------+----------------------------+

But, unfortunately, I didn't find implementation of history API in gophercloud

@alkersan alkersan changed the title [WIP] Openstack stop before destroy [WIP] provider/openstack stop before destroy Jun 16, 2016
@jtopjian
Copy link
Contributor

@alkersan Nice work!

I agree -- creating an acceptance test that confirms end-to-end functionality for this is difficult. I think creating a simple test that verifies the new argument works without error is good enough. In this case, just make a copy of the "basic" acceptance test but add stop_before_destroy = true 😄

@alkersan alkersan force-pushed the openstack_stop_before_detele branch from 80548de to bddc422 Compare June 21, 2016 19:41
@alkersan
Copy link
Contributor Author

@jtopjian ok, simple acc test is there and branch was rebased on fresh master

@alkersan alkersan changed the title [WIP] provider/openstack stop before destroy provider/openstack stop before destroy Jun 21, 2016
@alkersan alkersan force-pushed the openstack_stop_before_detele branch from bddc422 to 71924f9 Compare June 22, 2016 19:14
@jtopjian
Copy link
Contributor

@alkersan this looks great! I just ran the acceptance test suite and everything passed.

Would you be able to squash the commits into two: one for the Gophercloud deps and the other for everything else? Once that's done, I'll go ahead and merge this in 😄

@alkersan alkersan force-pushed the openstack_stop_before_detele branch from 71924f9 to c32d152 Compare June 23, 2016 06:21
@alkersan
Copy link
Contributor Author

@jtopjian done

@jtopjian jtopjian merged commit 5ce693d into hashicorp:master Jun 23, 2016
@jtopjian
Copy link
Contributor

@alkersan Thank you, sir!

@alkersan alkersan deleted the openstack_stop_before_detele branch June 23, 2016 15:15
@ghost
Copy link

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

Successfully merging this pull request may close these issues.

2 participants