search

p4lang / p4runtime

Specification documents for the P4Runtime control-plane API
Apache License 2.0
140 stars 86 forks source link

Add metadata to multicast group entry #446

Closed verios-google closed 8 months ago

verios-google commented 11 months ago

We have a use case for this. The change is small, backwards compatible, and consistent with table entries.

chrispsommers commented 11 months ago

Looks like you have some trailing whitespace in the .mdk and you need to regenerate the protobuf code, see instructions at bottom of this section: https://github.com/p4lang/p4runtime#detailed-processes. Thanks.

jafingerhut commented 10 months ago

@verios-google @smolkaj Do you use Stratum P4Runtime API implementation code in your work? If yes, am I correct in guessing that implementing these changes would require some (probably small) changes in Stratum to accomplish this? Are you willing to do that?

jafingerhut commented 10 months ago

Same question for whether these changes might require implementation code changes in this repo: https://github.com/p4lang/PI

smolkaj commented 10 months ago

@verios-google @smolkaj Do you use Stratum P4Runtime API implementation code in your work? If yes, am I correct in guessing that implementing these changes would require some (probably small) changes in Stratum to accomplish this? Are you willing to do that?

No, we do not use Stratum. So we'd kindly ask that people interested in that project extend it if desired. Any idea who the main people working on Stratum these days might be?

By the way, our P4Runtime server is part of SONIC and you can find it here: https://github.com/sonic-net/sonic-pins. We will be extending it to support this.

We do use BMv2, so extending https://github.com/p4lang/PI seems like a reasonable ask. @antoninbas any ideas how much work this may be?

jafingerhut commented 10 months ago

@saynb Are you the right person to ask about Stratum developer contacts?

@smolkaj I think it is perfectly reasonable to ask Antonin about development effort required to make such a change in p4lang/PI, but I would recommend that serious thought be given to the idea that someone besides Antonin actually implements it. Otherwise, the base of developers who know how to make such changes will not be growing.

antoninbas commented 10 months ago

We do use BMv2, so extending https://github.com/p4lang/PI seems like a reasonable ask. @antoninbas any ideas how much work this may be?

This is very simple to implement. Should be about the same scope as your recent PR (https://github.com/p4lang/PI/pull/598).

The metadata binary string can probably be added directly to the existing Group struct: https://github.com/p4lang/PI/blob/e86801b69a51fc3462ed67ac2f7fa6762e827460/proto/frontend/src/pre_mc_mgr.h#L92

smolkaj commented 10 months ago

but I would recommend that serious thought be given to the idea that someone besides Antonin actually implements it. Otherwise, the base of developers who know how to make such changes will not be growing.

Sure, I agree. I imlemented https://github.com/p4lang/PI/pull/598 just a few days ago.

Thanks @antoninbas for the quick effort estimation & implementation idea.

saynb commented 10 months ago

@saynb Are you the right person to ask about Stratum developer contacts?

I am not the primary person but I can help connect the right people in addition to me. ipdk-stratum : @ffoulkes @vsureshkumarp would be the contacts.

ONF stratum : Probably @pudelkoM ?

pudelkoM commented 10 months ago

While most of us don't work at ONF anymore, I can certainly take a look at any potential PR and merge it.

jafingerhut commented 10 months ago

It appears that automated CI checks for this PR never completed. I do not know why.

Likely one of those checks that would fail if it had completed is one for checking in the auto-generated output files from the modified .proto files in your PR. Instructions for regenerating those files can be found near the end of this section of the README: https://github.com/p4lang/p4runtime#detailed-processes

Committing such updated files should trigger CI checks to re-run (or any other commit to this PR).

chrispsommers commented 10 months ago

It appears that automated CI checks for this PR never completed. I do not know why.

Likely one of those checks that would fail if it had completed is one for checking in the auto-generated output files from the modified .proto files in your PR. Instructions for regenerating those files can be found near the end of this section of the README: https://github.com/p4lang/p4runtime#detailed-processes

Committing such updated files should trigger CI checks to re-run (or any other commit to this PR).

See my comment https://github.com/p4lang/p4runtime/pull/446#issuecomment-1675433135