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

Update GNMI design #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ganglyu
Copy link
Collaborator

@ganglyu ganglyu commented May 31, 2023

Explain GNMI path and value for DASH API.

@ganglyu
Copy link
Collaborator Author

ganglyu commented May 31, 2023

@Pterosaur @qiluo-msft

update {
path {
origin: "sonic_db"
elem {name: "DPU0"} elem {name: "APPL_DB"} elem {name: "DASH_ACL_RULE_TABLE"} elem {name: "group1:3"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

If DPU0 is not specified, can it be defaulted to 'localhost'?

2. The first element is device name, and it should be one of the following values: `localhost`, `DPU0`, `DPU1`, ..., `DPUx`
3. The second element is SONiC database name, and it should be `APPL_DB` for DASH API
4. The third element is table name, please refer to [DASH APP DB](https://github.com/sonic-net/DASH/blob/main/documentation/general/dash-sonic-hld.md#32-dash-app-db)
5. The fourth element is table key. For example, in the `DASH_ACL_RULE_TABLE`, the table key consists of two fields: `group_id` and `rule_num`. These fields are separated by a colon `:` in the APPL_DB. Therefore, the fourth element would be `group1:3`
Copy link
Collaborator

Choose a reason for hiding this comment

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

i thought the fourth element should be part ofthe protobuf definition. it should not be part of the path. @qiluo-msft

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GNMI delete operation only has path, and it does not have value. So, there's no protobuf in GNMI delete operation.

https://github.com/openconfig/gnmi/blob/master/proto/gnmi/gnmi.proto#L337

Copy link
Collaborator

Choose a reason for hiding this comment

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

i thought protobuf has the key already defined. then you need to state how would people know how to form a key for a specific table. https://github.com/sonic-net/sonic-dash-api/blob/master/proto/eni.proto#L38

1. GNMI path will have 4 elements, please refer to the above example
2. The first element is device name, and it should be one of the following values: `localhost`, `DPU0`, `DPU1`, ..., `DPUx`
3. The second element is SONiC database name, and it should be `APPL_DB` for DASH API
4. The third element is table name, please refer to [DASH APP DB](https://github.com/sonic-net/DASH/blob/main/documentation/general/dash-sonic-hld.md#32-dash-app-db)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this needs to be self-contained, the table name should be refered in this repo. do not refer to dash doc.

@lguohan
Copy link
Collaborator

lguohan commented Jun 22, 2023

overall order needs to be changed as well, we should first talk about gnmi, then protobuf, last one is redis db. I do not really think people care about redis db, that is implementation, people care about gnmi and protobuf. we should not talk gnmi at the end.

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