From 72e7c81dbe50560dedba7977f2aef245b58727d8 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Mon, 22 Aug 2022 18:04:28 -0400 Subject: [PATCH 1/3] ui: use task state to determine if task is active 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. --- ui/app/models/task-state.js | 8 ++- ui/tests/acceptance/allocation-detail-test.js | 49 ++++++++++++++----- ui/tests/pages/allocations/detail.js | 2 + 3 files changed, 46 insertions(+), 13 deletions(-) diff --git a/ui/app/models/task-state.js b/ui/app/models/task-state.js index 0f80cdc3041..6a16fc85317 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 fecebd6e53e..fe25f7b3395 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/pages/allocations/detail.js b/ui/tests/pages/allocations/detail.js index cabc446727e..5ad69151f35 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]'), From 1542604ec948f00afeb72ae06e38398434b928d6 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Mon, 22 Aug 2022 18:26:38 -0400 Subject: [PATCH 2/3] changelog: add entry for #14224 --- .changelog/14224.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/14224.txt diff --git a/.changelog/14224.txt b/.changelog/14224.txt new file mode 100644 index 00000000000..8e05f384d30 --- /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. +``` From 434ee9941680b6519c2da9a3fe265772d4f24d75 Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Tue, 23 Aug 2022 09:34:33 -0400 Subject: [PATCH 3/3] ui: fix tests for active tasks --- ui/tests/acceptance/exec-test.js | 22 ++++++++++++---------- ui/tests/acceptance/task-detail-test.js | 4 ++++ 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/ui/tests/acceptance/exec-test.js b/ui/tests/acceptance/exec-test.js index b37f164fa5d..b3a6df011ff 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 5fb181e5677..02a06e58bb5 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 });