Skip to content

fix: store interface counters and graph ids in the right columns - #400

Open
somethingwithproof wants to merge 2 commits into
Cacti:developfrom
somethingwithproof:fix/interface-counter-and-graph-columns
Open

somethingwithproof wants to merge 2 commits into
Cacti:developfrom
somethingwithproof:fix/interface-counter-and-graph-columns

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

Description

Two INSERT statements in lib/mactrack_functions.php list 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.

  1. build_InterfacesTable() adds the multicast and broadcast rates before the unicast, discard and error rates, but the column list has them last. Every rate from int_ifInUcastPkts onwards was stored in the wrong column of mac_track_interfaces.
  2. mactrack_find_host_graphs() builds each row as (graph template, data query), but the column list says snmp_query_id, graph_template_id. The two ids were stored in each other's columns in mac_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:

Column Showed until now
In Err (E/S) outbound broadcast packets per second
In Disc (D/S) inbound broadcast packets per second
UProto (UP/S) inbound unicast packets per second
Out Err (OE/S) inbound discards per second
Out Disc (OD/S) outbound unicast packets per second

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_id or graph_template_id from mac_track_interface_graphs. The graph links use local_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:

  • Interface rates are worked out each scan from the raw counters, which were always stored correctly, and the row is rewritten with ON DUPLICATE KEY UPDATE. The first scan of each device after upgrading shows correct rates.
  • Graph rows are rewritten with 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 to f082137 (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?

  • New 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 reports ifInUcastPkts holding the multicast rate, and so on down the list. The graph test reports graph_template_id holding 5 (the data query) instead of 9. Both pass with this change.
  • Full Pest suite: 226 tests before, 228 after, all passing on PHP 8.4.25 and 8.2.31 against Cacti 1.2.x.
  • Patch coverage (tests/bin/patch-coverage.php, 100% threshold): 100%, 5 of 5 changed lines.
  • MariaDB 10.11.19, through Cacti's own 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 -l on PHP 7.4 for the changed files.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation repository accordingly.

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>
@sonarqubecloud

Copy link
Copy Markdown

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.

1 participant