Skip to content

Name the data query reindex assert - #596

Closed
somethingwithproof wants to merge 6 commits into
Cacti:developfrom
somethingwithproof:refactor/reindex-assert
Closed

Name the data query reindex assert#596
somethingwithproof wants to merge 6 commits into
Cacti:developfrom
somethingwithproof:refactor/reindex-assert

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

Stacked on #595, which is stacked on #589. Merge in that order.

The data query reindex assert decides whether Cacti re-runs a data query. It was written out three times inside poll_host(), once per operator, each with the same twenty-four lines of logging and queueing wrapped around a one-line comparison.

The copies had drifted. The three REPLACE INTO poller_command statements are three spellings of the same SQL:

"... (poller_id, time, action,command) values (..."
"... (poller_id, time, action, command) ValueS (..."
"... (poller_id, time, action, command) VALUES (..."

Nothing else differed except the operator character in the log line.

What the assert actually does

reindex_assert_failed(op, assert_value, poll_result) returns TRUE when the assert has been violated. The relation is assert_value op poll_result.

Two details were load-bearing and easy to miss while this was spelled out three times:

  • = compares as text, < and > compare as numbers. A device reporting 007 is equal to 7 under > and not equal under =.
  • An assert_value of "0" never fails a < assert. That is the uptime case: a device that has not reported an uptime yet must not look like it rebooted. The guard is specific to <; it does not cover the other operators.

Equality is not a violation of either ordering operator, so > only fails on a strict increase.

None of that was written down anywhere. It is now in the function comment and pinned by tests.

Not a bug, checked

The < branch sits at a deeper indent than the other two and reads like it might be unreachable. It is not: it lives in a final else if (strcmp(reindex->assert_value, "0")), which is the "0" guard above. Worth stating because the indentation invites the wrong conclusion.

Tests

Eleven cases, reindex_assert_failed at 100% line coverage under gcov:

  • = compares as text, including 007 against 7
  • > and < compare as numbers, including 007 against 7 going the other way
  • equal values fail neither ordering operator
  • "0" never fails <, and the guard does not leak into =
  • U and No Such Instance are treated as "assume the assert holds", case-insensitively
  • an unrecognised operator does nothing
  • NULL arguments are refused
  • values past 32 bits compare correctly, which matters because sysUpTime is centiseconds and passes INT_MAX in under a year

Coverage across the extractions

Measured with gcov, not counted by hand:

reindex_assert_failed            100.00% of 14
poll_host_build_queries          100.00% of 29
poller_item_scope                100.00% of 6
poller_owner_scope               100.00% of 6
poll_host_release                  0.00% of 10
poll_host_release_connections      0.00% of 9
poll_host                          0.00% of 840

The two release helpers are the teardown from #595: db_release_connection() and mysql_thread_end(), neither of which has an observable result to assert. They are not covered and I am not going to pretend otherwise.

poll_host_build_queries reached 100% only after this branch added the active_profiles dimension. The first fixture pinned it at 1, so the multi-profile forms of query5 and query10 were never built. Those add AND rrd_next_step <= 0 so only items due this tick are polled.

Size

poll_host() is 1,516 lines, from 1,927 on develop. The chain itself went from 88 lines to 34.

60 of 60 tests pass. Clean ubuntu:24.04 build matching ci.yml, same four warnings as develop.

No CHANGELOG entry here; #578 is the changelog PR for this batch.

poll_host() is 1,926 lines and builds the same six queries twice, once for the
main poller and once for a remote one. The bodies differ by 33 of 141 lines,
and every difference is the same rule: the main poller reads items that are not
deleted, a remote poller reads the items assigned to it.

Nothing in that construction was reachable from a test, so a column added to
one copy and not the other would not have been caught.

Extract the rule as poller_item_scope() and poller_owner_scope(), covered by
five cases in test_linked against the shipped poller.o: the deleted filter on
the main poller, the ownership filter on a remote one, the empty fragment the
main poller needs so callers can interpolate unconditionally, and a degenerate
buffer.

No call site changes yet. Collapsing the two branches onto these helpers is the
next step and is worth its own review, because the shipped query1 spells its
tail 'poller_id=%i' while the others spell it 'poller_id = %i', so the unified
text will normalise that.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The two branches drifted: the remote copy never picked up the dbonupdate
handling the main copy grew, and nothing could have caught that because the
construction was not reachable from a test.

Captured what both branches emit across the vectors they switch on, collapsed
them onto poller_item_scope()/poller_owner_scope(), and diffed. Output is
byte-identical except query1 on a remote poller, which now spells its tail
'poller_id = N' rather than 'poller_id=N' to match the other five queries.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
set.dbonupdate is 1 on MySQL 8, which deprecated VALUES() in ON DUPLICATE KEY
UPDATE. The main poller switched to the row-alias form; the remote branch kept
the deprecated one because it had its own copy of the suffix. Closes Cacti#590.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The construction was 167 lines in the middle of a 1,795-line function, so
nothing could reach it. It now takes its inputs as arguments and fills a
struct, which is what lets the golden capture become a test that runs instead
of a file with instructions attached.

poll_host is 1,620 lines, down from 1,927 on develop.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
poll_host() leaves through three places and each spelled its teardown out
again. The copies were not in the same order and did not hold the same steps:
the device-row-missing path never called mysql_thread_end(), so a device
deleted mid-cycle leaked the client library's thread-local state once per
cycle. Closes Cacti#594.

The two early exits now share poll_host_release(). The normal exit uses
error_string after the point where that helper frees it, so it shares only
poll_host_release_connections() and keeps its own ordering.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The assert was written out three times inside poll_host(), once per operator,
each with the same twenty-four lines of logging and reindex queueing around a
one-line comparison. The three copies of the REPLACE INTO had drifted to three
different spellings of the same SQL.

Two details were buried in there and are now stated and covered: '=' compares
as text while '<' and '>' compare as numbers, and an assert_value of '0' never
fails a '<' assert, which is what stops a device with no recorded uptime from
looking like it rebooted.

The chain is 34 lines instead of 88. poll_host is 1,516 lines, down from 1,927
on develop.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof

Copy link
Copy Markdown
Member Author

Consolidated into #597, which carries this branch's commits unchanged.

Every pair of these ten conflicted on tests/unit/test_linked.c because each appended to the same registration array, so whichever merged first would have forced a rebase on the other nine. #597 is one review and one approval for the same 20 commits, still one logical change each.

Nothing here is dropped. Reopen this if you would rather review it separately.

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