-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add gNPSI HLD #2019
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
base: master
Are you sure you want to change the base?
Add gNPSI HLD #2019
Conversation
/azp run |
No pipelines are associated with this pull request. |
/azp run |
No pipelines are associated with this pull request. |
/azp run |
No pipelines are associated with this pull request. |
/azp run |
No pipelines are associated with this pull request. |
/azp run |
No pipelines are associated with this pull request. |
/azp run |
No pipelines are associated with this pull request. |
|
||
|
||
#### 10.1. Unit Test cases | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a sample gNPSI client commands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no specific cli tool at this moment, I assume you can use grpc_cli for this.
doc/gnpsi/gnpsi_hld.md
Outdated
"port": "<port_num>" | ||
} | ||
COUNTERS_DB: | ||
"COUNTERS:GNPSI:<remote_ip>/<port_num>": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See if you can keep last N clients and flush upon feature disable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the doc. We clean up the stats on each client disconnection and server restart.
|
||
|
||
 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to handle local loopback write internally instead of exposing the internal IPC between hsflowd and gNPSI server to the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the motivation of this approach was to make the gnpsi addition completely transparent to the internals of sflow agent (hsflowd), no modification of the hsflowd code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Complete transparency is achieved by sending the sFlow datagrams across the docker bridge from the sFlow container to a gNPSI container - this completely decouples the two features so that no changes are needed to the sFlow agent or to the sFlow container build. The gNPSI container knows its own IP address and can set up the entry in the Redis sFlow collector table - the sFlow container doesn't need to know anything about gNPSI, it simply follows the Redis configuration and sends the sFlow datagrams to the target IP address.
Lines 82 to 89 in dc17649
My reading of the HLD is that there are no close dependencies between sFlow and gNPSI features. gNPSI is a general purpose tunneling service for a local UDP ports (that can be used for sFlow, NetFlow, IPFIX etc) that is part of the gRPC Telemetry set of features. As such, gNPSI doesn’t belong inside the sFlow container as it would unnecessarily couple the two services and make development, testing and maintenance of both services more complicated. Loose coupling through Redis for configuration and a UDP streaming from the sFlow to gNPSI container would cleanly separate what should be independent SONiC features (sFlow / gRPC). |
/azp run |
No pipelines are associated with this pull request. |
* `/system/grpc-servers/grpc-server[name=gnpsi]/clients/client[address=<client_ip>][port=<port_num>]/state/sample-send-error` | ||
|
||
|
||
Yang model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated Yang model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding @lguohan for vis.
* `/server`: helper to start and stop relay server and the stats thread, also write to appl_state_db | ||
* `/utils`: stats util, credential util and authz logger util. | ||
|
||
#### Feature enablement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated a section for the default enablement of gNPSI program. This can now be controlled by FEATURE table and we can disable gNPSI program if not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lguohan, @venkatmahalingam for vis.
/azp run |
No pipelines are associated with this pull request. |
"admin_state": "ENABLE"|"DISABLE", | ||
"port": "<port_num>" | ||
} | ||
STATE_DB: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move to STATE_DB
for the state paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lguohan for vis.
No description provided.