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

P4Runtime API versioning #294

Closed
antoninbas opened this issue Feb 2, 2018 · 4 comments
Closed

P4Runtime API versioning #294

antoninbas opened this issue Feb 2, 2018 · 4 comments
Assignees

Comments

@antoninbas
Copy link
Member

@aghaffarkhah @wmohsin this is something we discussed offline but I cannot find any webpage describing this. Could you give an example of a protobuf file with version info?

@antoninbas antoninbas added this to the p4runtime-v1.0 milestone Apr 24, 2018
@wmohsin wmohsin changed the title Add version number to protobuf IDLs P4Runtime API versioning May 2, 2018
@wmohsin
Copy link
Contributor

wmohsin commented May 2, 2018

I researched into how protobuf/gRPC based services are versioned.
I found the following best-practice / recommendation: https://cloud.google.com/apis/design/versioning
This is based on the industry-standard semantic versioning: https://semver.org/

This uses a MAJOR.MINOR.PATCH style version number scheme. The rules are to increment:

  • MAJOR version when you make incompatible API changes,
  • MINOR version when you add functionality in a backwards-compatible manner, and
  • PATCH version when you make backwards-compatible bug fixes.

Also, the major version number must be encoded as the last component of the proto package name. This would be p4.P4Runtime.v1 and p4.config.v1.

With a major version, the API must be evolved in a proto backwards-compatible manner described at https://cloud.google.com/apis/design/versioning#backwards_compatibility. See design compatibility for more details. I expect MAJOR version bump to be a rare event.

I propose that we adopt semantic versioning and the above guidelines for versioning P4Runtime.

I believe we are currently in initial development, but getting very close to 1.0.0 public API:

  • link Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.

Some services (e.g. gNMI) have chosen to use a custom proto option to represent a version of the service:

option (gnmi_service) = "0.6.0";

This is mostly for documentation/visibility. I would advise against this due to concerns noted in custom_options. Also, this has caused compatibility issues on targets that need to use custom non-x86 proto_lib.

The next step is to review this in a P4-API WG meeting.

@antoninbas
Copy link
Member Author

Thanks for all the background information @wmohsin
I think there is one small typo:

This would be p4.P4Runtime.v1 and p4.config.v1.

should probably be

This would be p4.v1 and p4.config.v1.

P4Runtime is the name of the service.

@chrispsommers
Copy link
Contributor

I like this proposal. To determine the supported major versions, the controller can attempt a request using each major (semantic) version. Assuming we add a capability query request someday (https://github.com/p4lang/PI/issues/323) the controller can use this request to determine all the fine-grained capabilities, which could include major.minor.patch API version.

@antoninbas
Copy link
Member Author

Decision at the 05/16/2018 P4 API WG meeting was to move forward with this proposal.
As mentioned above, a client can check which major version is supported by attempting to connect to the appropriate service. It is probably a good idea to consider including a RPC in P4Runtime to query minor+patch version.
I will open a PR which implements this proposal, so I'm re-assigning the issue to me.

@antoninbas antoninbas assigned antoninbas and unassigned wmohsin May 17, 2018
antoninbas added a commit that referenced this issue May 19, 2018
We follow best practices for protobuf cloud services and add the major
version number to the package path. This change affects many C++ source
files in this project and I didn't find a good way to make the diff
smaller by using namespace "tricks". I did introduce namespace aliases
(p4rt = ::p4::v1 and p4config = ::p4::config::v1) to make future
potential changes easier.

Fixes #294

This commit also split p4/p4types.proto into p4/v1/p4data.proto and
p4/config/v1/p4types.proto, thus breaking the circular import in golang
between the p4 and p4.config packages.

Fixes #350
antoninbas added a commit that referenced this issue May 25, 2018
We follow best practices for protobuf cloud services and add the major
version number to the package path. This change affects many C++ source
files in this project and I didn't find a good way to make the diff
smaller by using namespace "tricks". I did introduce namespace aliases
(p4rt = ::p4::v1 and p4config = ::p4::config::v1) to make future
potential changes easier.

Fixes #294

This commit also split p4/p4types.proto into p4/v1/p4data.proto and
p4/config/v1/p4types.proto, thus breaking the circular import in golang
between the p4 and p4.config packages.

Fixes #350
antoninbas added a commit that referenced this issue May 30, 2018
* Add major version number to P4Runtime protobuf packages

We follow best practices for protobuf cloud services and add the major
version number to the package path. This change affects many C++ source
files in this project and I didn't find a good way to make the diff
smaller by using namespace "tricks". I did introduce namespace aliases
(p4rt = ::p4::v1 and p4config = ::p4::config::v1) to make future
potential changes easier.

Fixes #294

This commit also split p4/p4types.proto into p4/v1/p4data.proto and
p4/config/v1/p4types.proto, thus breaking the circular import in golang
between the p4 and p4.config packages.

Fixes #350

* Use p4v1 and p4configv1 namespace aliases

Instead of p4rt and p4config. This is based on a discussion with
reviewer @wmohsin about future-proofness, best practices and name
collisions.

Even though the fact that we were only using namespace aliases in
implementation files mean that there should be no name conflicts at link
time (namespace aliases are only visible in their translation unit and
do not affect the full-qualified name of linker symbols), we decided
that in the end it probably was simpler to just include the major
version number in the namespace alias. A change in major version number
should be a rare and disruptive event, and keeping the diff small when
updating the implementation may not be a valid concern. There will be
many things to figure out when & if we introduce P4Runtime v2 and it is
tough to predict how it will happen and which steps we will follow to
roll out the new version.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants