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/tests/Unit/TholdCalculatePercentTest.php b/tests/Unit/TholdCalculatePercentTest.php new file mode 100644 index 00000000..4266fa39 --- /dev/null +++ b/tests/Unit/TholdCalculatePercentTest.php @@ -0,0 +1,100 @@ + + */ + private function threshold() { + return ['percent_ds' => 'total', 'local_data_id' => 4]; + } + + /** + * @param float|int|string $denominator + * @param float|int|string $reading + * + * @return mixed + */ + private function percent($denominator, $reading = 50) { + return thold_calculate_percent($this->threshold(), $reading, [4 => ['total' => $denominator]]); + } + + /** + * @return void + */ + public function testReadingIsExpressedAsAPercentageOfTheReference(): void { + $this->assertEqualsWithDelta(25, $this->percent(200), 1.0e-9); + } + + /** + * A denominator below one used to truncate to zero, forcing the result to + * zero and keeping any configured low threshold in permanent breach. + * + * @return void + */ + public function testFractionalDenominatorIsNotTruncated(): void { + $this->assertEqualsWithDelta(1000, $this->percent(0.5, 5), 1.0e-9); + } + + /** + * @return void + */ + public function testNegativeDenominatorGivesANegativePercentage(): void { + $this->assertEqualsWithDelta(-25, $this->percent(-200), 1.0e-9); + } + + /** + * @return void + */ + public function testZeroDenominatorGivesZeroRatherThanDividingByZero(): void { + $this->assertSame(0, $this->percent(0)); + } + + /** + * @return void + */ + public function testNonNumericDenominatorGivesZero(): void { + $this->assertSame(0, $this->percent('U')); + } + + /** + * @return void + */ + public function testNonNumericReadingYieldsTheNoValueSentinel(): void { + $this->assertSame('', $this->percent(200, 'U')); + } + + /** + * @return void + */ + public function testMissingReferenceDataSourceYieldsTheNoValueSentinel(): void { + $this->assertSame('', thold_calculate_percent($this->threshold(), 50, [4 => ['other' => 200]])); + } +} diff --git a/tests/Unit/TholdGetCurrentvalTest.php b/tests/Unit/TholdGetCurrentvalTest.php new file mode 100644 index 00000000..4c1472cf --- /dev/null +++ b/tests/Unit/TholdGetCurrentvalTest.php @@ -0,0 +1,176 @@ + $overrides + * + * @return array + */ + private function threshold(array $overrides = []) { + return $overrides + [ + 'local_data_id' => 4, + 'name' => 'traffic_in', + 'data_source_type_id' => self::COUNTER, + 'rrd_step' => 300, + 'rrd_maximum' => 0, + 'lasttime' => 0, + 'oldvalue' => 100, + ]; + } + + /** + * @param array $thold + * @param float|int|string $reading + * + * @return mixed + */ + private function currentValue(array $thold, $reading) { + $reindexed = [4 => ['traffic_in' => $reading]]; + $time_reindexed = [4 => 1700000300]; + $item = []; + $currenttime = 0; + + return thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime); + } + + /** + * @return void + */ + public function testGaugeReturnsTheReadingUnchanged(): void { + $thold = $this->threshold(['data_source_type_id' => self::GAUGE]); + + $this->assertSame(42, $this->currentValue($thold, 42)); + } + + /** + * @return void + */ + public function testAbsoluteDividesTheReadingByTheStep(): void { + $thold = $this->threshold(['data_source_type_id' => self::ABSOLUTE]); + + $this->assertEqualsWithDelta(2, $this->currentValue($thold, 600), 1.0e-9); + } + + /** + * @return void + */ + public function testCounterReturnsTheDeltaOverTheStep(): void { + $thold = $this->threshold(['oldvalue' => 100]); + + $this->assertEqualsWithDelta(2, $this->currentValue($thold, 700), 1.0e-9); + } + + /** + * A counter that legitimately read zero last cycle is not the same as + * having no previous reading. Treating it as absent reports a rate of zero + * for the first interval after a device reboot. + * + * @return void + */ + public function testCounterTreatsAPreviousReadingOfZeroAsReal(): void { + $thold = $this->threshold(['oldvalue' => 0]); + + $this->assertEqualsWithDelta(2, $this->currentValue($thold, 600), 1.0e-9); + } + + /** + * @return void + */ + public function testCounterWithNoPreviousReadingYieldsZero(): void { + $thold = $this->threshold(['oldvalue' => '']); + + $this->assertSame(0, $this->currentValue($thold, 600)); + } + + /** + * A 32-bit counter that wraps has advanced by (2^32 - old) + new. Using + * 2^32-1 as the modulus loses exactly one count per wrap. + * + * @return void + */ + public function testThirtyTwoBitWrapUsesTheCorrectModulus(): void { + $thold = $this->threshold(['oldvalue' => 4294967290, 'rrd_step' => 1]); + + $this->assertEqualsWithDelta(11, $this->currentValue($thold, 5), 1.0e-9); + } + + /** + * @return void + */ + public function testSixtyFourBitWrapUsesTheCorrectModulus(): void { + $thold = $this->threshold(['oldvalue' => '18446744073709551610', 'rrd_step' => 1]); + + $this->assertEqualsWithDelta(11, $this->currentValue($thold, 5), 1.0e-9); + } + + /** + * RRD values can arrive in scientific notation. GMP accepts only integer + * strings, so these values must use the non-fatal floating-point fallback. + * + * @return void + */ + public function testSixtyFourBitWrapAcceptsScientificNotation(): void { + $thold = $this->threshold(['oldvalue' => '1.8446744073709552E+19', 'rrd_step' => 1]); + + $this->assertEqualsWithDelta(5, $this->currentValue($thold, 5), 1.0e-9); + } + + /** + * @return void + */ + public function testDeriveDividesTheDeltaByTheStep(): void { + $thold = $this->threshold(['data_source_type_id' => self::DERIVE, 'oldvalue' => 100]); + + $this->assertEqualsWithDelta(2, $this->currentValue($thold, 700), 1.0e-9); + } + + /** + * @return void + */ + public function testNonNumericReadingYieldsTheNoValueSentinel(): void { + $this->assertSame('', $this->currentValue($this->threshold(), 'U')); + } + + /** + * @return void + */ + public function testMissingDataSourceYieldsTheNoValueSentinel(): void { + $thold = $this->threshold(); + $reindexed = []; + $time_reindexed = [4 => 1700000300]; + $item = []; + $currenttime = 0; + + $this->assertSame('', thold_get_currentval($thold, $reindexed, $time_reindexed, $item, $currenttime)); + } +} 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: diff --git a/thold_functions.php b/thold_functions.php index 018cf99b..0439775c 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -772,6 +772,37 @@ function thold_expression_specialtype_rpn($operator, &$stack, $local_data_id, $c } } +/** + * Counts a wrapped counter has advanced by, given the previous and current + * readings. + * + * The modulus is 2^32 or 2^64, not one less than it, so the previous code lost + * exactly one count per wrap. 2^64 is above PHP_INT_MAX and would be parsed as + * a float, losing about eleven bits at that magnitude, so the 64-bit case goes + * through GMP. Cacti already requires ext-gmp. + * + * @param float|int|string $oldvalue Previous reading. + * @param float|int|string $newvalue Current reading. + * + * @return float|int + */ +function thold_counter_wrap_delta($oldvalue, $newvalue) { + if ($oldvalue > 4294967295) { + $old_integer = trim((string) $oldvalue); + $new_integer = trim((string) $newvalue); + + if (!preg_match('/^\d+$/D', $old_integer) || !preg_match('/^\d+$/D', $new_integer)) { + return (18446744073709551616.0 - (float) $oldvalue) + (float) $newvalue; + } + + $delta = gmp_add(gmp_sub(gmp_pow(2, 64), gmp_init($old_integer, 10)), gmp_init($new_integer, 10)); + + return (float) gmp_strval($delta); + } + + return (4294967296 - $oldvalue) + $newvalue; +} + function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexed, &$item, &$currenttime) { // adjust the polling interval by the last read, if applicable $currenttime = $rrd_time_reindexed[$thold_data['local_data_id']]; @@ -798,17 +829,14 @@ function thold_get_currentval(&$thold_data, &$rrd_reindexed, &$rrd_time_reindexe if (isset($item[$thold_data['name']]) && is_numeric($item[$thold_data['name']])) { switch ($thold_data['data_source_type_id']) { case 2: // COUNTER - if ($thold_data['oldvalue'] != 0 && is_numeric($thold_data['oldvalue'])) { + // A previous reading of zero is a real reading, not a missing one. + if (is_numeric($thold_data['oldvalue']) && $thold_data['oldvalue'] !== '') { if ($item[$thold_data['name']] >= $thold_data['oldvalue']) { // Everything is Normal $currentval = $item[$thold_data['name']] - $thold_data['oldvalue']; } else { // Possible overflow, see if its 32bit or 64bit - if ($thold_data['oldvalue'] > 4294967295) { - $currentval = (18446744073709551615 - $thold_data['oldvalue']) + $item[$thold_data['name']]; - } else { - $currentval = (4294967295 - $thold_data['oldvalue']) + $item[$thold_data['name']]; - } + $currentval = thold_counter_wrap_delta($thold_data['oldvalue'], $item[$thold_data['name']]); } if (strpos($thold_data['rrd_maximum'], '|query_') !== false) { @@ -1201,9 +1229,11 @@ function thold_calculate_percent($thold, $currentval, $rrd_reindexed) { } if (isset($rrd_reindexed[$thold['local_data_id']][$ds])) { - $t = (int) $rrd_reindexed[$thold['local_data_id']][$thold['percent_ds']]; + // Not cast to int: a denominator below one truncated to zero, which + // forced the percentage to zero and kept a low threshold alerting. + $t = $rrd_reindexed[$thold['local_data_id']][$thold['percent_ds']]; - if ($t > 0) { + if (is_numeric($t) && $t != 0) { $currentval = ($currentval / $t) * 100; } else { $currentval = 0; diff --git a/thold_process.php b/thold_process.php index b04857eb..40078c24 100644 --- a/thold_process.php +++ b/thold_process.php @@ -207,10 +207,12 @@ $currentval = ''; } + // Carry the previous value forward when this cycle has no reading; + // storing a timestamp here corrupts the next delta calculation. if (isset($item[$thold_data['name']])) { - $lasttime = $item[$thold_data['name']]; + $rawvalue = $item[$thold_data['name']]; } else { - $lasttime = $currenttime - $thold_data['rrd_step']; + $rawvalue = $thold_data['oldvalue']; } thold_daemon_debug(sprintf('Checked Name:%s, Graph:%s, Value:%s, Time:%s', $thold_data['thold_name'], $thold_data['local_graph_id'], $currentval, $currenttime), $thread); @@ -219,7 +221,7 @@ SET tcheck = 1, lastread = ?, lasttime = FROM_UNIXTIME(?), oldvalue = ? WHERE id = ?', - [$currentval, $currenttime, $lasttime, $thold_data['thold_id']] + [$currentval, $currenttime, $rawvalue, $thold_data['thold_id']] ); }