p4lang / PI

An implementation framework for a P4Runtime server
Apache License 2.0
165 stars 107 forks source link

Question: Any plans to enable modify/delete operations that refer to table entries by a 'handle' rather than the search key? #179

Closed jafingerhut closed 6 years ago

jafingerhut commented 7 years ago

From reading the current .proto file here: https://github.com/p4lang/PI/blob/master/proto/p4/p4runtime.proto

It appears that the only supported method for modifying or deleting an existing table entry is to give the search key fields, and perhaps also the full actions and action parameters, too. Please correct me if I am mistaken in this.

If it is true, are you considering enabling the support of the controller getting some kind of 'handle ids' back from the P4 device on successful insert operations, and letting the controller do modify/delete operations using those ids to identify the table entries?

I understand this requires some additional state to be maintained on the controller side. Such an 'identify via handle id' could potentially be merely one of 2 alternative ways to identify table entries, keeping the current approach as well.

Regardless of the answer, having an option to modify/delete a table entry by providing only the key, but not the action, nor the action parameters, sounds useful as well.

antoninbas commented 7 years ago

Only the match key fields are required to identify the entry, not the action... I am personally not considering enabling support for handle-based operations in P4Runtime. It would require supporting handles in APIs which manipulate direct resources as well, which means that for people defining architectures with new types of direct externs, best practices would require supporting both methods of access. The current P4Runtime design direction would also privilege controller-chosen ids, not switch-chosen handles. IMO, the application usually has to store the match keys no matter what, so we may as well not make it store the handle as well.

jafingerhut commented 7 years ago

Thanks for the response.

The .proto file has TableEntry with both 'match' and 'action' fields. Are you saying that when deleting a TableEntry, the 'action' need not be filled in?

If so, by what means do you plan to document this? e.g. comments in the .proto file, or text in a P4Runtime API specification document?

antoninbas commented 7 years ago

Yes, the action does not need to be filled in. In protobuf 3, all fields are optional by default. Based on that, we are considering an approach where we use the fields which are set as a filter. For example I could do a Read on all table entries from all tables by leaving all the fields empty in my ReadRequest. If I only populate the table id field, then I would be reading all the entries from that specific table. If I also populate the action id field, then I would also be filtering out all the entries which do not use this action. This could also be applicable to delete operations.

We will be releasing a spec before the end of the year. We are trying to find some framework that lets us write some documentation in a companion document and can generate browsable documentation by combining the information in the protubuf file and the companion document. If we describe all the semantics (including errors), we feel like it will encumber the protobuf file too much.

jafingerhut commented 7 years ago

It might not be what you are hoping to do, but the latest PSA document checked into the p4lang/p4-spec repository uses Madoko INCLUDE directives to include excerpts of syntactically correct, compilable, P4 programs.

For example, this line in the Madoko PSA.mdk file: https://github.com/p4lang/p4-spec/blob/master/p4-16/psa/PSA.mdk#L95

[INCLUDE=psa.p4:Type_defns]

Causes a consecutive range of lines bounded by comments containing the string "Type_defns" in the file psa.p4 to be included in the PSA.mdk file.

https://github.com/p4lang/p4-spec/blob/master/p4-16/psa/psa.p4#L45-L55

The Madoko documentation for this "include fragments" feature can be found here: http://madoko.org/reference.html#sec-includefrag

antoninbas commented 6 years ago

Closing this for now. Please re-open it if needed & bring it up at the WG meeting.