search

p4lang / tutorials

P4 language tutorials
Apache License 2.0
1.33k stars 878 forks source link

Cleanup usages of simple_switch_CLI #573

Closed stano45 closed 5 months ago

stano45 commented 5 months ago

Issue for context: https://github.com/p4lang/tutorials/issues/99

simple_switch_CLI is currently used by 4 files:

  1. utils/run_exercise.py
  2. utils/p4apprunner.py
  3. utils/mininet/appcontroller.py
  4. utils/mininet/single_switch_mininet.py

The usage within run_exercise.py depends on whether the cli_input field is set in the switch configuration (topology.json). https://github.com/p4lang/tutorials/blob/ee16fbde2a4c69ae66220a8112625b1b1559d61d/utils/run_exercise.py#L308

This field not configured in any of the topology files of any exercise in this repo, therefore it is safe to delete. Instead, the runtime_json field is set for all switches. The only exception is the p4runtime exercise, where the switches are programmed using a python controller.

I added a warning for the case that runtime_json is not set in the switch config to inform the user whenever a runtime configuration is missing for a switch.

In run_exercise.py, simple_switch_CLI is also mentioned as a message when starting the mininet CLI. Based on the simple_switch_CLI docs, this could still be useful for someone wanting to inspect the configurations or play around with the switches at runtime.

Just to make sure everything still works, I ran all exercises and compared the results. Everything still works as expected.

Interestingly though, I also found that the p4apprunner.py, appcontroller.py, single_switch_mininet.py and multi_switch_mininet.py are not used in any exercise, nor are they referenced in any readmes/guides (I ran all exercises after deleting these files). I am unsure about their purpose so I left them untouched in this PR. @jafingerhut can you please provide context on what these files are/were used for?

rst0git commented 5 months ago

This field not configured in any of the topology files of any exercise in this repo, therefore it is safe to delete.

Interestingly though, I also found that the p4apprunner.py, appcontroller.py, single_switch_mininet.py and multi_switch_mininet.py are not used in any exercise, nor are they referenced in any readmes/guides (I ran all exercises after deleting these files). I am unsure about their purpose so I left them untouched in this PR.

@stano45 These files and the cli_input functionality are provided as an example implementation for newcomers to experiment with different features of BMv2. While not explicitly used, they can still be utilized to explore different features of BMv2. It might be better to add documentation instead of remove it.

jafingerhut commented 5 months ago

Maybe it would be worth adding in one of the exercises a documented example of using this feature? I do not know off-hand which exercise to recommend, but ideally one where there are simple_switch_CLI configurations that are useful to make, that cannot be done through P4Runtime API, e.g. setting queue rates in the traffic manager.

stano45 commented 5 months ago

@rst0git @jafingerhut Thanks a lot for the context on this. In my opinion, setting the queue depth and rate fits the best in the mri exercise, where the goal is to observe the queue depth of each switch on a packet's route. I opened a new PR for clarity. PTAL

stano45 commented 5 months ago

Closed in favor of: https://github.com/p4lang/tutorials/pull/576