-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tools: backport frr-reload.py fixes from master #16722
tools: backport frr-reload.py fixes from master #16722
Conversation
While this is OKay-ish, we should also align with upper releases too... |
you mean opening the same (similar) PR towards |
Yes, stable/10.1, stable/10.0, stable/9.1, and stable/9.0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backport should only contain bug fixes. I see formatting and other changes too.
Removing formatting changes will take me more time now and more time from next developer who wants to backport a fix that is next to a line with a format diff. Is it really worth removing from this PR?
Can you point those out? |
Some of the fixes also don't even apply to 8.5. |
Why not do it the other way. If you see a problem in production in 8.5 at your end that is fixed on master. Why not just pull that? pulling all changes from master back to the old branch 8.5 carries some risk. It could break things in unexpected ways. Also, frr-reload is just a python script, if you really want all of these changes, why not just copy the script directly into the systems where you want it? you don't even need to update frr. |
We already do, but is then hard to maintain with divergence from upstream.
Fair. However you are suggesting to just use it in previous sentence... Why don't we work together and look at each one of those commits and decide which ones are worth backporting and which ones are not or too risky? :) |
That is a risk you have to accept in your own environment. That is different form merging all of these changes upstream :-) |
OK, I will review the commits to not/include in this PR, so to include only fixes, and re-request review. |
cf60747
to
59ac423
Compare
ci:rerun |
ci:rerun |
OSPFv2 no area x stub no-summary only resets 'no-summary' config. From frr-reload if the config line 'area x stub no-summary' is removed then it needs to remove completely. Before this change it took two frr-roload to remove the config which is inconsistent behavior. Fix is to frr-reload to add extra line to delete 'no area x stub'. Ticket:#3514775 Testing Done: Running config: router ospf ospf router-id 6.6.6.6 area 0.0.0.1 stub no-summary area 0.0.0.2 stub exit ! router ospf vrf sym_1 area 0.0.1.1 range 24.1.1.0/24 area 0.0.1.2 stub no-summary exit changed frr.conf: router ospf ospf router-id 6.6.6.6 area 0.0.0.2 stub exit ! router ospf vrf sym_1 area 0.0.1.1 range 24.1.1.0/24 exit Lines To Delete =============== router ospf no area 0.0.0.1 stub <<<< newly added router ospf vrf sym_1 no area 0.0.1.2 stub <<<< newly added router ospf no area 0.0.0.1 stub no-summary router ospf vrf sym_1 no area 0.0.1.2 stub no-summary After fix new running-config post reload: i router ospf ospf router-id 6.6.6.6 area 0.0.0.2 stub exit ! router ospf vrf sym_1 area 0.0.1.1 range 24.1.1.0/24 exit Before fix running-config post 1st reload: router ospf ospf router-id 6.6.6.6 area 0.0.0.1 stub area 0.0.0.2 stub exit ! router ospf vrf sym_1 area 0.0.1.1 range 24.1.1.0/24 area 0.0.1.2 stub exit Signed-off-by: Chirag Shah <[email protected]>
When reloading the following config: ``` router ospf6 area 0 range 2001:db8::/32 advertise exit ! interface eth0 ipv6 ospf6 area 0 exit ``` frr-reload.py doesn't execute "exit" commands. Because of that, it tries to execute "interface eth0" inside the "router ospf6" context and fails. To always execute all commands from the correct context, frr-reload.py should properly exit from every entered node. Fixes FRRouting#10132. Signed-off-by: Igor Ryzhov <[email protected]>
…eload.py Signed-off-by: Donatas Abraitis <[email protected]>
if frr.conf file contains 'interface x vrf <name> config it causes protocol (like ospf) neighbor session flap, as it deletes interface base config line ('interface x') from running config and readds with 'interface x vrf <name>' line from frr.conf. This deletion and readdition of lines leads to neighborship flaps. This issue is by product of (PR-10411 | FRRouting#10411) (commit id: 788a036) where running config for interface config no loger displays associated vrf line. Ticket: #3858146 Testing: frr.conf interface swp1.2 vrf vrf1012 ip ospf network point-to-point running-config: interface swp1.2 ip ospf network point-to-point exit Before fix: frr-reload logs: 2024-04-09 00:28:31,096 INFO: Executed "interface swp1.2 no ip ospf network point-to-point exit" 'interface swp1.2 vrf vrf1012\n ip ospf network point-to-point\nexit\n', After fix: frr-reload strips vrf line, thus no config change between frr.conf and running config. Signed-off-by: Chirag Shah <[email protected]>
If frr.conf has bgp as-path access-list clause without sequence number then upon performing frr-rleoad, the running config clause with sequence number will always be deleted and the new ones without sequence will be re-added. This could lead to blackholing until the config gets reapplied. Testing: frr.conf: bgp as-path access-list important_internet_bgp_as_numbers permit _16509_ Running config: bgp as-path access-list important_internet_bgp_as_numbers seq 5 permit _16509_ ! Before fix Upon frr-reload it deletes and readd line as without seq 2024-04-26 03:16:45,772 INFO: Executed "no bgp as-path access-list important_internet_bgp_as_numbers seq 5 permit _16509_" 'bgp as-path access-list important_internet_bgp_as_numbers permit _16509_\n' After fix: no form is not executed and no delta determine between frr.conf and running-config. Signed-off-by: Chirag Shah <[email protected]>
When using a regex (or anything that uses `\?` escapes) in python, raw strings (`r"content"`) should be used so python doesn't consume the escapes itself. Otherwise we get either broken behavior and/or `SyntaxWarning: invalid escape sequence '\['` Fixes: 8916953 ("build: fix a few python string escape warnings") Fixes: FRRouting#16522 Signed-off-by: Donatas Abraitis <[email protected]>
fix usage of regex string without proper escaping Signed-off-by: Giovanni Tataranni <[email protected]>
59ac423
to
1b805ff
Compare
I rebased this on to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, are you planning to do the same for 9.x, 10.x where these commits are missing?
Not planning this really, but would be nice to have |
I won't be able to investigate on errors in the CI, therefore I'm closing this PR. If there is interest to pick this one up again, feel free to reopen it. |
Backport fixes for frr-reload.py to stable/8.5 branch from master