Defining behavior for OneOf's in reads.

[jonathan-dilorenzo]

Spawning off an issue based on @antoninbas's comment on a BMv2 PR and my subsequent response.

Basically, at the moment, I believe that the Wildcard Reads section suggests that a wildcard read like this should be supported:

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
  ...
  packet_replication_engine_entry { }  # read all packet replication engine entries
}

And return, among other things, all packet replication engine entries. This is based on the simplest extrapolation from the example, which looks identical, but without the packet_replication_engine_entry line.

However, @antoninbas brought up the great point that packet_replication_engine_entry { some_as_of_yet_undefined_field_that_is_NOT_part_of_the_type_one_of } is an indistinguishable message from packet_replication_engine_entry { some_as_of_yet_undefined_field_that_is_part_of_the_type_one_of } (as per https://protobuf.dev/programming-guides/proto3/#backward), which is also what we mention as the reason why a full wildcard read can't just be:

device_id: <ID>
entities { }

So, I think we need to clarify this in some direction. Here are 3 possible ways to deal with this:

• Disallow reads with unknown fields. This would also allow for the simpler wildcard read request, but means that clients need to know what version of P4Runtime their server is using.
• Allow unknown fields, but ignore them AND treat oneofs as being unset if no known field belongs to the oneof. This would also allow the simpler wildcard read requests, and would allow clients to send any reads they want, but if they fill in a oneof within an entity type A, they now need to expect getting ONLY that returned as a result (for entities of type A) or all other entities of type A.
• Disallow unset oneofs in reads unless they are a submessage of another unset message (e.g. if I don't have a packet_replication_engine_entry at all, then I don't need to fill in its oneof). This seems to be what BMv2 does now (at least for the situation above). Probably here we would still allow unknown fields, but ignore them? This is consistent with the current semantics for entity reads, and clients can expect the result of their reads to include EXACTLY what they requested (since presumably the switch doesn't contain any entities of a type it is unaware of). It's a little complicated though.

Note: Presumably for writes, we'd expect servers to not accept anything containing unknown fields, and therefore this is a non-issue?

[smolkaj]

Disallow reads with unknown fields.

Should it be clear how to implement that? I thought the whole point was that the client-side use of unknown oneof fields was not observable on the server-side?

[jonathan-dilorenzo]

Sorry, with ANY unknown fields is what I meant. Which is why that's so restrictive.

[smolkaj]

Still don't understand how this can be implemented.

[jonathan-dilorenzo]

On the switch side? The standard proto parser has a configuration saying whether it should allow unknown fields or not. If you say 'false' that would be most of your implementation (barring sending back a good error message).

[smolkaj]

The standard proto parser has a configuration saying whether it should allow unknown fields or not.

I am aware of such an option for the text format parser, but I haven't seen it for the binary parser, where parsing/deparsing is generally abstracted away by gRPC scaffolding code and the gRPC service implementation works on parsed messages. Is there such an option for the binary parser, and can it be set through the C++ grpc scaffolding code?

[jonathan-dilorenzo]

After further discussion with Steffen offline, we found that protos have an UnknownFieldSet, which should get populated by GRPC. I.e. for some ReadRequest request, in C++, it should be possible to see if it has any unknown fields with !request.unknown_fields().empty().

With that, I think we're leaning towards option 1 of disallowing unknown fields. Planning to discuss in next API WG meeting. Any thoughts @antoninbas, @jafingerhut, and @chrispsommers?

[chrispsommers]

My head is spinning a little bit, I think it'll be easier to discuss in the next WG meeting. Thanks!

[jonathan-dilorenzo]

One note that @zhenk14 pointed out:

For servers with multiple controllers, if one controller is older than the server, option 1 would mean that the controller may also receive unknown entities back from a read request.

[smolkaj]

Could you provide a concrete example to help us understand this?

[jafingerhut]

Have you considered introducing a new style of wildcard read requests, unlike the ones that exist in today's spec, e.g. add a new optional boolean field "doWildCardReadRequest" as a field or sub-message inside of the entity on which you wish to perform a wildcard read request? The main reason I ask is that it seems like it might be less tricky to reason about for forwards/backwards compatibility corner cases.

[jonathan-dilorenzo]

Could you provide a concrete example to help us understand this?

I think 'this' here is referring to my comment:

For servers with multiple controllers, if one controller is older than the server, option 1 would mean that the controller may also receive unknown entities back from a read request.

If so, sure! Otherwise, lmk.

Imagine you have two controllers C1 and C2 talking to a server S. C1 uses the same version of the P4Runtime proto as S, which contains PacketReplicationEngineEntries and C2 uses an older version that doesn't. Consider the following steps:

1. C1 connects to S and becomes primary.
2. C1 programs a MulticastGroupEntry on S.
3. C2 connects to S and becomes primary.
4. C2 reads back all entries on S. Now, C2 receives a MulticastGroupEntry back, which, to it, is an unknown field.

Have you considered introducing a new style of wildcard read requests, unlike the ones that exist in today's spec, e.g. add a new optional boolean field "doWildCardReadRequest" as a field or sub-message inside of the entity on which you wish to perform a wildcard read request? The main reason I ask is that it seems like it might be less tricky to reason about for forwards/backwards compatibility corner cases.

I hadn't, but generally I would say that just adding yet another way to do something creates unfortunate clutter... Not crazy, but I think should generally be the last choice. In this case, we can work with the semantics as it is now, it's just not crystal clear what that semantics actually is. The current (possibly intended?) semantics, I think, is by far the trickiest one to explain (it's option 3 above), and adding yet another version wouldn't simplify that.

[smolkaj]

Thanks for the example, that makes sense.

[smolkaj]

Have you considered introducing a new style of wildcard read requests, unlike the ones that exist in today's spec, e.g. add a new optional boolean field "doWildCardReadRequest" as a field or sub-message inside of the entity on which you wish to perform a wildcard read request? The main reason I ask is that it seems like it might be less tricky to reason about for forwards/backwards compatibility corner cases.

There is additional context on this suggestion here: #462

[jonathan-dilorenzo]

From May meeting:

• We all prefer option 1 where we disallow reads with unknown fields.
• The read request shown in the spec is actually not valid (because there are multiple oneofs set in a single entities message).
• AI: Jonathan will open a PR to change reads to disallow unknown fields.

@jafingerhut and @antoninbas, does that sound like a good way to go to you too? Any concerns?

[antoninbas]

Assuming there is an elegant way to do it in the server implementation, option 1 sounds reasonable to me.
I think we can compare it to how a gRPC server would handle an unknown RPC, which was added later on. IIRC, the server will return UNIMPLEMENTED.

With this decision, will there also be a change to the semantics of wildcard reads? I.e., will an empty Entity message to read all known entities be supported?

[jonathan-dilorenzo]

Excellent! Yeah, I think returning UNIMPLEMENTED makes a lot of sense. I mentioned an elegant way (in C++ at least) a bit above, but let me add it here too:

I.e. for some ReadRequest request, in C++, it should be possible to see if it has any unknown fields with !request.unknown_fields().empty().

With this decision, will there also be a change to the semantics of wildcard reads? I.e., will an empty Entity message to read all known entities be supported?

Yes! That's exactly the goal. In general, unset oneofs will be treated as wildcards for that field, which is safe after this change.

[smolkaj]

@jonathan-dilorenzo, would we want to push on this before 1.4.0? If so we should add the label.

[jonathan-dilorenzo]

Good question, I'm not sure when I could plausibly write this up, though perhaps in a few months. What's the timeline for 1.4.0?

And/or does someone else have time to make these changes?

[smolkaj]

We haven't yet set a deadline, but I propose end of September: #480 (comment)

[chrispsommers]

@jonathan-dilorenzo if you want in 1.4.0 please add label and follow-up (Sept. 13 deadline)