diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 5e4f3db6..e17554e8 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -35,21 +35,12 @@ jobs: integration-test: runs-on: ${{ matrix.os }} - # A failure against the pinned release is a real failure. The develop entry - # is advisory: it is how a core regression becomes visible here, but it must - # not turn the plugin's own pull requests red. - continue-on-error: ${{ matrix.cacti != 'release/1.2.31' }} - strategy: fail-fast: false matrix: php: ['8.1', '8.2', '8.3', '8.4'] os: [ubuntu-latest] cacti: ['release/1.2.31'] - include: - - php: '8.4' - os: ubuntu-latest - cacti: 'develop' services: mariadb: @@ -95,7 +86,24 @@ jobs: echo "PHP_BINARY=$(command -v php)" >> "$GITHUB_ENV" - name: Run apt-get update - run: sudo apt-get update + run: | + for attempt in 1 2 3; do + if sudo timeout 3m apt-get \ + -o Dpkg::Lock::Timeout=60 \ + -o Acquire::Retries=3 \ + -o Acquire::http::Timeout=30 \ + -o Acquire::https::Timeout=30 \ + update; then + exit 0 + fi + + if [ "$attempt" -lt 3 ]; then + sleep 10 + fi + done + + echo 'apt-get update failed after three bounded attempts.' >&2 + exit 1 - name: Install System Dependencies run: sudo apt-get install -y apache2 snmp snmpd rrdtool fping diff --git a/includes/polling.php b/includes/polling.php index c5c2c1d6..0e0ca5a1 100644 --- a/includes/polling.php +++ b/includes/polling.php @@ -75,13 +75,8 @@ function thold_poller_output(&$rrd_update_array) { if ($local_data_ids != '') { if (read_config_option('thold_daemon_enable') == 'on') { - $chunks = ceil(sizeof($rrd_update_array) / 50); - - if ($chunks < 1) { - $chunks = 1; - } - - $rrd_update_array_chunks = array_chunk($rrd_update_array, $chunks, true); + // array_chunk() takes the size of each chunk, not how many to make. + $rrd_update_array_chunks = array_chunk($rrd_update_array, 50, true); foreach ($rrd_update_array_chunks as $rrd_update_array_chunk) { $rrd_reindexed = []; @@ -271,7 +266,7 @@ function thold_check_all_thresholds() { ) ) ) - AND h.poller_id = 1 OR h.poller_id IS NULL + AND (h.poller_id = 1 OR h.poller_id IS NULL) AND td.tcheck = 1 AND h.status = 3"; } else { @@ -322,28 +317,28 @@ function thold_check_all_thresholds() { $total_tholds = sizeof($tholds); + $checked = []; + foreach ($tholds as $thold) { thold_check_threshold($thold); + + $checked[] = (int) $thold['id']; } - if (read_config_option('remote_storage_method') == 1) { - if ($config['poller_id'] == 1) { - db_execute('UPDATE thold_data AS td - LEFT JOIN host AS h - ON td.host_id = h.id - SET tcheck = 0 - WHERE h.poller_id = 1 - OR h.poller_id IS NULL'); - } else { - db_execute_prepared('UPDATE thold_data AS td - INNER JOIN host AS h - ON td.host_id = h.id - SET td.tcheck = 0 - WHERE h.poller_id = ?', - [$config['poller_id']]); - } - } else { - db_execute('UPDATE thold_data AS td SET td.tcheck = 0'); + /* + * Clear the dirty flag on what was evaluated, and nothing else. Clearing + * by poller, or across the whole table, also cleared the thresholds whose + * data arrived after the select above, so those readings were dropped + * without ever being checked. The select already scopes to this poller, + * so the identifiers carry that scoping with them. + */ + if (cacti_sizeof($checked)) { + $placeholders = implode(', ', array_fill(0, cacti_sizeof($checked), '?')); + + db_execute_prepared('UPDATE thold_data + SET tcheck = 0 + WHERE id IN (' . $placeholders . ')', + $checked); } return $total_tholds; diff --git a/tests/Unit/PollerSchedulingTest.php b/tests/Unit/PollerSchedulingTest.php new file mode 100644 index 00000000..9a3b7be4 --- /dev/null +++ b/tests/Unit/PollerSchedulingTest.php @@ -0,0 +1,261 @@ + 2, + 'description' => 'core-switch-1', + 'hostname' => '10.0.0.1', + 'location' => 'rack 4', + 'site_id' => 1, + 'status' => 3, + 'status_fail_date' => '2026-01-01 00:00:00', + 'status_rec_date' => '2026-01-02 00:00:00', + 'status_last_error' => '', + 'snmp_engine_id' => '', + 'notes' => '', + ]); + } + + /** + * A threshold row with every column the evaluator reads, set so that it + * evaluates quietly. Only the identifier varies between rows here; what is + * under test is which rows are visited, not what visiting them decides. + * + * @param int $id + * + * @return array + */ + private function threshold($id) { + return [ + 'id' => $id, 'host_id' => 2, 'local_data_id' => 4, 'local_graph_id' => 9, + 'data_template_rrd_id' => 3, 'name' => 'CPU', 'name_cache' => 'CPU', + 'data_source_name' => 'traffic_in', 'thold_type' => 0, 'data_type' => 0, + 'lastread' => 50, 'oldvalue' => 50, 'lasttime' => 0, 'rrd_step' => 300, + 'thold_hi' => '', 'thold_low' => '', 'thold_warning_hi' => '', 'thold_warning_low' => '', + 'thold_fail_trigger' => 1, 'thold_warning_fail_trigger' => 1, + 'thold_fail_count' => 0, 'thold_warning_fail_count' => 0, + 'thold_alert' => 0, 'repeat_alert' => 0, + 'time_hi' => '', 'time_low' => '', 'time_warning_hi' => '', 'time_warning_low' => '', + 'time_fail_trigger' => 1, 'time_warning_fail_trigger' => 1, + 'time_fail_length' => 300, 'time_warning_fail_length' => 300, + 'bl_fail_count' => 0, 'bl_alert' => 0, 'bl_pct_down' => '', 'bl_pct_up' => '', + 'bl_fail_trigger' => 1, 'bl_ref_time_range' => 3600, 'bl_type' => 0, + 'bl_cf' => 'AVG', 'bl_thold_valid' => 0, 'cdef' => 0, + 'notify_warning' => 0, 'notify_alert' => 0, 'notify_extra' => '', + 'notify_warning_extra' => '', 'persist_ack' => '', 'reset_ack' => '', + 'acknowledgment' => '', 'exempt' => '', 'restored_alert' => '', + 'syslog_enabled' => '', 'syslog_priority' => 5, 'syslog_facility' => 1, + 'snmp_event_severity' => 3, 'snmp_event_description' => '', 'snmp_engine_id' => '', + 'trigger_cmd_high' => '', 'trigger_cmd_low' => '', 'trigger_cmd_norm' => '', + 'notes' => '', 'dnotes' => '', 'external_id' => '', + 'email_subject' => '', 'email_subject_warn' => '', 'email_subject_restoral' => '', + 'graph_timespan' => 7, 'show_units' => '', 'units_suffix' => '', 'decimals' => 2, + 'format_file' => '', 'thold_enabled' => 'on', 'thold_daemon_id' => 0, + ]; + } + + /** + * Only the thresholds that were evaluated get their dirty flag cleared. + * + * Clearing by poller, or across the whole table, also cleared the ones + * whose data arrived after the select, so those readings were dropped + * without ever being checked. + * + * @return void + */ + public function testOnlyTheEvaluatedThresholdsAreCleared(): void { + CactiStubs::willReturnFor('db_fetch_assoc', 'FROM thold_data', [ + $this->threshold(7), + $this->threshold(9), + ]); + + thold_check_all_thresholds(); + + $clears = array_values(array_filter(CactiStubs::callsTo('db_execute_prepared'), static function ($call) { + return strpos($call['sql'], 'SET tcheck = 0') !== false; + })); + + $this->assertCount(1, $clears); + $this->assertSame([7, 9], $clears[0]['params']); + $this->assertStringContainsString('WHERE id IN (?, ?)', $clears[0]['sql']); + } + + /** + * @return void + */ + public function testNothingIsClearedWhenNothingWasEvaluated(): void { + CactiStubs::willReturnFor('db_fetch_assoc', 'FROM thold_data', []); + + thold_check_all_thresholds(); + + $clears = array_filter(CactiStubs::$calls, static function ($call) { + return strpos($call['sql'], 'tcheck = 0') !== false; + }); + + $this->assertSame([], array_values($clears)); + } + + /** + * @return void + */ + public function testTheRunReportsHowManyThresholdsItEvaluated(): void { + CactiStubs::willReturnFor('db_fetch_assoc', 'FROM thold_data', [ + $this->threshold(1), + $this->threshold(2), + $this->threshold(3), + ]); + + $this->assertSame(3, thold_check_all_thresholds()); + } + + /** + * The whole table is never cleared in one statement. + * + * @return void + */ + public function testTheDirtyFlagIsNeverClearedTableWide(): void { + CactiStubs::willReturnFor('db_fetch_assoc', 'FROM thold_data', [ + $this->threshold(7), + ]); + + thold_check_all_thresholds(); + + foreach (CactiStubs::$calls as $call) { + if (strpos($call['sql'], 'tcheck = 0') !== false) { + $this->assertStringContainsString('WHERE id IN', $call['sql']); + } + } + } + + /** + * The daemon path batches readings before writing them, and the batch size + * is what array_chunk() takes. Passing a count meant 500 readings became + * 50 batches of 10 rather than 10 of 50, and each batch costs a round trip. + * + * @return void + */ + public function testTheDaemonWritesReadingsInBatchesOfFifty(): void { + CactiStubs::$configOptions['thold_daemon_enable'] = 'on'; + + $readings = []; + + for ($i = 1; $i <= 120; $i++) { + $readings[] = ['local_data_id' => $i, 'times' => [1700000000 => ['traffic_in' => 5]]]; + } + + // One threshold per reading, answering with just the batch that was asked + // about so each write matches the readings it was given. + CactiStubs::willReturnFor('db_fetch_assoc', 'SELECT id, local_data_id, thread_id', static function ($sql) { + preg_match('/IN \(([^)]*)\)/', $sql, $matches); + + return array_map(static function ($id) { + $id = (int) trim($id); + + return ['id' => $id, 'local_data_id' => $id, 'thread_id' => 1]; + }, explode(',', $matches[1])); + }); + + thold_poller_output($readings); + + $inserts = array_values(array_filter(CactiStubs::$calls, static function ($call) { + return strpos($call['sql'], 'plugin_thold_daemon_data') !== false; + })); + + // 120 readings at 50 to a batch is three writes, not one per reading. + $this->assertCount(3, $inserts); + } + + /** + * A structural guard rather than a behavioural one: the defect is SQL + * operator precedence, and proving it needs a database to run the query + * against. Without the parentheses, AND binds tighter than OR and the + * clause reads as + * + * (... AND h.poller_id = 1) OR (h.poller_id IS NULL AND tcheck = 1 AND status = 3) + * + * whose first branch carries neither the dirty-flag nor the device-status + * filter, so every enabled threshold is re-evaluated on every run. + * + * @return void + */ + public function testThePollerAlternationIsParenthesised(): void { + $src = file_get_contents(dirname(__DIR__, 2) . '/includes/polling.php'); + + $this->assertStringContainsString('AND (h.poller_id = 1 OR h.poller_id IS NULL)', $src); + $this->assertStringNotContainsString("AND h.poller_id = 1 OR h.poller_id IS NULL\n", $src); + } + + /** + * array_chunk() takes the size of each chunk. Passing a count meant 500 + * readings became 50 chunks of 10 rather than 10 chunks of 50, and each + * chunk costs a round trip. + * + * @return void + */ + public function testTheDaemonBatchesReadingsFiftyAtATime(): void { + $src = file_get_contents(dirname(__DIR__, 2) . '/includes/polling.php'); + + $this->assertStringContainsString('array_chunk($rrd_update_array, 50, true)', $src); + $this->assertStringNotContainsString('ceil(sizeof($rrd_update_array) / 50)', $src); + } +} diff --git a/tests/docker/Dockerfile b/tests/docker/Dockerfile new file mode 100644 index 00000000..518f7321 --- /dev/null +++ b/tests/docker/Dockerfile @@ -0,0 +1,29 @@ +# Test runner for the Thold plugin. +# +# Pinned to PHP 8.1 because that is the oldest interpreter the CI matrix +# covers; what passes here passes on 8.2-8.4. pcov rather than Xdebug: line +# coverage is the only debug feature the suite needs and pcov is far cheaper. +FROM php:8.1-cli-alpine@sha256:7949370448b0b4d9787776dc5968e0fd8d48763292344b5fbf21539441228a98 + +# git is needed by the changed-line coverage gate, which diffs against the +# base branch. +RUN apk add --no-cache git gmp-dev \ + && docker-php-ext-install gmp \ + && apk add --no-cache --virtual .build-deps $PHPIZE_DEPS \ + && pecl install pcov \ + && docker-php-ext-enable pcov \ + && apk del .build-deps + +COPY --from=composer:2@sha256:4d71c3c2109c61d5415544264b59ad4087e4c5b7244481723664138fd36d5040 /usr/bin/composer /usr/bin/composer + +# The plugin lives where Cacti would put it, because thold_functions.php +# resolves its own includes through $config['base_path'] . '/plugins/thold'. +# No network or database is involved; the Cacti framework functions themselves +# are stubbed in tests/bootstrap.php. +WORKDIR /cacti/plugins/thold + +ENV COMPOSER_ALLOW_SUPERUSER=1 \ + COMPOSER_NO_INTERACTION=1 \ + COMPOSER_CACHE_DIR=/tmp/composer-cache + +CMD ["sh", "-c", "composer install --no-progress --no-ansi && composer test"] diff --git a/tests/docker/docker-compose.yml b/tests/docker/docker-compose.yml new file mode 100644 index 00000000..99d38b47 --- /dev/null +++ b/tests/docker/docker-compose.yml @@ -0,0 +1,15 @@ +# Local mirror of the unit-test CI job. `docker compose -f +# tests/docker/docker-compose.yml run --rm phpunit` runs exactly what CI runs. +services: + phpunit: + build: + context: . + dockerfile: Dockerfile + image: cacti-thold-test:php8.1 + working_dir: /cacti/plugins/thold + volumes: + - ../..:/cacti/plugins/thold + - composer-cache:/tmp/composer-cache + +volumes: + composer-cache: