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/TholdCalculateExpressionTest.php b/tests/Unit/TholdCalculateExpressionTest.php new file mode 100644 index 00000000..b3a13714 --- /dev/null +++ b/tests/Unit/TholdCalculateExpressionTest.php @@ -0,0 +1,134 @@ + 1, + 'name' => 'CPU', + 'local_data_id' => 4, + 'local_graph_id' => 7, + 'expression' => $expression, + ]; + + $reindexed = []; + $time_reindexed = []; + + return thold_calculate_expression($thold, 0, $reindexed, $time_reindexed); + } + + /** + * @return array + */ + public static function expressionProvider() { + return [ + 'addition' => ['2,3,+', 5], + 'subtraction' => ['8,2,-', 6], + 'multiplication' => ['4,3,*', 12], + 'nested' => ['2,3,+,4,*', 20], + 'single value' => ['7', 7], + ]; + } + + /** + * @dataProvider expressionProvider + * + * @param string $expression + * @param float|int $expected + * + * @return void + */ + public function testExpressionsReduceToTheTopOfTheStack($expression, $expected): void { + $this->assertEqualsWithDelta($expected, $this->evaluate($expression), 1.0e-9); + } + + /** + * An expression that leaves more than one value is an authoring error. It + * used to return the first operand pushed, which looks like a plausible + * reading and is compared against the bounds as though it were one. + * + * @return void + */ + public function testUnbalancedExpressionIsRejectedRatherThanReturningAnOperand(): void { + $this->assertSame(0, $this->evaluate('1,2,3,+')); + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testUnsupportedTokenFailsTheExpression(): void { + $this->assertSame(0, $this->evaluate('2,NOSUCHOP')); + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * @return array + */ + public static function specialTokenProvider() { + return [ + 'graph maximum' => ['CURRENT_GRAPH_MAXIMUM_VALUE'], + 'graph minimum' => ['CURRENT_GRAPH_MINIMUM_VALUE'], + ]; + } + + /** + * CURRENT_GRAPH_MAXIMUM_VALUE was missing from the dispatch list even + * though the evaluator handles it, so every expression using it failed as + * an unsupported field and returned zero. + * + * Reaching the handler is the whole assertion: what it then reads out of + * the RRD needs a live rrdtool and is not this test's concern. + * + * @dataProvider specialTokenProvider + * + * @param string $token + * + * @return void + */ + public function testSpecialGraphTokensReachTheirHandler($token): void { + // The handler reads the step off the data source before asking rrdtool. + CactiStubs::willReturnFor('db_fetch_row_prepared', 'FROM data_template_data', ['rrd_step' => 300]); + + try { + $this->evaluate($token); + } catch (Throwable $reached_the_rrd_layer) { + // Expected: the handler runs and asks rrdtool for a value. + } + + $unsupported = array_filter(CactiStubs::$log, static function ($message) { + return strpos($message, 'Unsupported Field') !== false; + }); + + $this->assertSame([], array_values($unsupported)); + } +} diff --git a/tests/Unit/TholdExpressionStackOpsTest.php b/tests/Unit/TholdExpressionStackOpsTest.php new file mode 100644 index 00000000..f7904cf2 --- /dev/null +++ b/tests/Unit/TholdExpressionStackOpsTest.php @@ -0,0 +1,224 @@ + $stack + * @param string $operator + * + * @return array + */ + private function stackOp(array $stack, $operator) { + thold_expression_stackops_rpn($operator, $stack); + + return $stack; + } + + /** + * @param array $stack + * @param string $operator + * + * @return array + */ + private function setOp(array $stack, $operator) { + thold_expression_setops_rpn($operator, $stack); + + return $stack; + } + + /** + * @return void + */ + public function testDupCopiesTheTopOfTheStack(): void { + $this->assertSame([1, 7, 7], $this->stackOp([1, 7], 'DUP')); + } + + /** + * @return void + */ + public function testPopDiscardsTheTopOfTheStack(): void { + $this->assertSame([1], $this->stackOp([1, 7], 'POP')); + } + + /** + * @return void + */ + public function testExcExchangesTheTopTwoElements(): void { + $this->assertSame([2, 1], $this->stackOp([1, 2], 'EXC')); + } + + /** + * @return void + */ + public function testExcLeavesDeeperElementsAlone(): void { + $this->assertSame([9, 8, 2, 1], $this->stackOp([9, 8, 1, 2], 'EXC')); + } + + /** + * @return void + */ + public function testExcOnAShortStackFlagsAnError(): void { + $this->stackOp([1], 'EXC'); + + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testDupOnAnEmptyStackFlagsAnError(): void { + $this->assertSame([], $this->stackOp([], 'DUP')); + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testUnrecognisedSetOperatorLeavesTheStackUntouched(): void { + $this->assertSame([1, 2], $this->setOp([1, 2], 'NOSUCHOP')); + $this->assertFalse($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testRevReversesTheRequestedNumberOfElements(): void { + $this->assertSame([3, 2, 1], $this->setOp([1, 2, 3, 3], 'REV')); + } + + /** + * @return void + */ + public function testRevLeavesDeeperElementsAlone(): void { + $this->assertSame([9, 3, 2, 1], $this->setOp([9, 1, 2, 3, 3], 'REV')); + } + + /** + * @return void + */ + public function testRevOfZeroElementsLeavesTheStackUnchanged(): void { + $this->assertSame([1, 2], $this->setOp([1, 2, 0], 'REV')); + } + + /** + * @return void + */ + public function testFractionalSetCountIsRejected(): void { + $this->assertSame([1, 2, 3], $this->setOp([1, 2, 3, 2.5], 'REV')); + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testSortOrdersTheRequestedElementsAscending(): void { + $this->assertSame([1, 2, 3], $this->setOp([3, 1, 2, 3], 'SORT')); + } + + /** + * @return void + */ + public function testSortLeavesDeeperElementsAlone(): void { + $this->assertSame([9, 1, 2, 3], $this->setOp([9, 3, 1, 2, 3], 'SORT')); + } + + /** + * @return void + */ + public function testSortAndRevAreInversesOverTheSameSpan(): void { + $sorted = $this->setOp([3, 1, 2, 3], 'SORT'); + $sorted[] = 3; + + $this->assertSame([3, 2, 1], $this->setOp($sorted, 'REV')); + } + + /** + * @return void + */ + public function testAvgDividesTheSumByTheCount(): void { + $this->assertEqualsWithDelta([3], $this->setOp([2, 4, 2], 'AVG'), 1.0e-9); + } + + /** + * rrdtool's AVG ignores unknown samples and averages the rest, rather than + * failing the whole expression. + * + * @return void + */ + public function testAvgSkipsUnknownSamplesInsteadOfThrowing(): void { + $this->assertEqualsWithDelta([3], $this->setOp([2, 4, 'U', 3], 'AVG'), 1.0e-9); + } + + /** + * @return void + */ + public function testAvgSkipsNanSamples(): void { + $this->assertEqualsWithDelta([3], $this->setOp([2, 4, 'NAN', 3], 'AVG'), 1.0e-9); + } + + /** + * @return void + */ + public function testAvgOfOnlyUnknownSamplesIsUnknown(): void { + $this->assertSame(['U'], $this->setOp(['U', 'NAN', 2], 'AVG')); + } + + /** + * @return void + */ + public function testAvgPropagatesInfinity(): void { + $this->assertSame(['INF'], $this->setOp([2, 'INF', 2], 'AVG')); + $this->assertSame(['NEGINF'], $this->setOp([2, 'NEGINF', 2], 'AVG')); + } + + /** + * Popping more elements than the stack holds leaves the operator with no + * usable operands, so it must not push a result. + * + * @return array + */ + public static function setOperatorProvider() { + return [ + 'SORT' => ['SORT'], + 'REV' => ['REV'], + 'AVG' => ['AVG'], + ]; + } + + /** + * @dataProvider setOperatorProvider + * + * @param string $operator + * + * @return void + */ + public function testUnderflowFlagsAnErrorAndPushesNothing($operator): void { + $this->assertSame([], $this->setOp([1, 5], $operator)); + $this->assertTrue($GLOBALS['rpn_error']); + } +} diff --git a/tests/Unit/TholdRpnCdefTest.php b/tests/Unit/TholdRpnCdefTest.php new file mode 100644 index 00000000..cfe0e85c --- /dev/null +++ b/tests/Unit/TholdRpnCdefTest.php @@ -0,0 +1,121 @@ + + */ + public static function arithmeticProvider() { + return [ + 'addition' => [8, 2, self::ADD, 10], + 'subtraction' => [8, 2, self::SUB, 6], + 'multiplication' => [8, 2, self::MUL, 16], + 'division' => [8, 2, self::DIV, 4], + 'modulo' => [8, 3, self::MOD, 2], + 'float division' => [5, 2, self::DIV, 2.5], + 'negative operand' => [-8, 2, self::DIV, -4], + ]; + } + + /** + * @dataProvider arithmeticProvider + * + * @param float|int $x + * @param float|int $y + * @param int $op + * @param float|int $expected + * + * @return void + */ + public function testArithmeticOperations($x, $y, $op, $expected): void { + $this->assertEqualsWithDelta($expected, thold_rpn($x, $y, $op), 1.0e-9); + } + + /** + * @return void + */ + public function testDivisionByZeroReturnsTheInvalidSentinel(): void { + $this->assertSame('', thold_rpn(8, 0, self::DIV)); + } + + /** + * @return void + */ + public function testModuloByZeroReturnsTheInvalidSentinelInsteadOfThrowing(): void { + $this->assertSame('', thold_rpn(8, 0, self::MOD)); + } + + /** + * @return array + */ + public static function nonNumericProvider() { + return [ + 'text first operand' => ['abc', 2], + 'text second operand' => [8, 'abc'], + ]; + } + + /** + * @dataProvider nonNumericProvider + * + * @param mixed $x + * @param mixed $y + * + * @return void + */ + public function testNonNumericOperandsReturnTheInvalidSentinel($x, $y): void { + $this->assertSame('', thold_rpn($x, $y, self::ADD)); + $this->assertNotEmpty(CactiStubs::$log); + } + + /** + * An unknown sample is coerced to zero rather than rejected, so an + * expression over a gappy data source still produces a number. + * + * @return void + */ + public function testUnknownOperandsAreTreatedAsZero(): void { + $this->assertSame(2, thold_rpn('U', 2, self::ADD)); + $this->assertSame(2, thold_rpn(2, 'U', self::ADD)); + } + + /** + * @return void + */ + public function testUnrecognisedOperationReturnsTheInvalidSentinel(): void { + $this->assertSame('', thold_rpn(8, 2, 99)); + } +} 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..e8053a40 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -611,20 +611,42 @@ function thold_expression_specvals_rpn($operator, &$stack, $count) { } } -function thold_expression_stackops_rpn($operator, &$stack) { +/** + * Apply one stack-manipulation RPN operator. + * + * @param string $operator DUP, POP or EXC. + * @param array $stack Evaluation stack, modified in place. + */ +function thold_expression_stackops_rpn(string $operator, array &$stack): void { global $rpn_error; - if ($operator == 'DUP') { - $v1 = thold_expression_rpn_pop($stack); - array_push($stack, $v1); - array_push($stack, $v1); - } elseif ($operator == 'POP') { - thold_expression_rpn_pop($stack); - } else { - $v1 = thold_expression_rpn_pop($stack); - $v2 = thold_expression_rpn_pop($stack); - array_push($stack, $v2); - array_push($stack, $v1); + switch ($operator) { + case 'DUP': + $v1 = thold_expression_rpn_pop($stack); + + if ($rpn_error) { + return; + } + + array_push($stack, $v1, $v1); + + break; + case 'POP': + thold_expression_rpn_pop($stack); + + break; + default: + // EXC. Popping already reverses the pair, so push them back in pop order. + $v1 = thold_expression_rpn_pop($stack); + $v2 = thold_expression_rpn_pop($stack); + + if ($rpn_error) { + return; + } + + array_push($stack, $v1, $v2); + + break; } } @@ -640,68 +662,80 @@ function thold_expression_time_rpn($operator, &$stack) { } } -function thold_expression_setops_rpn($operator, &$stack) { +/** + * Apply one set RPN operator to the top $count elements of the stack. + * + * SORT, REV and AVG each pop a count first. AVG follows rrdtool: unknown + * samples are skipped rather than poisoning the sum, and an all-unknown span + * yields UNKN. + * + * @param string $operator SORT, REV or AVG. + * @param array $stack Evaluation stack, modified in place. + */ +function thold_expression_setops_rpn(string $operator, array &$stack): void { global $rpn_error; - if ($operator == 'SORT') { - $count = thold_expression_rpn_pop($stack); - $v = []; + if (!in_array($operator, ['SORT', 'REV', 'AVG'], true)) { + return; + } - if ($count > 0) { - for ($i = 0; $i < $count; $i++) { - $v[] = thold_expression_rpn_pop($stack); - } + $count = thold_expression_rpn_pop($stack); - sort($v, SORT_NUMERIC); + if ($rpn_error || !is_numeric($count) || $count <= 0) { + return; + } - foreach ($v as $val) { - array_push($stack, $val); - } - } - } elseif ($operator == 'REV') { - $count = thold_expression_rpn_pop($stack); - $v = []; + if ((float) $count !== floor((float) $count)) { + $rpn_error = true; - if ($count > 0) { - for ($i = 0; $i < $count; $i++) { - $v[] = thold_expression_rpn_pop($stack); - } + return; + } - $v = array_reverse($v); + $count = (int) $count; - foreach ($v as $val) { - array_push($stack, $val); - } + // Popping yields the span top-down; keep it that way and index deliberately. + $values = []; + + for ($i = 0; $i < $count; $i++) { + $values[] = thold_expression_rpn_pop($stack); + } + + if ($rpn_error) { + return; + } + + if ($operator === 'SORT') { + sort($values, SORT_NUMERIC); + } + + // REV needs no work: $values is already reversed relative to the stack. + if ($operator !== 'AVG') { + foreach ($values as $value) { + $stack[] = $value; } - } elseif ($operator == 'AVG') { - $count = thold_expression_rpn_pop($stack); - if ($count > 0) { - $total = 0; - $inf = false; - $neginf = false; + return; + } - for ($i = 0; $i < $count; $i++) { - $v = thold_expression_rpn_pop($stack); + $total = 0; + $known = 0; - if ($v == 'INF') { - $inf = true; - } elseif ($v == 'NEGINF') { - $neginf = true; - } else { - $total += $v; - } - } + foreach ($values as $value) { + if ($value == 'INF' || $value == 'NEGINF') { + $stack[] = $value == 'INF' ? 'INF' : 'NEGINF'; - if ($inf) { - array_push($stack, 'INF'); - } elseif ($neginf) { - array_push($stack, 'NEGINF'); - } else { - array_push($stack, $total / $count); - } + return; + } + + if (!is_numeric($value)) { + continue; } + + $total += $value; + $known++; } + + $stack[] = $known === 0 ? 'U' : $total / $known; } function thold_expression_ds_value($operator, &$stack, $data_sources) { @@ -885,23 +919,16 @@ function thold_calculate_expression($thold, $currentval, &$rrd_reindexed, &$rrd_ $stackops = ['DUP', 'POP', 'EXC']; $time = ['NOW', 'TIME', 'LTIME']; $spectypes = ['CURRENT_DATA_SOURCE', 'CURRENT_GRAPH_MINIMUM_VALUE', - 'CURRENT_GRAPH_MINIMUM_VALUE', 'CURRENT_DS_MINIMUM_VALUE', + 'CURRENT_GRAPH_MAXIMUM_VALUE', 'CURRENT_DS_MINIMUM_VALUE', 'CURRENT_DS_MAXIMUM_VALUE', 'VALUE_OF_HDD_TOTAL', 'ALL_DATA_SOURCES_NODUPS', 'ALL_DATA_SOURCES_DUPS']; // our expression array $expression = explode(',', $thold['expression']); - // out current data sources - $data_sources = $rrd_reindexed[$thold['local_data_id']]; - - if (cacti_sizeof($data_sources)) { - foreach ($data_sources as $key => $value) { - $nds[$key] = $value; - } - - $data_sources = $nds; - } + // out current data sources. A threshold whose data source produced no + // readings this cycle has no entry here, so default rather than index blind. + $data_sources = $rrd_reindexed[$thold['local_data_id']] ?? []; // replace all data tabs in the rpn with values if (cacti_sizeof($expression)) { @@ -1056,7 +1083,16 @@ function thold_calculate_expression($thold, $currentval, &$rrd_reindexed, &$rrd_ } } - return $stack[0]; + // rrdtool yields the top of the stack. Anything left below it means the + // expression was unbalanced, which is an authoring error worth reporting. + if (cacti_sizeof($stack) != 1) { + cacti_log("ERROR: RPN Expression did not reduce to a single value! THold:'" . $thold['name'] . "', Expression:'" . $thold['expression'] . "', Stack:'" . implode(',', $stack) . "'", false, 'THOLD'); + $rpn_error = true; + + return 0; + } + + return end($stack); } function thold_substitute_snmp_query_data($string, $device_id, $snmp_query_id, $snmp_index, $max_chars = 0) { @@ -4787,13 +4823,21 @@ function thold_rpn($x, $y, $z, $local_data_id = 0) { break; case 4: if ($y == 0) { - return (-1); + cacti_log("WARNING: Erroneous CDEF logic, division by zero. Data ID $local_data_id", false, 'THOLD'); + + return ''; } return $x / $y; break; case 5: + if ((int) $y == 0) { + cacti_log("WARNING: Erroneous CDEF logic, modulo by zero. Data ID $local_data_id", false, 'THOLD'); + + return ''; + } + return $x % $y; break;