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

HLD for Save on Set #1297

Merged
merged 7 commits into from
Jul 24, 2024
Merged

HLD for Save on Set #1297

merged 7 commits into from
Jul 24, 2024

Conversation

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 20, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@rlucus rlucus mentioned this pull request Mar 20, 2023
tomek-US
tomek-US previously approved these changes Mar 29, 2023
Copy link

@tomek-US tomek-US left a comment

Choose a reason for hiding this comment

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

LGTM

@zhangyanzhao
Copy link
Collaborator

#### Unit Test cases

- No behavior change if the feature is not enabled.
- A gNMI.Set() call should generate a signal to save ConfigDB.
Copy link
Collaborator

@venkatmahalingam venkatmahalingam May 17, 2023

Choose a reason for hiding this comment

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

Looks like we invoke "write memory" click call for every gNMI set operation when the save-on-set is enabled, not sure signalling is the word usage, please fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded

##### Telemetry Executable
A new command-line parameter will be added to control the behavior of the save-on-set functionality. By default, i.e. when the option is not specified, the gNMI server will behave as it did before this change - the configuration will not be saved to a file without explicit action from the administrator for example by execution of a command via ssh connection.

The new parameter will be: --with-save-on-set and when present it will configure a function pointer variable gnmi.SaveOnSet to point to a function that actually performs the save operation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we getting this feature enable/disable from config_db field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added clarification that the startup script will read from the DB to pass the value along to the command line argument on startup.

@zhangyanzhao
Copy link
Collaborator

code PR is missed and can you please add @rlucus ?

rlucus added a commit to rlucus/sonic-buildimage that referenced this pull request Mar 27, 2024
@zhangyanzhao
Copy link
Collaborator

code PR is not ready, move to backlog for future release

baxia-lan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request May 29, 2024
* add support for save-on-set to gnmi

sonic-net/SONiC#1297

* leave default if not set

* move to telemetry.sh
@bhagatgit bhagatgit requested review from qiluo-msft and bhagatgit and removed request for qiluo-msft May 30, 2024 05:41
bhagatgit
bhagatgit previously approved these changes May 30, 2024
@qiluo-msft
Copy link
Contributor

Could you add the code PRs in a markdown table in this HLD PR's description?

doc/mgmt/gnmi/save_on_set.md Outdated Show resolved Hide resolved
doc/mgmt/gnmi/save_on_set.md Show resolved Hide resolved
@rlucus
Copy link
Contributor Author

rlucus commented Jun 14, 2024

@bhagatgit
Copy link
Contributor

Approving/merging this HLD as the code PRs are already submitted.

@bhagatgit bhagatgit merged commit 4ab89a9 into sonic-net:master Jul 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: DeferredForNextRelease
Development

Successfully merging this pull request may close these issues.

10 participants