-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
p4app v2 #54
p4app v2 #54
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@theojepsen : Can you update the top-level README.md to provide details on how to use this new version?
@robertsoule-barefoot Please see the updated top-level README |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going in the right direction. It's hard to review such a big diff but I left a few comments.
Please also consider adding Apache license headers to the files which are missing one.
sw.WriteTableEntry(table_entry) | ||
|
||
|
||
# object hook for josn library, use str instead of unicode object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo, s/josn/json
docker/scripts/p4runtime_switch.py
Outdated
self.grpc_port = P4RuntimeSwitch.next_grpc_port | ||
P4RuntimeSwitch.next_grpc_port += 1 | ||
|
||
if thrift_port is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simple_switch_grpc does not support the Thrift RPC server by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think whoever wrote the P4RuntimeSwitch
class based it on P4Switch
. As you mentioned, the simple_switch_grpc
target doesn't support Thrift by default. However, according to the target's README, it could support Thrift, so I think we should keep this code in.
docker/scripts/p4runtime_switch.py
Outdated
for _ in range(SWITCH_START_TIMEOUT * 2): | ||
if not os.path.exists(os.path.join("/proc", str(pid))): | ||
return False | ||
if check_listening_on_port(self.grpc_port): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of think that it's better to actually attempt the connection even if you have access to open ports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point of this code is to wait for the switch to start before attempting a connection. I think the original author of this code (@sethfowler ?) put it in for a reason.
@@ -0,0 +1,195 @@ | |||
#!/usr/bin/env python2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually starting to doubt that this file is used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It was copied in from p4lang/tutorials. I removed the file.
docker/scripts/p4runtime_switch.py
Outdated
info(tableEntryToString(entry)) | ||
self.insertTableEntry(entry) | ||
|
||
def insertTableEntry(self, entry=None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obviously a lot of P4Runtime functionalities are missing from this class. I imagine support can be added as time goes by, but I believe that the app should also have access to the underlying P4Runtime connection.
I forgot to mention it my review, please make sure you squash your commits before merging. |
We have received a lot of feedback about using p4app. The theme of most of the feedback is having more control over running a p4app. People are used to using Mininet, and expect the same flexibility from p4app. This was difficult with p4app's previous design, which only exposed a JSON configuration file to the user. If you wanted more functionality, you would have to hack the p4app scripts.
The new version of p4app gives you more control. Instead of writing a JSON manifest file, you write your p4app using a python script, just the way you would use mininet. p4app is now a python library that wraps-up mininet, providing helpers for compiling your P4 program and setting up the topology.
The simplest example is a switch that behaves like a wire:
https://github.com/theojepsen/p4app/blob/tj/v2/examples/wire.p4app/main.py
p4app also uses P4Runtime for configuring the switch:
https://github.com/theojepsen/p4app/blob/tj/v2/examples/ring.p4app/main.py#L32
Both P4-14 and P4-16 are supported:
https://github.com/theojepsen/p4app/blob/tj/v2/examples/compile-only.p4app/main.py
You can run different P4 programs on different switches:
https://github.com/theojepsen/p4app/blob/tj/v2/examples/two-programs.p4app/main.py#L24
You can also use P4Runtime to read counters and setup multicast groups:
https://github.com/theojepsen/p4app/blob/tj/v2/examples/counter.p4app/main.py#L13
https://github.com/theojepsen/p4app/blob/tj/v2/examples/multicast.p4app/main.py#L44