fixes #16951 - ipv6 compute orchestration #3943
Conversation
|
This can be tested with theforeman/foreman-digitalocean#16 |
| @@ -124,7 +128,7 @@ def setComputeDetails | |||
| attrs.each do |foreman_attr, fog_attr| | |||
| if foreman_attr == :mac | |||
| return false unless match_macs_to_nics(fog_attr) | |||
| elsif foreman_attr == :ip | |||
| elsif [:ip, :ip6].include?(foreman_attr) | |||
| value = vm.send(fog_attr) || find_address | |||
There was a problem hiding this comment.
find_address isn't IPv6 aware here, it'll call vm.ip_addresses which can return v4 addresses if the compute claims to provide v6 but vm.send(fog_attr) returns nil. You can reproduce this by not ticking the IPv6 box when using fog-digitalocean.
It probably needs to be changed to filter addresses from vm.ip_addresses depending on their type?
timogoebel
force-pushed
the
16951-ipv6-compute
branch
from
2205166
to
a946f5d
Compare
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
timogoebel
force-pushed
the
16951-ipv6-compute
branch
from
a946f5d
to
b109613
Compare
|
@domcleal : The code is more reliable now. [test foreman] |
|
[test foreman] |
| end | ||
| end | ||
|
|
||
| if ip.blank? && ip6.blank? && (compute_provides?(:ip) || compute_provides?(:ip6)) | ||
| return failure(_("failed to acquire ip addresses from compute resource for %s") % name) |
There was a problem hiding this comment.
Capitalise "Failed" and "IP", important in translated (and therefore user-visible) strings.
| end | ||
| ip | ||
| end | ||
|
|
||
| def filter_ip_addresses(addresses, type) | ||
| check_method = type == :ip6 ? :ipv6? : :ipv4? | ||
| addresses.map {|ip| IPAddr.new(ip) rescue nil}.compact.select { |ip| ip.send(check_method) }.map(&:to_s) |
There was a problem hiding this comment.
select { |ip| ip.send(check_method) } can be written as select(&check_method)
| elsif foreman_attr == :ip | ||
| value = vm.send(fog_attr) || find_address | ||
| elsif [:ip, :ip6].include?(foreman_attr) | ||
| value = vm.send(fog_attr) || find_address(foreman_attr) | ||
| self.send("#{foreman_attr}=", value) | ||
| return false unless validate_foreman_attr(value, ::Nic::Base, foreman_attr) |
There was a problem hiding this comment.
If the provider claims to provide :ip6 but it doesn't (e.g. foreman_digitalocean, don't tick IPv6), then this still throws an error if value.blank? and there's another blank value in the database. Don't call it if the value's blank?
2016-11-01T14:36:30 c50b235a [sql] [W] ip6 is already used by
There was a problem hiding this comment.
Oh, sorry. I have tested this with a fresh install and an empty database and it worked fine. I will make the necessary adjustments and add a test case for this.
| test "are set for CR providing IPv6" do | ||
| an_ip6 = '2001:db8::1' | ||
| @cr.stubs(:provided_attributes).returns({:ip6 => :ip6}) | ||
| @host.vm.expects(:ip6).returns(an_ip6) |
There was a problem hiding this comment.
A test that returns a blank value for :ip6 while claiming to provide ip4 + ip6 would be a good idea.
timogoebel
force-pushed
the
16951-ipv6-compute
branch
from
b109613
to
703ed78
Compare
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@domcleal : Thanks for giving this another spin. I have fixed the issue and tested with more hosts. It should work fine now. Also improved the test coverage of this. |
timogoebel
force-pushed
the
16951-ipv6-compute
branch
from
703ed78
to
1127dd1
Compare
|
Merged, thanks @timogoebel. |
No description provided.