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

Support for initial entries #432

Merged
merged 24 commits into from
Aug 11, 2023

Conversation

jafingerhut
Copy link
Contributor

@jafingerhut jafingerhut commented May 28, 2023

Fixes #426

@jafingerhut
Copy link
Contributor Author

As of commit 2 of this PR, this is a first draft of a proposal for addressing issue #426

There are still several embedded questions marked TODO at this point. Please comment with suggested answers to those questions if you have any.

// P4 source code within its `entries` table property.
//
// TODO: Should const be true for all entries read from a table
// declared with `const entries`? Would that be considered a
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing implementations will not populate this field, evaluating to 0=false, even for constant entries, resulting in bogus information. I don't know if this constitutes "backwards incompatibility," or it simply means existing implementations are suddently non-conformant with a new requirement. If the consensus is to adopt the proposal despite this fact, we would need at a minimum to write an advisory that this field should be ignored unless the implementation conforms with the spec version which adopts this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does seem like an undesirable state. One way to avoid such a situation is to define that when reading entries from a table with const entries, or from a default entry declared with const, that this new TableEntry const flag will always be false, to ensure that old and new servers return the same result for these kinds of read requests both before and after this spec change is made.

If we did that, we would try to document this as clearly as we can, and the reason why (because it would not be immediately obvious to someone new to the P4Runtime spec a year or two from now), and that the only time the server would set the const flag to true in a response to a read request would be for a table that had a const flag on an entry in a table with this new style of initial entries.

Copy link
Collaborator

@chrispsommers chrispsommers May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your logic is sound, but it doesn't feel satisfying or intuitive overall - too many conditions to wrap one's head around. It seems like a long-term solution to a short-term problem. I'm not sure how objectionable it is - clients of this older server could simply not try to use the field until the server supports it, same as any other new feature. Perhaps we should get others to weigh in on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love to get others to weigh in on this. @smolkaj @jonathan-dilorenzo @thantry Can you or someone you know at Google who is interested in getting initial entries actually working in an implementation (as opposed to merely specified in P4.org documents) please take a look, or get someone else to take a look, at this PR in detail and comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still like others to weigh in on this question, but thinking about this some more, it does seem in the spirit of Protobuf based messages to consider it still 'backwards compatible' if an old message has a new field with additional information added to it. If we have a new const field in TableEntry messages, and new P4Runtime servers in the future return true for this field for entries declared with const entries in the source code, even though older P4Runtime servers return no such field at all, that seems like something that P4Runtime API clients should just need to handle gracefully.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With commit 12 on this PR, I have changed the proposed comment being commented on here to say that this new const field for TableEntry messages WILL be set to true by the server for all entries read from a table with const entries, and also for reading the default action from a table that has const default_action. We can change that before approving, of course, depending on whether anyone else has arguments against this choice.

Copy link
Member

@smolkaj smolkaj Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for missing this until now.

I am not someone who is planning to immediately use initial entries at this point, but can answer from a protobuf / semantic versioning perspective.

This requires a MINOR version bump in terms of https://semver.org/.

This is a "backward compatible API change" in the sense that an "old client" (using the old .proto) will continue to work without problems with a "new server" who adopts the new API:

  • READ: When an old client gets a message with the new field (in binary format), protobuf will simply ignore the extra data and the client will not see it.
  • WRITE: When an old client writes a message without the new field (in binary format), a new server will see the field as being set to false. That works out fine for this proposal.

Note that a new client won't automagically work with an old server, as mentioned by other above: they will read back all entries with is_const: false even if the entries are const. But since the client made a deliberate choice here to adopt the new API, it's also their job to ensure they can support older servers as well, if they wish. ("This is a backward compatible API change" means that upgrading servers to the new API won't break clients -- it doesn't talk about what happens if clients start using the new API with servers that don't support it...)

@jafingerhut Perhaps we should leave a comment saying that the field will be set by P4Runtime servers starting at version x.y.z?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version x.y.z of what project? Stratum? p4lang/PI? The P4Runtime API specification?

Copy link
Member

@smolkaj smolkaj Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Servers implementing version x.y.z of the P4Runtime standard.

I think it's probably enough to say something like "Added in 1.4.0"?

EDIT: I also opened #441, I think we should do this consistently going forward.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added two "Added in 1.4.0" comments with my latest commit to this PR, one for is_const in TableEntry, and the other for has_initial_entries in Table.

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good proposal so far, pending TODOs.

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More comments.

@jafingerhut
Copy link
Contributor Author

Jonathan DiLorenzo: Should we have tables with const entries have has_initial_entries AND is_const_table both true?

Andy: I will make this change in another commit on this PR and let people know it is ready for review.

@jafingerhut
Copy link
Contributor Author

I have updated this PR to follow the suggestion made during the 2023-Jul-14 API work group meeting, so that tables declared in source code with const entries continue to have is_const_table true as before, but they also have the new flag has_initial_entries true.

The new style of tables that the language spec now supports that can have entries without const before entries, will only have has_initial_entries true, but is_const_table will be false.

Copy link
Member

@smolkaj smolkaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fantastic! I'm very excited about this new feature. Thank you Andy!

I left a few minor comments, mostly nitpicking.

docs/v1/P4Runtime-Spec.mdk Outdated Show resolved Hide resolved
docs/v1/P4Runtime-Spec.mdk Outdated Show resolved Hide resolved
docs/v1/P4Runtime-Spec.mdk Outdated Show resolved Hide resolved
docs/v1/P4Runtime-Spec.mdk Outdated Show resolved Hide resolved
docs/v1/P4Runtime-Spec.mdk Outdated Show resolved Hide resolved
docs/v1/P4Runtime-Spec.mdk Show resolved Hide resolved
of tables into the device.

The format of the entries file is a sequence of 0 or more `Update`
messages. All of them have `type` equal to `INSERT`, and `entity` is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect just a WriteRequest?
The protobuf parsers don't really support sequences of messages out of the box AFAIK, so if it was really just a concatenation of parsable protobuf blobs that would make things pretty hard to consume.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example entries file written by p4c in text format: https://github.com/p4lang/p4c/blob/main/testdata/p4_16_samples_outputs/table-entries-ternary-bmv2.p4.entries.txt

It doesn't look like a WriteRequest message to me, since there are no fields of a WriteRequest message present there, only a sequence of 0 or more Update messages. If you tell me that is a valid WriteRequest message, I am happy to document it that way in the P4Runtime spec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, the link you provided does show a valid WriteRequest.

It doesn't look like a WriteRequest message to me, since there are no fields of a WriteRequest message present there

Actually it does: the repeated updates field is a WriteRequest field [1], and the textproto you linked to contains a bunch of updates { ... }.

This is admittedly subtle, because the text proto format doesn't include an outer wrapper for the message that is being encoded. Instead, it just shows the fields of the message at the top-level.

That being said, the text format is ambiguous, this could be a dump of any proto message with a repeated Update field. And for the binary format, this distinction matters. So we shouldn't blindly claim this is a WriteRequest. Do you happen to know where in the p4c source code this output gets created? From there it will be straightforward to tell the correct message type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the p4c code that writes the entries file: https://github.com/p4lang/p4c/blob/main/control-plane/p4RuntimeSerializer.cpp#L973-L976

in case the line numbers go askew, the method name is P4RuntimeEntriesConverter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you would like to examing the binary and/or JSON format of the entries file generated by current open source p4c, in addition to the text format, they are all in the zip file attached to this comment.
tmp.zip

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find the place where things get serialized, but I would consider the following a good-enough hint that the message is a p4.v1.WriteRequest: https://github.com/p4lang/p4c/blob/main/control-plane/p4RuntimeSerializer.cpp#L1327-L1328

I also opened p4lang/p4c#4070 so people don't have to guess in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an easy way for you to parse the contents of the binary format entries file in the file tmp.zip attached in my previous comment, to determine whether it is a valid binary WriteRequest protobuf message? I suspect it probably is, but do not know any quick way to check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed with a Google-internal CLI tool that entries.bin can be parsed as a p4.v1.WriteRequest.

proto/p4/config/v1/p4info.proto Outdated Show resolved Hide resolved
proto/p4/v1/p4runtime.proto Outdated Show resolved Hide resolved
proto/p4/v1/p4runtime.proto Outdated Show resolved Hide resolved
Propose that it is true if and only if the table property `entries` is
present, _and_ the list of entries is not empty.
@jafingerhut
Copy link
Contributor Author

@smolkaj @jonathan-dilorenzo @chrispsommers I have made 7 commits to this PR since the 2023-Jul-14 API work group meeting, primarily due to comments made during the meeting, and from Steffen's careful review after the meeting, and I think it is the better for it.

I have no current plans to change it any more, until and unless there is additional review feedback that warrants additional changes.

@smolkaj
Copy link
Member

smolkaj commented Jul 18, 2023

This LGTM! There seem to be 3 remaining threads that would be nice to resolve, 2 of them being nits. Other than that, from my side this is ready to be merged.

Copy link
Member

@smolkaj smolkaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the remaining open comment in the appendix regarding the protobuf schema.

@jafingerhut
Copy link
Contributor Author

LGTM modulo the remaining open comment in the appendix regarding the protobuf schema.

With commit 22 on this PR, I have attempted to clarify the description of the protobuf schema of an entries file.

@smolkaj
Copy link
Member

smolkaj commented Jul 19, 2023

Looks great to me.

@jafingerhut
Copy link
Contributor Author

@saynb @jamescchoi This has been through careful review by several others already, so hopefully should also meet with your approval, but in case you want to take a look, this is something we are going to be asked to implement by some folks, so good to be aware of the details.

docs/v1/P4Runtime-Spec.mdk Outdated Show resolved Hide resolved
docs/v1/P4Runtime-Spec.mdk Show resolved Hide resolved
jafingerhut added a commit to jafingerhut/p4c that referenced this pull request Jul 23, 2023
These changes update the contents of P4Info and table entries files as
generated by p4c, to match what has been agreed upon by the P4 API
work group in this pull request
p4lang/p4runtime#432
in the source code, and thus `is_const_table` and
`has_initial_entries` will both be false. A corner case is that if
it has `entries = { }` with no `const` before `entries`, &ie; an
empty list of entries, that is also a normal table.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It occurs to me that handling this corner-case gracefully may be useful for cases where source code is dynamically-generated based on e.g. macros or other techniques. In such cases, certain "build profiles" may result in an empty entries list.

Copy link

@jamescchoi jamescchoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to be well thought out and pretty complete.
The comment is optional.

Note that for a table with `const entries`, or with `entries` having
one or more individual entries declared `const`, it would be an error
for a P4Runtime client to send a `WriteRequest` to a P4Runtime server
with the contents of the entries file, because the server should

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be good to further clarify that error applies to the entries with const only, not the whole contents of the entries file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the latest commit on this PR as of writing this comment, I have replaced the last paragraph of the new appendix with the following, in an attempt to clarify:

Note that if a P4Runtime client attempted to send a `WriteRequest` to
a P4Runtime server with the contents of the entries file, the server
must return an error for each entry that has `is_const` true, as
described in Section [#sec-table-entry].

@jafingerhut
Copy link
Contributor Author

@smolkaj @chrispsommers Happy to discuss this at the next P4 API work group meeting on 2023-Aug-11, in hopes of seeing if it has the right approvals for merging.

@smolkaj
Copy link
Member

smolkaj commented Aug 8, 2023

We have 4 approvals, so I think we are good to go.
Let's wait until the meeting before we hit merge, just in case.

@smolkaj smolkaj merged commit 1e771c4 into p4lang:main Aug 11, 2023
4 checks passed
jafingerhut added a commit to p4lang/p4c that referenced this pull request Aug 16, 2023
* Continuing the implementation of initial entries support in p4c
These changes update the contents of P4Info and table entries files as
generated by p4c, to match what has been agreed upon by the P4 API
work group in this pull request
p4lang/p4runtime#432

* Comment-only change to poke CI.

* Update p4runtime commit SHA

* Update p4runtime commit SHA for Bazel builds

* Correctly handle case when table->getEntries() is null

* Update a few more expected output files, and make linter happy

* Update bazel/p4c_deps.bzl

Co-authored-by: Radostin Stoyanov <[email protected]>

---------

Co-authored-by: Radostin Stoyanov <[email protected]>
chrispsommers added a commit that referenced this pull request Jan 26, 2024
* Update Go dependencies (#438)

* Fixes #439 (#440)

Change CI  workflow to skip publishing if PR spawned by dependabot

* Adding dev branches to workflows (#443)

* Support for initial entries (#432)

* Define P4Runtime API support for tables with initial entries

* Add TODO asking whether the format for the contents of entries files
should be specified in the P4Runtime spec.

* Fix a couple of things found by linter and compiling protobuf

* Update autogenerated files

* Document that TableEntry const field must be false in write requests

* Add an appendix describing the contents of entries files generated by p4c

* Clarify some wording.

* Fix Madoko lint check

* Replace TODO with cross reference to new appendix on entries files
and clean up Madoko formatting in that appendix.

* Replace TODO with an optimistic footnote.

* Propose that TableEntry has new field const true for const entries
and also for const default_action

* Update auto-generated files

* Define has_initial_entries to be true for tables with `const entries`
Also fix a couple of spelling typos.

* Update auto-generated files

* Address several review comments

* Address some more review comments.

* Update auto-generated files again

* Slight change in definition of has_initial_entries flag
Propose that it is true if and only if the table property `entries` is
present, _and_ the list of entries is not empty.

* Update auto-generated files

* Add "added in 1.4.0" notes to the two new fields

* Clarify the description of the content of an entries file

* Fix a typo, and add is_const field to list of TableEntry fields

* Address review comment in new appendix

* Fix #434: Remove obsolete TODO section in README (#447)

* Fix #434: Remove obsolete TODO section in README
Update the link to the auto-generated versions of the P4Runtime
specification on the P4.org web site.

Update the section "P4 Language Version Applicability" to version
1.2.4 of the P4_16 language specification, but list 3 known exceptions
of features that have not been explicitly addressed yet.

* Add P4_16 v1.2.4 language spec features that may need addressing
in a future version of the P4Runtime API specification.

* Update discussion of entry priorities in constant tables (#457)

* Update discussion of entry priorities in constant tables

* Correct description of entry priority for constant tables

* Bump golang.org/x/net from 0.9.0 to 0.17.0 (#461)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.9.0 to 0.17.0.
- [Commits](golang/net@v0.9.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Remove 4 P4 language spec compatibility issues from the list (#459)

* Remove 4 P4 language spec compatibility issues from the list
During 2023-Sep-08 P4.org API work group meeting, it was agreed that
there are no changes required to the P4Runtime API specification to be
compatible with these updates in the language spec.

* Add clarifying behavior of table with no `key` property back in
since there are potentially open issues around p4c implementation and
how it generates size field of tables in P4Info files that should be
considered before considering that issue resolved.

* Add metadata to multicast group entry (#446)

Same role as the metadata field for table entry

* Add proto_build_test rule that tests building the protos defined in the workspace. (#460)

* Update license kind to Apache2.0 instead of generic notice (#464)

* Bump google.golang.org/grpc from 1.56.1 to 1.56.3 (#465)

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.56.1 to 1.56.3.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.56.1...v1.56.3)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Antonin Bas <[email protected]>
Co-authored-by: Chris Sommers <[email protected]>
Co-authored-by: Andy Fingerhut <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: verios-google <[email protected]>
Co-authored-by: anksaiki <[email protected]>
Co-authored-by: anksaiki <[email protected]>
chrispsommers added a commit that referenced this pull request Feb 22, 2024
* Update Go dependencies (#438)

* Fixes #439 (#440)

Change CI  workflow to skip publishing if PR spawned by dependabot

* Adding dev branches to workflows (#443)

* Support for initial entries (#432)

* Define P4Runtime API support for tables with initial entries

* Add TODO asking whether the format for the contents of entries files
should be specified in the P4Runtime spec.

* Fix a couple of things found by linter and compiling protobuf

* Update autogenerated files

* Document that TableEntry const field must be false in write requests

* Add an appendix describing the contents of entries files generated by p4c

* Clarify some wording.

* Fix Madoko lint check

* Replace TODO with cross reference to new appendix on entries files
and clean up Madoko formatting in that appendix.

* Replace TODO with an optimistic footnote.

* Propose that TableEntry has new field const true for const entries
and also for const default_action

* Update auto-generated files

* Define has_initial_entries to be true for tables with `const entries`
Also fix a couple of spelling typos.

* Update auto-generated files

* Address several review comments

* Address some more review comments.

* Update auto-generated files again

* Slight change in definition of has_initial_entries flag
Propose that it is true if and only if the table property `entries` is
present, _and_ the list of entries is not empty.

* Update auto-generated files

* Add "added in 1.4.0" notes to the two new fields

* Clarify the description of the content of an entries file

* Fix a typo, and add is_const field to list of TableEntry fields

* Address review comment in new appendix

* Fix #434: Remove obsolete TODO section in README (#447)

* Fix #434: Remove obsolete TODO section in README
Update the link to the auto-generated versions of the P4Runtime
specification on the P4.org web site.

Update the section "P4 Language Version Applicability" to version
1.2.4 of the P4_16 language specification, but list 3 known exceptions
of features that have not been explicitly addressed yet.

* Add P4_16 v1.2.4 language spec features that may need addressing
in a future version of the P4Runtime API specification.

* Update discussion of entry priorities in constant tables (#457)

* Update discussion of entry priorities in constant tables

* Correct description of entry priority for constant tables

* Bump golang.org/x/net from 0.9.0 to 0.17.0 (#461)

Bumps [golang.org/x/net](https://github.com/golang/net) from 0.9.0 to 0.17.0.
- [Commits](golang/net@v0.9.0...v0.17.0)

---
updated-dependencies:
- dependency-name: golang.org/x/net
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Remove 4 P4 language spec compatibility issues from the list (#459)

* Remove 4 P4 language spec compatibility issues from the list
During 2023-Sep-08 P4.org API work group meeting, it was agreed that
there are no changes required to the P4Runtime API specification to be
compatible with these updates in the language spec.

* Add clarifying behavior of table with no `key` property back in
since there are potentially open issues around p4c implementation and
how it generates size field of tables in P4Info files that should be
considered before considering that issue resolved.

* Add metadata to multicast group entry (#446)

Same role as the metadata field for table entry

* Add proto_build_test rule that tests building the protos defined in the workspace. (#460)

* Update license kind to Apache2.0 instead of generic notice (#464)

* Bump google.golang.org/grpc from 1.56.1 to 1.56.3 (#465)

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.56.1 to 1.56.3.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.56.1...v1.56.3)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Pin Bazel version to 6.4.0 to fix regression (#471)

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Antonin Bas <[email protected]>
Co-authored-by: Chris Sommers <[email protected]>
Co-authored-by: Andy Fingerhut <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: verios-google <[email protected]>
Co-authored-by: anksaiki <[email protected]>
Co-authored-by: anksaiki <[email protected]>
Co-authored-by: Steffen Smolka <[email protected]>
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.

Enhancements to P4Runtime API for P4 table "initial entries" proposal
7 participants