upgrade FRR to version 10.0.1, upgrade libyang2 to 2.1.148. #20269
Conversation
sudhanshukumar22
requested review from
qiluo-msft,
xumia and
lguohan
as code owners
sudhanshukumar22
force-pushed
the
frr_10.0.1_upgrade
branch
3 times, most recently
from
8b65364
to
41c2e20
Compare
|
I see that all other builds except vs build is successful. This means FRR patches were OK there. But in VS build, there are whitespace errors. Can we avoid whitespace errors in VS build. Probably the git version in VS build is old one ? 24-09-18T13:04:08.3140753Z mv frr-pythontools_10.0.1-sonic-0_all.deb frr-dbgsym_10.0.1-sonic-0_amd64.deb frr-snmp_10.0.1-sonic-0_amd64.deb frr-snmp-dbgsym_10.0.1-sonic-0_amd64.deb frr_10.0.1-sonic-0_amd64.deb /sonic/target/debs/buster/ |
sudhanshukumar22
force-pushed
the
frr_10.0.1_upgrade
branch
4 times, most recently
from
f5d881c
to
6471cba
Compare
|
/azp run Azure.sonic-buildimage |
|
Commenter does not have sufficient privileges for PR 20269 in repo sonic-net/sonic-buildimage |
sudhanshukumar22
force-pushed
the
frr_10.0.1_upgrade
branch
4 times, most recently
from
941cc35
to
7103396
Compare
|
@cscarpitta , @liushilongbuaa @kperumalbfn can you check why elastic tests are failing though builds are going fine. |
|
/azpw ms_conflict |
|
/azpw ms_checker |
|
@StormLiangMS Can you please help fix the ms_conflict? |
There was a problem hiding this comment.
@sudhanshukumar22 There is no FRR submodule update in this PR. Can you please update submodule?
| @@ -67,7 +67,9 @@ RUN apt-get install -y net-tools \ | |||
| # For libkrb5-dev | |||
| comerr-dev \ | |||
| libgssrpc4 \ | |||
| libkdb5-10 | |||
| libkdb5-10 \ | |||
| libprotobuf-c-dev \ | |||
There was a problem hiding this comment.
Why are these changes required?
There was a problem hiding this comment.
when we upgraded the FRR to 10.0.1, it needs these new packages to compile.
src/libyang2/Makefile
Outdated
| @@ -2,10 +2,10 @@ | |||
| SHELL = /bin/bash | |||
| .SHELLFLAGS += -e | |||
|
|
|||
| LIBYANG_URL = https://sonicstorage.blob.core.windows.net/debian/pool/main/liby/libyang | |||
| LIBYANG_URL = https://deb.debian.org/debian/pool/main/liby/libyang2 | |||
There was a problem hiding this comment.
Why is this URL change required? Should we rather update sonicstorage to cache libyang2?
There was a problem hiding this comment.
Currently, libyang2 was not present in the sonicstorage repository. So, we had to change the path. I don't know how to update the sonic storage path with these libraries.
There was a problem hiding this comment.
I agree with Sudharasan, SONIC should not be grabbing libyang2 from an outside repository as that you have no ability to keep the version the same or control it.
|
|
||
| DSC_FILE = libyang2_$(LIBYANG2_FULLVERSION).dsc | ||
| ORIG_FILE = libyang2_$(LIBYANG2_VERSION).orig.tar.gz | ||
| ORIG_FILE = libyang2_$(LIBYANG2_VERSION).orig.tar.xz |
There was a problem hiding this comment.
Is the file format change expected?
There was a problem hiding this comment.
Yes, the file present in repository is libyang2_2.1.148.orig.tar.xz
| #The package libyang2.1.148 is taken from debian trixie, which only has dpkg-dev version 1.21.22 | ||
| #The bullseye package has dpkg-dev version 1.20.13 | ||
| #The VS package has dpkg-dev version 1.19.8 | ||
| sed -i 's/dpkg-dev (>= 1.22.5)/dpkg-dev (>= 1.19.8)/' debian/control |
There was a problem hiding this comment.
I don't think this is the right way to do it. It changes both for bullseye and bookworm. Can we be specific here so that this change is applicable only for bullseye
There was a problem hiding this comment.
The current master branch has been built for bookworm platform. But, I found during compilation that we are compiling FRR for bookworm version of debian, bullseye version and VS platform also. Hence, we need to take care for all 3 versions.
| @@ -6,30 +6,12 @@ | |||
| 0006-Link-local-scope-was-not-set-while-binding-socket-for-bgp-ipv6-link-local-neighbors.patch | |||
There was a problem hiding this comment.
Can we make sure we change the patch number to align with the removed patches?
There was a problem hiding this comment.
I wanted to keep the old patches(which are no longer applicable) so that upgrading from the earlier FRR will be easy.
| @@ -27,12 +34,12 @@ index b1f8f19594..bb3cd62950 100644 | |||
| - zebra_announce_del(&bm->zebra_announce_head, dest); | |||
| } | |||
| } | |||
|
|
|||
| bgp_evpn_remote_ip_hash_destroy(vpn); | |||
There was a problem hiding this comment.
Why is this change introduced?
There was a problem hiding this comment.
The patch was failing to be applied in its current shape. So, I had to manually apply the existing patch and recreate the patch file backwards.
Note that bgp_evpn_remote_ip_hash_destroy(vpn); is an existing code. It has not been added by me. This line has been autogenerated during patch creation by git diff command,
| bgp_evpn_remote_ip_hash_destroy(vpn); | ||
| bgp_evpn_vni_es_cleanup(vpn); |
There was a problem hiding this comment.
Can you please clarify why is this change introduced?
There was a problem hiding this comment.
The patch was failing to be applied in its current shape. So, I had to manually apply the existing patch and recreate the patch file backwards.
Note that bgp_evpn_vni_es_cleanup(vpn); is an existing code. It has not been added by me. This line has been autogenerated during patch creation by git diff command,
There was a problem hiding this comment.
Also, see in line number 34, we have a space. In the new patch apply, we have a strict whitespace check.
| @@ -630,6 +630,7 @@ static inline struct nexthop_group *rib_get_fib_backup_nhg( | ||
| } | ||
|
|
||
| @@ -628,6 +628,7 @@ extern int rib_add_gr_run(afi_t afi, vrf_id_t vrf_id, uint8_t proto, |
There was a problem hiding this comment.
Do we need it as part of this patch?
There was a problem hiding this comment.
This line is autogenerated. This file is not modified by me in the patch. This is an existing patch, but failed to apply cleanly because of changes in the files. I manually applied them and created the patch backwards, Probably the functions must have been moved in the newer files. So, the patch is showing a new line 628.
Old patch file (0046-zebra-Modify-show-zebra-dplane-providers-to-give-mor.patch)
diff --git a/zebra/rib.h b/zebra/rib.h
index 2e62148ea0..b78cd218f6 100644
--- a/zebra/rib.h
+++ b/zebra/rib.h
@@ -630,6 +630,7 @@ static inline struct nexthop_group *rib_get_fib_backup_nhg(
}
extern void zebra_vty_init(void);
+extern uint32_t zebra_rib_dplane_results_count(void);
extern pid_t pid;
New patch file(0046-zebra-Modify-show-zebra-dplane-providers-to-give-mor.patch)
diff --git a/zebra/rib.h b/zebra/rib.h
index a721f4bac..15f877b66 100644
--- a/zebra/rib.h
+++ b/zebra/rib.h
@@ -628,6 +628,7 @@ extern int rib_add_gr_run(afi_t afi, vrf_id_t vrf_id, uint8_t proto,
uint8_t instance);
extern void zebra_vty_init(void);
+extern uint32_t zebra_rib_dplane_results_count(void);
extern pid_t pid;
| From: Rajasekar Raja <rajasekarr@nvidia.com> | ||
| Date: Thu, 15 Feb 2024 11:23:51 -0800 | ||
| Subject: [PATCH 07/11] bgpd : backpressure - Handle BGP-Zebra(EPVN) Install | ||
| From 2552ac0c492cdec01e36b48b63c057c6ad162701 Mon Sep 17 00:00:00 2001 |
There was a problem hiding this comment.
@raja-rajasekar @donaldsharp can you please review this change?
| From: Donald Sharp <sharpd@nvidia.com> | ||
| Date: Thu, 25 Jan 2024 13:07:37 -0500 | ||
| Subject: [PATCH 05/11] bgpd: backpressure - cleanup bgp_zebra_XX func args | ||
| From 879558ccd9b0f4f43c708f43a3e0fcf38bebeab7 Mon Sep 17 00:00:00 2001 |
There was a problem hiding this comment.
@raja-rajasekar @donaldsharp can you please review this change?
I have updated the submodule here. |
sudhanshukumar22
force-pushed
the
frr_10.0.1_upgrade
branch
from
7103396
to
d0751b2
Compare
|
/azpw run Azure.sonic-buildimage |
|
/AzurePipelines run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sudhanshukumar22
force-pushed
the
frr_10.0.1_upgrade
branch
from
0488eb8
to
ba72f6b
Compare
|
This is mandatory feature for 202411 release. @kperumalbfn @yuezhoujk |
sudhanshukumar22
force-pushed
the
frr_10.0.1_upgrade
branch
from
026521f
to
178052c
Compare
|
/azpw run Azure.buildimage |
|
/AzurePipelines run Azure.buildimage |
|
No pipelines are associated with this pull request. |
|
/azpw run |
|
/AzurePipelines run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw ms_conflict |
sudhanshukumar22
force-pushed
the
frr_10.0.1_upgrade
branch
from
178052c
to
5b05e81
Compare
| 0030-zebra-backpressure-Zebra-push-back-on-Buffer-Stream-.patch | ||
| 0031-bgpd-backpressure-Add-a-typesafe-list-for-Zebra-Anno.patch | ||
| 0032-bgpd-fix-flushing-ipv6-flowspec-entries-when-peering.patch | ||
| 0033-bgpd-backpressure-cleanup-bgp_zebra_XX-func-args.patch | ||
| 0034-gpd-backpressure-Handle-BGP-Zebra-Install-evt-Creat.patch | ||
| 0035-bgpd-backpressure-Handle-BGP-Zebra-EPVN-Install-evt-.patch |
There was a problem hiding this comment.
It looks like many bgpd-backpressure-related patch files are from a newer FRR source. Would you consider changing the migrated version from 10.0.1 to 10.1.1 or later?
There was a problem hiding this comment.
It was decided in the community that we will upgrade to 10.0.1 and created a community branch for 10.0.1. Hence, we are going for 10.0.1 branch.
sudhanshukumar22
force-pushed
the
frr_10.0.1_upgrade
branch
from
5b05e81
to
993d8b8
Compare
|
/azpw ms_conflict |
| @@ -46,7 +46,7 @@ | |||
| [submodule "src/sonic-frr/frr"] | |||
| path = src/sonic-frr/frr | |||
| url = https://github.com/sonic-net/sonic-frr.git | |||
| branch = frr/8.5 | |||
| branch = frr-10.0.1 | |||
There was a problem hiding this comment.
any particular reason the convention for branch name has changed? Make life easy for future people in figuring out how to go forwards/backwards to older/newer versions of sonic as much as is possible.
There was a problem hiding this comment.
The initial branch (frr/10.0.1) was created by the FRR convenor. But the branch did not point to the correct FRR10.0.1 commit-id. Later, the branch could not be deleted. Now, they have created a new branch frr-10.0.1, which points to the release commit of frr10.0.1.
| @@ -1348,7 +1354,7 @@ static ssize_t netlink_srv6_msg_encode(int cmd, | |||
| if (!netlink_srv6_localsid_msg_encode( | |||
| cmd, ctx, data, datalen, fpm, force_nhg)) | |||
| return 0; | |||
| } else if (!sid_zero(&nexthop->nh_srv6->seg6_segs)) { | |||
| } else if (!sid_zero((const struct seg6_seg_stack *)nexthop->nh_srv6->seg6_segs)) { | |||
There was a problem hiding this comment.
why the (const struct seg6_seg_stack *) it's already that right?
There was a problem hiding this comment.
struct nexthop_srv6 {
/* SRv6 localsid info for Endpoint-behaviour */
enum seg6local_action_t seg6local_action;
struct seg6local_context seg6local_ctx;
/* SRv6 Headend-behaviour */
struct seg6_seg_stack *seg6_segs;
};
As we can see, nh_srv6->seg6_segs is already a pointer. Putting & was making it a double pointer. So, we removed &. That was the change.
I added (const struct seg6_seg_stack *) because it was giving a warning.
| @@ -105,11 +113,10 @@ index 69240a3b83..920df835a4 100644 | |||
| - safi, true); | |||
| + if (table->safi == SAFI_FLOWSPEC) { | |||
| + bgp_pbr_update_entry(bgp, p, info, table->afi, table->safi, | |||
| + true); | |||
| + true); | |||
There was a problem hiding this comment.
The patch is auto generated from the commit by "git format-patch -1" command.
| @@ -171,21 +171,21 @@ index 69240a3b83..920df835a4 100644 | |||
| { | |||
| struct zapi_route api; | |||
| struct peer *peer; | |||
| - | |||
There was a problem hiding this comment.
The patch is auto generated from the commit by "git format-patch -1" command.
|
The patch In all honesty having to look at a patch of a patch to figure out if something is correct or not is a pretty awful way to work. Sonic needs to figure this out in a way that allows moving forward to new versions of a code base without having to guess where things have been moved around to. |
sudhanshukumar22
force-pushed
the
frr_10.0.1_upgrade
branch
2 times, most recently
from
30ea838
to
0c4c492
Compare
[Sudhanshu] The patch 0031-bgpd-backpressure-Add-a-typesafe-list-for-Zebra-Anno.patch contained mostly header file changes and list initialisations. These codes were already present in FRR10.0.1 and were removed. Sonic community want to take FRR10.0.1 release as is. They want to maintain their changes by way of patch. The problem with this is that final code is now much different after applying the patches. Also, they have tightly coupled their testcases with the patches. So, it is difficult to remove the patch and assume that their scripts will pass, |
sudhanshukumar22
force-pushed
the
frr_10.0.1_upgrade
branch
from
84ea342
to
840a73a
Compare
…ing basic BGP neighborship formation with routes using BGP docker
sudhanshukumar22
force-pushed
the
frr_10.0.1_upgrade
branch
from
840a73a
to
e38bc1a
Compare
[component/folder touched]: Description intent of your changes
FRR upgrade in community from 8.5.4 to 10.0.1. Upgraded libyang to 2.1.148.
Tested using BGP docker, BGP neighborship with route learning worked fine.
[List of changes]
applied patch
0001-Reduce-severity-of-Vty-connected-from-message.patch
0002-Allow-BGP-attr-NEXT_HOP-to-be-0.0.0.0-due-to-allevia.patch
0003-nexthops-compare-vrf-only-if-ip-type.patch
0004-frr-remove-frr-log-outchannel-to-var-log-frr.log.patch
0005-Add-support-of-bgp-l3vni-evpn.patch
0006-Link-local-scope-was-not-set-while-binding-socket-for-bgp-ipv6-link-local-neighbors.patch
0007-ignore-route-from-default-table.patch
0021-Disable-ipv6-src-address-test-in-pceplib.patch
0022-cross-compile-changes.patch
0028-zebra-fix-parse-attr-problems-for-encap.patch
0036-zebra-backpressure-Fix-Null-ptr-access-Coverity-Issu.patch
0038-zebra-Actually-display-I-O-buffer-sizes.patch
0039-zebra-Actually-display-I-O-buffer-sizes-part-2.patch
0041-bgpd-backpressure-Fix-to-avoid-CPU-hog.patch
0043-zebra-Use-the-ctx-queue-counters.patch
0044-zebra-Modify-dplane-loop-to-allow-backpressure-to-fi.patch
0045-zebra-Limit-queue-depth-in-dplane_fpm_nl.patch
0047-bgpd-backpressure-fix-evpn-route-sync-to-zebra.patch
0049-bgpd-backpressure-Improve-debuggability.patch
0051-bgpd-backpressure-fix-ret-value-evpn_route_select_in.patch
0052-bgpd-backpressure-log-error-for-evpn-when-route-inst.patch
Modified patches:
0008-Use-vrf_id-for-vrf-not-tabled_id.patch
0010-bgpd-Change-log-level-for-graceful-restart-events.patch
0025-bgp-community-memory-leak-fix.patch
0030-zebra-backpressure-Zebra-push-back-on-Buffer-Stream-.patch
0033-bgpd-backpressure-cleanup-bgp_zebra_XX-func-args.patch
0034-gpd-backpressure-Handle-BGP-Zebra-Install-evt-Creat.patch
0035-bgpd-backpressure-Handle-BGP-Zebra-EPVN-Install-evt-.patch
0037-bgpd-Increase-install-uninstall-speed-of-evpn-vpn-vn.patch
0040-bgpd-backpressure-Fix-to-withdraw-evpn-type-5-routes.patch
0042-zebra-Use-built-in-data-structure-counter.patch
0046-zebra-Modify-show-zebra-dplane-providers-to-give-mor.patch
0048-bgpd-backpressure-fix-to-properly-remove-dest-for-bg.patch
0050-bgpd-backpressure-Avoid-use-after-free.patch
deleted patches:
0009-bgpd-Ensure-suppress-fib-pending-works-with-network-.patch
0011-zebra-Static-routes-async-notification-do-not-need-t.patch
0012-zebra-Rename-vrf_lookup_by_tableid-to-zebra_vrf_look.patch
0013-zebra-Move-protodown_r_bit-to-a-better-spot.patch
0014-zebra-Remove-unused-dplane_intf_delete.patch
0015-zebra-Remove-unused-add-variable.patch
0016-zebra-Remove-duplicate-function-for-netlink-interfac.patch
0017-zebra-Add-code-to-get-set-interface-to-pass-up-from-.patch
0018-zebra-Use-zebra-dplane-for-RTM-link-and-addr.patch
0019-zebra-remove-duplicated-nexthops-when-sending-fpm-msg.patch
0020-zebra-Fix-non-notification-of-better-admin-won.patch
0023-zebra-The-dplane_fpm_nl-return-path-leaks-memory.patch
0024-lib-use-snmp-s-large-fd-sets-for-agentx.patch
0026-bgp-fib-suppress-announce-fix.patch
0027-lib-Do-not-convert-EVPN-prefixes-into-IPv4-IPv6-if-n.patch
0029-zebra-nhg-fix-on-intf-up.patch
0031-bgpd-backpressure-Add-a-typesafe-list-for-Zebra-Anno.patch
0032-bgpd-fix-flushing-ipv6-flowspec-entries-when-peering.patch
0053-bgpd-Set-md5-TCP-socket-option-for-outgoing-connections-on-listener.patch
Tests performed: Tested BGP neighborship formation and route learning using BGP docker
Signed-off-by: sudhanshu.kumar@broadcom.com