From edf59597d21d105bdc5b130a44b21f22fcbd4db9 Mon Sep 17 00:00:00 2001 From: Phil Renaud Date: Mon, 6 Mar 2023 10:06:31 -0500 Subject: [PATCH] [ui] Fix: Wildcard-datacenter system/sysbatch jobs stopped showing client links/chart (#16274) * Fix for wildcard DC sys/sysbatch jobs * A few extra modules for wildcard DC in systemish jobs * doesMatchPattern moved to its own util as match-glob * DC glob lookup using matchGlob * PR feedback --- .changelog/16274.txt | 3 + ui/app/abilities/variable.js | 39 +-- ui/app/utils/match-glob.js | 52 ++++ ui/app/utils/properties/job-client-status.js | 5 +- ui/tests/acceptance/job-detail-test.js | 40 +++ ui/tests/helpers/module-for-job.js | 10 + ui/tests/unit/abilities/variable-test.js | 302 ------------------- ui/tests/unit/utils/match-glob-test.js | 267 ++++++++++++++++ 8 files changed, 378 insertions(+), 340 deletions(-) create mode 100644 .changelog/16274.txt create mode 100644 ui/app/utils/match-glob.js create mode 100644 ui/tests/unit/utils/match-glob-test.js diff --git a/.changelog/16274.txt b/.changelog/16274.txt new file mode 100644 index 000000000..325516738 --- /dev/null +++ b/.changelog/16274.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: fixed an issue where system/sysbatch jobs with wildcard datacenters (like ["dc*"]) were not showing client status charts +``` diff --git a/ui/app/abilities/variable.js b/ui/app/abilities/variable.js index 4f8e86351..d7a9b415d 100644 --- a/ui/app/abilities/variable.js +++ b/ui/app/abilities/variable.js @@ -2,6 +2,7 @@ import { computed, get } from '@ember/object'; import { or } from '@ember/object/computed'; import AbstractAbility from './abstract'; +import doesMatchPattern from 'nomad-ui/utils/match-glob'; const WILDCARD_GLOB = '*'; const WILDCARD_PATTERN = '/'; @@ -188,7 +189,7 @@ export default class Variable extends AbstractAbility { const matches = []; for (const pathName of pathNames) { - if (this._doesMatchPattern(pathName, path)) matches.push(pathName); + if (doesMatchPattern(pathName, path)) matches.push(pathName); } return matches; @@ -207,42 +208,6 @@ export default class Variable extends AbstractAbility { return this._smallestDifference(allMatchingPaths, path); } - _doesMatchPattern(pattern, path) { - const parts = pattern?.split(WILDCARD_GLOB); - const hasLeadingGlob = pattern?.startsWith(WILDCARD_GLOB); - const hasTrailingGlob = pattern?.endsWith(WILDCARD_GLOB); - const lastPartOfPattern = parts[parts.length - 1]; - const isPatternWithoutGlob = parts.length === 1 && !hasLeadingGlob; - - if (!pattern || !path || isPatternWithoutGlob) { - return pattern === path; - } - - if (pattern === WILDCARD_GLOB) { - return true; - } - - let subPathToMatchOn = path; - for (let i = 0; i < parts.length; i++) { - const part = parts[i]; - const idx = subPathToMatchOn?.indexOf(part); - const doesPathIncludeSubPattern = idx > -1; - const doesMatchOnFirstSubpattern = idx === 0; - - if (i === 0 && !hasLeadingGlob && !doesMatchOnFirstSubpattern) { - return false; - } - - if (!doesPathIncludeSubPattern) { - return false; - } - - subPathToMatchOn = subPathToMatchOn.slice(0, idx + path.length); - } - - return hasTrailingGlob || path.endsWith(lastPartOfPattern); - } - _computeLengthDiff(pattern, path) { const countGlobsInPattern = pattern ?.split('') diff --git a/ui/app/utils/match-glob.js b/ui/app/utils/match-glob.js new file mode 100644 index 000000000..878a3b72f --- /dev/null +++ b/ui/app/utils/match-glob.js @@ -0,0 +1,52 @@ +// @ts-check + +const WILDCARD_GLOB = '*'; + +/** + * + * @param {string} pattern + * @param {string} comparable + * @example matchGlob('foo', 'foo') // true + * @example matchGlob('foo', 'bar') // false + * @example matchGlob('foo*', 'footbar') // true + * @example matchGlob('foo*', 'bar') // false + * @example matchGlob('*foo', 'foobar') // false + * @example matchGlob('*foo', 'barfoo') // true + * @example matchGlob('foo*bar', 'footbar') // true + * @returns {boolean} + */ +export default function matchGlob(pattern, comparable) { + const parts = pattern?.split(WILDCARD_GLOB); + const hasLeadingGlob = pattern?.startsWith(WILDCARD_GLOB); + const hasTrailingGlob = pattern?.endsWith(WILDCARD_GLOB); + const lastPartOfPattern = parts[parts.length - 1]; + const isPatternWithoutGlob = parts.length === 1 && !hasLeadingGlob; + + if (!pattern || !comparable || isPatternWithoutGlob) { + return pattern === comparable; + } + + if (pattern === WILDCARD_GLOB) { + return true; + } + + let subStringToMatchOn = comparable; + for (let i = 0; i < parts.length; i++) { + const part = parts[i]; + const idx = subStringToMatchOn?.indexOf(part); + const doesStringIncludeSubPattern = idx > -1; + const doesMatchOnFirstSubpattern = idx === 0; + + if (i === 0 && !hasLeadingGlob && !doesMatchOnFirstSubpattern) { + return false; + } + + if (!doesStringIncludeSubPattern) { + return false; + } + + subStringToMatchOn = subStringToMatchOn.slice(0, idx + comparable.length); + } + + return hasTrailingGlob || comparable.endsWith(lastPartOfPattern); +} diff --git a/ui/app/utils/properties/job-client-status.js b/ui/app/utils/properties/job-client-status.js index 63654d63e..026195811 100644 --- a/ui/app/utils/properties/job-client-status.js +++ b/ui/app/utils/properties/job-client-status.js @@ -1,4 +1,5 @@ import { computed } from '@ember/object'; +import matchGlob from '../match-glob'; const STATUS = [ 'queued', @@ -26,7 +27,9 @@ export default function jobClientStatus(nodesKey, jobKey) { // Filter nodes by the datacenters defined in the job. const filteredNodes = nodes.filter((n) => { - return job.datacenters.indexOf(n.datacenter) >= 0; + return job.datacenters.find((dc) => { + return !!matchGlob(dc, n.datacenter); + }); }); if (job.status === 'pending') { diff --git a/ui/tests/acceptance/job-detail-test.js b/ui/tests/acceptance/job-detail-test.js index 4dc485cf1..c2ee119e9 100644 --- a/ui/tests/acceptance/job-detail-test.js +++ b/ui/tests/acceptance/job-detail-test.js @@ -30,6 +30,18 @@ moduleForJobWithClientStatus( }) ); +moduleForJobWithClientStatus( + 'Acceptance | job detail with client status (system with wildcard dc)', + () => + server.create('job', { + id: 'system-wildcard-dc', + status: 'running', + datacenters: ['canada-*-1'], + type: 'system', + createAllocations: false, + }) +); + moduleForJob('Acceptance | job detail (sysbatch)', 'allocations', () => server.create('job', { type: 'sysbatch', shallow: true }) ); @@ -59,6 +71,20 @@ moduleForJobWithClientStatus( } ); +moduleForJobWithClientStatus( + 'Acceptance | job detail with client status (sysbatch with namespace and wildcard dc)', + () => { + const namespace = server.create('namespace', { id: 'test' }); + return server.create('job', { + status: 'running', + datacenters: ['*'], + type: 'sysbatch', + namespaceId: namespace.name, + createAllocations: false, + }); + } +); + moduleForJob('Acceptance | job detail (sysbatch child)', 'allocations', () => { const parent = server.create('job', 'periodicSysbatch', { childrenCount: 1, @@ -94,6 +120,20 @@ moduleForJobWithClientStatus( } ); +moduleForJobWithClientStatus( + 'Acceptance | job detail with client status (sysbatch child with namespace and wildcard dc)', + () => { + const namespace = server.create('namespace', { id: 'test' }); + const parent = server.create('job', 'periodicSysbatch', { + childrenCount: 1, + shallow: true, + namespaceId: namespace.name, + datacenters: ['*'], + }); + return server.db.jobs.where({ parentId: parent.id })[0]; + } +); + moduleForJob( 'Acceptance | job detail (periodic)', 'children', diff --git a/ui/tests/helpers/module-for-job.js b/ui/tests/helpers/module-for-job.js index 2a2ca4234..884a2e328 100644 --- a/ui/tests/helpers/module-for-job.js +++ b/ui/tests/helpers/module-for-job.js @@ -239,6 +239,16 @@ export function moduleForJobWithClientStatus( datacenter: 'dc1', status: 'ready', }); + + clients.push( + server.create('node', { datacenter: 'dc2', status: 'ready' }) + ); + clients.push( + server.create('node', { datacenter: 'dc3', status: 'ready' }) + ); + clients.push( + server.create('node', { datacenter: 'canada-west-1', status: 'ready' }) + ); job = jobFactory(); clients.forEach((c) => { server.create('allocation', { jobId: job.id, nodeId: c.id }); diff --git a/ui/tests/unit/abilities/variable-test.js b/ui/tests/unit/abilities/variable-test.js index b31e509c3..937c74331 100644 --- a/ui/tests/unit/abilities/variable-test.js +++ b/ui/tests/unit/abilities/variable-test.js @@ -715,308 +715,6 @@ module('Unit | Ability | variable', function (hooks) { }); }); - module('#_doesMatchPattern', function () { - const edgeCaseTest = 'this is a ϗѾ test'; - - module('base cases', function () { - test('it handles an empty pattern', function (assert) { - // arrange - const pattern = ''; - const emptyPath = ''; - const nonEmptyPath = 'a'; - - // act - const matchingResult = this.ability._doesMatchPattern( - pattern, - emptyPath - ); - const nonMatchingResult = this.ability._doesMatchPattern( - pattern, - nonEmptyPath - ); - - // assert - assert.ok(matchingResult, 'Empty pattern should match empty path'); - assert.notOk( - nonMatchingResult, - 'Empty pattern should not match non-empty path' - ); - }); - - test('it handles an empty path', function (assert) { - // arrange - const emptyPath = ''; - const emptyPattern = ''; - const nonEmptyPattern = 'a'; - - // act - const matchingResult = this.ability._doesMatchPattern( - emptyPattern, - emptyPath - ); - const nonMatchingResult = this.ability._doesMatchPattern( - nonEmptyPattern, - emptyPath - ); - - // assert - assert.ok(matchingResult, 'Empty path should match empty pattern'); - assert.notOk( - nonMatchingResult, - 'Empty path should not match non-empty pattern' - ); - }); - - test('it handles a pattern without a glob', function (assert) { - // arrange - const path = '/foo'; - const matchingPattern = '/foo'; - const nonMatchingPattern = '/bar'; - - // act - const matchingResult = this.ability._doesMatchPattern( - matchingPattern, - path - ); - const nonMatchingResult = this.ability._doesMatchPattern( - nonMatchingPattern, - path - ); - - // assert - assert.ok(matchingResult, 'Matches path correctly.'); - assert.notOk(nonMatchingResult, 'Does not match non-matching path.'); - }); - - test('it handles a pattern that is a lone glob', function (assert) { - // arrange - const path = '/foo'; - const glob = '*'; - - // act - const matchingResult = this.ability._doesMatchPattern(glob, path); - - // assert - assert.ok(matchingResult, 'Matches glob.'); - }); - - test('it matches on leading glob', function (assert) { - // arrange - const pattern = '*bar'; - const matchingPath = 'footbar'; - const nonMatchingPath = 'rockthecasba'; - - // act - const matchingResult = this.ability._doesMatchPattern( - pattern, - matchingPath - ); - const nonMatchingResult = this.ability._doesMatchPattern( - pattern, - nonMatchingPath - ); - - // assert - assert.ok( - matchingResult, - 'Correctly matches when leading glob and matching path.' - ); - assert.notOk( - nonMatchingResult, - 'Does not match when leading glob and non-matching path.' - ); - }); - - test('it matches on trailing glob', function (assert) { - // arrange - const pattern = 'foo*'; - const matchingPath = 'footbar'; - const nonMatchingPath = 'bar'; - - // act - const matchingResult = this.ability._doesMatchPattern( - pattern, - matchingPath - ); - const nonMatchingResult = this.ability._doesMatchPattern( - pattern, - nonMatchingPath - ); - - // assert - assert.ok(matchingResult, 'Correctly matches on trailing glob.'); - assert.notOk( - nonMatchingResult, - 'Does not match on trailing glob if pattern does not match.' - ); - }); - - test('it matches when glob is in middle', function (assert) { - // arrange - const pattern = 'foo*bar'; - const matchingPath = 'footbar'; - const nonMatchingPath = 'footba'; - - // act - const matchingResult = this.ability._doesMatchPattern( - pattern, - matchingPath - ); - const nonMatchingResult = this.ability._doesMatchPattern( - pattern, - nonMatchingPath - ); - - // assert - assert.ok( - matchingResult, - 'Correctly matches on glob in middle of path.' - ); - assert.notOk( - nonMatchingResult, - 'Does not match on glob in middle of path if not full pattern match.' - ); - }); - }); - - module('matching edge cases', function () { - test('it matches when string is between globs', function (assert) { - // arrange - const pattern = '*is *'; - - // act - const result = this.ability._doesMatchPattern(pattern, edgeCaseTest); - - // assert - assert.ok(result); - }); - - test('it handles many non-consective globs', function (assert) { - // arrange - const pattern = '*is*a*'; - - // act - const result = this.ability._doesMatchPattern(pattern, edgeCaseTest); - - // assert - assert.ok(result); - }); - - test('it handles double globs', function (assert) { - // arrange - const pattern = '**test**'; - - // act - const result = this.ability._doesMatchPattern(pattern, edgeCaseTest); - - // assert - assert.ok(result); - }); - - test('it handles many consecutive globs', function (assert) { - // arrange - const pattern = '**is**a***test*'; - - // act - const result = this.ability._doesMatchPattern(pattern, edgeCaseTest); - - // assert - assert.ok(result); - }); - - test('it handles white space between globs', function (assert) { - // arrange - const pattern = '* *'; - - // act - const result = this.ability._doesMatchPattern(pattern, edgeCaseTest); - - // assert - assert.ok(result); - }); - - test('it handles a pattern of only globs', function (assert) { - // arrange - const pattern = '**********'; - - // act - const result = this.ability._doesMatchPattern(pattern, edgeCaseTest); - - // assert - assert.ok(result); - }); - - test('it handles unicode characters', function (assert) { - // arrange - const pattern = `*Ѿ*`; - - // act - const result = this.ability._doesMatchPattern(pattern, edgeCaseTest); - - // assert - assert.ok(result); - }); - - test('it handles mixed ASCII codes', function (assert) { - // arrange - const pattern = `*is a ϗѾ *`; - - // act - const result = this.ability._doesMatchPattern(pattern, edgeCaseTest); - - // assert - assert.ok(result); - }); - }); - - module('non-matching edge cases', function () { - const failingCases = [ - { - case: 'test*', - message: 'Implicit substring match', - }, - { - case: '*is', - message: 'Parial match', - }, - { - case: '*no*', - message: 'Globs without match between them', - }, - { - case: ' ', - message: 'Plain white space', - }, - { - case: '* ', - message: 'Trailing white space', - }, - { - case: ' *', - message: 'Leading white space', - }, - { - case: '*ʤ*', - message: 'Non-matching unicode', - }, - { - case: 'this*this is a test', - message: 'Repeated prefix', - }, - ]; - - failingCases.forEach(({ case: failingPattern, message }) => { - test('should fail the specified cases', function (assert) { - const result = this.ability._doesMatchPattern( - failingPattern, - edgeCaseTest - ); - assert.notOk(result, `${message} should not match.`); - }); - }); - }); - }); - module('#_computeLengthDiff', function () { test('should return the difference in length between a path and a pattern', function (assert) { // arrange diff --git a/ui/tests/unit/utils/match-glob-test.js b/ui/tests/unit/utils/match-glob-test.js new file mode 100644 index 000000000..055b3c6f2 --- /dev/null +++ b/ui/tests/unit/utils/match-glob-test.js @@ -0,0 +1,267 @@ +import matchGlob from 'nomad-ui/utils/match-glob'; +import { module, test } from 'qunit'; + +module('Unit | Utility | match-glob', function () { + module('#_doesMatchPattern', function () { + const edgeCaseTest = 'this is a ϗѾ test'; + + module('base cases', function () { + test('it handles an empty pattern', function (assert) { + // arrange + const pattern = ''; + const emptyPath = ''; + const nonEmptyPath = 'a'; + + // act + const matchingResult = matchGlob(pattern, emptyPath); + const nonMatchingResult = matchGlob(pattern, nonEmptyPath); + + // assert + assert.ok(matchingResult, 'Empty pattern should match empty path'); + assert.notOk( + nonMatchingResult, + 'Empty pattern should not match non-empty path' + ); + }); + + test('it handles an empty path', function (assert) { + // arrange + const emptyPath = ''; + const emptyPattern = ''; + const nonEmptyPattern = 'a'; + + // act + const matchingResult = matchGlob(emptyPattern, emptyPath); + const nonMatchingResult = matchGlob(nonEmptyPattern, emptyPath); + + // assert + assert.ok(matchingResult, 'Empty path should match empty pattern'); + assert.notOk( + nonMatchingResult, + 'Empty path should not match non-empty pattern' + ); + }); + + test('it handles a pattern without a glob', function (assert) { + // arrange + const path = '/foo'; + const matchingPattern = '/foo'; + const nonMatchingPattern = '/bar'; + + // act + const matchingResult = matchGlob(matchingPattern, path); + const nonMatchingResult = matchGlob(nonMatchingPattern, path); + + // assert + assert.ok(matchingResult, 'Matches path correctly.'); + assert.notOk(nonMatchingResult, 'Does not match non-matching path.'); + }); + + test('it handles a pattern that is a lone glob', function (assert) { + // arrange + const path = '/foo'; + const glob = '*'; + + // act + const matchingResult = matchGlob(glob, path); + + // assert + assert.ok(matchingResult, 'Matches glob.'); + }); + + test('it matches on leading glob', function (assert) { + // arrange + const pattern = '*bar'; + const matchingPath = 'footbar'; + const nonMatchingPath = 'rockthecasba'; + + // act + const matchingResult = matchGlob(pattern, matchingPath); + const nonMatchingResult = matchGlob(pattern, nonMatchingPath); + + // assert + assert.ok( + matchingResult, + 'Correctly matches when leading glob and matching path.' + ); + assert.notOk( + nonMatchingResult, + 'Does not match when leading glob and non-matching path.' + ); + }); + + test('it matches on trailing glob', function (assert) { + // arrange + const pattern = 'foo*'; + const matchingPath = 'footbar'; + const nonMatchingPath = 'bar'; + + // act + const matchingResult = matchGlob(pattern, matchingPath); + const nonMatchingResult = matchGlob(pattern, nonMatchingPath); + + // assert + assert.ok(matchingResult, 'Correctly matches on trailing glob.'); + assert.notOk( + nonMatchingResult, + 'Does not match on trailing glob if pattern does not match.' + ); + }); + + test('it matches when glob is in middle', function (assert) { + // arrange + const pattern = 'foo*bar'; + const matchingPath = 'footbar'; + const nonMatchingPath = 'footba'; + + // act + const matchingResult = matchGlob(pattern, matchingPath); + const nonMatchingResult = matchGlob(pattern, nonMatchingPath); + + // assert + assert.ok( + matchingResult, + 'Correctly matches on glob in middle of path.' + ); + assert.notOk( + nonMatchingResult, + 'Does not match on glob in middle of path if not full pattern match.' + ); + }); + }); + + module('matching edge cases', function () { + test('it matches when string is between globs', function (assert) { + // arrange + const pattern = '*is *'; + + // act + const result = matchGlob(pattern, edgeCaseTest); + + // assert + assert.ok(result); + }); + + test('it handles many non-consective globs', function (assert) { + // arrange + const pattern = '*is*a*'; + + // act + const result = matchGlob(pattern, edgeCaseTest); + + // assert + assert.ok(result); + }); + + test('it handles double globs', function (assert) { + // arrange + const pattern = '**test**'; + + // act + const result = matchGlob(pattern, edgeCaseTest); + + // assert + assert.ok(result); + }); + + test('it handles many consecutive globs', function (assert) { + // arrange + const pattern = '**is**a***test*'; + + // act + const result = matchGlob(pattern, edgeCaseTest); + + // assert + assert.ok(result); + }); + + test('it handles white space between globs', function (assert) { + // arrange + const pattern = '* *'; + + // act + const result = matchGlob(pattern, edgeCaseTest); + + // assert + assert.ok(result); + }); + + test('it handles a pattern of only globs', function (assert) { + // arrange + const pattern = '**********'; + + // act + const result = matchGlob(pattern, edgeCaseTest); + + // assert + assert.ok(result); + }); + + test('it handles unicode characters', function (assert) { + // arrange + const pattern = `*Ѿ*`; + + // act + const result = matchGlob(pattern, edgeCaseTest); + + // assert + assert.ok(result); + }); + + test('it handles mixed ASCII codes', function (assert) { + // arrange + const pattern = `*is a ϗѾ *`; + + // act + const result = matchGlob(pattern, edgeCaseTest); + + // assert + assert.ok(result); + }); + }); + + module('non-matching edge cases', function () { + const failingCases = [ + { + case: 'test*', + message: 'Implicit substring match', + }, + { + case: '*is', + message: 'Parial match', + }, + { + case: '*no*', + message: 'Globs without match between them', + }, + { + case: ' ', + message: 'Plain white space', + }, + { + case: '* ', + message: 'Trailing white space', + }, + { + case: ' *', + message: 'Leading white space', + }, + { + case: '*ʤ*', + message: 'Non-matching unicode', + }, + { + case: 'this*this is a test', + message: 'Repeated prefix', + }, + ]; + + failingCases.forEach(({ case: failingPattern, message }) => { + test('should fail the specified cases', function (assert) { + const result = matchGlob(failingPattern, edgeCaseTest); + assert.notOk(result, `${message} should not match.`); + }); + }); + }); + }); +});