Fixes #17641 - Use force when destroying VMs #4104

Merged
merged 1 commit into from Dec 21, 2016

Conversation

Projects
None yet
5 participants
@agx
Member

agx commented Dec 12, 2016

There's no point in waiting several seconds for any guest utils to shut
down the VM when we want to get rid of it as quickly as possible. This
also works around cases like

2016-12-12T10:33:22 [app] [W] Failed to destroy a compute foo (VMware) instance bar.example.com: ToolsUnavailable: Der Vorgang kann nicht abgeschlossen werden, weil VMware Tools auf dieser virtuellen Maschine nicht ausgeführt wird.
| RbVmomi::Fault: ToolsUnavailable: Der Vorgang kann nicht abgeschlossen werden, weil VMware Tools auf dieser virtuellen Maschine
nicht ausgeführt wird.
| /usr/share/foreman/vendor/ruby/2.1.0/gems/rbvmomi-1.8.2/lib/rbvmomi/connection.rb:61:in parse_response' | /usr/share/foreman/vendor/ruby/2.1.0/gems/rbvmomi-1.8.2/lib/rbvmomi/connection.rb:90:incall'
| /usr/share/foreman/vendor/ruby/2.1.0/gems/rbvmomi-1.8.2/lib/rbvmomi/basic_types.rb:205:in _call' | /usr/share/foreman/vendor/ruby/2.1.0/gems/rbvmomi-1.8.2/lib/rbvmomi/basic_types.rb:74:inblock (2 levels) in init'
| /usr/share/foreman/vendor/ruby/2.1.0/gems/fog-1.34.0/lib/fog/vsphere/requests/compute/vm_power_off.rb:17:in vm_power_off' | /usr/share/foreman/vendor/ruby/2.1.0/gems/fog-1.34.0/lib/fog/vsphere/models/compute/server.rb:94:instop'
| /usr/share/foreman/vendor/ruby/2.1.0/gems/fog-1.34.0/lib/fog/vsphere/models/compute/server.rb:107:in destroy' | /usr/share/foreman/app/models/compute_resource.rb:155:indestroy_vm'
| /usr/share/foreman/app/models/concerns/orchestration/compute.rb:164:in delCompute' | /usr/share/foreman/app/models/concerns/orchestration.rb:168:inexecute'

where VMWare misdetecs the Openvm tools status.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Dec 12, 2016

@agx, thanks for your PR! By analyzing the history of the files in this pull request, we identified @abenari, @ohadlevy and @domcleal to be potential reviewers.

@agx, thanks for your PR! By analyzing the history of the files in this pull request, we identified @abenari, @ohadlevy and @domcleal to be potential reviewers.

@theforeman-bot

This comment has been minimized.

Show comment
Hide comment
@theforeman-bot

theforeman-bot Dec 12, 2016

Member

There were the following issues with the commit message:

  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

Member

theforeman-bot commented Dec 12, 2016

There were the following issues with the commit message:

  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column
  • commit message for fd586d3 is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@dLobatog

Please check out the comment inline, should be ready to merge after that. Thanks @agx !

app/models/compute_resource.rb
@@ -184,7 +184,7 @@ def create_vm(args = {})
end
def destroy_vm(uuid)
- find_vm_by_uuid(uuid).destroy
+ find_vm_by_uuid(uuid).destroy :force => true

This comment has been minimized.

@dLobatog

dLobatog Dec 14, 2016

Member

@agx If this is just for VMWare - mind to move the call to vmware.rb? It'd be as simple as rewriting the method in that file, you can see other methods like new_vm etc... have been overridden there already.

The reason is just that we cannot ensure all fog compute resource providers accept this option (or other options at all). Azure for example does not accept any argument on .destroy calls

@dLobatog

dLobatog Dec 14, 2016

Member

@agx If this is just for VMWare - mind to move the call to vmware.rb? It'd be as simple as rewriting the method in that file, you can see other methods like new_vm etc... have been overridden there already.

The reason is just that we cannot ensure all fog compute resource providers accept this option (or other options at all). Azure for example does not accept any argument on .destroy calls

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Dec 16, 2016

Member
Member

agx commented Dec 16, 2016

Fixes #17641 - Use force when destroying VMs
There's no point in waiting several seconds for any guest utils to shut
down the VM when we want to get rid of it as quickly as possible.  This
also works around cases like

  2016-12-12T10:33:22 [app] [W] Failed to destroy a compute foo (VMware) instance bar.example.com: ToolsUnavailable: Der Vorgang kann nicht abgeschlossen werden, weil VMware Tools auf dieser virtuellen Maschine nicht ausgeführt wird.
   | RbVmomi::Fault: ToolsUnavailable: Der Vorgang kann nicht abgeschlossen werden, weil VMware Tools auf dieser virtuellen Maschine
  nicht ausgeführt wird.
   | /usr/share/foreman/vendor/ruby/2.1.0/gems/rbvmomi-1.8.2/lib/rbvmomi/connection.rb:61:in `parse_response'
   | /usr/share/foreman/vendor/ruby/2.1.0/gems/rbvmomi-1.8.2/lib/rbvmomi/connection.rb:90:in `call'
   | /usr/share/foreman/vendor/ruby/2.1.0/gems/rbvmomi-1.8.2/lib/rbvmomi/basic_types.rb:205:in `_call'
   | /usr/share/foreman/vendor/ruby/2.1.0/gems/rbvmomi-1.8.2/lib/rbvmomi/basic_types.rb:74:in `block (2 levels) in init'
   | /usr/share/foreman/vendor/ruby/2.1.0/gems/fog-1.34.0/lib/fog/vsphere/requests/compute/vm_power_off.rb:17:in `vm_power_off'
   | /usr/share/foreman/vendor/ruby/2.1.0/gems/fog-1.34.0/lib/fog/vsphere/models/compute/server.rb:94:in `stop'
   | /usr/share/foreman/vendor/ruby/2.1.0/gems/fog-1.34.0/lib/fog/vsphere/models/compute/server.rb:107:in `destroy'
   | /usr/share/foreman/app/models/compute_resource.rb:155:in `destroy_vm'
   | /usr/share/foreman/app/models/concerns/orchestration/compute.rb:164:in `delCompute'
   | /usr/share/foreman/app/models/concerns/orchestration.rb:168:in `execute'

where VMWare misdetecs the openvm-tools status.
@theforeman-bot

This comment has been minimized.

Show comment
Hide comment
@theforeman-bot

theforeman-bot Dec 20, 2016

Member

There were the following issues with the commit message:

  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

Member

theforeman-bot commented Dec 20, 2016

There were the following issues with the commit message:

  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column
  • commit message for 8b0977b is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Dec 20, 2016

Member

@dLobatog patch updated as you requested

Member

agx commented Dec 20, 2016

@dLobatog patch updated as you requested

@dLobatog

This comment has been minimized.

Show comment
Hide comment
@dLobatog

dLobatog Dec 21, 2016

Member

Thanks - @agx the problem with using it in general for all compute resources is that Foreman relies on fog (https://github.com/fog/) for these operations. We'd have to go over each of the ones that do not support :force, add it, then wait until a new release, then incorporate the release in Foreman.

I'll wrap your commit to the 72nd column & merge, thanks again

Member

dLobatog commented Dec 21, 2016

Thanks - @agx the problem with using it in general for all compute resources is that Foreman relies on fog (https://github.com/fog/) for these operations. We'd have to go over each of the ones that do not support :force, add it, then wait until a new release, then incorporate the release in Foreman.

I'll wrap your commit to the 72nd column & merge, thanks again

@dLobatog dLobatog merged commit 6816de9 into theforeman:develop Dec 21, 2016

5 of 6 checks passed

foreman Build finished.
Details
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound No violations found. Woof!
katello Build finished.
Details
upgrade Build finished.
Details
@timogoebel

This comment has been minimized.

Show comment
Hide comment
@timogoebel

timogoebel Dec 21, 2016

Member

@agx : Could you please "fix" the commit message? I'll he happy to quickly test this after and we're ready to 🚢 .

Member

timogoebel commented Dec 21, 2016

@agx : Could you please "fix" the commit message? I'll he happy to quickly test this after and we're ready to 🚢 .

@agx

This comment has been minimized.

Show comment
Hide comment
@agx

agx Dec 21, 2016

Member
Member

agx commented Dec 21, 2016

@timogoebel

This comment has been minimized.

Show comment
Hide comment
@timogoebel

timogoebel Dec 21, 2016

Member

@agx : Daniel has already merged this, so nevermind. I was a few seconds too slow. :-)
Thanks for your contribution!

Member

timogoebel commented Dec 21, 2016

@agx : Daniel has already merged this, so nevermind. I was a few seconds too slow. :-)
Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment