Page MenuHomeVyOS Platform

frrender: after a failed FRR apply, later commits do not reapply the FRR configuration
In progress, HighPublicBUG

Description

When a commit's FRR apply fails or is cut off (for example bgpd is killed by watchfrr while frr-reload runs), vyos still treats the new FRR configuration as applied. A retry of the same change prints "No configuration changes to commit", and unrelated commits (interfaces, system settings) do not run frr-reload at all. The CLI shows the new configuration while FRR keeps running the old one, until a later commit changes the FRR configuration again.

Cause: FRRender.generate() stores cached_config_dict (and cached_dhcp_gateways) before apply() runs, and the next commit skips the reload when the new dict equals the cache. The cache is not compared with what FRR runs.

Steps to reproduce:

  1. Stop FRR.
  2. Commit set protocols bgp parameters router-id 192.0.2.5: the commit fails at the FRR step.
  3. Start FRR again and retry the same commit: it reports success, but frr-reload does not run. FRR keeps router-id 192.0.2.1 while the CLI shows 192.0.2.5.
  4. An unrelated commit (login banner) does not change that.
  5. Only a commit that changes the FRR configuration (here a route-map description) brings FRR in line with the CLI.

With below fix, the retry in step 3 runs frr-reload and FRR gets the router-id the CLI shows.

Possible fix direction. In python/vyos/frrender.py, the cache could be updated only after the configuration has reached FRR:

  • generate() would keep the newly rendered configuration aside instead of updating the cache;
  • apply() would update the cache only after frr-reload and the save succeed;
  • after a failed render, reload or save the cache would stay empty, so the next commit runs frr-reload again.

It would change behaviour: after a failed FRR apply, the next commit would reload FRR once, even if it does not change the FRR configuration (measured with the local change). If FRR were still down at that point, that commit would fail at the reload instead of reporting success (from the code; not measured).

Details

Version
1.5.1, 1.5-rolling-202610041311
Is it a breaking change?
Unspecified (possibly destroys the router)
Issue type
Bug (incorrect behavior)

Event Timeline

I reproduced this on a rolling VM and have a fix. I can not open a pull request for it: GitHub limits me to 5 open pull requests in vyos/vyos-1x and all 5 are in use. The commit is on my fork, anyone can take it as it is or I open the PR as soon as a slot is free:

https://github.com/ruben-herold/vyos-1x/commit/5189df1f8 (branch T9442-frrender-cache)

Reproduction in one session, as in the description: set router-id 192.0.2.1, commit, stop FRR, change the router-id to 192.0.2.5, commit (fails at the FRR step), start FRR, commit again. The retry reports success, but FRR still has 192.0.2.1 while the CLI shows 192.0.2.5.

The cause is the one from the description. generate() sets cached_config_dict and cached_dhcp_gateways before apply() runs, so a failed or cut off apply leaves the new configuration cached as applied.

The fix keeps the rendered configuration aside in generate() and makes it the cached one at the end of apply(), after frr-reload.py succeeded. A render that does not reach apply() is not cached either. vyos-configd, vyos-commitd and vyos-frr-render.py all call if frr.generate(...): frr.apply(), so they need no change. With the fix the retry in the reproduction reloads FRR and FRR has 192.0.2.5.

What the commit contains:

  • python/vyos/frrender.py (+18/-3)
  • src/tests/test_frrender.py, a unit test with a mocked frr-reload.py and five cases (applied is cached, rendered but not applied is not, failed reload test is not, failed reload is not, retry after a failure is applied). With the old code four of them fail, with the fix all five pass.

Tested on a VM:

  • the reproduction above
  • test_protocols_static.py: 8 tests OK, 3 skipped
  • test_protocols_bgp.py, every test run on its own: 39 of 41 pass. test_bgp_30_import_vrf_routemap hangs in the vrf.py commit and test_bgp_107_default_local_pref_default_value fails, both also with the unmodified frrender.py, so they are not caused by this change.

The trade-off is the one noted in the description: after a failed apply the next commit reloads FRR once, even if it did not change the FRR configuration, and if FRR is still down that commit fails at the reload instead of reporting success.

Not tested: the upgrade path and a failure while the reload is half done.