Revert frr service to systemd control and daemons to watchfrr control #3486
Conversation
|
@lguohan Can we please review this since it is addressing 5 reported issues? |
|
@nikos-github Can you please comment on my other questions? |
@pavel-shirshov All questions answered. |
|
@pavel-shirshov All questions answered. We really need to resolve the issues these changes are fixing. |
|
@nikos-github We don't want python3 inside bgp docker container. |
@pavel-shirshov It's been like this since the default routing stack changed to frr and python2 support is going away. What do you think is breaking? frr-pythontools has to work and we want to rely on frr for support as is. There is no functional requirement to diverge on this. Increase in size is not a good enough reason. This functionality existed, it got broken and needs to be reinstated. |
|
@nikos-github Image size it's a big deal for us. So let's wait for Guohan's review |
@pavel-shirshov We are in agreement here but I think we need to take into account that (a) The functionality existed and with python3 before it broke and we approved this. Why should this be a problem now that we are re-instating the functionality? (b) We don't want for something like that to diverge from frr and maintain a patch and rather rely on frr to do the heavy lifting of support for the tools (c) python2 support is going away in 3 months and while we may stick to python2 is sonic for a very long time, I don't believe we can afford to be making such exceptions and incur an overhead for upstream functionality (d) Image size can be reduced by moving non-mainstream dockers to a docker registry for download/pull on demand. |
|
@nikos-github Will the change work with warm-reboot procedure? Can you please check? |
@pavel-shirshov It needs to change to include watchfrr kill first: Do you want me to change it or do you want to do that? Let me know. |
| rm -f /var/run/rsyslogd.pid | ||
|
|
||
| service rsyslog start | ||
| service frr start |
There was a problem hiding this comment.
we already have supervised to start frr process, it is good to unified all process management inside the docker. We have unified way to manage process restart, docker restart, process crash messages. I do not really understand why we need to change that.
There was a problem hiding this comment.
I don't really understand either why we had to change from systemd in the first place. There was no functional requirement in the first place to change to supervisord. I don't see the goodness in unifying all process management inside the docker under supervisord especially since we ended up with multiple problems out of it. The routing application should be standalone and free of any such dependancies. It also doesn't need this unification. These changes were agreed in the August meeting we had and I sent meeting minutes. I also fail to understand why we are holding this PR as hostage when it fixes 5 blocking critical issues for a month now.
…atically (#20268) #### Why I did it src/sonic-utilities ``` * ed624895 - (HEAD -> master, origin/master, origin/HEAD) SONIC CLI for CLI-Sessions feature (#3175) (2 hours ago) [i-davydenko] * c6637553 - Move from bootctl to mokutil when checking for Secure Boot status (#3486) (3 hours ago) [DavidZagury] * 5fc0ee6c - [spm]: Clean up timers auto generation logic. (#3523) (3 hours ago) [Nazarii Hnydyn] ``` #### How I did it #### How to verify it #### Description for the changelog
Diffs are fixing the following issues: #3194 #3195 #3196 #3197 #3484
As agreed, frr service is reverted back to being controlled via systemd and its daemons via watchfrr. This is fixing a bunch of issues encountered and also allows for incremental configs to be pushed to frr in a non-disruptive way. The bgpcfgd daemon and fpm, will continue being under supervisord's control.
@lguohan @pavel-shirshov Please review.
FYI: @ben-gale @MichelMoriniaux @mslocrian @heidinet2007