-
Notifications
You must be signed in to change notification settings - Fork 907
Update Flowcache exercise #679
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
Conversation
Signed-off-by: Dscano <d.scano89@gmail.com>
My plan is to wait until you say you want me to review it (it is fine if that time is now). |
I have to push other fixes, so I'll ping you when there's a reasonable amount of "new code" ready. |
Signed-off-by: Dscano <d.scano89@gmail.com>
Signed-off-by: Dscano <d.scano89@gmail.com>
Signed-off-by: Dscano <d.scano89@gmail.com>
Signed-off-by: Dscano <d.scano89@gmail.com>
Signed-off-by: Dscano <d.scano89@gmail.com>
Signed-off-by: Dscano <d.scano89@gmail.com>
Signed-off-by: Dscano <d.scano89@gmail.com>
@jafingerhut When you want, feel free to do a first round of review. I still need to test the idle timeout setting, since I didn’t receive any notification about it in the control plane, and I’m not sure why that’s happening. |
The units of
and then later when you add the table entry into that table:
When I ran your P4 program and mycontroller.py program with that change, I did not see any timeout messages generated while the ping was running between hosts. When I stopped the ping, a few seconds later the idle timeout messages started being printed in the window where mycontroller.py was running. |
@Dscano I read through the flowcache.p4 source, and it looks good to me. No suggestions for any changes that I could find. |
Signed-off-by: Dscano <d.scano89@gmail.com>
Thanks for the tip and support, @jafingerhut. I’ve pushed the final version of the exercise. |
Signed-off-by: Dscano <d.scano89@gmail.com>
@@ -131,14 +116,15 @@ seen by your P4 parser. | |||
The `mycontroller.py` file is a basic controller plane that does the following: | |||
1. Establishes a gRPC connection to the switches for the P4Runtime service. | |||
2. Pushes the P4 program to each switch. | |||
3. Writes tunnel ingress and tunnel egress rules for two tunnels between h1 and h2. |
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 section title "Step 2: Implement Tunnel Forwarding" looks like it needs updating for this exercise?
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.
correct, I have addressed it
What do you think of having mycontroller.py delete entries in the I'm not very familiar with using asyncio in Python, so I don't know when I would have time to do a good solid review of |
traceback.print_exc() | ||
await asyncio.sleep(2) | ||
|
||
async def processPacket(message): |
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 line shortly after this comment:
print("Received %d PacketIn messages" % (len(payload)))
is confusing. When I see "Received 52 PacketIn messages" printed by mycontroller.py, it suggests that it received 52 separate PacketIn messages, when in fact it has received only 1, with a packet length of 52 bytes.
A question on the current behavior: If I send only 1 packet from h1 to h2 and then send no more, would you expect it to be received by h2? I would hope that the system behaves roughly like this: (1) s1 receives the packet, looks it up in flow_cache, and on a miss sends a copy via PacketIn to the controller Steps (4) through (6) would be the same, except for the next switch on the path from h1 to h2, probably switch s2. So the latency of the first packet from h1 to h2 would be quite high, but it should eventually get there. When I try to send a single packet from h1 to h2, I see that the controller does get a PacketIn message from s1, but I don't think it ever gets one from s2. What do you think? |
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.
Looks like this is mostly here, so approving and merging in. I will create separate issues or PRs as desired for future fixes or enhancements.
I used asyncio because I wanted to try it, but it's my first time. Moreover, I'm trying to address your comments |
@Dscano Understood. With the changes in this PR, I noticed that if I send only one packet from h1 to h2, it goes through, which is good. If I then wait 3 seconds until the table entry added sends an idle timeout notification, I see several messages output by the mycontroller.py program that it is receiving length 0 PacketIn messages. I think that is a sign of a bug in Python code in the tutorials repo, where it is creating an empty PacketIn message and returning it for processing to mycontroller.py, when instead it should not be returning any PacketIn message at all. |
Indeed, I noticed it. Thank you for your suggestion. I’m going to investigate the Python code in depth and will make a separate PR to fix it. |
README
.README
and P4 according to the code style used inbasic.p4