Repository navigation
fix: store interface counters and graph ids in the right columns - #400
Open
somethingwithproof wants to merge 2 commits into
Open
somethingwithproof wants to merge 2 commits into
somethingwithproof wants to merge 2 commits into
Conversation
The multicast and broadcast deltas were concatenated ahead of the unicast, discard and error deltas, so every value from int_ifInUcastPkts onwards was written into the wrong column of mac_track_interfaces. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…olumns mactrack_find_host_graphs() built each row as graph template then data query, but the column list names snmp_query_id first, so the two ids were stored in each other's columns. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
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
Two INSERT statements in
lib/mactrack_functions.phplist their columns in one order and build their values in another. Only position ties a value to its column, so values ended up in the wrong place. This PR puts the values in the same order as the columns. It changes no SQL style and adds no migration.build_InterfacesTable()adds the multicast and broadcast rates before the unicast, discard and error rates, but the column list has them last. Every rate fromint_ifInUcastPktsonwards was stored in the wrong column ofmac_track_interfaces.mactrack_find_host_graphs()builds each row as (graph template, data query), but the column list sayssnmp_query_id, graph_template_id. The two ids were stored in each other's columns inmac_track_interface_graphs.Related Issue
Related to #399. Not reported separately; found while reviewing the scanner SQL.
Motivation and Context
What users saw. In the Interfaces view, with "Show Totals" off, the five per-second columns showed other counters:
Sorting by those columns used the same wrong values. Busy ports could look like they were dropping or rejecting large numbers of packets, and real error rates were hidden. These parts were right all along: the "Show Totals" columns, the in/out utilisation percentages, the octet rates, and the Errors and Discards status filters and pills. The filters and pills use flags that are worked out before the INSERT, not the stored rates.
Nothing in the plugin reads
snmp_query_idorgraph_template_idfrommac_track_interface_graphs. The graph links uselocal_graph_id, so no graph link pointed to the wrong place. The swap only affects anyone who queries the table directly.After the fix. Each rate column shows the counter it is named for, and the graph table stores each id in its own column.
Existing data. No upgrade step is needed. Both tables are rewritten on every scan:
ON DUPLICATE KEY UPDATE. The first scan of each device after upgrading shows correct rates.ON DUPLICATE KEY UPDATE, and that sets both id columns again. The table's primary key does not include either column. Rows are corrected on the next scan of any device that is linked to a Cacti host and has at least one interface graph. Rows for a device that no longer has any matching graph are left as they are. That was already the case before this change, and nothing reads those two columns.History. The counter mix-up came in with the multicast and broadcast columns in
ab86e80(August 2017, merged in #24) and has shipped in every release since 4.0. The graph id swap goes back tof082137(March 2010) and has shipped since 2.4.This PR touches the same two statements as the separate prepared-statements PR. Whichever one merges second will need a rebase.
How Has This Been Tested?
tests/Unit/InterfaceColumnMappingTest.php. It runs both functions with stubbed SNMP and database calls, matches each value in the generated INSERT to its column, and checks that each value is in the column named for it. On develop both tests fail. The counter test reportsifInUcastPktsholding the multicast rate, and so on down the list. The graph test reportsgraph_template_idholding5(the data query) instead of9. Both pass with this change.tests/bin/patch-coverage.php, 100% threshold): 100%, 5 of 5 changed lines.lib/database.php, with the plugin's real schema: two scans of one interface, each counter rising by a different amount. The script also builds the graph lookup in temporary tables. It was run on PHP 7.4.33 and 8.3.36 with Cacti 1.2.14, 1.2.25 and 1.2.31. Develop stored the shifted rates and swapped ids in all four runs, and this branch stored the correct values in all four. Running this branch's scan on top of the database develop had written corrected both tables in one scan.php -lon PHP 7.4 for the changed files.Types of changes
Checklist: