From af5c01a070e83691e6664d4467100c2d544af2ed Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Tue, 23 Aug 2022 15:50:40 -0400 Subject: [PATCH] ui: use task state to determine if task is active (#14224) The current implementation uses the task's finishedAt field to determine if a task is active of not, but this check is not accurate. A task in the "pending" state will not have finishedAt value but it's also not active. This discrepancy results in some components, like the inline stats chart of the task row component, to be displayed even whey they shouldn't. --- .changelog/14224.txt | 3 ++ ui/app/models/task-state.js | 8 ++- ui/tests/acceptance/allocation-detail-test.js | 49 ++++++++++++++----- ui/tests/acceptance/exec-test.js | 22 +++++---- ui/tests/acceptance/task-detail-test.js | 4 ++ ui/tests/pages/allocations/detail.js | 2 + 6 files changed, 65 insertions(+), 23 deletions(-) create mode 100644 .changelog/14224.txt diff --git a/.changelog/14224.txt b/.changelog/14224.txt new file mode 100644 index 000000000..8e05f384d --- /dev/null +++ b/.changelog/14224.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: Fixed a bug that caused the allocation details page to display the stats bar chart even if the task was pending. +``` diff --git a/ui/app/models/task-state.js b/ui/app/models/task-state.js index 0f80cdc30..6a16fc853 100644 --- a/ui/app/models/task-state.js +++ b/ui/app/models/task-state.js @@ -1,5 +1,5 @@ import { computed } from '@ember/object'; -import { alias, none, and } from '@ember/object/computed'; +import { alias, and } from '@ember/object/computed'; import Fragment from 'ember-data-model-fragments/fragment'; import { attr } from '@ember-data/model'; import { @@ -19,7 +19,6 @@ export default class TaskState extends Fragment { @attr('date') finishedAt; @attr('boolean') failed; - @none('finishedAt') isActive; @and('isActive', 'allocation.isRunning') isRunning; @computed('task.kind') @@ -58,6 +57,11 @@ export default class TaskState extends Fragment { return classMap[this.state] || 'is-dark'; } + @computed('state') + get isActive() { + return this.state === 'running'; + } + restart() { return this.allocation.restart(this.name); } diff --git a/ui/tests/acceptance/allocation-detail-test.js b/ui/tests/acceptance/allocation-detail-test.js index fecebd6e5..fe25f7b33 100644 --- a/ui/tests/acceptance/allocation-detail-test.js +++ b/ui/tests/acceptance/allocation-detail-test.js @@ -158,24 +158,47 @@ module('Acceptance | allocation detail', function (hooks) { }); test('each task row should list high-level information for the task', async function (assert) { - const task = server.db.taskStates - .where({ allocationId: allocation.id }) - .sortBy('name')[0]; - const events = server.db.taskEvents.where({ taskStateId: task.id }); - const event = events[events.length - 1]; + const job = server.create('job', { + groupsCount: 1, + groupTaskCount: 3, + withGroupServices: true, + createAllocations: false, + }); + + const allocation = server.create('allocation', 'withTaskWithPorts', { + clientStatus: 'running', + jobId: job.id, + }); const taskGroup = server.schema.taskGroups.where({ jobId: allocation.jobId, name: allocation.taskGroup, }).models[0]; - const jobTask = taskGroup.tasks.models.find((m) => m.name === task.name); - const volumes = jobTask.volumeMounts.map((volume) => ({ - name: volume.Volume, - source: taskGroup.volumes[volume.Volume].Source, - })); + // Set the expected task states. + const states = ['running', 'pending', 'dead']; + server.db.taskStates + .where({ allocationId: allocation.id }) + .sortBy('name') + .forEach((task, i) => { + server.db.taskStates.update(task.id, { state: states[i] }); + }); + + await Allocation.visit({ id: allocation.id }); + + Allocation.tasks.forEach((taskRow, i) => { + const task = server.db.taskStates + .where({ allocationId: allocation.id }) + .sortBy('name')[i]; + const events = server.db.taskEvents.where({ taskStateId: task.id }); + const event = events[events.length - 1]; + + const jobTask = taskGroup.tasks.models.find((m) => m.name === task.name); + const volumes = jobTask.volumeMounts.map((volume) => ({ + name: volume.Volume, + source: taskGroup.volumes[volume.Volume].Source, + })); - Allocation.tasks[0].as((taskRow) => { assert.equal(taskRow.name, task.name, 'Name'); assert.equal(taskRow.state, task.state, 'State'); assert.equal(taskRow.message, event.displayMessage, 'Event Message'); @@ -185,6 +208,10 @@ module('Acceptance | allocation detail', function (hooks) { 'Event Time' ); + const expectStats = task.state === 'running'; + assert.equal(taskRow.hasCpuMetrics, expectStats, 'CPU metrics'); + assert.equal(taskRow.hasMemoryMetrics, expectStats, 'Memory metrics'); + const volumesText = taskRow.volumes; volumes.forEach((volume) => { assert.ok( diff --git a/ui/tests/acceptance/exec-test.js b/ui/tests/acceptance/exec-test.js index b37f164fa..b3a6df011 100644 --- a/ui/tests/acceptance/exec-test.js +++ b/ui/tests/acceptance/exec-test.js @@ -27,11 +27,15 @@ module('Acceptance | exec', function (hooks) { }); this.job.taskGroups.models.forEach((taskGroup) => { - server.create('allocation', { + const alloc = server.create('allocation', { jobId: this.job.id, taskGroup: taskGroup.name, forceRunningClientStatus: true, }); + server.db.taskStates.update( + { allocationId: alloc.id }, + { state: 'running' } + ); }); }); @@ -135,12 +139,11 @@ module('Acceptance | exec', function (hooks) { let runningTaskGroup = this.job.taskGroups.models.sortBy('name')[1]; runningTaskGroup.tasks.models.forEach((task, index) => { + let state = 'running'; if (index > 0) { - this.server.db.taskStates.update( - { name: task.name }, - { finishedAt: new Date() } - ); + state = 'dead'; } + this.server.db.taskStates.update({ name: task.name }, { state }); }); await Exec.visitJob({ job: this.job.id }); @@ -159,12 +162,11 @@ module('Acceptance | exec', function (hooks) { let runningTaskGroup = this.job.taskGroups.models.sortBy('name')[1]; let changingTaskStateName; runningTaskGroup.tasks.models.sortBy('name').forEach((task, index) => { + let state = 'running'; if (index > 0) { - this.server.db.taskStates.update( - { name: task.name }, - { finishedAt: new Date() } - ); + state = 'dead'; } + this.server.db.taskStates.update({ name: task.name }, { state }); if (index === 1) { changingTaskStateName = task.name; @@ -187,7 +189,7 @@ module('Acceptance | exec', function (hooks) { ); if (changingTaskState) { - changingTaskState.set('finishedAt', undefined); + changingTaskState.set('state', 'running'); } }); diff --git a/ui/tests/acceptance/task-detail-test.js b/ui/tests/acceptance/task-detail-test.js index 5fb181e56..02a06e58b 100644 --- a/ui/tests/acceptance/task-detail-test.js +++ b/ui/tests/acceptance/task-detail-test.js @@ -22,6 +22,10 @@ module('Acceptance | task detail', function (hooks) { allocation = server.create('allocation', 'withTaskWithPorts', { clientStatus: 'running', }); + server.db.taskStates.update( + { allocationId: allocation.id }, + { state: 'running' } + ); task = server.db.taskStates.where({ allocationId: allocation.id })[0]; await Task.visit({ id: allocation.id, name: task.name }); diff --git a/ui/tests/pages/allocations/detail.js b/ui/tests/pages/allocations/detail.js index cabc44672..5ad69151f 100644 --- a/ui/tests/pages/allocations/detail.js +++ b/ui/tests/pages/allocations/detail.js @@ -50,6 +50,8 @@ export default create({ time: text('[data-test-time]'), volumes: text('[data-test-volumes]'), + hasCpuMetrics: isPresent('[data-test-cpu] .inline-chart progress'), + hasMemoryMetrics: isPresent('[data-test-mem] .inline-chart progress'), hasUnhealthyDriver: isPresent('[data-test-icon="unhealthy-driver"]'), hasProxyTag: isPresent('[data-test-proxy-tag]'),