-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
libyang: upgrade to v3 step 1 #21679
base: master
Are you sure you want to change the base?
Conversation
/azp run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
@ganglyu @qiluo-msft please review ... this needs to be merged before I can submit PRs for the migration of the libyang 1.0.73 code to v3 ... |
Hi @bradh352 , could you help me understand why |
Thank you very much for your contribution! We have encountered numerous issues with libyang 1.0.73, which is currently used by sonic-yang-models. On the other hand, libyang 1.0.184 and libyang 2.1.148, which are used by FRR, have not presented any issues to our knowledge. My suggestion is to install libyang3 in this PR without replacing the existing dependency. |
@@ -4,7 +4,7 @@ | |||
}, | |||
"ASIC_SENSORS_INVALID_POLLER_INTERVAL": { | |||
"desc": "Configure an invalid ASIC Sensors polling interval", | |||
"eStrKey" : "Pattern" | |||
"eStrKey" : "Range" |
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.
Why do we need to modify the unit test?
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.
From the commit message:
sonic-yang-models: Update test cases in preparation for libyang3
Convert some yang model test cases that useeStr
for failure matching
to useeStrKey
instead to standardize expected failure matching so
when messsages change they can be updated in one place instead of all
over.Also, split out some new
eStrKey
values that are needed for libyang3
due to different messages, and document the libyang3 values needed once
upgraded but keep libyang1 values for now.Finally, some tests were using both
eStr
andeStrKey
and theeStr
portion is not needed to perform a meaningful validation, plus the
eStr
in these cases are not valid for libyang3.
So basically, its just to make the review of the upgrade to libyang3 easier. I've actually got libyang3 working and passing these test cases so I wanted a bulk of the test case changes done here to make it easier to review the more complex porting of the libyang1 python swig module to libyang3 python cffi.
I honestly believe not having multiple versions of libyang to be extremely important. I think their API is stabilizing. The only thing required was to apply some commits already upstream in FRR 10.2 to support 3.7.8. From the commit message:
I think we'd be foolish to kick the can down the road while we're working on it now. I also haven't verified if libyang2 and libyang3 can be installed simultaneously like libyang1 and libyang3 can. Of course if you tell me there's no way you could possibly accept this PR because of this you leave me little choice but I don't see a sufficient argument at this time against it. |
From the commit message:
So basically, its just to make the review of the upgrade to libyang3 easier. I've actually got libyang3 working and passing these test cases so I wanted a bulk of the test case changes done here to make it easier to review the more complex porting of the libyang1 python swig module to libyang3 python cffi. |
Guess I need to figure out python wheel packaging for cross compilation to support marvell armhf (its an architecture-specific package since it uses cffi)? I clearly am not testing such a use case currently in my local environment. Anyone have any pointers for this? |
I will engage the owner of SONiC FRR to review the related changes. |
I'm not an expert, but I believe the Sonic build pipeline uses the armhf system to build the Sonic image for Marvell armhf. Therefore, we shouldn't encounter any cross-compilation issues in the Sonic build pipeline. |
Looks like azure isn't actually cross compiling at all, its running an armhf image that is advertising armv8l support which I think just means the kernel knows its a 64bit cpu ... but the system was compiled for armv7l/armhf, so python is I think determining the system based on I tried using a 64bit arm vm on my macbook to build sonic, but it immediately fails as it thinks I need to use QEMU multiarch ... even hacking the makefile to disable that and it fails later on. Parallels doesn't allow me to directly install an armhf image, so I guess I'm dead in the water trying to test this on my own hardware right now. I might need to pick up an rpi5 or something to build but that seems like it might be really really slow.... So I'll probably try to throw in some test commits to see if a simple 'setarch armhf' works around the issue. |
801cc52
to
4a1b620
Compare
/azp run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
4a1b620
to
86f10cd
Compare
/azp run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
852d821
to
8059654
Compare
This does the base upgrade from libyang2 to libyang3. It also drops the completely unused libyang1 build (as the version used is in the libyang directory which is 1.0.73). NOTE: libyang3 depends on pkgconf, which supersedes pkg-config so there is some churn to docker image building switching packages but it works for all packages as a replacement.
This also backports a series of commits from FRR to support libyang3, upgrading FRR itself isn't currently in scope: * FRRouting/frr@290c6e3 * FRRouting/frr@8e2b2ca * FRRouting/frr@22eccbf * FRRouting/frr@87c9060 We also omit the _UNINSTALLS because it messes up dependency tracking with other packages that depend on libyang3
libyang1 would treat "true" and true as identical, same with "1234" and 1234. We need to restore this behavior so we don't break users. Sent upstream here: CESNET/libyang#2344
8059654
to
c147a45
Compare
/azp run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
libyang 1.0.73 provided python bindings via swig. Those have been removed and libyang3 now requires the use of the cffi bindings which are in a separate repo. The binding names themselves are actually different ('yang' vs 'libyang') so both should be installable simultaneously. This is built as a debian package rather than a wheel since it contains binary elements.
needed for backwards compatibility for existing configurations.
Sent upstream here: CESNET/libyang-python#129
We need to force libyang3-py3 to be built and tested, and confirm it can properly co-exist with libyang1 and its python bindings. Its currently not used, but will be in a followup.
c147a45
to
cb16d28
Compare
/azp run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
cb16d28
to
7f9fa2d
Compare
/azp run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
This mostly simplifies the existing test cases and standardizes behavior where they may be incompatible with libyang3. The main goal is to have this in place before the migration to libyang3 to make it easier to evaluate the actual code review for porting aspects for libyang3. Convert some yang model test cases that use `eStr` for failure matching to use `eStrKey` instead to standardize expected failure matching so when messsages change they can be updated in one place instead of all over. Also, split out some new `eStrKey` values that are needed for libyang3 due to different messages, and document the libyang3 values needed once upgraded but keep libyang1 values for now. Finally, some tests were using both `eStr` and `eStrKey` and the `eStr` portion is not needed to perform a meaningful validation, plus the `eStr` in these cases are not valid for libyang3.
7f9fa2d
to
ba4de30
Compare
/azp run Azure.sonic-buildimage |
Azure Pipelines successfully started running 1 pipeline(s). |
@saiarcot895 any other feedback after those changes? Also any thoughts on potentially pulling in the sonic-yang-models test case migration to libyang3-py3 I have pending here: bradh352/sonic-buildimage@852d821...f171545 ? |
@bradh352 seems like there will be many PRs for the goal. Could you just submit all PR (ok to keep in draft mode), and capture them in a table in this PR description? |
Is there any way to have a discussion with someone that's an actual maintainer or manager so I can understand the process needed to actually accomplish a cross repository series of PR merges? I'm worried about the effort involved in coordination. I think I'll submit this PR as the only "non-breaking" PR. Any other PRs, however, will break building and testing until all PRs across the various projects are merged. It also means that none of the PRs will pass any tests since there are all cross-dependent. Part of the issue is the sonic buildsystem itself, where dependencies are tracked in sonic-buildimage for subprojects like swss-common, sonic-utilities, etc. Then another issue is sonic-utilities uses sonic-yang-mgmt, but there wasn't enough abstraction and they actually depend on libyang v1 directly, so once sonic-yang-mgmt is updated, sonic-utilities break. Its also not possible to piece-meal it in small chunks because libyang3-dev and libyang(1)-dev conflict, its only working to build right now because libyang(1)-dev is installed later and everything that gets built later is still depending on that. I don't see a way to have it properly uninstall and reinstall the dev packages as needed as its using Make dependencies rather than some more sophisticated build system ... and once the dependency on the "-install" is satisified, if you do the I really don't want this to get hung up and never merged due to these complexities. |
Could you just submit all PR (ok to keep in draft mode), and capture them in a table in this PR description? -> This is just for code review purpose. |
I'm honestly not sure how to submit another sonic-buildimage PR that depends on this PR. The patches won't merge cleanly as they're modifying some of the same files. I can create PRs in other repos, though. |
Nothing else from me from a packaging perspective.
One option is to include the changes in this PR into that PR. It will make that PR look bigger, but at least it may be easier to follow along. |
Thanks, may give that a shot |
PR Tracking Table
All future PRs in this series use commits stacked on top of this one. So the PRs may be larger, care will need to be taken to look at the individual commits to see what is new.
Why I did it
SONiC currently uses 3 versions of libyang: 1.0.73, 1.0.184, and 2.1.148. All are end of life and potentially have security issues.
There are also numerous libyang related bugs that have been "worked around" in SONiC, such as PR #21078. Hopefully upgrading to a modern version will resolve such issues.
This PR has been extracted from a larger changeset to make it easier to review. Once this is merged, a follow-up PR will be made to port any existing 1.0.73 code to libyang3 (this is mostly done already, but spans multiple repositories). There are some test case modifications included to simplify the review of the upgrade to the libyang3, it should be straight forward to review these changes.
I sent an email to the mailing list about this here:
https://lists.sonicfoundation.dev/g/sonic-dev/message/938
This PR is must be applied first, but there is no need to wait since this just starts setting the groundwork for the changes.
Work item tracking
N/A
How I did it
This PR gets rid of 1.0.184 as it appears unused, and upgrades 2.1.148 to 3.7.8 (including backporting FRR patches for libyang support). It also generates a build for the new libyang3 python bindings as they are no longer included in the libyang sources but are instead in an external repository. Libyang1 and Libyang3 are designed that both can be installed simultaneously (except the dev packages).
I have also already started upstreaming patches to libyang and libyang-python for inclusion for issues that have been found that impact SONiC use-cases.
Please review each commit separately in this PR, it would be easier than reviewing it as a whole. The commit messages should explain what is going on and why it is needed.
How to verify it
See that the build still succeeds and existing test cases still pass
Which release branch to backport (provide reason below if selected)
N/A
Tested branch (Please provide the tested image version)
master as of 20250207
Description for the changelog
libyang: upgrade to v3 step 1
Link to config_db schema for YANG module changes
N/A
A picture of a cute animal (not mandatory but encouraged)
Signed-off-by: Brad House (@bradh352)