Repository navigation
Fix the Graphs tab on Cacti 1.2.x and classify device types with fewer queries (stacked on #77) - #79
Merged
Conversation
… pages Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
TheWitness
approved these changes
Oct 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Description
Stacked on #77 (base
hardening-review-fixes). Merge #77 first; this branch is rebased onto its headfb5c39eand retargets to develop afterwards.Two changes, each in its own commit:
lib/html_graph.phpcallsgraph_view_has_untemplated_graphs(), which 1.2.x declares ingraph_view.phponly, so a plugin page that builds the graph filter dies with "Call to undefined function".includes/compat.phpdeclares the same function when core has not. Cacti develop has no such function and does not call it, so nothing changes there.hmib_assign_host_types()inincludes/types.phpruns one UPDATE per stored type, in id order, against the devices that still have no type, with the bound LIKE match that Harden the Host MIB pages and poller, and fix review findings #77 introduced.Related Issue
No open issue covers either change.
Motivation and Context
Graphs tab: on Cacti 1.2.32 the request
hmib.php?action=graphsreturned a page ending inFatal error: Uncaught Error: Call to undefined function graph_view_has_untemplated_graphs() in lib/html_graph.php on line 217. The INFO file declares compatibility with 1.2.25 and later.Rescan:
rescan_types()(as of #77) loops over the distinct reported pairs of unclassified devices and, for each pair, over every stored type until one UPDATE changes a row. The UPDATE does not use the pair, so each pass of the outer loop repeats the same work. With 200 unclassified devices and 20 stored types that is about 4,000 statements; with 2,000 devices about 40,000 and 51 s.What the new function does differently from the loop in #77, stated exactly:
ORDER BY id. The loop reads them in table order, which is not defined.Everything else is kept from #77: the bound LIKE comparison on
sysObjectIDandsysDescr(no regular expression branch, no OR), first matching type wins, only devices whosehost_typeis 0 change, and nothing runs when no device with both values reported is unclassified. All values stay strings, so IPv4 and IPv6 addresses,0xprefixes and numeric-looking text are compared as text.The two differences were accepted by the maintainers on #77's review thread and are kept documented here.
How Has This Been Tested?
Tests, in the repository's Pest layout:
tests/Unit/GraphViewCompatTest.php: the function is declared, returns the right answer for none, one and many untemplated graphs, asks core'sget_allowed_graphs()for one row with the device restriction when there is one, leaves core's own declaration alone (a subprocess declares it first and requires the file), and is loaded by the Graphs tab right after core's graph library.tests/E2E/DevicesPageTest.phprequests the Graphs tab; on 1.2.x that test was skipped on Harden the Host MIB pages and poller, and fix review findings #77 and now runs.tests/Unit/AssignHostTypesTest.php: one UPDATE per stored type for 1, 20 and 200 types, in id order, with the id and the two patterns bound, the match limited tohost_type = 0, noRLIKE, and no stored value in the SQL text for quotes, backslashes, IPv4, IPv6,0x1A,1e3, leading zeros, empty patterns and a statement fragment.tests/Integration/AssignHostTypesDbTest.php, against MariaDB with the Cacti schema, in three SQL modes (strict, not strict, strict with ONLY_FULL_GROUP_BY): the new function and the per-pair loop of Harden the Host MIB pages and poller, and fix review findings #77 (kept in the test as the reference, with the stored types read in id order) classify 150 seeded fixtures identically, each with 3 to 12 stored types, 5 to 40 devices, some already typed, overlapping patterns, wildcards, IPv4, IPv6,0xand1e3values; a check that the comparison is not vacuous; a device that has a type keeps it; IPv4/IPv6/numeric-looking values match as text; a stored pattern is a LIKE substring, not a regular expression; no stored types and no devices; the case where the loop stopped early; and the statement counts (5 types and 20 devices: 100 against 5; 20 types and 200 devices: 4,000 against 20). The fixtures come from a seeded xorshift generator intests/support/prng.php, so a failing fixture is rebuilt from its seed number; the files added here use norand,mt_rand,uniqidorlcg_value.The test database tables are copied aside and restored. Tests skip when no Cacti schema is reachable.
Run in Docker (PHP 8.3 with Xdebug, MariaDB 11.8.9 loaded from each core's
cacti.sql) in the CI order: volume owned by www-data:runner, root-owned log,php cli/plugin_manage.php --plugin=hmib --install --enableas root, the log step, Pest with Clover as a non-root user withdate.timezone = America/New_York, the per-test coverage pass, thentests/bin/merge-coverage.phpandtests/bin/patch-coverage.php <clover> fb5c39e 100:hmib.phpandhmib_types.phpare measured through the end-to-end tests; the new logic lives inincludes/compat.phpandincludes/types.php, which are added to the coverage<source>list. The three lines changed in the two page scripts are arequire_oncein each and the call to the new function.The Graphs tab was also checked on Cacti 1.2.25, 1.2.32 and develop with a throwaway request (200, no fatal error, no PHP warning).
Also run:
php -lon every PHP file,tests/bin/validate-manifest.php,tests/bin/check-i18n-pot.php fb5c39e, all clean.Not run: PHP 8.2 and 8.4 (the CI matrix) and the CI workflow itself.
Performance
Measured with a throwaway harness (not part of the pull request) that runs
rescan_types()cut from #77 (fb5c39e) and from this branch against the same MariaDB 11.8.9 container, with PDO standing in for Cacti's database layer. Median of 3 runs (one run for the 2,000 device "before" rows); wall time fromhrtime, CPU fromgetrusage, statements fromSHOW SESSION STATUS LIKE 'Questions', which also counts the delete and insert of the placeholder type that both versions run. 20 stored types. "Unmatched": every device reports a pair no stored type matches (the worst case for the loop). "Matched": each device matches one stored type.Peak memory was the same in both versions (0.6 MB at 200 devices, 1.6 MB at 2,000). The statement count after the change grows with the number of stored types (one each), not with the number of devices. Each statement scans the unclassified rows of
plugin_hmib_hrSystemonce, with LIKE on both columns, so a very large table with very many stored types still pays for that scan per type; this was not measured beyond 2,000 devices and 20 types. The Graphs tab change adds no query beyond the one core's function makes when the filter is built on 1.2.x. Not measured: Cacti's owndb_*wrappers and a browser.Screenshots (if appropriate):
None.
Types of changes
The rescan differences listed under Motivation concern undefined table order and a rare early stop of the previous loop, not the documented rules; they are the one place a result can differ.
Checklist: