Avoid duplicating network definitions in multiple locations #510
Conversation
| nil | ||
| return nil unless self.crowbar["crowbar"]["network"].key?(type) | ||
| return nil unless @node["network"]["networks"].key?(type) | ||
| @node["network"]["networks"][type].merge(self.crowbar["crowbar"]["network"][type]) |
There was a problem hiding this comment.
Style/RedundantSelf: Redundant self detected. (https://github.com/bbatsov/ruby-style-guide#no-self-unless-required)
| # Error out if we were handed an invalid conduit mapping. | ||
| unless base_ifs.all?{ |i|i.is_a?(String) && ::Nic.exists?(i) } | ||
| raise ::ArgumentError.new("Conduit mapping \"#{conduit}\" for network \"#{name}\" is not sane: #{base_ifs.inspect}") | ||
| unless base_ifs.all? { |i|i.is_a?(String) && ::Nic.exists?(i) } |
There was a problem hiding this comment.
Style/SpaceAroundBlockParameters: Space after closing | missing.
vuntz
force-pushed
the
network-db2
branch
3 times, most recently
from
a6f5bf1
to
4866a44
Compare
| base_ifs = conduit_map[conduit]["if_list"] | ||
| addr = if network.address | ||
| IP.coerce("#{network.address}/#{network.netmask}") | ||
| else |
There was a problem hiding this comment.
Style/EmptyElse: Redundant else-clause.
There was a problem hiding this comment.
Some people in the team like to keep the explicit else here, so keeping it.
vuntz
force-pushed
the
network-db2
branch
3 times, most recently
from
bbecf03
to
f5eb81c
Compare
The usage attribute is actually the same as the network name, so it's just redundant. Also, the node attribute is completely useless.
The full network definitions are already available through the network-config-default role. However, we still allow overriding some settings on node-specific attributes, as this can be useful (to specify a node-specific conduit, for instance).
The full network definitions are already available through the network-config-default role. However, we still allow overriding some settings on node-specific attributes, as this can be useful (to specify a node-specific conduit, for instance).
|
lgtm (but hav not tested it) |
There's no need to provide an interface or interface list anymore: this will now be resolved on demand. This makes it a bit cheaper to create network objects.
We do this instead of accessing attributes, in order to centralize the location of the network definitions, so it can be changed.
Use the correct NodeObject methods instead.
We only require the address there. The definition can still be overridden in node attributes if needed, but the code is now able to look at the network definition directly in the network role.
When a node is discovered, but not allocated, it is not assigned an IP address in the admin network yet. Due to the dhcp template interpreting "no IP" as "deny booting", this resulted in reboot of discovered nodes blocking the nodes. Now in such cases, we simply take the current IP address of the node.
We were blindly converting attributes such as router_pref, vlan and mtu to integer; we should only do this if they're not nil.
This avoids having to know about how the network attributes are structured.
We need the network role on the nodes for the "installing" state as the provisioner-server node (usually, the admin server) needs to have access to the full network definition for that node, which is only possible if the network role is assigned to the node.
|
Since making #575 I suddenly feel I understand a lot of this code, so this stays on my review TODO list. |
| networks = {} | ||
| crowbar["crowbar"]["network"].each do |name, data| | ||
| # note that node might not be part of network proposal yet (for instance: | ||
| # if discovered, and IP got allocated by user) |
There was a problem hiding this comment.
@vuntz I might be missing something here, but if the node is not part of the network proposal yet, I think we'd not even get here. Because crowbar["crowbar"]["network"] should also be empty in that case.
| @@ -39,7 +39,7 @@ def self.list_networks(node) | |||
| def self.get_network_by_type(node, type) | |||
| unless node[:crowbar][:network].nil? | |||
| [type, "admin"].uniq.each do |usage| | |||
| if found = node[:crowbar][:network].find { |net, data| data[:usage] == usage } | |||
| if found = node[:crowbar][:network].find { |net, data| net == usage } | |||
There was a problem hiding this comment.
Unrelated to that line (but to the commit and I can't comment on the commit message): Not sure if that file is ever used for testing but in crowbar_framework/spec/fixtures/offline_chef/role_crowbar-admin_crowbar_com.json is still [:crowbar][:network][:admin][:usage] and [:crowbar][:network][:bmc][:usage].
But can be done in a follow-up PR.
I guess it'd be nice, although not mandatory. |
|
+1 (but I haven't tested it) |
This makes dealing with the network config more difficult, as some bits might get outdated.
Instead, we centralize the definitions in the network role, and have proper API for accessing this easily.
This goes with crowbar/crowbar-ha#119, crowbar/crowbar-hyperv#25 and crowbar/crowbar-openstack#415.