Skip to content
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

Closed

Conversation

gtataranni
Copy link
Contributor

Backport fixes for frr-reload.py to stable/8.5 branch from master

@ton31337
Copy link
Member

ton31337 commented Sep 2, 2024

While this is OKay-ish, we should also align with upper releases too...

@gtataranni
Copy link
Contributor Author

While this is OKay-ish, we should also align with upper releases too...

you mean opening the same (similar) PR towards stable/10.0 and stable/10.1 too?
I can do that, if that's what you mean

@ton31337
Copy link
Member

ton31337 commented Sep 2, 2024

Yes, stable/10.1, stable/10.0, stable/9.1, and stable/9.0.

Copy link
Member

@Jafaral Jafaral left a 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.

@gtataranni
Copy link
Contributor Author

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?

...and other changes too.

Can you point those out?

@Jafaral
Copy link
Member

Jafaral commented Sep 3, 2024

Some of the fixes also don't even apply to 8.5.

@Jafaral
Copy link
Member

Jafaral commented Sep 3, 2024

Can you point those out?

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.

@Jafaral
Copy link
Member

Jafaral commented Sep 3, 2024

From a quick scan without looking at every commit:
781b6bc formatting.
c469dab does't apply to 8.5.

@gtataranni
Copy link
Contributor Author

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.

pulling all changes from master back to the old branch 8.5 carries some risk. It could break things in unexpected ways.

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? :)

@Jafaral
Copy link
Member

Jafaral commented Sep 3, 2024

Fair. However you are suggesting to just use it in previous sentence...

That is a risk you have to accept in your own environment. That is different form merging all of these changes upstream :-)

@gtataranni
Copy link
Contributor Author

OK, I will review the commits to not/include in this PR, so to include only fixes, and re-request review.
I will do my best exclude those changes not related to 8.5: this is where I will need the most support from the reviewers/maintainers.
Regarding formatting changes, I am still of the opinion that these could be backported as they do not add any risk, and will facilitate future backports instead. Ultimately though, the decision is yours.

@gtataranni gtataranni marked this pull request as draft September 3, 2024 17:31
@gtataranni gtataranni force-pushed the backport/frr-reload-10.0 branch from cf60747 to 59ac423 Compare September 5, 2024 09:51
@github-actions github-actions bot added size/L and removed size/XL labels Sep 5, 2024
@gtataranni gtataranni marked this pull request as ready for review September 5, 2024 09:52
@gtataranni gtataranni requested a review from Jafaral September 5, 2024 09:52
@gtataranni gtataranni changed the title Backport frr-reload.py fixes from master tools: backport frr-reload.py fixes from master Sep 5, 2024
@Jafaral
Copy link
Member

Jafaral commented Sep 14, 2024

ci:rerun

@gtataranni
Copy link
Contributor Author

gtataranni commented Jan 13, 2025

@Jafaral Changes done, please review again.
No rebase needed as this PR is against stable/8.5, and there are no changes there. However, we'd like to include (backport) #16723 too.

@gtataranni
Copy link
Contributor Author

ci:rerun

chiragshah6 and others added 5 commits January 13, 2025 15:56
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]>
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]>
ton31337 and others added 2 commits January 13, 2025 15:56
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]>
@gtataranni gtataranni force-pushed the backport/frr-reload-10.0 branch from 59ac423 to 1b805ff Compare January 13, 2025 14:56
@gtataranni
Copy link
Contributor Author

I rebased this on to stable/8.5, I did a mistake before and did not realise that also moved

Copy link
Member

@ton31337 ton31337 left a 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?

@gtataranni
Copy link
Contributor Author

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

@gtataranni
Copy link
Contributor Author

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.

@gtataranni gtataranni closed this Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants