Detect paravirt amazon instances without hint files #722
Conversation
we should have enough other methods now, hints have been around for a while. this detects xen, not ec2. also cleans up some confusing whitespacing in the test.
tas50
force-pushed
the
better_ec2_detection_v2
branch
from
26cef78
to
045c0a9
Compare
tas50
force-pushed
the
better_ec2_detection_v2
branch
from
045c0a9
to
b7bd80d
Compare
|
@chef/client-core ready for review |
|
merge in #723 and then I'm 👍 |
Official AMIs ship with the ec2metadata binary which we can look for. With this change we should be detecting the majority of scenarios without a hint file. has_ec2_mac has been renamed has_xen_mac since we're actually looking for a xen mac. Due to that the attempt to connect to the metadata only needs to happen when we only find the mac address since that can be either AWS or Xen. If we find dmi data, a hint, or the metadata binary we can safely assume the metadata endpoint will respond. I also consolidated the "no hints or signs of AWS" scenario into a single spec since it seems to make more sense that way.
tas50
force-pushed
the
better_ec2_detection_v2
branch
from
b7bd80d
to
3c5a0e0
Compare
|
Merged in and tweaked the changes that @btm made to warn on the MAC detection logic |
|
👍 after appveyor is green appveyor failure looks like a legit test failure: looks like travis passed but shows failed due to some kind of caching issue.... |
|
@btm any idea on the expects for the warn not triggering. It's clearly showing up in the output when you run the specs, but not triggering. I shuffled things around a bit, but no luck. |
|
@tas50 can you make sure this branch is still pushed? |
|
@tas50 n/m, I see your repositories are named silly on github. |
|
@btm any magical ideas about that travis failure? |
| @plugin[:network][:interfaces][:eth0][:arp] = {"169.254.1.0"=>"00:50:56:c0:00:08"} | ||
| it "warns that the arp table method is deprecated" do | ||
| expect(Ohai::Log).to receive(:warn).with(/will be removed/) | ||
| @plugin.has_ec2_mac? |
There was a problem hiding this comment.
Do you mean has_xen_mac? or looks_like_ec2?
There was a problem hiding this comment.
That was it. Thanks @mcquin
| # the Xen environment is *not* EC2 | ||
| hint?('ec2') || ( has_ec2_dmi? || has_ec2_mac?) && can_metadata_connect?(Ohai::Mixin::Ec2Metadata::EC2_METADATA_ADDR,80) | ||
| return true if (has_ec2metadata_bin? || has_ec2_dmi?) || (has_xen_mac? && can_metadata_connect?(Ohai::Mixin::Ec2Metadata::EC2_METADATA_ADDR,80)) |
There was a problem hiding this comment.
The original logic and the new logic here look different to me (but I'm having trouble parsing it). It looks like the original logic would check can_metadata_connect? unless a hint was provided. The new logic only checks can_metadata_connect? if has_xen_mac? is true and both has_ec2metadata_bin? and has_ec2_dmi? are false. The variation I'm seeing here is that the new logic doesn't check can_metadata_connect? when has_ec2_dmi? is true.
There was a problem hiding this comment.
The original (pre-8.10) behavior was hint or xen mac + try to connect. I incorrectly changed it in 8.10 so that it would try to connect if it found the DMI data. The connection attempt was only in place because the MAC detection triggers on any Xen based system and is very unreliable. If we find the binary or the DMI data we know 100% that we're on Amazon so there's no need to try to open the socket before we later try to open the socket again.
Detect paravirt amazon instances without hint files
Official AMIs ship with the ec2metadata binary which we can look for. With this change we should be detecting the majority of scenarios without a hint file.
has_ec2_mac has been renamed has_xen_mac since we're actually looking for a xen mac. Due to that the attempt to connect to the metadata only needs to happen when we only find the mac address since that can be either AWS or Xen. If we find dmi data, a hint, or the metadata binary we can safely assume the metadata endpoint will respond.
I also consolidated the "no hints or signs of AWS" scenario into a single spec since it seems to make more sense that way.