[HLD] SONiC ConfigDB support Yang default values and profiles. #989
Conversation
…ause we choice bash approach.
|
I was just wondering whether it's possible to introduce a way to "shadow" the fields that existed in |
liuh-80
changed the title
Currently there are 2 scenario about delete:
For 1, Delete means change database schema, maybe we should use Xoff = 0 or Xoff=-1 instead of delete. For 2, still not quite undertsand the scenario. Will setup a offline discussion for this. |
For Anyway, now that we are introducing a new mechanism to support multiple overlay DB, I believe it should not be restricted in the buffer area. Probably the requirement is also applicable to other features. |
| - Merge default value to API result. | ||
|
|
||
| ## 2.3 Other code change | ||
| - Following method will change to get default value from DefaultValueProvider |
There was a problem hiding this comment.
API design changed, will update this later.
| - Merge default value to API result. | ||
|
|
||
| ## 2.3 Other code change | ||
| - Following method will change to get default value from DefaultValueProvider |
There was a problem hiding this comment.
API design changed, will update this later.
doc/mgmt/SONiC_overlay_ConfigDB.md
Outdated
|
|
||
| | | Pros | Cons | | ||
| | ------------------------------------------------------------- | ------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | Get default value from Yang model in read API. | Redis config DB keeps no change. | 3 MB memory per-process because need load Yang model and reference libyang.<br>50ms to load yang model.<br>8ms to read default value 10000 times. | |
There was a problem hiding this comment.
Yes, currently the load and mapping only happened when first time read default values.
Will update design document with this.
doc/mgmt/SONiC_overlay_ConfigDB.md
Outdated
|
|
||
| ## 2.2 New class | ||
|
|
||
| - YangModelLoader class |
There was a problem hiding this comment.
It's a new C++ class, and it's a internal class, not visible to user.
doc/mgmt/SONiC_overlay_ConfigDB.md
Outdated
| - For delete operation, data in static config table will not be delete, profile underlay use PROFILE_DELETE table to handle delete/revert: | ||
| - When delete a static config item, profile underlay will add the item key to PROFILE_DELETE table. | ||
| - When read static config, any key in PROFILE_DELETE will not exist in result. | ||
| - When user set deleted item back, the item key will be remove from PROFILE_DELETE table. |
doc/mgmt/SONiC_overlay_ConfigDB.md
Outdated
| } | ||
| } | ||
| ``` | ||
| - On mellanox devices, when set buffer mode to 'dynamic' mode, buffer manager will use swsscommon API to delete buffer pool to reclam unused buffer resource. |
doc/mgmt/SONiC_overlay_ConfigDB.md
Outdated
|
|
||
| ### How to get static config | ||
|
|
||
| - Static config will stored in a new redis database 'PROFILE_DB', database index is 15. |
There was a problem hiding this comment.
Will test this issue with this PR: sonic-net/sonic-swss-common#625
Will update here after all UT passed.
liuh-80
changed the title
|
/easycla |
2 similar comments
|
/easycla |
|
/easycla |
|
/easycla |
liuh-80
force-pushed
the
dev/liuh/configdb_default_value
branch
from
e4b10a6 to
8de3a60
Compare
|
Fix EasyCLA issue by merge all change to one and do a force push. |
liuh-80
force-pushed
the
dev/liuh/configdb_default_value
branch
from
3838154 to
ece1aa7
Compare
|
|
||
| - Profile | ||
|
|
||
| - Profile is part of OS image, should upgrade with OS upgrade. |
There was a problem hiding this comment.
Does the profile here represent a generic concept? It is a bit confusing because there is a similar word buffer profile...
There was a problem hiding this comment.
The 'profile' is a common concept, profile DB will not only be used by buffer config.
|
|
||
| - Backward compatibility with existing code and applications. | ||
|
|
||
| - For backward compatibility when initialize buffer config from minigraph, config will be write to both config tables and profile tables. |
There was a problem hiding this comment.
Is it possible to add terminology to explain config tables and profile tables? It's hard to understand ...
There was a problem hiding this comment.
Fixed, add more detail and change this to config DB and profile DB.
| ``` | ||
| - On mellanox devices, when set buffer mode to 'dynamic' mode, buffer manager will use swsscommon API to delete buffer pool to reclaim unused buffer resource. | ||
| - When delete a profile item 'ingress_lossless_pool', the key of this item will be write to PROFILE_DELETE table. and swsscommon read API will not return this item anymore. | ||
| - Revert operation: when user set buffer mode back, buffer manager will write the deleted item back, when this happen, the key of the item will be removed from PROFILE_DELETE table. and swsscommon read API will return this item. |
There was a problem hiding this comment.
To be more precise,
- if the content user writes back is the same as that in the
profile table, removing the key fromPROFILE_DELETEtable suffice - otherwise, the content should also be written to
config table
Correct?
There was a problem hiding this comment.
It's not correct, when the content writes back is same with item with profile table, following happen:
- Removing the key from PROFILE_DELETE table
- Write content to config table, this is because the 'SET' operation is from user, so it will be a user config, even it exactly same with the data in profile DB.
| - Profile will stored in a new redis database 'PROFILE_DB', database index is 15. | ||
| - Profile DB tables will have exactly same name and schema with config DB tables. | ||
| - Data will read form profile DB with swsscommon API. | ||
| - Profile DB will save and persist for warm-reboot and fast-reboot. |
There was a problem hiding this comment.
What happens if user perform a SONiC-SONiC upgrade?
There was a problem hiding this comment.
When upgrade from old version of SONiC which does not support this feature to new version:
- The config DB will keep no change, all data in config DB profile table will be treat as user config.
- Profile DB will be created, and DB migrator will initialize Profile DB.
So, from user aspect:
- all existing buffer pool/profile keeps no change.
- There maybe new buffer pool/profile exist, which introduced by the new version of OS.
I will update this to reboot part.
|
|
||
| ## 3.3 Fast-reboot | ||
|
|
||
| - Fast-reboot will reload minigraph, profile DB will re-render in reload minigraph. |
There was a problem hiding this comment.
AFAIK, minigraph won't be reloaded during fast reboot. It should read the config_db. no?
Same as cold reboot.
There was a problem hiding this comment.
Thanks, fixed, I check the reboot code and fix the mistake here.
| } | ||
| } | ||
| ``` | ||
| - On mellanox devices, when set buffer mode to 'dynamic' mode, buffer manager will use swsscommon API to delete buffer pool to reclaim unused buffer resource. |
There was a problem hiding this comment.
Just FYI, on Mellanox devices, no buffer pool or buffer profile will be removed to reclaim unused buffers.
But it's possible that a user decides to remove some buffer pool, eg.
- to migrate from double-ingress-pool mode to single-ingress-pool mode
- or to execute
config qos clear
There was a problem hiding this comment.
Fixed, my mistake, the buffermgrd are deleting items in appldb and statedb. config DB keeps no change.
| - Profile: defined in j2 template. | ||
| - Currently all config stored in config DB, so above configs can't be update after OS upgrade. | ||
| - Potential risk, Yang model default value conflict with hardcoded value: | ||
| - Default value hardcoded in source code. |
There was a problem hiding this comment.
Just to double check, did you mean the hardcoded value in the templates?
There was a problem hiding this comment.
No, it's ins the source code, for example in hostcfgd, when read config from config DB, if some item does not exist in config DB, hostcfgd will use default values hardcoded in it's code.
| - DB migrator will re-render profile DB to handle schema change. | ||
| - Profile DB will generate by sonic-cfggen command. | ||
| - DB migrator code will improve by deprate hardcoded config and complex upgrade logic. | ||
| - Some profile configuration change are disruptive, for example buffer config change,those change need fast-reboot to make sure change refelected on ASIC. |
There was a problem hiding this comment.
IMO it should also work to do a warm reboot to upgrade the schema
There was a problem hiding this comment.
Fixed, the code/warm/fast reboot will use DB migrator to upgrade schema.
|
|
||
| - Existing API keeps no change. | ||
| - Add decorator API to return default value and profile data. | ||
| - Load yang model as lazy as possible. |
There was a problem hiding this comment.
Maybe we can cache the parsed yang model information somewhere globally so that we just need to parse the yang model once and can use them many times.
But I'm not sure whether it is worth doing so since caching/storing it also takes time.
There was a problem hiding this comment.
Fixed by add more detail, current design is to only load once and cache it.
doc/mgmt/sonic-profile-delete.yang
Outdated
| } | ||
|
|
||
| organization | ||
| "BRCM"; |
There was a problem hiding this comment.
Change BRCM to SONiC.
doc/mgmt/sonic-profile-delete.yang
Outdated
| list PROFILE_DELETE { | ||
| key "itemkey"; | ||
| scommon:key-delim "|"; | ||
| scommon:key-pattern "PROFILE_DELETE_TABLE|{itemkey}"; |
There was a problem hiding this comment.
We dont need key-delim and key-pattern, right?
There was a problem hiding this comment.
Fixed, 2 lines removed.
doc/mgmt/sonic-profile-delete.yang
Outdated
| } | ||
|
|
||
| container sonic-profile-delete { | ||
| scommon:db-name "CONFIG_DB"; |
There was a problem hiding this comment.
Config-DB is by default, we don't need it here.
|
@stephenxs @venkatmahalingam , thanks for your comments, I have updated the HLD and fix it, please check. |
…liuh-80/SONiC into dev/liuh/configdb_default_value
Add HLD for SONiC config DB support load Yang model default value and profiles.
Here are PRs related with this HLD: