Extending the spec with one shot action selector programming #42
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
requested review from
ghost
mentioned this pull request
teverman
suggested changes
Aug 22, 2018
proto/p4/v1/p4runtime.proto
Outdated
| @@ -264,6 +265,16 @@ message Action { | |||
| repeated Param params = 4; | |||
| } | |||
|
|
|||
| message ActionProfileActions { | |||
| repeated ActionProfileAction action_profile_action = 1; | |||
There was a problem hiding this comment.
Per protobuf style guide, repeated field names should be plural.
docs/v1/P4Runtime-Spec.mdk
Outdated
| `ActionProfileMember` and `ActionProfileGroup` messages. Programming | ||
| some entries with one shots, and other entries with | ||
| `ActionProfileMember` and `ActionProfileGroup` messages is not | ||
| allowed. |
There was a problem hiding this comment.
What error message does server produce in this case?
docs/v1/P4Runtime-Spec.mdk
Outdated
| `ActionProfileMember` and `ActionProfileGroup` messages. Programming | ||
| some entries with one shots, and other entries with | ||
| `ActionProfileMember` and `ActionProfileGroup` messages is not | ||
| allowed. |
There was a problem hiding this comment.
Is a server free to support only the one-shot method?
There was a problem hiding this comment.
I would say yes, the spec should say that
- a server MUST support the one shot method
- it is recommended for a server to support both methods
I think the "MUST" for the first bullet point is justified by the fact that if you support the id-based method there is no reason not to be able to support the one shot method.
antoninbas
requested changes
Aug 22, 2018
docs/v1/P4Runtime-Spec.mdk
Outdated
| P4Runtime supports syntactic sugar to program a table, which is | ||
| implemented with an action selector, in one shot. This means that a | ||
| table entry, an action profile group, and a set of action profile | ||
| members can programmed with a single update message. Using one shots |
docs/v1/P4Runtime-Spec.mdk
Outdated
| implemented with an action selector, in one shot. This means that a | ||
| table entry, an action profile group, and a set of action profile | ||
| members can programmed with a single update message. Using one shots | ||
| has the advanatage that the controller does not need to keep track of |
There was a problem hiding this comment.
typo: "s/advanatage/advantage"
docs/v1/P4Runtime-Spec.mdk
Outdated
|
|
||
| * `watch` is the controller defined 32-bit port number that the | ||
| action's liveness depends on. At runtime, the action must be | ||
| excluded from selection if the this watch port is down. |
docs/v1/P4Runtime-Spec.mdk
Outdated
| excluded from selection if the this watch port is down. | ||
|
|
||
| Semantically, one shots are equivalent to programming the table entry, | ||
| group, and members individually. The necessary group id and member ids |
There was a problem hiding this comment.
I wouldn't say necessary, it's just one possible / likely way of implementing it
There was a problem hiding this comment.
I meant for the "The necessary ids ..." to be part of the semantics. I've changed the text to clarify this.
docs/v1/P4Runtime-Spec.mdk
Outdated
| are bound to unused ids. | ||
|
|
||
| For example, consider again the action selector defined in the | ||
| beginning of this section. This table could be programmed with the |
There was a problem hiding this comment.
Maybe add a reference to the code block instead to be clearer (see http://madoko.org/reference.html#sec-ids-labels)
docs/v1/P4Runtime-Spec.mdk
Outdated
|
|
||
| For example, consider again the action selector defined in the | ||
| beginning of this section. This table could be programmed with the | ||
| following oneshot update: |
docs/v1/P4Runtime-Spec.mdk
Outdated
| `ActionProfileMember` and `ActionProfileGroup` messages. Programming | ||
| some entries with one shots, and other entries with | ||
| `ActionProfileMember` and `ActionProfileGroup` messages is not | ||
| allowed. |
There was a problem hiding this comment.
I would say yes, the spec should say that
- a server MUST support the one shot method
- it is recommended for a server to support both methods
I think the "MUST" for the first bullet point is justified by the fact that if you support the id-based method there is no reason not to be able to support the one shot method.
teverman
approved these changes
Aug 22, 2018
antoninbas
approved these changes
Aug 22, 2018
|
|
||
| A P4Runtime server *must* support the one shot style of programming | ||
| tables with an action selector implementation. Support for the | ||
| `ActionProfileMember` and `ActionProfileGroup` style is *optional*. |
There was a problem hiding this comment.
I think "optional" is too weak, I would prefer "recommended"
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.