Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Sonic CoPP config and management #606
Sonic CoPP config and management #606
Changes from all commits
fdc7cff
df77a4f
03b7a4a
262e258
3a1ba14
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I have implemented a 3rd option that addresses a limitation of the first option so that we can load the default COPP configs in the ConfigDB to keep inline with SONiC architecture and some requirements for config CLIs.
To address the issue of an old COPP default config stored in config_db.json file (on config save) overwriting the new image COPP default config with new values, we are comparing the default COPP config file with the ConfigDB entries in sonic-cfggen and only writing back config modifications to the config_db.json file. No COPP config will be stored in config_db.json if no modifications have been made. Only "user modifications" will be stored back in the config_db.json. This change in sonic-cfggen is generic enough for other use cases as well. Let me know if this 3rd option is a possibility for this feature.
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.
The thought is, lets not have default in config_db at all. In case of warmboot, we will again have the issue if entries are in config_db and new image has a different default value. In which case you've to add the similar logic in the warmboot script before saving the redis_db to skip saving default values. Also I think, sonic-cfggen should not be used to decide which TABLES must be written to config_db.json file vs not written to file.
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.
Ok, we can have a separate discussion on this specific issue. The rest of the doc looks good.
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.
@prsunny even though we remove copp.json from checksum calculation it will still throw error on warm-reboot from old image.
Eg: current_image let's say 201911 next_image 202011 (that have new copp design)
when we issue warm-boot on 201911 image to go to 202011 asic checksum validation done by 201911 will fail as it will try to get checksum of 2020 image but it will not be same as 201911.
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.
Agree, but I think, would recommend to use "warm-reboot-f".