Skip to content

Sonic CoPP config and management#606

Merged
prsunny merged 5 commits into
masterfrom
prsunny-coppmgr
Jul 30, 2020
Merged

Sonic CoPP config and management#606
prsunny merged 5 commits into
masterfrom
prsunny-coppmgr

Conversation

@prsunny

@prsunny prsunny commented Apr 22, 2020

Copy link
Copy Markdown
Contributor
  • Introduce CoPP Manager
  • Refer Feature Table before installing Traps

@prsunny

prsunny commented Apr 22, 2020

Copy link
Copy Markdown
Contributor Author

@ritalhui, @dgsudharsan, @padmanarayana, please help review

Comment thread doc/copp/CoPP Config and Management.md Outdated
Comment thread doc/copp/CoPP Config and Management.md Outdated
Comment thread doc/copp/CoPP Config and Management.md Outdated
Few modifications based on review comments, internal discussions
Comment thread doc/copp/CoPP Config and Management.md Outdated
Comment thread doc/copp/CoPP Config and Management.md
Comment thread doc/copp/CoPP Config and Management.md
Comment thread doc/copp/CoPP Config and Management.md
Comment thread doc/copp/CoPP Config and Management.md Outdated
@dgsudharsan

Copy link
Copy Markdown
Contributor

The Recent changes look good to me


1. The implementation is to do a migration of APP DB entries to new schema.
2. Let ```db_migrator``` remove all previously saved APP_DB entries and let syncd reconcile and remove the TRAP entries during warmboot. Later, ```coppmgr``` can reapply the traps afresh. Also remove COPP_TABLES from being saved in [backup_database](https://github.com/Azure/sonic-utilities/blob/master/scripts/fast-reboot#L234). This may require additional tests to confirm system behaviour during warmboot.
3. Remove '00-copp.config.json' from being included for checksum calculation in ```files/build_scripts/generate_asic_config_checksum.py```

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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

@prsunny

prsunny commented Jul 28, 2020

Copy link
Copy Markdown
Contributor Author

@dgsudharsan , @michaelli10 , @wendani , can you please sign off?

In coldboot, if user saves the config and the new release happened to have new CoPP values, it will not be applied since previous default values are present in config_db

2. ```coppmgr``` reading directly from ```copp_cfg.json``` and apply the default CoPP Tables.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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.

@prsunny prsunny merged commit 0c15e81 into master Jul 30, 2020
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.

6 participants