Route provider route deletion action does not work

Details

Description

Route provider in chef-client has two bugs in route deletion code:
1) is_running flag is not set correctly (an issue with ruby syntax, is_running=true does not call attr_accessor code, it just creates a local variable)
2) When we fix the first issue, route existence check starts failing in cases when a route exists on a server, but has different gateway (and should not be removed).

I'll provide a patch for this issue today.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Oleksiy Kovyrin added a comment -

https://github.com/opscode/chef/pull/276 contains the patch with test cases covering load_current_resource functionality on linux.

Show
Oleksiy Kovyrin added a comment - https://github.com/opscode/chef/pull/276 contains the patch with test cases covering load_current_resource functionality on linux.
Hide
Permalink
Bryan McLellan [Opscode] added a comment -

Reviewed and marked to me merged. Thanks Oleksiy!

Show
Bryan McLellan [Opscode] added a comment - Reviewed and marked to me merged. Thanks Oleksiy!
Hide
Permalink
Bryan McLellan [Opscode] added a comment -

I have a failing spec, I'm not sure how this is correctly converting from hex to decimal.

1) Chef::Provider::Route Chef::Provider::Route load_current_resource on linux should detect existing routes and set is_running attribute correctly
     Failure/Error: provider.is_running.should be_true
       expected false to be true
     # ./spec/unit/provider/route_spec.rb:63:in `block (4 levels) in <top (required)>'
Show
Bryan McLellan [Opscode] added a comment - I have a failing spec, I'm not sure how this is correctly converting from hex to decimal. 1) Chef::Provider::Route Chef::Provider::Route load_current_resource on linux should detect existing routes and set is_running attribute correctly Failure/Error: provider.is_running.should be_true expected false to be true # ./spec/unit/provider/route_spec.rb:63:in `block (4 levels) in <top (required)>'
Hide
Permalink
Oleksiy Kovyrin added a comment -

Let me try it here, I'm pretty sure it worked before I pushed it...

Show
Oleksiy Kovyrin added a comment - Let me try it here, I'm pretty sure it worked before I pushed it...
Hide
Permalink
Oleksiy Kovyrin added a comment -

Just checked like this, tests pass:

$ bundle exec rspec -cbfs spec/unit/provider/route_spec.rb 

Chef::Provider::Route
  Chef::Provider::Route load_current_resource
    on linux
      should set is_running to false when a route is not detected
      should detect existing routes and set is_running attribute correctly
      should use gateway value when matching routes
  Chef::Provider::Route action_add
    should add the route if it does not exist
    should not add the route if it exists
  Chef::Provider::Route action_delete
    should delete the route if it exists
    should not delete the route if it does not exist
  Chef::Provider::Route generate_command for action_add
    should include a netmask when a one is specified
    should not include a netmask when a one is specified
    should include ' via $gateway ' when a gateway is specified
    should not include ' via $gateway ' when a gateway is not specified
  Chef::Provider::Route generate_command for action_delete
    should include a netmask when a one is specified
    should not include a netmask when a one is specified
    should include ' via $gateway ' when a gateway is specified
    should not include ' via $gateway ' when a gateway is not specified
  Chef::Provider::Route config_file_contents for action_add
    should include a netmask when a one is specified
    should not include a netmask when a one is specified
    should include ' via $gateway ' when a gateway is specified
    should not include ' via $gateway ' when a gateway is not specified
  Chef::Provider::Route config_file_contents for action_delete
    should return an empty string
  Chef::Provider::Route generate_config method
    should write a route file on centos platform
    should write a route file on redhat platform
    should write a route file on fedora platform
    should put all routes for a device in a route config file

Finished in 0.01928 seconds
24 examples, 0 failures
Show
Oleksiy Kovyrin added a comment - Just checked like this, tests pass: $ bundle exec rspec -cbfs spec/unit/provider/route_spec.rb Chef::Provider::Route Chef::Provider::Route load_current_resource on linux should set is_running to false when a route is not detected should detect existing routes and set is_running attribute correctly should use gateway value when matching routes Chef::Provider::Route action_add should add the route if it does not exist should not add the route if it exists Chef::Provider::Route action_delete should delete the route if it exists should not delete the route if it does not exist Chef::Provider::Route generate_command for action_add should include a netmask when a one is specified should not include a netmask when a one is specified should include ' via $gateway ' when a gateway is specified should not include ' via $gateway ' when a gateway is not specified Chef::Provider::Route generate_command for action_delete should include a netmask when a one is specified should not include a netmask when a one is specified should include ' via $gateway ' when a gateway is specified should not include ' via $gateway ' when a gateway is not specified Chef::Provider::Route config_file_contents for action_add should include a netmask when a one is specified should not include a netmask when a one is specified should include ' via $gateway ' when a gateway is specified should not include ' via $gateway ' when a gateway is not specified Chef::Provider::Route config_file_contents for action_delete should return an empty string Chef::Provider::Route generate_config method should write a route file on centos platform should write a route file on redhat platform should write a route file on fedora platform should put all routes for a device in a route config file Finished in 0.01928 seconds 24 examples, 0 failures
Hide
Permalink
Bryan McLellan [Opscode] added a comment -

What version of Ruby?

Passes for me on 1.8.7-p302 (ubuntu), and 1.8.7-p352 (rvm)
Fails for me on 1.9.2-p318 (rvm) and 1.9.3-p125 (rvm)

Show
Bryan McLellan [Opscode] added a comment - What version of Ruby? Passes for me on 1.8.7-p302 (ubuntu), and 1.8.7-p352 (rvm) Fails for me on 1.9.2-p318 (rvm) and 1.9.3-p125 (rvm)
Hide
Permalink
Oleksiy Kovyrin added a comment -

I was testing on 1.8. Checking with 1.9 now.

Show
Oleksiy Kovyrin added a comment - I was testing on 1.8. Checking with 1.9 now.
Hide
Permalink
Oleksiy Kovyrin added a comment -

Reproduced, fixing now.

Show
Oleksiy Kovyrin added a comment - Reproduced, fixing now.
Hide
Permalink
Oleksiy Kovyrin added a comment -

https://github.com/opscode/chef/pull/276 - I've updated the pull request to work with ruby 1.9. It was broken before, but with bad tests coverage it has not been caught earlier.

Show
Oleksiy Kovyrin added a comment - https://github.com/opscode/chef/pull/276 - I've updated the pull request to work with ruby 1.9. It was broken before, but with bad tests coverage it has not been caught earlier.
Hide
Permalink
Oleksiy Kovyrin added a comment -

Any updates on this?

Show
Oleksiy Kovyrin added a comment - Any updates on this?
Hide
Permalink
Bryan McLellan [Opscode] added a comment -

Reresolving for merge.

We froze 0.10.10 for release last week so we're fixing for regressions right now and we'll merge this as soon as we're done with that.

Show
Bryan McLellan [Opscode] added a comment - Reresolving for merge. We froze 0.10.10 for release last week so we're fixing for regressions right now and we'll merge this as soon as we're done with that.
Hide
Permalink
Oleksiy Kovyrin added a comment -

I really believe you should pull this in to be included in 0.10.10. Route resource is seriously broken without these patches, in my opinion no feature freeze should prevent you from accepting a bugfix.

Show
Oleksiy Kovyrin added a comment - I really believe you should pull this in to be included in 0.10.10. Route resource is seriously broken without these patches, in my opinion no feature freeze should prevent you from accepting a bugfix.
Hide
Permalink
Bryan McLellan [Opscode] added a comment -

The line becomes more about bugfixes causing other regressions for other
users. If you believe you can fully exercise this provider within two days
we can probably include the patch.

Show
Bryan McLellan [Opscode] added a comment - The line becomes more about bugfixes causing other regressions for other users. If you believe you can fully exercise this provider within two days we can probably include the patch.
Hide
Permalink
Oleksiy Kovyrin added a comment -

We run this patch in production for about a week now on an infrastructure of more than a hundred servers with different configurations - didn't have any issues and it actually removes routes now (used to simply ignore them). So, if other users do not complaint, most probably because route provider was so horribly broken, that nobody could use it anyways. Now it has actually becomes useful for all kinds of crazy static routing we need to do on our servers.

Anyways, it is up to you to accept it or not, but I believe it should be pretty useful for the community. Should you have any questions/suggestions/ideas about the patches, feel free to contact me via the ticket and I'll do my best to help.

Show
Oleksiy Kovyrin added a comment - We run this patch in production for about a week now on an infrastructure of more than a hundred servers with different configurations - didn't have any issues and it actually removes routes now (used to simply ignore them). So, if other users do not complaint, most probably because route provider was so horribly broken, that nobody could use it anyways. Now it has actually becomes useful for all kinds of crazy static routing we need to do on our servers. Anyways, it is up to you to accept it or not, but I believe it should be pretty useful for the community. Should you have any questions/suggestions/ideas about the patches, feel free to contact me via the ticket and I'll do my best to help.
Hide
Permalink
Bryan McLellan [Opscode] added a comment -

Merged to master, thanks Oleksiy.

Show
Bryan McLellan [Opscode] added a comment - Merged to master, thanks Oleksiy.

People

  • Assignee:
    Oleksiy Kovyrin
    Reporter:
    Oleksiy Kovyrin
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: