Target port alternative for switch tests

[codecov[bot]]

Codecov Report

Merging #953 (e7c7aa4) into master (59e9ba4) will decrease coverage by 0.16%. The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master     #953      +/-   ##
==========================================
- Coverage   82.14%   81.98%   -0.17%     
==========================================
  Files          47       47              
  Lines        5769     5773       +4     
==========================================
- Hits         4739     4733       -6     
- Misses       1030     1040      +10     

Flag	Coverage Δ
ata	62.46% <69.23%> (-0.49%)	:arrow_down:
aux	67.37% <76.92%> (-0.03%)	:arrow_down:
base	65.59% <76.92%> (-0.05%)	:arrow_down:
dhcp	66.69% <76.92%> (+<0.01%)	:arrow_up:
many	66.38% <76.92%> (-0.25%)	:arrow_down:
mud	71.40% <76.92%> (+<0.01%)	:arrow_up:
switch	67.07% <76.92%> (-0.02%)	:arrow_down:
topo	65.84% <76.92%> (+<0.01%)	:arrow_up:
unit	32.63% <15.38%> (-0.01%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
daq/proto/session_server_pb2.py	100.00% <ø> (ø)
daq/runner.py	83.91% <62.50%> (-0.16%)	:arrow_down:
daq/host.py	91.07% <100.00%> (-0.28%)	:arrow_down:
daq/topology.py	95.29% <100.00%> (ø)
daq/stream_monitor.py	87.60% <0.00%> (-2.48%)	:arrow_down:
daq/tcpdump_helper.py	78.57% <0.00%> (-2.39%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 59e9ba4...e7c7aa4. Read the comment docs.

[noursaidi]

This line which isn't part of this PR seems to hardcode the system configuration file which is different to the local/system.yaml - that needs updating so it uses the system configuration file

[anurag6]

There's still something weird about how you're passing the device port around... The "device port" concept is universal and should be consistent across all modes. I think the trick is figuring out how to signal the logic of what to do about things, but it's not a "device" consideration.

I think I disagree. In the future if device_coupler gets the capability of managing/viewing the access switch through gNMI, it would be device_coupler that knows what access port the device is on. I think it makes sense to populate it at the device_coupler level and pass it on.

[grafnu]

I don't see how that changes the basic assertion: Each device has a port, there is a field in the device structure that says what port it's on, so when there is a known port then that field should be set correctly. The problem is your current solution bleeds the abstraction into places it shouldn't go, and I'm not convinced by the speculative future use case... Maybe in the future the system doesn't know anything about the port, but that still doesn't argue for having the port encoded in weird ways now.

On Tue, Mar 1, 2022 at 3:14 PM Anurag Porripireddi @.***> wrote:

There's still something weird about how you're passing the device port around... The "device port" concept is universal and should be consistent across all modes. I think the trick is figuring out how to signal the logic of what to do about things, but it's not a "device" consideration.

I think I disagree. In the future if device_coupler gets the capability of managing/viewing the access switch through gNMI, it would be device_coupler that knows what access port the device is on. I think it makes sense to populate it at the device_coupler level and pass it on.

— Reply to this email directly, view it on GitHub https://github.com/faucetsdn/daq/pull/953#issuecomment-1055956751, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIEPDZHB7JGATJQN5SPPDTU52QE3ANCNFSM5PCN72TQ . 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.

You are receiving this because you commented.Message ID: @.***>

[grafnu]

Ok -- please just add a clarifying comment (just what you said here)

On Tue, Mar 1, 2022 at 5:01 PM Anurag Porripireddi @.***> wrote:

@.**** commented on this pull request.

In device_coupler/start_daq https://github.com/faucetsdn/daq/pull/953#discussion_r817264186:

@@ -6,4 +6,4 @@ ENDPOINT_CFG=/tmp/startup.yaml DAQ_CONFIG=device_coupler/config/daq_config.yaml echo "site_description: generated" > $ENDPOINT_CFG yq e -n ".switch_setup.endpoint.ip=\"$docker_br_ip\"" >> $ENDPOINT_CFG -sudo PYTHONPATH=$daq_path:$daq_path/faucet:$daq_path/forch:$daq_path/mininet PATH=$PATH:$daq_path/mininet FAUCET_EVENT_SOCK=$daq_path/inst/faucet_event.sock cmd/start $DAQ_CONFIG $ENDPOINT_CFG "$@" +sudo PYTHONPATH=$daq_path:$daq_path/faucet:$daq_path/forch:$daq_path/mininet PATH=$PATH:$daq_path/mininet FAUCET_EVENT_SOCK=$daq_path/inst/faucet_event.sock cmd/start $DAQ_CONFIG $ENDPOINT_CFG "$@" || true

TimeoutError. It happens in test_aux as well and it doesnt fail since -e isn't set. It istesting the timeout and expects it to happen.

— Reply to this email directly, view it on GitHub https://github.com/faucetsdn/daq/pull/953#discussion_r817264186, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIEPD4X2PJR5KPFNKOQD4DU524VHANCNFSM5PCN72TQ . 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.

You are receiving this because you commented.Message ID: @.***>