Skip to content

Prefer building using installed kernel and sanitize the generated header file#125

Open
alanbach wants to merge 1 commit into
taodd:mainfrom
alanbach:use-installed-kernel-sanitize-va-list-update
Open

Prefer building using installed kernel and sanitize the generated header file#125
alanbach wants to merge 1 commit into
taodd:mainfrom
alanbach:use-installed-kernel-sanitize-va-list-update

Conversation

@alanbach
Copy link
Copy Markdown
Collaborator

@alanbach alanbach commented Jun 2, 2026

  • Build using installed kernel rather than
    running kernel. This allows building on container
    based builders such as Github and Gitlab runners.

  • Sanitize the generated header file to prevent build failures on new tool chains.

@alanbach alanbach force-pushed the use-installed-kernel-sanitize-va-list-update branch 2 times, most recently from e6919cf to 2c3a836 Compare June 2, 2026 15:53
@taodd taodd requested a review from Copilot June 3, 2026 06:07
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the build system to generate the Ceph BTF-derived header in a way that is less dependent on the running kernel (helping container/CI builders), and adds a post-processing step to sanitize the generated header for newer toolchains.

Changes:

  • Switch Ceph BTF header generation from /lib/modules/$(uname -r) to selecting an installed kernel under /lib/modules.
  • Add logic to locate a usable base BTF/vmlinux source from several common paths.
  • Sanitize the generated header with sed to normalize a toolchain-sensitive typedef name.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread Makefile
Comment on lines +156 to +160
kver=$$(find /lib/modules -mindepth 1 -maxdepth 1 -type d -printf '%f\n' 2>/dev/null | sort -V | tail -1); \
[ -n "$$kver" ] || { echo "No installed kernels found under /lib/modules" >&2; exit 1; }; \
kdir="/lib/modules/$$kver"; \
CEPH_KO=$$(find "$$kdir" -type f -name 'ceph.ko*' 2>/dev/null | head -1); \
[ -n "$$CEPH_KO" ] || { echo "No ceph.ko* found under $$kdir" >&2; exit 1; }; \
Comment thread Makefile
Comment on lines +162 to +173
for cand in \
"$$kdir/vmlinux" \
"/usr/lib/debug/lib/modules/$$kver/vmlinux" \
"/usr/lib/debug/boot/vmlinux-$$kver" \
"/sys/kernel/btf/vmlinux" \
"/boot/vmlinux-$$kver"; do \
if [ -r "$$cand" ]; then \
base_btf="$$cand"; \
break; \
fi; \
done; \
\
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

+1, this copilot comment has a good point

@taodd
Copy link
Copy Markdown
Owner

taodd commented Jun 3, 2026

@alanbach
This enables container builds of kfstrace, but no CI job actually builds kfstrace in a container (the centos/rocky jobs build only radostrace osdtrace). Could we add a job thatinstalls a kernel + ceph module + debuginfo into the container and runs make kfstrace, so this path is verified? Otherwise the recipe can regress unnoticed

Copy link
Copy Markdown
Owner

@taodd taodd left a comment

Choose a reason for hiding this comment

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

Thanks @alanbach for addressing this problem, overall looks good, except this one minor change required, it would be good to add the kfstrace build tests to the existing CI's container build flow.

Comment thread Makefile
Comment on lines +162 to +173
for cand in \
"$$kdir/vmlinux" \
"/usr/lib/debug/lib/modules/$$kver/vmlinux" \
"/usr/lib/debug/boot/vmlinux-$$kver" \
"/sys/kernel/btf/vmlinux" \
"/boot/vmlinux-$$kver"; do \
if [ -r "$$cand" ]; then \
base_btf="$$cand"; \
break; \
fi; \
done; \
\
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

+1, this copilot comment has a good point

  running kernel. This allows building on container
  based builders such as Github and Gitlab runners.

* Sanitize the generated header file to prevent
  build failures on new toolchains.
@alanbach alanbach force-pushed the use-installed-kernel-sanitize-va-list-update branch from 2c3a836 to 30ec8a9 Compare June 5, 2026 17:35
@alanbach
Copy link
Copy Markdown
Collaborator Author

alanbach commented Jun 5, 2026

Thanks @alanbach for addressing this problem, overall looks good, except this one minor change required, it would be good to add the kfstrace build tests to the existing CI's container build flow.

Hello @taodd Thank you so much for the review. I applied the changes from the Copilot review and re-committed the PR.

@alanbach
Copy link
Copy Markdown
Collaborator Author

alanbach commented Jun 5, 2026

@alanbach This enables container builds of kfstrace, but no CI job actually builds kfstrace in a container (the centos/rocky jobs build only radostrace osdtrace). Could we add a job thatinstalls a kernel + ceph module + debuginfo into the container and runs make kfstrace, so this path is verified? Otherwise the recipe can regress unnoticed

Hello @taodd As we are aiming to build packages for major distros, the proposed Makefile change should catch if there are no usable BPF bits available. The rest falls on the packager to make sure correct dependencies are installed. Similar to other build dependencies present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants