[docker-fpm-frr/bgpcfgd]: Update interface of bgpcfgd from swsssdk to swsscommon #3264
Conversation
swsscommon Signed-off-by: Ze Gan <ganze718@gmail.com>
Pterosaur
changed the title
|
have we installed python-swss-common in frr docker if not then you need to install it. |
Signed-off-by: Ze Gan <ganze718@gmail.com>
If the metadata update message is after all of bgp update message, we will lose the update of bgp. So we need check the bgp meesage cache when we update the ASN. Signed-off-by: Ze Gan <ganze718@gmail.com>
I think it has been installed. I try to run the following command at bgp container. |
Signed-off-by: Ze Gan <ganze718@gmail.com>
| run_command(command) | ||
| if "name" in data: | ||
| command = "vtysh -c 'configure terminal' -c 'router bgp {}' -c 'neighbor {} description {}'".format(self.bgp_asn, key, data['name']) | ||
| run_command(command) |
There was a problem hiding this comment.
The warning does nothing to handle the failure case. This is inherently problematic because it allows for DB to be out of sync with running config of the routing application. The config event will be lost and the only way to recover this (since it's not saved in startup config either) is to restart bgpcfgd. In general this code doesn't seem to be robust and the requirements are not well defined/clear.
There was a problem hiding this comment.
this is a configuration, if the command is not correct it means the configuration is incorrect. what do you mean by robust? Since it is configuration issue, the issue is outside this program, the right thing to do for this program is to log error. If you have better suggestion, we would appreciate to hear.
There was a problem hiding this comment.
If for whatever reason BGP is restarting or is not running, I doubt the command will go through even if correct. Basically there are race conditions where the command may be correct, but there will be a failure that is not handled and will leave the system in an inconsistent state. The only way to recover would be to restart bgpcfgd. In order to provide a suggestion, can you please explain what is the purpose of bgpcfgd?
In comparison to what the jinja templates have, its functionality seems very incomplete overall for anyone in actual deployments and can not possibly work the same way with the templates when BGP restarts because of the missing functionality the templates provide and bgpcfgd doesn't.
If it is for unit testing purposes, then it shouldn't be part of the image.
There was a problem hiding this comment.
what you suggest is valid concern, but it is out of scope of this refactoring. the bgpcfgd allow us to shutdown/enable bgp session.
Signed-off-by: Ze Gan <ganze718@gmail.com>
|
retest this please |
1 similar comment
|
retest this please |
… swsscommon (sonic-net#3264) Update interfaces of bgpcfd from swsssdk to swsscommon to unify a suit of interface with other component. Meanwhile, we can listen multiple tables at one thread under swsscommon interface. Signed-off-by: Ze Gan ganze718@gmail.com - What I did Move the interface of bgpcfgd from swsssdk to swsscommon. Because bgpcfgd need to listen more events in the future and we want to maintain one kind of APIs, swsscommon is more suitable than swsssdk. - How I did it Refactor the BGPConfigDaemon to two components, Daemon and BGPConfigManager. We can register new managers to the Daemon object if we want to listen more events.
…atically (#18708) #### Why I did it src/sonic-utilities ``` * ad464a9f - (HEAD -> master, origin/master, origin/HEAD) [Mellanox] Support new platform SN5400 in generic configuration update (#3272) (19 hours ago) [DavidZagury] * 29d4e88f - Add multi ASIC support for syslog rate limit feature (#3235) (21 hours ago) [Junchao-Mellanox] * f81317c3 - [Mellanox] added component versions to techsupport (#3264) (2 days ago) [sophiek] ``` #### How I did it #### How to verify it #### Description for the changelog
Update interfaces of bgpcfd from swsssdk to swsscommon to unify a suit of interface with other component. Meanwhile, we can listen multiple tables at one thread under swsscommon interface.
Signed-off-by: Ze Gan ganze718@gmail.com
- What I did
Move the interface of bgpcfgd from swsssdk to swsscommon. Because bgpcfgd need to listen more events in the future and we want to maintain one kind of APIs, swsscommon is more suitable than swsssdk.
- How I did it
Refactor the BGPConfigDaemon to two components,
DaemonandBGPConfigManager. We can register new managers to the Daemon object if we want to listen more events.- How to verify it
If it can pass all existing test cases, which means this update is success.
- Description for the changelog
Update interface of bgpcfgd from swsssdk to swsscommon
- A picture of a cute animal (not mandatory but encouraged)