Skip to content

Conversation

angelazhang8
Copy link
Contributor

Motivation

Our switches currently support per-group hash modes and resource usage modes. We are currently not leveraging this capability. This P4 API change allows for more granular control of the WCMP implementation in GPINS.

Summary of changes

Add BucketSelectionMode (with modes DEFAULT_MODE_DETERMINED_BY_ACTION_SELECTOR, HASH, and RANDOM) and SizeSemantics (with modes DEFAULT_SIZE_DETERMINED_BY_ACTION_SELECTOR, SUM_OF_WEIGHTS, and SUM_OF_MEMBERS) to ActionProfileActionSet in p4runtime.proto.

@chrispsommers
Copy link
Collaborator

You still need to update the generated code, right?

@jonathan-dilorenzo
Copy link
Contributor

Suggested a bunch of comments, otherwise looks great (once generated code is updated too).

@jonathan-dilorenzo
Copy link
Contributor

jonathan-dilorenzo commented Aug 13, 2025

Ok! This LGTM! @verios-google, @matthewtlam, @jafingerhut, and @chrispsommers, do we have any idea what's causing the ubuntu-22.04,latest build to fail? And whether it's passing at head? It looks like there's some weird issue with protobufs for Go based on my cursory read.

Edit: I guess not since #559 fails that build too and doesn't change the proto at all? So then, do we feel comfortable merging this @jafingerhut and @chrispsommers?

@matthewtlam
Copy link
Contributor

Ok! This LGTM! @verios-google, @matthewtlam, @jafingerhut, and @chrispsommers, do we have any idea what's causing the ubuntu-22.04,latest build to fail? And whether it's passing at head? It looks like there's some weird issue with protobufs for Go based on my cursory read.

Edit: I guess not since #559 fails that build too and doesn't change the proto at all? So then, do we feel comfortable merging this @jafingerhut and @chrispsommers?

I think it makes sense to just merge this PR. It seems that this build has been failing since Jun 23 https://github.com/p4lang/p4runtime/actions/runs/15837959752.

The build fails out of the blue upon taking a closer look as we can see @verios-google's last change and a series of passing scheduled builds.
Screenshot 2025-08-13 at 9 04 54 AM

Copy link
Contributor

@matthewtlam matthewtlam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proto changes LGTM

@jafingerhut
Copy link
Contributor

FYI, I think Google folks are still the most vocal proponents of using Bazel for builds, so I'd ask if anyone there would be interested in looking into why the builds are failing for that particular Ubuntu/Bazel version combination, and see whether something should be changed there.

Copy link
Contributor

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

The new additions look reasonable to me. Looking forward to reviewing the corresponding additions to the P4Runtime spec that go with these (on a different PR).

@jonathan-dilorenzo
Copy link
Contributor

Alright, let me open an issue for the Bazel stuff and we can take a look at that separately. @jafingerhut or @chrispsommers, could I ask you to merge this?

@chrispsommers
Copy link
Collaborator

#562

@chrispsommers chrispsommers merged commit baaf393 into p4lang:main Aug 14, 2025
7 of 8 checks passed
@jafingerhut
Copy link
Contributor

I see that @chrispsommers has already approved the proto changes in the corresponding earlier PR #560 so will go ahead and merge this despite it failing one of the CI tests. I am guessing that likely that failing CI test will be either (a) updated to work fine on this PR's changes, or (b) someone will discover that Ubuntu/Bazel version combo isn't supported, and therefore should no longer be a CI test for this repo.

@jonathan-dilorenzo
Copy link
Contributor

Thanks to both of you!

angelazhang8 added a commit to angelazhang8/p4runtime that referenced this pull request Sep 12, 2025
…fileActionSet in P4Runtime. (p4lang#561)

* Create a separate PR for just the P4Runtime proto changes.

Signed-off-by: Angela Zhang <[email protected]>

* address comments

---------

Signed-off-by: Angela Zhang <[email protected]>
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.

5 participants