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

Iterate on the payload generation for gSII. #6

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

robshakir
Copy link
Contributor

@robshakir robshakir commented Oct 17, 2024

This PR makes an initial proposal for how the payload of gSII RPCs should look.

There are two cases that I think we need to consider:

  • state that is part of an existing OpenConfig model
  • protos that are generated for a subset of some other schema

For the OpenConfig case, ideally we would not generate new schemas, and be able to re-use existing schema components (and thus want to be able to write these components in YANG). This requires minor extension of the ygot toolchain to generate protobufs (see openconfig/ygot@master...volatile where this change is made). The protos that are included herein use this approach.

The alternative is just to have hand-crafted protos as payload -- but this seems like it would be error prone in some cases.

The PR contains two generated protos:

  • One that allows for interface admin status to be manipulated.
  • A second that allows for QoS 1r2c policers to have their PIR manipulated.

 * (A) proto/obj/interfaces:
   - Check in a suggested mechanism for generating a payload in
     gSII from the YANG schema.
 * (A) .gitignore
  - Ensure that deps are not checked into the repository.
 * (D) v1/proto/obj/...
  - Remove object hierarchy from generated protobufs.
 * (A) v1/yang/interfaces
  - Create a generation approach for the openconfig-interfaces module
    demonstrating a payload that is generated from YANG.
 * (A) scripts/generate_protos.sh
  - Generate protobufs for gSII payload.
 * (A) **/*.pb.go
  - Check in generated protobufs.
 * (M) v1/yang/..., v1/proto/...
   - Use a ygot branch that recognises `volatile` as a reserved keyword
     to generate protobufs for interfaces and qos schemas.
 * (A) go.{mod,sum}
  - Initialise Go module.
Comment on lines +23 to +24
string name = 1 [(yext.schemapath) = "/interfaces/interface/config/name|/interfaces/interface/name"];
Interface interface = 2;
Copy link

@brianneville brianneville Oct 17, 2024

Choose a reason for hiding this comment

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

(prefacing this by saying i dont know much about yang-protobuf generation)

is it intended that some field numbers increase monotonically while others are generated as the FNV hash of the schemapath (via https://github.com/openconfig/ygot/blob/100bd44fc21215ffcc4862f37d90f7c257b5fa2b/protogen/protogen.go#L1058)?
This seems to affect the fields in messages which are declared inside other messages, and differs from the way that protobuf generation is done for something like hercules (https://github.com/openconfig/hercules/blob/ca3575e85500fa089dfe0b8cd3ea71943267102e/proto/openconfig/openconfig_interfaces/openconfig_interfaces.proto#L37)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is intended. YANG lists are generated as repeated <message> fields. Where <message> is a generated message of the form:

message FooKey {
  string key_one = 1;
  string key_two = 2;
  ...
  string key_fourty = 40;
  Foo foo = 41;
}

Where Foo is the actual "member" of the list, and each of the monotonically increasing fields are the keys in the YANG list's key statement. Since these are stably ordered by the YANG schema itself we can save ourselves a few bytes and just give them a number.

For all other fields within a list or container, we don't have a stable numbering -- so we calculate the field ID using a fnv hash. We published something as to why this was reasonable here.

Choose a reason for hiding this comment

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

cool, sounds good!
thanks for linking that doc

@brianneville
Copy link

This change adds entries under volatile paths - can the motivation.md doc be updated to reflect this name if it has been finalized?
right now the motivation.md doc makes references to dynamic paths, see:
https://github.com/openconfig/gsii/blob/606a993/docs/motivation.md#data-modelling

@robshakir
Copy link
Contributor Author

This change adds entries under volatile paths - can the motivation.md doc be updated to reflect this name if it has been finalized? right now the motivation.md doc makes references to dynamic paths, see: https://github.com/openconfig/gsii/blob/606a993/docs/motivation.md#data-modelling

Yes -- #1 covers this change within the motivation.md doc. I'll also put together a more succinct specification document once the design becomes more stable. I'm working on a reference implementation to make sure that we understand some of the nuances over at gSIIgo.

@brianneville
Copy link

Yes -- #1 covers this change within the motivation.md doc. I'll also put together a more succinct specification document once the design becomes more stable. I'm working on a reference implementation to make sure that we understand some of the nuances over at gSIIgo.

ack - thanks for including that in #1.
Is gSIIgo going to be made public when completed?

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.

3 participants