Allow more devices per gateway setup#917
Conversation
Codecov Report
@@ Coverage Diff @@
## master #917 +/- ##
==========================================
+ Coverage 82.24% 82.44% +0.20%
==========================================
Files 46 46
Lines 5774 5857 +83
==========================================
+ Hits 4749 4829 +80
- Misses 1025 1028 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
So, the underlying code (in topology) needs the device, not just the
gateway and port set... It maintains a data structure that links devices
and gateways, and therefore ports, so it's not sufficient to pass just the
port_set, it needs the device (and device.gateway) to function properly
without *major* refactoring. So, basically the gateway ports only exist if
there is a device that references that gateway. There's no provision for a
"gateway without a device" It comes down to this function in topology.py:
def register_device(self, device):
"""Register a device with the network topology"""
LOGGER.info('Registering device %s port_set %s', device,
device.gateway.port_set)
self._populate_set_devices(device, device.gateway.port_set)
self._ensure_gateway_interfaces(device.gateway.port_set)
... which, as you see, needs both device and device.gateway.port_set
As for the name. I was intending the function to be one level up from that
specifics about what it's doing now (setting up the network ports), which
is why I went with registering. It's not necessarily allocating anything,
as such, and I don't think it should be specific to ports. Basically, it's
trying to say "prepare yourself for this device to do something" and then
do all the necessary parts of that (which includes the ports, but might
include other stuff). I could imagine changing register_device() to
prepare_device()
or prepare_for_device() if those make more sense?
…On Tue, Aug 31, 2021 at 11:02 AM henry54809 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In daq/runner.py
<#917 (comment)>:
> @@ -814,6 +816,9 @@ def _create_gateway(self, device):
gateway.set_tap_intf(self.network.tap_intf)
try:
+ if device:
+ device.gateway = gateway
+ self.network.register_device(device)
I don't think the "register_device" name is appropriate here. This
function is really for setting the network up for gateway (which should be
device independent). I think rename to self.network.allocate_gateway_ports
(or something like that) and pass in the port_set is all that's needed
here. Then you can revert line 688, 794-795 and 819-821.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#917 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIEPD6LDO4H2IEB5ITFRSDT7UKMBANCNFSM5DEPHPYQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
The "direct_device_traffic" or "direct_port_traffic" in target_set_activate essentially does the same thing, which is informing the network of this device and sets up necessary ports/ acls. There can be some consolidations so the network setups per device aren't in 2 places.
|
|
Ok yes -- good call... updated to use direct_device_traffic
|
| time.sleep(self._settle_sec) | ||
|
|
||
| def direct_device_traffic(self, device, port_set): | ||
| def direct_device_traffic(self, device): |
There was a problem hiding this comment.
I think this is gonna break FOT. By setting the arg port_set to None, the function cleans up the remote_tap.
There was a problem hiding this comment.
Ok, fixed. Verified with the new fot tests that check for this case as well.
|
Updated to not clear directories if block mode is enabled. PTAL. |
|
I think by adding a run_retention and make the block_time work across restarts can achieve your intended behavior. block_existing isn't clear on what existing thing DAQ is looking for and what is being blocked. Setting block_existing also changes whether to delete existing run data is implicit and can cause confusion.
|
config/system/ata.yaml
Outdated
| native_vlan: 122 | ||
| max_hosts: 3 | ||
| max_hosts: 10 | ||
| block_time: 10 |
There was a problem hiding this comment.
I think this and scan_interval should have a _sec postfix to match existing default_timeout_sec long_dhcp_response_sec etc. Also could there be another name for scan_interval? The name is very close to monitor_scan_sec.
|
Ok, renamed parameters and adjusted behavior! |
henry54809
left a comment
henry54809
left a comment
There was a problem hiding this comment.
Just a question about the usage of block time in non-single shot mode, everything looks good to go.
daq/base_gateway.py
Outdated
| """Discovers a host using arp-scan in a list of subnets.""" | ||
| cmd = 'arp-scan --retry=2 --bandwidth=512K --interface=%s --destaddr=%s -s %s %s' | ||
| host, intf = self._get_scan_interface() | ||
| scans = 5 |
There was a problem hiding this comment.
I will think about another way of doing this but we can leave it for now. Scanning it 5 times can take a long time.
There was a problem hiding this comment.
I updated it to make it configurable. I'm actually not sure what the right value is, so setting it down to 2 by default, and then easy to bump-up later in config if necessary. Also now possible to disable!
daq/runner.py
Outdated
| self._blocked_devices.add(device.mac) | ||
| LOGGER.info('Target device %s in now blocked (%s)', device, len(self._blocked_devices)) | ||
| block_sec = self.run_trigger.get('device_block_sec') or LONG_TIME_SEC | ||
| device.set_block(block_sec) |
There was a problem hiding this comment.
the block time should also be honored in non-single shot mode right? It looks like block time is only set here.
|
Fixed/addressed both comments (making arp_scan_count configurable). |
3 participants
No description provided.