FRRouting / frr

The FRRouting Protocol Suite
https://frrouting.org/
Other
3.35k stars 1.25k forks source link

frr-reload always seems to reapply configs #6062

Open devicenull opened 4 years ago

devicenull commented 4 years ago

Starting with https://github.com/FRRouting/frr/commit/3fa139a65be70e5a81b5f078530094f09a247416, frr-reload seems to always try to reapply the running config, even if nothing has changed.

Note how the revision immediately before this produces no diffs (as it should be)

2020-03-20 11:54:54,030  INFO: Called via "Namespace(bindir=u'/usr/bin', confdir=u'/etc/frr', daemon=u'', debug=False, filename='/etc/frr/frr.conf', input=None, overwrite=False, reload=False, rundir=u'/var/run/frr', stdout=False, test=True)"
2020-03-20 11:54:54,030  INFO: Loading Config object from file /etc/frr/frr.conf
2020-03-20 11:54:54,098  INFO: Loading Config object from vtysh show running
[root@nj1207 testconfigs]# ./frr-reload.py.3fa139a65be70e5a81b5f078530094f09a247416 /etc/frr/frr.conf  --test
2020-03-20 11:54:57,448  INFO: Called via "Namespace(bindir=u'/usr/bin', confdir=u'/etc/frr', daemon=u'', debug=False, filename='/etc/frr/frr.conf', input=None, overwrite=False, reload=False, rundir=u'/var/run/frr', stdout=False, test=True)"
2020-03-20 11:54:57,448  INFO: Loading Config object from file /etc/frr/frr.conf
2020-03-20 11:54:57,516  INFO: Loading Config object from vtysh show running
!
frr version 7.4-dev
frr defaults traditional
hostname nj1207
service integrated-vtysh-config
!
router bgp 64515
 bgp router-id 66.55.136.229
 bgp graceful-restart restart-time 3
!
end
line vty
!

end

Lines To Add
============

frr version 7.4-dev

frr defaults traditional

hostname nj1207

service integrated-vtysh-config

router bgp 64515

router bgp 64515
 bgp router-id 66.55.136.229

router bgp 64515
 bgp graceful-restart restart-time 3

line vty
devicenull commented 4 years ago

It's because the sh run command actually outputs to stderr:

[root@nj1207 testconfigs]# /usr/bin/vtysh --config_dir /etc/frr -c 'show run ' | /usr/bin/tail -n +4 | /usr/bin/vtysh --config_dir /etc/frr -m -f -
!
frr version 7.4-dev
frr defaults traditional
hostname nj1207
service integrated-vtysh-config
!
router bgp 64515
 bgp router-id 66.55.136.229
 bgp graceful-restart restart-time 3
!
end
line vty
!

end
[root@nj1207 testconfigs]# /usr/bin/vtysh --config_dir /etc/frr -c 'show run ' | /usr/bin/tail -n +4 | /usr/bin/vtysh --config_dir /etc/frr -m -f - 2>/dev/null
[root@nj1207 testconfigs]#
devicenull commented 4 years ago

It's because vtysh in markfile mode writes to stderr:

https://github.com/FRRouting/frr/blob/e3ee60b18bdfb73a369b81d57944a1c657ef2f56/vtysh/vtysh.c#L711

I'm not sure what the correct fix is (not sure why this output is going to stderr)

nix-power commented 4 years ago

in 7.3.1 i am facing issue that frr reload applies runing config even when config file changes (/etc/frr/frr.conf)

I have checked the functionality in 7.2.1 and it found working as expected

dteach-rv commented 4 years ago

It looks like there was a concerted effort to handle the vtysh -> stderr output change in frr-reload.py. But the subprocess call in def load-from_show_running was missed, which is why frr-reload.py is trying to re-add every command. It's comparing the file config to an empty running config.

adding stderr=subprocess.STDOUT to the subprocess.check_output call seems to have fixed the issue for me. try: config_text = subprocess.check_output( bindir + "/vtysh --config_dir " + confdir + " -c 'show run " + daemon + "' | /usr/bin/tail -n +4 | " + bindir + "/vtysh --config_dir " + confdir + " -m -f -", shell=True, stderr=subprocess.STDOUT)

dteach-rv commented 4 years ago

I also noticed that "exit" as a context termination line is being grouped with "exit-address-family" and "exit-vnc" as a subcontext. This breaks stanzas like rpki rpki polling_period 3600 rpki cache x.x.x.x 3323 preference 1 rpki cache y.y.y.y 3323 preference 2 exit I'm not sure if exit is a valid subcontext terminator with bgpd or other daemons. My my purposes, I moved it out of that elif statement and added it to the "exit-vrf" elif statement.