OHAI-540 revisited #289

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
6 participants
Member

paulrossman commented Feb 22, 2014

Version # change in 9f9045a was reverted in later commit.

@btm btm referenced this pull request Mar 5, 2014

Closed

OHAI-540 #252

lib/ohai/plugins/gce.rb
-def has_google_dmi?
- ::File.read(GOOGLE_SYSFS_DMI).include?('Google')
+def has_dmi?
+ has_signature?('Google')
@btm

btm Mar 10, 2014

Owner

Is there a specific key that we expect to find Google in? Can we depend on the dmi plugin to fetch that information for us rather than calling dmidecode again just for this?

@paulrossman

paulrossman Mar 20, 2014

Member

I'm using the method described in the GCE docs

@btm

btm Mar 21, 2014

Owner

The docs appear to be looking at a specific string which we're not doing here:

sudo dmidecode -s bios-vendor | grep Google

Could you run this on a GCE instance? I suspect it will include or just be Google:

sudo ohai dmi/bios/vendor

It looks like we currently run dmidecode in six different plugins (four of them being virtualization plugins), but I don't see the benefit in collecting this data multiple times when we can get it once from the dmi plugin.

depends "dmi/bios/vendor"

# ...

if dmi[:bios][:vendor] == "Google"
   # /..
end
lib/ohai/mixin/metadata_server.rb
+ return nil
+ end
+ return nil unless response.code == "200"
+ if is_json?(response.body)
@lamont-granquist

lamont-granquist Mar 10, 2014

Contributor

Can you do something more like this instead?

   return nil unless response.code == "200"
   data = safe_json_parse(response.body)
   Ohai::Log.error("response body was not json") if data.nil?
   data
end

def safe_json_parse(data)
  parser = Yajl::Parser.new
  begin
    parser.parse(data)
  rescue Yajl::ParseError
    nil
  end
end

Yajl::Parser#parse takes a string so the StringIO is unnecessary, and there's no need to double parse the data.

@paulrossman

paulrossman Mar 20, 2014

Member

Yes, this is better. Change made.

+
+ def server_available?(addr, port, headers)
+ http = Net::HTTP.new(addr, port)
+ http.open_timeout = 2
@btm

btm Mar 10, 2014

Owner

This is taking about 15 seconds to fail if addr is on an existing network, not two seconds.

I recreated this method without headers and raise "omg #{se.inspect}" instead

irb(main):025:0> server_available?("192.168.5.120", 80)
omg #<Timeout::Error: execution expired>
=> false

In the past we've avoided calling the metadata server unless we have some proof it will be there because we don't want to risk adding multiple seconds of delay to Ohai for people on other networks. If the DMI hint is reliable, we should be able to avoid calling has_metadata? in #looks_like_gce.

@paulrossman

paulrossman Mar 20, 2014

Member

If DMI isn't available, there are likely larger issues to contend with. I can either make this a secondary check or remove it completely. What is your preference?

@btm

btm Mar 20, 2014

Owner

In the past we've made it secondary, but I think it was because the primary test wasn't as reliable as it is here. I'd keep the metadata server check as a secondary test for now.

@paulrossman

paulrossman Mar 20, 2014

Member

btm@ I can't seem to replicate the timeout behavior you're seeing. Maybe a ruby version difference?

@btm

btm Mar 21, 2014

Owner
require 'net/http'

def server_available?(addr, port, headers)
  http = Net::HTTP.new(addr, port)
  http.open_timeout = 2
  http.read_timeout = 2
  response = http.get("/", headers)
  response.code == "200"
rescue Exception => e
  puts "Connection Failed: #{e}"
  false
end

start_time = Time.now
puts "Starting: #{start_time}"
server_available?("169.254.169.254", 80, {'X-Google-Metadata-Request' => 'True'})
puts "Complete: #{Time.now}, elapsed: #{Time.now-start_time}"
@btm

btm Mar 21, 2014

Owner

This timeout is about what you expect when writing it:

$ ruby /tmp/gce.rb
Starting: Fri Mar 21 06:04:55 -0700 2014
Connection Failed: execution expired
Complete: Fri Mar 21 06:04:57 -0700 2014, elapsed: 2.004359

I get this time on 1.8.7 and 1.9.3 against local networks and unrouted networks (169.254.169.254) on my Ubuntu VM.

@btm

btm Mar 21, 2014

Owner

Yeah, I can't remember what I was doing to get a longer than 2 second timeout here. Let's ignore that and move this call to happen only if we have confirmed DMI already.

- before(:each) do
+
+ let(:hint_path_nix) { '/etc/chef/ohai/hints/gce.json' }
+ let(:hint_path_win) { 'C:\chef\ohai\hints/gce.json' }
@btm

btm Mar 10, 2014

Owner

You could use Ohai::Config.platform_specific_path here.

+ before do
+ File.stub(:exist?).with(hint_path_nix).and_return(false)
+ File.stub(:exist?).with(hint_path_win).and_return(false)
+ end
@lamont-granquist

lamont-granquist Mar 10, 2014

Contributor

All you're testing in all of these is the hints behavior. If someone was running the spec tests on GCE this test in particular would actually fail since it would really run the plugin and would find the metadata server and/or DMI information. You should at least wrap all of the current tests with stubbing has_dmi? and has_metadata? to be false. It would also probably be good to unit test has_dmi? and has_metadata? in isolation.

@lamont-granquist

lamont-granquist Mar 10, 2014

Contributor

And Ohai::Mixin::Metadata server has no tests, and that's the one I'd be most worried about. Would be useful to have tests that stubbed Yajl to throw an exception and then checked it returned nil, etc

lib/ohai/mixin/metadata_server.rb
+ http.read_timeout = 2
+ response = http.get("/", headers)
+ response.code == "200"
+ rescue StandardError => se
@btm

btm Mar 21, 2014

Owner

This should include Timeout::Error for 1.8.7 compatibility.

rescue StandardError,Timeout::Error => se

http://martinciu.com/2010/12/ruby-timeout-error-is-not-a-standard-error.html

Starting: Fri Mar 21 06:07:31 -0700 2014
/usr/lib/ruby/1.8/timeout.rb:64:in `open': execution expired (Timeout::Error)
  from /usr/lib/ruby/1.8/net/http.rb:560:in `connect'
  from /usr/lib/ruby/1.8/net/http.rb:560:in `connect'
  from /usr/lib/ruby/1.8/net/http.rb:553:in `do_start'
  from /usr/lib/ruby/1.8/net/http.rb:542:in `start'
  from /usr/lib/ruby/1.8/net/http.rb:1035:in `request'
  from /usr/lib/ruby/1.8/net/http.rb:772:in `get'
  from /tmp/gce.rb:7:in `server_available?'
  from /tmp/gce.rb:16

@sersut sersut added this to the Ohai 6 milestone May 21, 2014

+
+module Ohai
+ module Mixin
+ module MetadataServer
@sersut

sersut May 21, 2014

Owner

Is there any reason to change the namespace of this module @paulrossman? Unless there is a reason we don't want to do this since there might be third party plugins depending on this code.

+ http.open_timeout = 2
+ http.read_timeout = 2
+ response = http.get("/", headers)
+ response.code == "200"
@sersut

sersut May 21, 2014

Owner

Hrmmm is there a reason why we're setting the response code to 200?

Owner

sersut commented May 21, 2014

Also we've moved the Chef Client versions greater than 11.12.0 to Ohai 7. Wanted to make sure this is intended to work with versions smaller than that or with Chef 10.X.

@sersut sersut removed the Attack List label Jun 25, 2014

@mcquin mcquin closed this Jul 7, 2014

@thommay thommay added Type: Bug and removed Bug labels Jan 24, 2017

@lamont-granquist lamont-granquist locked and limited conversation to collaborators Nov 16, 2017

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