search

p4lang / p4runtime

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

Allow for SDN ports that are not usable as watch ports. #400

Closed jonathan-dilorenzo closed 1 year ago

jonathan-dilorenzo commented 2 years ago

@rhalstea and I were hoping that we could amend the specification to have some allowance for SDN ports that are not usable as watch ports. I don't know precisely what the concerns would be with this, so I figured I'd create a quick PR to get the discussion started.

Probably, we would, at least, want to add some information about what constitutes 'usable' in this context. I'm imagining that there would be some outside-of-P4RT notion of informing the controller which ports are valid for watch ports vs not (as I think is already the case for SDN ports in general?).

Should we discuss this in a meeting? Are there particular concerns related to having these two different types of SDN ports basically?

rhalstea commented 2 years ago

Adding some color ... we're thinking about LAGs (static or dynamic) here. "SDN ports" seems to be defined broad enough to encompass LAGs, but AFAIK they are not usually allowed in watch_ports since their state isn't handled by the hardware layer.

smolkaj commented 2 years ago

Suggestion: Instead of introducing a new notion of "usable" ports, how about simply saying in the spec that targets may not support all ports as watch_ports, giving an example justification, and specifying what targets should do in that case.

antoninbas commented 2 years ago

+1 to @smolkaj's comment AFAIK, there is no requirement that watch_port be implemented in hardware, hence some targets may support logical ports (including LAGs) as watch_ports.

rhalstea commented 2 years ago

Sounds good. Any objection to the following wording for the last sentence?

The value must be empty or the SDN port of an existing port on the device. If the value is not an SDN port, or the device doesn't support the SDN port (e.g. on some platforms LAGs cannot be used) as a watch port the server must return INVALID_ARGUMENT.

smolkaj commented 2 years ago

Should we perhaps prefer UNIMPLEMENTED when the port is not supported?

My theory would be that INVALID_ARGUMENT is reserved for cases that would be invalid across all targets. @antoninbas would know better if this is in line with current usage of those error codes though.

antoninbas commented 1 year ago

Yes, UNIMPLEMENTED would be the most appropriate choice here IMO

jonathan-dilorenzo commented 1 year ago

SGTM! Modified! PTAL.

smolkaj commented 1 year ago

Looks you accidentally combined the changes here with some WCMP changes?

jonathan-dilorenzo commented 1 year ago

Ugh. Annoying mixture of Critique and Git... Thanks! Should be fixed now!