Add external DHCP subnet config #714
Conversation
Codecov Report
@@ Coverage Diff @@
## master #714 +/- ##
==========================================
+ Coverage 81.97% 82.46% +0.49%
==========================================
Files 29 29
Lines 4128 4147 +19
==========================================
+ Hits 3384 3420 +36
+ Misses 744 727 -17
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
PTAL |
config/system/default.yaml
Outdated
| mininet_subnet: | ||
| base_ip: 10.20.0.0 | ||
| prefix_length: 16 | ||
| # default test host subnet |
There was a problem hiding this comment.
is this default or internal? They have slightly different meanings -- just wondering about congruence. I'm fine either way (don't actually know what it's used for), but would at least want the comment to match the field :-)
There was a problem hiding this comment.
This matches the default specified in code so I put the default in comment here. This is used when instantiating a mininet network. http://mininet.org/api/classmininet_1_1net_1_1Mininet.html#a1ed0f0c8ba06a398e02f3952cc4c8393
I thought of 1 issue where test hosts and devices' IPs may conflict. If mininet assigns test host an IP (it usually just increments from the subnet) and external DHCP server assigns the same IP to a device; this was why I made the mininet's subnet configurable.
There was a problem hiding this comment.
Re: default vs. internal -- it's very confusing as written so please update to make it more clear.
Don't the test hosts have to be on the same subnet as the device? If not, then the device will try to send the test host traffic to the gateway mac address which would fail? I guess I don't understand the scenario... Might need a conversation outside of this PR (not sure it directly impacts this PR either).
daq/network.py
Outdated
| subnet_str = "%s/%s" % (subnet['base_ip'], subnet['prefix_length'] | ||
| ) if subnet else DEFAULT_MININET_SUBNET | ||
| self._mininet_subnet = ip_network(subnet_str) | ||
| subnet = config.get('mininet_subnet', self.DEFAULT_MININET_SUBNET) |
proto/system_config.proto
Outdated
| // For specifying ip subnets assigned by external DHCP server | ||
| repeated IpSubnet external_dhcp_subnets = 54; | ||
| // For specifying ip subnets assigned by external DHCP server / static ips | ||
| repeated string external_subnets = 54; |
There was a problem hiding this comment.
in terms of future-proofing, can you think of any potential information (properties) we'd maybe need to keep along with the subnet? E.g., DNS server or some other property? I can't specifically think of something -- just asking the question to make sure we're considering things. In general, having this be an object (not necessarily IpSubnet but maybe something like SubnetSpec) makes the system more flexible in the future since we can then add a property to each entry. If it's encoded as a string, then there's no path forward for adding annotations.
In the past, every time I've done something like "a list of strings" then I end up regretting it because I often then want to add some kind of information to it and then you're SOL.
|
PTAL |
2 participants
No description provided.