8000 Update Flowcache exercise by Dscano · Pull Request #679 · p4lang/tutorials · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 10 commits into from
Apr 20, 2025
Merged

Update Flowcache exercise #679

merged 10 commits into from
Apr 20, 2025

Conversation

Dscano
Copy link
Contributor
@Dscano Dscano commented Apr 1, 2025
  1. Add Flowcache to main README.
  2. Update the Flowcache README and P4 according to the code style used in basic.p4

Signed-off-by: Dscano <d.scano89@gmail.com>
@jafingerhut
Copy link
Collaborator

My plan is to wait until you say you want me to review it (it is fine if that time is now).

@Dscano
Copy link
Contributor Author
Dscano commented Apr 1, 2025

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.

Dscano added 7 commits April 6, 2025 16:08
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>
@Dscano
Copy link
Contributor Author
Dscano commented Apr 16, 2025

@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.

@jafingerhut
Copy link
Collaborator

The units of idle_timeout_ns when installing entries in a table with support_timeout = true is nanoseconds. You might want to try adding a line in your Python program like this:

NSEC_PER_SEC = 1000 * 1000 * 1000

and then later when you add the table entry into that table:

idle_timeout_ns = 3 * NSEC_PER_SEC

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.

@jafingerhut
Copy link
Collaborator

@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>
@Dscano
Copy link
Contributor Author
Dscano commented Apr 17, 2025

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.
Copy link
Collaborator

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?

Copy link
Contributor Author
@Dscano Dscano Apr 17, 2025

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

@jafingerhut
Copy link
Collaborator

What do you think of having mycontroller.py delete entries in the flow_cache table in response to receiving idle timeout notifications of an entry, since it hasn't been used in a while?

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 mycontroller.py that uses those features. In my limited testing of the versions of the code in the solutions directory, it seems to be doing something reasonable.

traceback.print_exc()
await asyncio.sleep(2)

async def processPacket(message):
Copy link
Collaborator

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.

@jafingerhut
Copy link
Collaborator

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
(2) the controller gets the PacketIn, does what is needed to add a new table entry to s1's flow_cache table, and also sends the packet via PacketOut to s1, directed to the correct output port that the packet should go to.
(3) s1 receives the PacketOut message, and sends it out the correct output port towards h2, without looking anything up in the flow_cache table.

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?

Copy link
Collaborator
@jafingerhut jafingerhut left a 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.

@jafingerhut jafingerhut merged commit ab29d06 into p4lang:master Apr 20, 2025
1 check passed
@Dsca
8000
no
Copy link
Contributor Author
Dscano commented Apr 21, 2025

I used asyncio because I wanted to try it, but it's my first time. Moreover, I'm trying to address your comments

@jafingerhut
Copy link
Collaborator

@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.

@Dscano
Copy link
Contributor Author
Dscano commented Apr 21, 2025

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0