-
Notifications
You must be signed in to change notification settings - Fork 449
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
Improve detection of Windows EC2 nodes by using UUID information #1052
Conversation
Signed-off-by: Tim Smith <[email protected]>
Signed-off-by: Tim Smith <[email protected]>
This is actually how Amazon recommends doing it. It works no matter how the VM was built. Signed-off-by: Tim Smith <[email protected]>
Signed-off-by: Tim Smith <[email protected]>
Signed-off-by: Tim Smith <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question about whether we need to be defensive about wmi.query possibly returning an empty result set. Otherwise LGTM!
lib/ohai/plugins/ec2.rb
Outdated
if RUBY_PLATFORM =~ /mswin|mingw32|windows/ | ||
require "wmi-lite/wmi" | ||
wmi = WmiLite::Wmi.new | ||
if wmi.query("select uuid from Win32_ComputerSystemProduct")[0]["identifyingnumber"] =~ /^ec2/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there a chance that wmi.query(...)[0] will return nil (in the event of an empty result set) and then you'd be trying to call :[] on NilClass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about that after I pushed it up. If we get an empty result set we didn't have a hint and we have no way to know we're on EC2. The plugin will fail and get skipped and the node won't be recognized as EC2, which is actually what we want. Ohai rescues the world (for better or worse) so we do sorta awful things like that throughout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'm not sure how we'd not have a UUID, although I suppose anything is possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does Get-WmiObject Win32_ComputerSystemProduct | Select-Object IdentifyingNumber, UUID
return on a Windows system in EC2?
You're searching for UUID but actually returning 'IdentifyingNumber' which is typically a serial number. That makes more sense than UUID if we're looking for a string with /^ec2/ in it.
In which case I think you just want this:
if wmi.first_of("Win32_ComputerSystemProduct")["identifyingnumber"] =~ /^ec2/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a much better query. Thanks
Appveyor failure is valid. Right now the code is trying to run on windows in Appveyor and failing. Needs some mocking love. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix appveyor and consider my nit about the more correct WmiLite::Wmi
method, and 👍
Signed-off-by: Tim Smith <[email protected]>
We're currently relying on "amazon" Organization data being present in the Windows installation. This doesn't work if the VM was migrated from VMware and it doesn't work if the org builds their own AMIs that include their organization. Amazon recommends checking UUID data and we're already doing that on Linux. Lets do that on Windows.