Add corosync qdevice - #29269
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds new OpenWrt feed packages and supporting patches to provide Corosync clustering components (libqb, kronosnet, corosync) and the corosync-qnetd quorum device daemon for maintaining quorum in 2-node clusters.
Changes:
- Introduce new packages:
libs/libqb,net/kronosnet(+ crypto backend subpackages),net/corosync(splitlibcorosync+corosync), andnet/corosync-qdevice. - Add OpenWrt-specific patches to corosync-qdevice certutil scripts (bash shebang + CA/noise-file handling).
- Add an init script and a post-install hook to initialize the qnetd NSS DB.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/libqb/Makefile | New libqb package definition and install/dev-install rules. |
| net/kronosnet/Makefile | New kronosnet package definition with crypto backend subpackages and install rules. |
| net/corosync/Makefile | New corosync packaging split into libcorosync and corosync, with install/dev-install rules. |
| net/corosync-qdevice/Makefile | New corosync-qdevice package, init installation, and postinst initialization of NSS DB. |
| net/corosync-qdevice/files/corosync-qdevice.init | New procd init script to run corosync-qnetd. |
| net/corosync-qdevice/patches/0001-bash-path.patch | Forces bash shebang for upstream certutil scripts. |
| net/corosync-qdevice/patches/0002-initialize-ca.patch | OpenWrt adaptations: skip chown, adjust noise generation and hashing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
397ccd1 to
b071be8
Compare
b071be8 to
8fa0a14
Compare
95ba6ad to
4920e20
Compare
4920e20 to
d78e2fa
Compare
BKPepe
left a comment
BKPepe
left a comment
There was a problem hiding this comment.
In an ideal world, I would love for these patches to be in a state where they can be upstreamed, and it would be even better if they were actually merged upstream. After all, it would save a lot of work in OpenWrt, as we wouldn't have to rebase them every time due to offsets, changes, and so on. The patches definitely need to have some kind of header—someone created them, after all. And looking at two of the patches, removing libxml2 and doxygen2man can certainly be handled better in the upstream project rather than just deleting lines downstream.
d78e2fa to
90f9a50
Compare
90f9a50 to
c2a3d89
Compare
|
Fixed the SONAME symlink issue: changed |
c2a3d89 to
8cd2191
Compare
|
Added |
|
|
||
| case "$1" in | ||
| corosync-qdevice) | ||
| corosync-qnetd -v 2>&1 | grep -F "$2" |
There was a problem hiding this comment.
This is done by the generic tests already. Was this not working?
There was a problem hiding this comment.
This is the error from CI:
corosync-qdevice: No executables in the package provided version 3.0.4
corosync-qdevice: Generic tests failed
But only libqb and corosync-qdevice had a failure. I've removed the two test.sh files that weren't needed.
8cd2191 to
28357be
Compare
|
The --disable-man option was merged into libqb (ClusterLabs/libqb#515), but there is no ETA for a new release. Whenever they release it we can update the package and remove a patches. The PR for corosync-qdevice (corosync/corosync-qdevice#40) has seen no activity. Last commit to the repo was 2 month ago, I don't expect this to be included soon. I've added two I think I've implemented all the changes BKPepe and GeorgeSapkin asked for. |
|
@skirmess I tried building the packages using a custom feed, but Edit: Looks like an Ubuntu 25.10 issue with the new rust-based coreutils. |
|
After fixing the compilation issue, I can report that the package runs fine, but I think it might need a dependency on |
ae058ab to
143b87e
Compare
|
Good point — Since we already have a patch that modifies that line anyway (to remove |
143b87e to
77a4a39
Compare
|
Removed two debugging utilities that are not useful on embedded routers:
Both packages remain functional for their intended purpose (library/cluster engine). Removing these binaries also fixes the CI failures caused by executables that do not support version output. |
77a4a39 to
06d4acb
Compare
06d4acb to
8d801b8
Compare
|
Fixed commit message prefixes — changed from path-based prefixes (e.g. |
| $(INSTALL_DIR) $(1)/usr/include/qb | ||
| $(INSTALL_DATA) $(PKG_INSTALL_DIR)/usr/include/qb/* $(1)/usr/include/qb | ||
| $(INSTALL_DIR) $(1)/usr/lib | ||
| $(INSTALL_DATA) $(PKG_INSTALL_DIR)/usr/lib/lib*.so* $(1)/usr/lib/ |
| $(INSTALL_DIR) $(1)/usr/include/corosync | ||
| $(INSTALL_DATA) $(PKG_INSTALL_DIR)/usr/include/corosync/* $(1)/usr/include/corosync/ | ||
| $(INSTALL_DIR) $(1)/usr/lib | ||
| $(INSTALL_DATA) $(PKG_INSTALL_DIR)/usr/lib/lib*.so* $(1)/usr/lib/ |
| $(INSTALL_DIR) $(1)/usr/include/ | ||
| $(INSTALL_DATA) $(PKG_INSTALL_DIR)/usr/include/* $(1)/usr/include/ | ||
| $(INSTALL_DIR) $(1)/usr/lib | ||
| $(INSTALL_DATA) $(PKG_INSTALL_DIR)/usr/lib/lib*.so* $(1)/usr/lib/ |
| $(INSTALL_DIR) $(1)/usr/lib/pkgconfig | ||
| $(INSTALL_DATA) $(PKG_INSTALL_DIR)/usr/lib/pkgconfig/*.pc $(1)/usr/lib/pkgconfig | ||
| $(INSTALL_DIR) $(1)/usr/lib/kronosnet | ||
| $(INSTALL_DATA) $(PKG_INSTALL_DIR)/usr/lib/kronosnet/*.so* $(1)/usr/lib/kronosnet |
| SERVICE_DAEMONIZE=1 | ||
| SERVICE_WRITE_PID=1 |
| - chown --reference=: GNU coreutils only, not available in Busybox | ||
| - ps -elf: GNU ps flags, not supported by Busybox | ||
| - w: not available on OpenWrt | ||
| - sha1sum: not compiled into Busybox by default; use sha256sum which is |
8d801b8 to
0efe0ea
Compare
|
Thanks for the review, Copilot. Addressed the following:
The following suggestions were not applied, since verifying them against the actual build showed they don't apply to our configuration or would break it:
All four packages were rebuilt from scratch in the SDK to confirm nothing broke. |
0efe0ea to
a6a9983
Compare
|
Updated dependencies to their latest upstream releases (rebased into the existing commits since this PR hasn't been merged yet, rather than adding new commits):
corosync and corosync-qdevice have no new upstream releases, so those two commits are unchanged. Rebuilt the full dependency chain (libqb → kronosnet → corosync → corosync-qdevice) from scratch in the SDK to confirm everything still links and builds correctly with the updated versions. |
a6a9983 to
85023e1
Compare
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Commit checks
- 85023e1 "corosync-qdevice: add corosync-qdevice 3.0.4" — the message lists the patches as "two patches for musl/busybox compatibility: fix bash shebang path".
0001-configure-add-with-bash-option.patchdoes not fix a shebang path: it adds a--with-bash=PATHconfigure option so the target path can be set during cross-compilation (upstream-reviewed by Jan Friesse), and the actual selection happens viaCONFIGURE_ARGS += --with-bash=/bin/bash, which the message doesn't mention. Worth rewording so the message matches what the patch and the Makefile do.
Reviewed 4 commits. Other findings are inline.
Generated by Claude Code
|
|
||
| case "$1" in | ||
| libqb) | ||
| ls /usr/lib/libqb.so.* > /dev/null |
There was a problem hiding this comment.
This won't silence the generic version check that motivated adding it. Per CONTRIBUTING.md, "Test scripts", test.sh runs in addition to the generic tests; the file that overrides version detection is test-version.sh ("When this script is present, the generic version detection is skipped entirely"). libqb ships no executable, so the generic probe still runs against it.
The in-tree pattern for a shared-library-only package is libs/expat/test-version.sh — a case "$PKG_NAME" with exit 0 for the library package and exit 1 for unknown ones. Note this mechanism landed on master after this branch was cut (the branch's own copy of CONTRIBUTING.md doesn't mention it), so a rebase is probably worth doing before renaming.
Same applies to net/corosync-qdevice/test.sh. Also, net/corosync and net/kronosnet ship neither test.sh nor test-version.sh, even though libcorosync and kronosnet are library-only in exactly the same way as libqb — that contradicts the PR comment stating test scripts were added to all four packages.
Generated by Claude Code
| define Build/InstallDev | ||
| $(INSTALL_DIR) $(1)/usr/share/pkgconfig | ||
| $(INSTALL_DATA) $(PKG_INSTALL_DIR)/usr/share/pkgconfig/corosync-qdevice.pc $(1)/usr/share/pkgconfig | ||
| $(INSTALL_DIR) $(1)/usr/bin | ||
| $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/bin/* $(1)/usr/bin | ||
| $(INSTALL_DIR) $(1)/usr/sbin | ||
| $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/sbin/* $(1)/usr/sbin | ||
| endef |
There was a problem hiding this comment.
Build/InstallDev copies target runtime binaries (and the shell scripts in usr/bin) into the staging dir. Nothing links against corosync-qdevice — it's a leaf package — so the staging tree only ever needs the .pc file at most. This is the same change you already made for net/corosync after review feedback ("Removed the binary install from Build/InstallDev").
| define Build/InstallDev | |
| $(INSTALL_DIR) $(1)/usr/share/pkgconfig | |
| $(INSTALL_DATA) $(PKG_INSTALL_DIR)/usr/share/pkgconfig/corosync-qdevice.pc $(1)/usr/share/pkgconfig | |
| $(INSTALL_DIR) $(1)/usr/bin | |
| $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/bin/* $(1)/usr/bin | |
| $(INSTALL_DIR) $(1)/usr/sbin | |
| $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/sbin/* $(1)/usr/sbin | |
| endef | |
| define Build/InstallDev | |
| $(INSTALL_DIR) $(1)/usr/share/pkgconfig | |
| $(INSTALL_DATA) $(PKG_INSTALL_DIR)/usr/share/pkgconfig/corosync-qdevice.pc $(1)/usr/share/pkgconfig | |
| endef |
Generated by Claude Code
| $(INSTALL_DIR) $(1)/usr/bin | ||
| $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/bin/* $(1)/usr/bin | ||
| $(INSTALL_DIR) $(1)/usr/sbin | ||
| $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/sbin/* $(1)/usr/sbin |
There was a problem hiding this comment.
Wildcard installs mean the shipped file list silently changes on every version bump, and here they also pull in the corosync-qdevice client daemon and its tools, which the package TITLE/description ("corosync-qnetd daemon") and the init script don't cover. net/corosync/Makefile in this same PR enumerates each binary explicitly (lines 65-72) after the reviewers asked for it; please do the same here so it's visible which binaries this package actually ships.
Generated by Claude Code
| define Package/corosync-qdevice/postinst | ||
| #!/bin/sh | ||
| if [ -z "$${IPKG_INSTROOT}" ] && ! [ -f "/etc/corosync/qnetd/nssdb/cert9.db" ]; then | ||
| /usr/bin/corosync-qnetd-certutil -i | ||
| fi | ||
| endef |
There was a problem hiding this comment.
The NSS DB is only ever created when the package is installed on a running system. When corosync-qdevice is baked into an image (ImageBuilder / make image), postinst runs with IPKG_INSTROOT set, the branch is skipped, and nothing re-runs it on first boot — so /etc/corosync/qnetd/nssdb/cert9.db never exists and corosync-qnetd fails to start. Certificate generation also can't sensibly happen in the build root anyway.
Two options that cover both install paths: install an /etc/uci-defaults/ script that does the same check on first boot, or do the check in start_service() in the init script before launching the daemon.
Separately, the script's exit status is corosync-qnetd-certutil's, so a certutil failure leaves the package in an unconfigured state; an explicit exit 0 (or || true) is the usual guard.
Generated by Claude Code
| + # All files owned by root on OpenWrt | ||
| + return |
There was a problem hiding this comment.
An unconditional return at the top leaves the whole original function body as unreachable dead code, which is exactly the shape that makes a patch un-upstreamable — and upstreamability was the explicit request in the earlier review. Deleting the body outright, or guarding it (command -v chown >/dev/null / a configure-time knob), states the intent without leaving dead lines behind.
Also, this patch has no Signed-off-by: line, while 0001-configure-add-with-bash-option.patch does.
Generated by Claude Code
| echo "Creating new noise file $noise_file" | ||
|
|
||
| - (ps -elf; date; w) | sha1sum | (read sha_sum rest; echo $sha_sum) > "$noise_file" | ||
| + (ps w; date) | sha256sum | (read sha_sum rest; echo $sha_sum) > "$noise_file" |
There was a problem hiding this comment.
ps w isn't a safe assumption on OpenWrt: BusyBox's w option comes from FEATURE_PS_WIDE, which depends on ... && !BUSYBOX_CONFIG_DESKTOP. In a build with BUSYBOX_CONFIG_DESKTOP=y, ps rejects w and the noise file degenerates to sha256sum of date output alone — a fully predictable NSS seed for the qnetd CA key.
/dev/urandom is always present on OpenWrt and sidesteps the BusyBox feature dependency entirely, while keeping the patch a one-liner:
| + (ps w; date) | sha256sum | (read sha_sum rest; echo $sha_sum) > "$noise_file" | |
| + dd if=/dev/urandom bs=1024 count=1 2>/dev/null | sha256sum | (read sha_sum rest; echo $sha_sum) > "$noise_file" |
If you take this, the ps -elf / w bullets in the patch header need updating too.
Generated by Claude Code
| SECTION:=net | ||
| CATEGORY:=Network |
There was a problem hiding this comment.
nit: Package/kronosnet/install only ships lib*.so.*, so this is a pure library package, yet it lands under Network while libcorosync — equally library-only — is SECTION:=libs / CATEGORY:=Libraries in net/corosync/Makefile in this same PR. Same question for the nine kronosnet-compress-* / kronosnet-crypto-* plugin subpackages, which are dlopened .sos under /usr/lib/kronosnet/. Is the Network placement deliberate (upstream naming / discoverability), or should these follow libcorosync?
Generated by Claude Code
| PKG_FIXUP:=autoreconf | ||
| PKG_INSTALL:=1 | ||
|
|
||
| PKG_BUILD_DEPENDS:=nss corosync |
There was a problem hiding this comment.
nit: redundant — Package/corosync-qdevice already has DEPENDS:=+libcorosync +libnss ..., which gives the build-order dependency. This is the same reasoning you used to drop PKG_BUILD_DEPENDS from net/kronosnet/Makefile after review feedback. net/corosync/Makefile:16 (PKG_BUILD_DEPENDS:=libqb kronosnet, covered by libcorosync's DEPENDS:=+kronosnet +libqb) has the same leftover.
Generated by Claude Code
libqb provides high-performance, reusable features for client/server architecture, including IPC, logging, and ringbuffer. Used by Corosync and other cluster software. Man page generation (and its libxml2 dependency) is disabled via --disable-man, which is not needed at runtime. Link: https://github.com/ClusterLabs/libqb Signed-off-by: Sven Kirmess <sven.kirmess@kzone.ch>
kronosnet (knet) is a network abstraction layer designed for High Availability use cases. It provides multipoint-to-multipoint communication with link aggregation, redundancy, and optional compression/encryption. Used as the transport layer by Corosync. The wireshark dissector introduced in 1.34 is disabled via --disable-wireshark-dissector, since wireshark is not available in OpenWrt and not needed at runtime. Compression and crypto backends are split into separate packages: - kronosnet-compress-lz4, lz4hc, zlib, lzo2, lzma, bzip2, zstd - kronosnet-crypto-nss: Mozilla NSS backend - kronosnet-crypto-openssl: OpenSSL backend Link: https://github.com/kronosnet/kronosnet Signed-off-by: Sven Kirmess <sven.kirmess@kzone.ch>
Corosync provides cluster infrastructure including reliable messaging, membership, and quorum. Split into two packages: - libcorosync: shared libraries only - corosync: daemon and management tools, depends on libcorosync Link: https://corosync.github.io/corosync/ Signed-off-by: Sven Kirmess <sven.kirmess@kzone.ch>
corosync-qnetd is a daemon providing an external quorum vote for Corosync clusters, allowing a 2-node cluster to maintain quorum when one node fails. Commonly used with Proxmox VE clusters. Only the qnetd server side is built (--disable-qdevices); the qdevice client side is meant to run on the cluster nodes, not on the qnetd host. Includes two patches for musl/busybox compatibility: - add a --with-bash=PATH configure option, since AC_PATH_PROG picks up the build host's bash instead of the target's during cross-compilation - fix corosync-qnetd-certutil for busybox (chown, sha256sum, urandom) Link: https://github.com/corosync/corosync-qdevice Signed-off-by: Sven Kirmess <sven.kirmess@kzone.ch>
85023e1 to
ba3385b
Compare
|
Thanks for the thorough review. Addressed all of it:
One thing this review caught that's worth calling out: while checking the |
openwrt-ai
left a comment
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the 4 rebased commits (diff against 85023e1b, the head of my last review). Commit messages match their contents this round — the corosync-qdevice message now describes the --with-bash=PATH configure option correctly, and the --disable-qdevices / /dev/urandom changes are reflected. Both net/corosync-qdevice patches apply to the 3.0.4 tarball cleanly with no fuzz or offset. Findings are inline.
Generated by Claude Code
| CATEGORY:=Network | ||
| TITLE:=Corosync QNetd - quorum device daemon | ||
| URL:=https://github.com/corosync/corosync-qdevice | ||
| DEPENDS:=+libcorosync +libnss +nss-utils +bash |
There was a problem hiding this comment.
With --disable-qdevices now set, the only things built are corosync-qnetd, corosync-qnetd-tool and the corosync-qnetd-certutil shell script — and none of them link against corosync. In the 3.0.4 tarball, qdevices/Makefile.am:86-87 has corosync_qnetd_CFLAGS/LDADD = $(nss_CFLAGS)/$(nss_LIBS) plus libsystemd only, corosync_qnetd_tool has no LDADD at all, and the cmap/votequorum/corosync_common pkg-config variables are referenced exclusively by corosync_qdevice_CFLAGS/LDADD (Makefile.am:157-160), which is now inside the disabled BUILD_QDEVICES conditional. None of the qnetd sources include corosync/* or qb/* headers either.
So +libcorosync looks like it now only pulls libcorosync → kronosnet → libqb → libnl onto the device at runtime for nothing, which matches the commit message ("the qdevice client side is meant to run on the cluster nodes, not on the qnetd host").
Note this can't just be deleted: configure.ac:100-102 still does PKG_CHECK_MODULES for libcorosync_common/libcmap/libvotequorum unconditionally, so the corosync headers are still needed at build time — dropping it from DEPENDS means bringing back PKG_BUILD_DEPENDS:=corosync, which this commit removes. Is the runtime dependency intentional?
Generated by Claude Code
| define Package/corosync-qdevice/postinst | ||
| #!/bin/sh | ||
| if [ -z "$${IPKG_INSTROOT}" ] && [ ! -f "/etc/corosync/qnetd/nssdb/cert9.db" ]; then | ||
| /usr/bin/corosync-qnetd-certutil -i || true | ||
| fi | ||
| endef |
There was a problem hiding this comment.
nit: with the uci-defaults script added, this block is now unreachable in practice. default_postinst executes the package's /etc/uci-defaults/ entries (and rms them) before it sources postinst-pkg — see functions.sh:384-398, wired up by package-pack.mk:546-547. So on a live install the uci-defaults script has already created cert9.db by the time this runs, and on a baked image the IPKG_INSTROOT guard skips it anyway. The uci-defaults script covers both paths on its own; keeping two copies of the same check just means two places to update.
Generated by Claude Code
|
|
||
| case "$1" in | ||
| corosync-qdevice) | ||
| corosync-qnetd -v 2>&1 | grep -F "$2" |
There was a problem hiding this comment.
This one still has the problem you fixed for libs/libqb: test.sh runs in addition to the generic tests, it doesn't replace them — only test-version.sh makes "the generic version detection … skipped entirely".
The generic probe is exactly what failed for this package in the CI run you linked earlier (corosync-qdevice: No executables in the package provided version 3.0.4 / Generic tests failed), and nothing since then changes that: corosync-qnetd only accepts the short flag — getopt(argc, argv, "46dfhvc:l:m:p:S:s:") in qdevices/corosync-qnetd.c, no long_options table — so a generic --version/-V probe still comes back empty, and -h/unknown options print usage without the version string. corosync-qnetd-tool and corosync-qnetd-certutil don't report a version at all.
Renaming this to test-version.sh and switching to the env-var interface (case "$PKG_NAME" / grep -F "$PKG_VERSION" instead of $1/$2) is what actually silences the generic check.
Generated by Claude Code
| # shellcheck shell=busybox | ||
|
|
||
| case "$PKG_NAME" in | ||
| libqb) |
There was a problem hiding this comment.
nit: parity — if libqb needs this because it ships no executable, the same is true of libcorosync (net/corosync/Makefile, Package/libcorosync/install ships only lib*.so.*), kronosnet, and the nine kronosnet-compress-* / kronosnet-crypto-* plugin subpackages, none of which have a test.sh or test-version.sh in this PR. Is the generic check known to pass for those (e.g. it skips packages with zero executables, which would make this file unnecessary too), or were they just not hit in CI yet?
Generated by Claude Code
📦 Package Details
Maintainer: @skirmess, @sshaikh
Description:
Adds 4 new packages providing Corosync cluster infrastructure and the
corosync-qnetdquorum device daemon. These allow a 2-node Proxmox VE(or other Corosync-based) cluster to maintain quorum when one node fails.
Packages added (in dependency order):
libs/libqb– high-performance IPC/logging librarynet/kronosnet– network transport layer for Corosync (+ crypto subpackages)net/corosync– cluster engine split intolibcorosync+corosyncnet/corosync-qdevice– quorum device daemon (corosync-qnetd)🧪 Run Testing Details
✅ Formalities
If your PR contains a patch:
git am