Skip to content

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

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

Add gNPSI HLD #2019

wants to merge 8 commits into from

Conversation

RazorBach
Copy link

No description provided.

Copy link

linux-foundation-easycla bot commented Jun 23, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

No pipelines are associated with this pull request.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

No pipelines are associated with this pull request.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

No pipelines are associated with this pull request.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

No pipelines are associated with this pull request.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

No pipelines are associated with this pull request.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

No pipelines are associated with this pull request.



#### 10.1. Unit Test cases

Copy link
Collaborator

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?

Copy link
Author

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.

"port": "<port_num>"
}
COUNTERS_DB:
"COUNTERS:GNPSI:<remote_ip>/<port_num>": {
Copy link
Collaborator

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.

Copy link
Author

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.



![sFlow switch stack with gNPSI](images/image2.png "new switch stack")

Copy link
Collaborator

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?

Copy link
Contributor

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.

Copy link

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.

@pphaal
Copy link

pphaal commented Jun 25, 2025

After the introduction of gNPSI, a new gNPSI server process would be brought up within the sFlow container. `hsflowd` would configure an internal local loopback collector just like a sFlow collector.
**Advantage**: This local loopback approach ensures a streamlined architecture and simplifies the maintenance of the upstream code. Refer to the [Alternative](?tab=t.0#heading=h.5bgbtjgtpqgx) section on the rationale behind choosing this design.
When sampled packets come in, hsflowd would send it to the local loopback collector. Then gNPSI process would read from local loopback and relay samples to the subscribed clients.
![sFlow switch stack with gNPSI](images/image2.png "new switch stack")

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).

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

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
Copy link
Author

Choose a reason for hiding this comment

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

Updated Yang model

Copy link
Contributor

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
Copy link
Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

No pipelines are associated with this pull request.

"admin_state": "ENABLE"|"DISABLE",
"port": "<port_num>"
}
STATE_DB:
Copy link
Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lguohan for vis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📋 In Plan Features
Development

Successfully merging this pull request may close these issues.

5 participants