zabbix: add agent active checks compression - #29998
Conversation
|
Upstream implementation submitted as zabbix/zabbix#180: zabbix/zabbix#180 |
|
Official Zabbix feature request: ZBXNEXT-10703. The upstream implementation patch is attached there. |
|
Nice! I'll take a closer look when I'm not fighting with instance problems at my cloud hoster. EDIT: Due to the hour, it now means once I'm back from work tomorrow. |
|
Looks like zabbix-extra-network postinst is broken. Not sure how I missed that. Prior to this PR, that is. |
|
Fixing the CI in #29999 |
|
I've decided to wait a couple of days to see if there are any comments upstream (and because I am busy). If not, I will run both the upstream PR and this PR through some automated testing, and run-test locally, for a second set of eyeballs on the PR. It might need some minor rebasing, shortly, as I have noticed 7.0.28 is out, so I will be updating to pull the new version. We should probably good until the end of the first week of August before the next version bump, after that. |
|
Based on @danielfdickinson's comment, I am putting this on hold, well on blocked label! And btw: great decision! |
|
I should be able to look at this this weekend. |
|
Sorry, the weekend got away from me. I intend to look at it later this week. |
8e9b829 to
981b5a4
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Re-reviewed after the force-push (rebase onto zabbix 7.0.28 plus a regeneration of the 120- patch context). The content is otherwise unchanged from what was reviewed at 8e9b829.
The refresh itself looks clean — the new @@ -175,6 +175,17 @@ StartAgents=1 hunk sits on the post-010-zabbix_agentd-tweak-config-file-for-openwrt.patch tree (its context line # ServerActive=127.0.0.1 and the StartAgents=1 function context are both artifacts of that earlier patch), and the diffstat in the patch header still matches the body exactly.
Spot-checks that came back clean, for the record:
zbx_tcp_send_ext(sock, data, strlen(data), 0, protocol, 0)is identical to thezbx_tcp_send(s, d)macro ininclude/zbxcomms.hapart from the flags argument, so the substitution is faithful.reserved = 0is correct:zbx_tcp_send_ext()compresses internally and only skips it whenreserved != 0("compress if not compressed yet"), so callers must not pre-compress.- All in-tree callers of
zbx_comms_exchange_with_redirect()are updated — the two inactive_checks.c,send_heartbeat_msg(), and both sender variants. Nothing else in the tree calls it, so the signature change does not break the server/proxy variants. -DHAVE_ZLIBis genuinely required rather than belt-and-braces:src/libs/zbxcompress/compress.ccarries a stubbedzbx_compress()in its#elsebranch that just fails, so without the define the compressed send would fail at runtime rather than at link time.- The
CONFIGURE_VARSapproach works becauseconfigure.acunconditionally folds$ZLIB_LIBS/$ZLIB_LDFLAGSintoAGENT_LIBS/AGENT_LDFLAGS, even though the probe that populates them only runs for server/proxy — which matches your reportedlibz.so.1result. PKG_RELEASEbump to4is right for adding a patch, and--with-zlib=$(STAGING_DIR)/usris the correct base-directory form (the old value produced-I.../usr/include/include).
Nothing below blocks a merge: one patch-metadata item, one upgrade-path question, and two nits.
Generated by Claude Code
981b5a4 to
a0ee1b0
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; no new issues found.
The delta since 981b540 is confined to the three points raised last time, and each is addressed:
Upstream-Status: Submitted [.../zabbix/zabbix/pull/180]plus aLink:to ZBXNEXT-10703 now sit in the120-patch header, above the---separator, so they survivegit amand stay with the patch once merged.- The
CONFIGURE_VARS +=continuation lines moved from tabs to two spaces, matchingPKG_CONFIG_DEPENDSand the rest of the file. Harmless either way for make (it is a variable assignment, not a recipe), but consistent now. - The commit message gained the conffile-preservation paragraph and the upstream references, and the PR body carries the 7.0.28-r4 retest results and the upgrade note.
No hunk headers or patch bodies changed, so the earlier verification of the zbx_tcp_send_ext() substitution, reserved = 0, the complete set of zbx_comms_exchange_with_redirect() call sites, -DHAVE_ZLIB, and the PKG_RELEASE bump still holds. The three FormalityCheck runs on a0ee1b0 are green.
I could not verify zabbix/zabbix#180 from this session (only openwrt/packages is reachable), so the upstream link is taken at face value.
Generated by Claude Code
|
See zabbix/zabbix#180 (comment). There are some small changes needed to the upstream version, but, as I mention in a follow-up, I think defaulting to compression does make sense rather than out of the box backwards configuration compatibility with servers/proxies three major versions ago (4.x series). |
|
The upstream PR has been updated with a ZBXNEXT-10703 changelog entry documenting the classic agent zlib build dependency. Compression remains enabled by default, following the upstream review follow-up; EnableCompression=0 preserves compatibility with pre-4.0 servers/proxies. No OpenWrt package code change is needed for these review points because this PR already supplies and verifies the agent-only zlib build flags. |
|
@dormancygrace 7.0.29 has been released do I am updating the package, but once that is merged and you have rebased, I will make time to test this on my machine. @BKPepe Upstream doesn't look like they necessarily comment on proposed updates until they actually merge them, and that can take some time, so I am disinclined to wait longer for comments upstream, as I have run the upstream PR through automated review and there wasn't much to change, and @dormancygrace has made the changes (and the changes don't affect this PR as they were more about upstream admin/metadata). |
|
@dormancygrace Actually I do have a thought. For the OpenWrt package, could you add a |
a0ee1b0 to
872a080
Compare
|
Implemented, thanks for the suggestion.
I cross-built both variants for
The no-zlib binary also passed Updated head: |
872a080 to
4d256ef
Compare
|
Fixed the Makefile assignment style reported by FormalityCheck and updated the branch to |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the delta since a0ee1b0: the new ZABBIX_ENABLE_AGENT_COMPRESSION build option, the conditional zlib wiring in the Makefile, and the #ifdef HAVE_ZLIB guards added to the 120- patch. The patch hunks that were verified in the previous round are unchanged apart from those guards, so the earlier checks on zbx_tcp_send_ext(), reserved = 0, and the complete set of zbx_comms_exchange_with_redirect() call sites still hold.
Clean on re-check:
+ZABBIX_ENABLE_AGENT_COMPRESSION:zlibmatches the conditional-DEPENDS form already used in this Makefile (+ZABBIX_CURL:libcurl), andConfig.infollows the file's existing tab indentation.- The
#else static int zbx_config_enable_compression;fallback plus the guard inactive_checks_thread()is the right pair: the config parameter stays parseable on a no-zlib build andEnableCompression=1becomes a no-op instead of a runtime send failure. PKG_RELEASEis still correct at 4 against master's 3 — one release bump covers the whole added patch, andPKG_VERSIONis untouched.- Three FormalityCheck runs on 4d256ef are green.
One item I'd resolve before merge — the agent's zlib dependency is now narrower than the condition under which the agent actually links libz, so a build that disables compression while also selecting zabbix-server/zabbix-proxy/zabbix-sender/zabbix-get can ship zabbix-agentd with an undeclared libz.so.1 dependency. Details inline on the Makefile. The rest is one stale patch diffstat, one upstream-divergence question, and two nits.
Generated by Claude Code
4d256ef to
2506e8b
Compare
|
This PR is next on my list when I am back to OpenWrt - should be later today. BTW PKG_RELEASE here should probably be '2' not '1' as suggested - it will just complain about lack of PKG_RELEASE bump if you use '1'. |
2506e8b to
2b27c00
Compare
|
Thanks — fixed. I rebased onto the 7.0.29 package update and set PKG_RELEASE:=2. Both the compression-enabled agent build and the mixed agentd + sender build with agent compression disabled pass on 7.0.29-r2. I also split the agent feature guard from HAVE_ZLIB, so another selected Zabbix component cannot accidentally enable agent compression. Updated head: 2b27c00. |
|
@dormancygrace I've finally had a chance to do some testing here. It works well with and without SSL (as expected with your testing). @BKPepe could you take a look and unblock / provide a Copilot second opinion? |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the delta since 4d256ef: the rebase onto 7.0.29, the widened zlib dependency on Package/zabbix-agentd/Default, the HAVE_ZLIB → ZBX_AGENT_COMPRESSION guard rename, and the patch-header note plus refreshed diffstat.
Clean on re-check:
- The new conditional-DEPENDS list mirrors
ZABBIX_NEEDS_ZLIBsymbol for symbol, which closes the previous round's finding. It also covers the cross-variant cases correctly:ZABBIX_NEEDS_ZLIBis a plain global, not per-BUILD_VARIANT, so thebasicbuild linkslibzunder exactly the same conditions asfull— putting the list on the sharedPackage/zabbix-agentd/Defaultrather than onzabbix-agentdalone is the right call. - The refreshed patch diffstat matches the body. Recounted all eight files: 40 insertions, 11 deletions, and each per-file line agrees (
active_checks.c+10/-3,zabbix_agentd.c+8/-0). PKG_RELEASE:=2on top of master's 7.0.29-r1 is right for adding a patch, andPKG_VERSIONis untouched by this PR.- The
#else static int zbx_config_enable_compression;fallback still keepsEnableCompressionparseable on a build with the option off, so an existing conffile carrying the setting does not become a startup error. - The
OpenWrt note:paragraph sits above the---separator in the patch header, so it survivesgit amand travels with the patch. - CI on 2b27c00: the three FormalityCheck runs are green; the architecture build matrix is still queued/in progress, nothing failed.
One item I'd resolve before merge — the guard rename dropped the only thing that selected Zabbix's real zbx_compress() in agent-only builds, which is the package's default configuration. Details inline on the Makefile, along with a question about whether the on-wire capture was re-run on this revision. Plus one nit.
Generated by Claude Code
2b27c00 to
ec0fdc4
Compare
|
Thank you for testing both configurations on real hardware. That confirms the package-side compression path works with and without SSL in addition to the cross-build and protocol checks. |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the delta since 2b27c00: -DHAVE_ZLIB restored under ZABBIX_NEEDS_ZLIB, the reverted Package/zabbix-agentd/Default dependency list, and the reworded Config.in help. The 120- patch is byte-identical to the previously reviewed revision, so the earlier checks on zbx_tcp_send_ext(), reserved = 0, the complete set of zbx_comms_exchange_with_redirect() call sites, the diffstat and the patch header all still hold.
Clean on re-check:
TARGET_CPPFLAGS += -DHAVE_ZLIBinsideifneq ($(ZABBIX_NEEDS_ZLIB),)is the right placement, and strictly better than what I suggested last round:CONFIG_ZABBIX_ENABLE_AGENT_COMPRESSIONis itself a member ofZABBIX_NEEDS_ZLIB, so the default agent-only build gets the realzbx_compress()whileZBX_AGENT_COMPRESSIONstays the independent agent switch. That closes the previous round's finding.- The forced
-DHAVE_ZLIBis benign where configure also defines it — both expand to1, so the redefinition is identical and warning-free. - The commit message matches the diff, including the new "Keep HAVE_ZLIB defined whenever any selected Zabbix component needs the real compression implementation" paragraph and the restored conffile-preservation caveat.
PKG_RELEASE:=2on master's 7.0.29-r1 is correct for adding a patch;PKG_VERSIONuntouched.- CI on ec0fdc4: the three FormalityCheck runs are green, nothing failed.
One item I'd resolve before merge, and it is the same one as last round rather than a new one: reverting the widened zlib dependency puts the agent package's declared dependency back to being narrower than the condition under which the agent binary actually links libz. The justification given ("each of those packages already owns its zlib dependency") is about the sibling packages, not about zabbix-agentd itself, so I don't think it settles the case. Details and a concrete check that would confirm or kill it are inline on the Makefile. Plus one nit on a stranded comment.
Generated by Claude Code
|
@dormancygrace I've been making my share of annoying little mistakes on my own PRs. I've had to remember that when there is no urgency, so try to slow down and not worry if it takes an extra day or two. (Not sure how long I'll remember that lesson though; I tend to get impatient, especially since elsewhere it tends to be about getting things done yesterday). So, just a friendly, don't worry, and a reminder (as much to me as you) to take it easy and remember that the only pressure is from oneself. |
ec0fdc4 to
b5f3fb8
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the delta since ec0fdc4: the restored conditional zlib list on Package/zabbix-agentd/Default and the two reworded comment blocks in the Makefile. The 120- patch and Config.in are byte-identical to the previously reviewed revision, so the earlier checks on zbx_tcp_send_ext(), reserved = 0, the complete set of zbx_comms_exchange_with_redirect() call sites, the diffstat and the patch header all still hold.
Clean on re-check:
- The restored list mirrors
ZABBIX_NEEDS_ZLIBsymbol for symbol —ZABBIX_ENABLE_AGENT_COMPRESSIONplus the five package symbols — sozabbix-agentdnow declareszlibin exactly the configurations where the shared build passesZLIB_LIBS/--with-zliband the agent binary can acquire alibz.so.1 NEEDEDentry. That closes the finding carried over from the last two rounds. Where the linker turns out not to need it the declaration is merely redundant, which is the harmless direction, and it restores parity with master's unconditional+zlib. - Placing the list on
/Defaultrather than onzabbix-agentdalone remains right:ZABBIX_NEEDS_ZLIBis a plain global, not per-BUILD_VARIANT, sozabbix-agentd-basiclinkslibzunder the same conditions as thefullvariant — including viazabbix-get/zabbix-sender, which arefullbut still flip the global. - No other Zabbix package can pull zlib into the shared build:
zabbix-extra-*andzabbix-frontend-serverare allVARIANT:=no-configure, so the six symbols are the complete set. - The comment split is now accurate — the
ZBX_AGENT_COMPRESSIONblock describes only the feature switch, and the zlib-probe rationale sits directly above-DHAVE_ZLIBinside theifneq ($(ZABBIX_NEEDS_ZLIB),)block. - The commit message matches the diff, including the new "The programs share one configure and build … Mirror the shared-build conditions in the agent package dependency" paragraph, which documents exactly this change.
PKG_RELEASE:=2on master's 7.0.29-r1 is still correct for adding a patch;PKG_VERSIONuntouched.- CI on b5f3fb8: the three FormalityCheck runs are green, nothing failed.
Nothing blocking from my side on this revision. One wording nit inline on Config.in, where the help text still reflects the intermediate revision that had the narrow dependency.
Generated by Claude Code
b5f3fb8 to
13f9863
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; no new issues found.
The delta since b5f3fb8 is a single hunk: the ZABBIX_ENABLE_AGENT_COMPRESSION help text in Config.in, which now reads "the agent drops its zlib dependency, unless another selected Zabbix package makes the shared build link zlib anyway." That closes the last round's nit — the qualifier is now about zabbix-agentd's own dependency rather than about the sibling packages, which matches both the restored PACKAGE_*:zlib list on Package/zabbix-agentd/Default and the commit message's "When disabled for an agent-only build" scoping. Indentation follows the file's existing tabs.
Nothing else moved: the 120- patch, the Makefile and the commit message are byte-identical to the previously reviewed revision, so the earlier verification still holds — the zbx_tcp_send_ext() substitution, reserved = 0, the complete set of zbx_comms_exchange_with_redirect() call sites, the ZABBIX_NEEDS_ZLIB mirroring in both -DHAVE_ZLIB and the agent DEPENDS, the patch header diffstat, and PKG_RELEASE:=2 on master's 7.0.29-r1 with PKG_VERSION untouched.
CI on 13f9863: the three FormalityCheck runs are green, nothing failed.
Nothing blocking from my side on this revision.
Generated by Claude Code
13f9863 to
310531c
Compare
|
Rebased onto current Validation before the push:
New head: |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the delta since 13f9863: the rebase onto master and Zabbix 7.0.30, and the refreshed conf/zabbix_agentd.conf hunk in the 120- patch.
Clean on re-check:
PKG_RELEASE:=2on master's 7.0.30-r1 is correct for adding a patch;PKG_VERSION/PKG_HASHcome from master's 070a299 and are untouched by this commit.- The seven source hunks in the
120-patch (zbxcommshigh.h,commshigh.c,active_checks.c,active_checks.h,zabbix_agentd.c, both sender variants) are byte-identical to the previously reviewed revision, so the earlier checks on thezbx_tcp_send_ext()substitution,reserved = 0, and the complete set ofzbx_comms_exchange_with_redirect()call sites still hold. - The patch-header diffstat still matches the body — the conf hunk is unchanged in size (+11), so 40/11 is still right.
- The commit message matches its diff.
- CI on 310531c: nothing has failed; the checks are still pending.
Two things I'd resolve before merge, both introduced by this rebase rather than by the feature itself:
- The refreshed conf hunk was generated against the pristine 7.0.30 tarball instead of the tree after
010-zabbix_agentd-tweak-config-file-for-openwrt.patch, so its context line and line numbers are wrong for the tree it lands on. It survives only onpatch's default fuzz 2. Details and the reproduction inline on the patch. admin/zabbix/Makefileandadmin/zabbix/Config.inat this head are byte-identical to their state atec0fdc4apart from the version bump, so the work fromb5f3fb8and13f9863— thePACKAGE_*:zliblist onPackage/zabbix-agentd/Default, the comment split, the Config.in help wording and the corresponding commit-message paragraph — is gone. That re-opens the dependency finding you closed inb5f3fb8. Details inline on the Makefile.
Plus one nit on the stale PR body.
Generated by Claude Code
The classic agent sent active-check payloads without the Zabbix compression flag even when EnableCompression was enabled. Honor the setting for active checks and add a default-enabled ZABBIX_ENABLE_AGENT_COMPRESSION build option. When disabled for an agent-only build, omit the direct zlib dependency and send active checks uncompressed. Keep HAVE_ZLIB defined whenever any selected Zabbix component needs the real compression implementation. The programs share one configure and build. If another selected Zabbix component enables zlib, the agent binary can also acquire a libz NEEDED entry. Mirror the shared-build conditions in the agent package dependency so installing that package alone still pulls in libz. Servers and proxies older than 4.0 can still be used by setting EnableCompression=0. Existing agent conffiles are preserved on upgrade and do not gain the new commented example, so administrators of such old servers must add this option explicitly. Signed-off-by: Gleb Pesin <[email protected]>
310531c to
cbf161c
Compare
Package details
Maintainer: @danielfdickinson
Enable compression for classic Zabbix agent active-check traffic. The new
EnableCompressionagent configuration option defaults to1and can be set to0for servers or proxies older than 4.0.The classic agent now passes the selected protocol flags through the redirect-aware exchange path.
zabbix_senderremains uncompressed. The build also supplies zlib explicitly because upstream's configure script probes it only when server or proxy support is enabled.Upgrade note
/etc/zabbix_agentd.confis a preserved conffile. Existing installations do not receive the new commented example when upgrading, although compression now defaults to enabled to match agent2. Administrators using a server or proxy older than 4.0 must addEnableCompression=0to their preserved config.Run testing details
mipsel_24kc, GCC 16.2.0)Validation against the exact current source:
--fuzz=0, then ranmake package/zabbix/refresh; every patch, including120-zabbix-agentd-compress-active-checks.patch, remained unchanged;zabbix-agentd-7.0.30-r2.apkwith agent compression enabled; the build usedZBX_AGENT_COMPRESSIONandHAVE_ZLIB, the binary hasNEEDED libz.so.1, and the APK depends onzlib;libz.so.1dependency, and the APK does not depend onzlib;zabbix-agentd + zabbix-senderselection with agent compression disabled; the build usedHAVE_ZLIBwithoutZBX_AGENT_COMPRESSION, the agent binary hasNEEDED libz.so.1, and the agent APK correctly retains itszlibdependency;qemu-mipsel: configuration validation acceptsEnableCompression=0and1and rejects2;0x01, a 109-byte original JSON payload and zero reserved length; enabled mode used flags0x03, a 98-byte zlib payload and the correct 109-byte uncompressed length;Upstream
The downstream patch header records both references.
Formalities
CONTRIBUTING.md.git am.