Skip to content

rgw: upgrade Apache Arrow submodule to 15.0.0 - #56585

Merged
ivancich merged 3 commits into
ceph:mainfrom
ivancich:wip-upgrade-arrow+flight-15.0.2
May 10, 2024
Merged

rgw: upgrade Apache Arrow submodule to 15.0.0#56585
ivancich merged 3 commits into
ceph:mainfrom
ivancich:wip-upgrade-arrow+flight-15.0.2

Conversation

@ivancich

@ivancich ivancich commented Mar 29, 2024

Copy link
Copy Markdown
Member

Upgrades Apache Arrow. Update RGW Arrow Flight code to work with Apache Arrow API changes.

Fixes: https://tracker.ceph.com/issues/64803

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows
  • jenkins test rook e2e

@ivancich

ivancich commented Apr 1, 2024

Copy link
Copy Markdown
Member Author

jenkins test make check

@ivancich

ivancich commented Apr 1, 2024

Copy link
Copy Markdown
Member Author

jenkins test api

@ivancich

ivancich commented Apr 3, 2024

Copy link
Copy Markdown
Member Author

Ping @kalebskeithley @galsalomon66 .

@cbodley

cbodley commented Apr 3, 2024

Copy link
Copy Markdown
Contributor

strange error from 'make check'

fatal: Fetched in submodule path 'src/arrow', but it did not contain e4351b255efc67c961f797f56a5fbb3516b58953. Direct fetching of that commit failed.

@ivancich
ivancich force-pushed the wip-upgrade-arrow+flight-15.0.2 branch from 3f95e54 to 61f6223 Compare April 3, 2024 22:05
@ivancich

ivancich commented Apr 3, 2024

Copy link
Copy Markdown
Member Author

I was hoping I could make local changes to the submodule, but that generated a different commit hash, and that could not be fetched. I think to maintain local changes we'd have to either make it a subdirectory rather than a submodule, or temporarily create our own repo for the modified version.

There was an accidental ABI change that originated in 15.0.1 and continues to 15.0.2, and is expected to revert in 16.*. Do we need to care about that, @kalebskeithley and @galsalomon66 ?

@ivancich

ivancich commented Apr 4, 2024

Copy link
Copy Markdown
Member Author

jenkins test make check

@ivancich

ivancich commented Apr 4, 2024

Copy link
Copy Markdown
Member Author

jenkins test api

@ivancich

ivancich commented Apr 5, 2024

Copy link
Copy Markdown
Member Author

So this isn't building because Arrow apparently wants xsimd version >= 8.1.0. Need to figure out how best to resolve that.

@galsalomon66

Copy link
Copy Markdown
Contributor

I was hoping I could make local changes to the submodule, but that generated a different commit hash, and that could not be fetched. I think to maintain local changes we'd have to either make it a subdirectory rather than a submodule, or temporarily create our own repo for the modified version.

There was an accidental ABI change that originated in 15.0.1 and continues to 15.0.2, and is expected to revert in 16.*. Do we need to care about that, @kalebskeithley and @galsalomon66 ?

i did a limited test(standalone application) against 15.0.2, so .. I can not tell something about this ABI change.
do you know what is broken? can we use a different version (15.0.0)?

@cbodley

cbodley commented Apr 5, 2024

Copy link
Copy Markdown
Contributor

So this isn't building because Arrow apparently wants xsimd version >= 8.1.0. Need to figure out how best to resolve that.

i see that epel9 has xsimd-devel-9.0.1 but ubuntu jammy (22.04LTS) is on libxsimd-dev 7.6.0. ubuntu 23.04 has 8.1.0 but we'd probably need to add a submodule as long as we support jammy

@badone

badone commented Apr 9, 2024

Copy link
Copy Markdown
Contributor

Fails to build on F39 as well with a cmake ERROR.

CMake Error at cmake_modules/ThirdpartyToolchain.cmake:307 (find_package): By not providing "Findxsimd.cmake" in CMAKE_MODULE_PATH this project has asked CMake to find a package configuration file provided by "xsimd", but CMake did not find one.

If I install xsimd-devel manually it gets past that error and builds successfully so might need to add that package to install-deps of some other dependency list?

@ivancich

ivancich commented Apr 9, 2024

Copy link
Copy Markdown
Member Author

I was hoping I could make local changes to the submodule, but that generated a different commit hash, and that could not be fetched. I think to maintain local changes we'd have to either make it a subdirectory rather than a submodule, or temporarily create our own repo for the modified version.
There was an accidental ABI change that originated in 15.0.1 and continues to 15.0.2, and is expected to revert in 16.*. Do we need to care about that, @kalebskeithley and @galsalomon66 ?

i did a limited test(standalone application) against 15.0.2, so .. I can not tell something about this ABI change. do you know what is broken? can we use a different version (15.0.0)?

Yeah, I'm OK using 15.0.0 until the ABI issue is correctred in 16.*.

@ivancich
ivancich force-pushed the wip-upgrade-arrow+flight-15.0.2 branch from 61f6223 to 3be99f0 Compare April 9, 2024 15:40
@ivancich ivancich changed the title rgw: upgrade Apache Arrow submodule to 15.0.2 rgw: upgrade Apache Arrow submodule to 15.0.0 Apr 9, 2024
@ivancich

ivancich commented Apr 9, 2024

Copy link
Copy Markdown
Member Author

Please note: the branch name (ivancich:wip-upgrade-arrow+flight-15.0.2) is now a misnomer since we're in fact upgrading to 15.0.0. But there's now way to rename a branch in a PR other than to create a new PR and lose the easy access to the discussion.

@ivancich
ivancich force-pushed the wip-upgrade-arrow+flight-15.0.2 branch from 3be99f0 to fbea608 Compare April 9, 2024 20:37
@badone

badone commented Apr 10, 2024

Copy link
Copy Markdown
Contributor

I can confirm this now resolves the issue with building on f39, thanks @ivancich

@badone

badone commented Apr 10, 2024

Copy link
Copy Markdown
Contributor

jenkins test make check

@badone

badone commented Apr 10, 2024

Copy link
Copy Markdown
Contributor

jenkins test api

@ivancich

Copy link
Copy Markdown
Member Author

@galsalomon66 I believe I've resolved the xsimd issue. It's now not building due to arrow changes forcing s3select changes. Are you ready with the minimal patch or commit off of main that would make s3select compatible with Arrow 15.0.0, so we can add it to this PR and get it merged?

Comment thread cmake/modules/BuildArrow.cmake Outdated
Comment on lines 72 to 74

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much better than another submodule 👍

@galsalomon66

galsalomon66 commented May 2, 2024

Copy link
Copy Markdown
Contributor

@ivancich

ceph/s3select#155

this PR contains changes related to v15.0.0
the changes are in the s3select-test-suite which does not participate in ceph-build
and are not part of the teuthology.

it is not mandatory to merge it.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
@ivancich
ivancich force-pushed the wip-upgrade-arrow+flight-15.0.2 branch 3 times, most recently from 61f6223 to cc2adc7 Compare May 3, 2024 16:17
@ivancich

ivancich commented May 3, 2024

Copy link
Copy Markdown
Member Author

So it looks like this will pass CI tests as I added a commit to the s3select submodule and then pulled in the updated submodule.

@ivancich
ivancich requested a review from cbodley May 3, 2024 18:53
Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
@ivancich
ivancich force-pushed the wip-upgrade-arrow+flight-15.0.2 branch from cc2adc7 to b035d92 Compare May 3, 2024 18:57
@ivancich ivancich added the wip-eric-testing-2 for ivancich testing label May 6, 2024
@ivancich

ivancich commented May 9, 2024

Copy link
Copy Markdown
Member Author

jenkins test make check

@cbodley

cbodley commented May 9, 2024

Copy link
Copy Markdown
Contributor

Unmodified Submodules
please include 'arrow submodule' in your commit message, if this change is intentional.

@mattbenjamin

Copy link
Copy Markdown
Contributor

yay!

…sues

Update arrow submodule.

Signed-off-by: J. Eric Ivancich <ivancich@redhat.com>
@ivancich
ivancich force-pushed the wip-upgrade-arrow+flight-15.0.2 branch from b035d92 to 06d0574 Compare May 9, 2024 18:25
@ivancich
ivancich merged commit 1dd82d9 into ceph:main May 10, 2024
@ivancich ivancich removed needs-qa wip-eric-testing-2 for ivancich testing labels May 10, 2024
@Svelar

Svelar commented Jul 29, 2024

Copy link
Copy Markdown
Member

Should this be backported to squid?

@cbodley

cbodley commented Jul 29, 2024

Copy link
Copy Markdown
Contributor

Should this be backported to squid?

we weren't planning to backport. is there a need for it?

@Svelar

Svelar commented Jul 30, 2024

Copy link
Copy Markdown
Member

we weren't planning to backport. is there a need for it?

apache/arrow#13902
From v10.0.0, RISC-V architecture are suppoerted.

@ThomasLamprecht

ThomasLamprecht commented Jun 3, 2025

Copy link
Copy Markdown
Contributor

Should this be backported to squid?

we weren't planning to backport. is there a need for it?

@cbodley The old arrow version fails compilation with upcoming Debian 13 "Trixie"'s libabseil version, as the old arrow version is using C++11 while Trixie's abseil lib is requiring at least C++17. This might be fixed otherwise, e.g., I did not look into how bad it would be to compile the old arrow version as C++17, but updating arrow would reduce some tech debt and fix it too.

As Ceph Squid is currently the most recent stable release it would be great to have this fixed in one way or another.
FWIW, I could try to prepare a backport if that's preferred and nobody jumps on this, as I should soon™ be finished with evaluating such a backport locally anyway.

Edit: forgot to mention: Debian currently does not have a libabseil an apache arrow package in its repositories, so one cannot simply turn on the WITH_SYSTEM_ARROW flag and that's why the built-in version matters a bit more there.

@cbodley

cbodley commented Jun 3, 2025

Copy link
Copy Markdown
Contributor

As Ceph Squid is currently the most recent stable release it would be great to have this fixed in one way or another. FWIW, I could try to prepare a backport if that's preferred and nobody jumps on this, as I should soon™ be finished with evaluating such a backport locally anyway.

great, no objection to backporting

Edit: forgot to mention: Debian currently does not have a libabseil package in it's repositories, so one cannot simply turn on the WITH_SYSTEM_ARROW flag and that's why the built-in version matters a bit more there.

thanks. it would be nice to address this, because it would allow Ceph to drop this submodule entirely (WITH_SYSTEM_ARROW was made the default for rpm builds a couple years ago in #46535)

@ThomasLamprecht

Copy link
Copy Markdown
Contributor

alright, so I'll try to prepare this. Should I do a single tracking issue and PR for all backports that are required to get Ceph Squid to build on Debian Trixie, or do you prefer to have these split up and submitted separately?

A bit more context: As of now I backported the Apache Arrow upgrade (#56585), using newer python APIs in mgr (#58199) that would bump the minimum required Python version to 3.9 though.

And finally there is the removal of the Python distutils package with Python 3.12 as per PEP 632, which was partially addressed in #59888 and that has even been partially backported for other reasons, the commit ffcc157 ("pybind/mgr: replace imports of distutils.util") is still missing and if one searches the code base on the current main branch one still finds quite a few uses of distutils inside various Python files (e.g. src/pybind/rbd/setup.py), so not sure yet if that part is actually fully addressed already.

But anyhow, that's now derailing the topic of this PR rather a bit much (sorry), but I'd be still thankful for a hint for my question about split PR or not.

@cbodley

cbodley commented Jun 4, 2025

Copy link
Copy Markdown
Contributor

alright, so I'll try to prepare this. Should I do a single tracking issue and PR for all backports that are required to get Ceph Squid to build on Debian Trixie, or do you prefer to have these split up and submitted separately?

i won't be able to help with review/testing of the python stuff, so i'd prefer a separate backport for arrow changes

rtgibbons pushed a commit to rtgibbons/proxmox-ceph that referenced this pull request Jun 10, 2025
Apache Arrow was basically never updated and we got a ancient version
of 6.5, which breaks compilation due to using c++11, which the abseil
library version from Bookworm doesn't support anymore.

Upstream upgraded it's arrow git submodule [0] in response to compilation
errors for the (future) Ceph 20 Tentacle release, but it's not (yet)
backported to Squid.

As we check in the processed source tarball we cannot just update a
git submodule here, but rather I copied over the src/arrow directory
from ceph tentacle branch.

[0]: ceph/ceph#56585

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
darthShadow added a commit to darthShadow/ceph that referenced this pull request Aug 1, 2025
Signed-off-by: Anagh Kumar Baranwal <6824881+darthShadow@users.noreply.github.com>
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.

7 participants