Skip to content

Commit

Permalink
ui: use task state to determine if task is active (#14224)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
lgfa29 authored Aug 23, 2022
1 parent dc1c50e commit 3953d41
Show file tree
Hide file tree
Showing 6 changed files with 65 additions and 23 deletions.
3 changes: 3 additions & 0 deletions .changelog/14224.txt
Original file line number Diff line number Diff line change
@@ -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.
```
8 changes: 6 additions & 2 deletions ui/app/models/task-state.js
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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')
Expand Down Expand Up @@ -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);
}
Expand Down
49 changes: 38 additions & 11 deletions ui/tests/acceptance/allocation-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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(
Expand Down
22 changes: 12 additions & 10 deletions ui/tests/acceptance/exec-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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' }
);
});
});

Expand Down Expand Up @@ -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 });
Expand All @@ -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;
Expand All @@ -187,7 +189,7 @@ module('Acceptance | exec', function (hooks) {
);

if (changingTaskState) {
changingTaskState.set('finishedAt', undefined);
changingTaskState.set('state', 'running');
}
});

Expand Down
4 changes: 4 additions & 0 deletions ui/tests/acceptance/task-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down
2 changes: 2 additions & 0 deletions ui/tests/pages/allocations/detail.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]'),

Expand Down

0 comments on commit 3953d41

Please sign in to comment.