Skip to content
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

Try to support wildcard PacketReplicationEngine reads. #610

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fruffy
Copy link
Contributor

@fruffy fruffy commented Mar 21, 2024

Fixes #609.

@antoninbas
Copy link
Member

I am not sure #609 is a bug according to the spec.

A request may omit or use default values for parts of the entity key to achieve wildcard behavior. Please refer to the P4 Entity Messages section for details on what parts of the entity can be wildcarded in a given request.

It is mostly up to individual entities to define what a wildcard read means. For the PacketReplicationEngineEntry entity, wildcard reads separate multicast and clone entries.

One issue with that change is that if we ever add a new PRE entry type (by adding a new field to the oneof), it will not be possible to distinguish between an unset oneof (which after this change would mean wildcard read of all PRE entries regardless of type) and a oneof set to a field which is not (yet) recognized by the server. IMO, this breaks compatibility (between new client and old server).

We actually discuss this issue with oneof in this very section (https://p4.org/p4-spec/docs/p4runtime-spec-working-draft-html-version.html#sec-wildcard-reads), and we link to https://protobuf.dev/programming-guides/proto3/#backward

@smolkaj
Copy link
Member

smolkaj commented Mar 21, 2024

Very interesting. I wasn't aware of this subtle backward-compatibility issue, here and in general for protos.

@smolkaj
Copy link
Member

smolkaj commented Mar 21, 2024

@jonathan-dilorenzo and I discussed this an agree that NOT_SET is an absurd name for an enum value that encodes "unset" or "set but to an unknown oneof case". Maybe proto4 will get this right....

@jonathan-dilorenzo
Copy link

I don't think that's a particularly reasonable read of the specification @antoninbas, though this suggests we should clarify it. The main wildcard read section pretty clearly suggests that any empty top-level entity should read all entities of that type with this example:

device_id: <ID>
entities {
  extern_entry { }  # read all extern instances for all supported extern types
  table_entry { }  # read all table entries for all tables
  action_profile_member { }  # read all members for all action profiles
  action_profile_group { }  # read all groups for all action profiles
  ...
}

And the TableEntry section on Wildcard reads only mentions a possibility that provides way more fields:

device_id: 3
entities {
  table_entry {
    table_id: 0
    priority: 0
    controller_metadata: 0
    metadata: ""
  }
}

So it would be reasonable to expect that the full wildcard case (where you specify almost nothing) was similarly left out of the PacketReplicationEngineEntry section.

That said, I agree that this is actually an unacceptable interpretation based on the backwards compatibility issue. Which either would mean that the server should return everything if it doesn't know to return something specific, or (as we did with entities) that any oneof must always be set (if it's at the next level of an entity you want to read I guess).

@jonathan-dilorenzo
Copy link

Opened up an issue against the P4Runtime to discuss this: p4lang/p4runtime#476 and fix the specification in some direction before submitting this PR.

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.

PacketReplicationEngineEntry does not support wild card reads
4 participants