From 605d7a245f7e7fe11bdd59f90a6721cd10e15b43 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 6 Dec 2018 17:11:01 -0800 Subject: [PATCH 1/6] Model isRunning based on the client status of the allocation --- ui/app/models/allocation.js | 3 +++ ui/app/models/task-state.js | 7 ++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/ui/app/models/allocation.js b/ui/app/models/allocation.js index 1a7e030a5..5e370100b 100644 --- a/ui/app/models/allocation.js +++ b/ui/app/models/allocation.js @@ -1,5 +1,6 @@ import { inject as service } from '@ember/service'; import { computed } from '@ember/object'; +import { equal } from '@ember/object/computed'; import Model from 'ember-data/model'; import attr from 'ember-data/attr'; import { belongsTo } from 'ember-data/relationships'; @@ -38,6 +39,8 @@ export default Model.extend({ return STATUS_ORDER[this.get('clientStatus')] || 100; }), + isRunning: equal('clientStatus', 'running'), + // When allocations are server-side rescheduled, a paper trail // is left linking all reschedule attempts. previousAllocation: belongsTo('allocation', { inverse: 'nextAllocation' }), diff --git a/ui/app/models/task-state.js b/ui/app/models/task-state.js index e0c8918c8..c8e25912b 100644 --- a/ui/app/models/task-state.js +++ b/ui/app/models/task-state.js @@ -1,11 +1,12 @@ -import { none } from '@ember/object/computed'; import { computed } from '@ember/object'; -import { alias } from '@ember/object/computed'; +import { alias, none } from '@ember/object/computed'; import Fragment from 'ember-data-model-fragments/fragment'; import attr from 'ember-data/attr'; import { fragment, fragmentOwner, fragmentArray } from 'ember-data-model-fragments/attributes'; export default Fragment.extend({ + allocation: fragmentOwner(), + name: attr('string'), state: attr('string'), startedAt: attr('date'), @@ -13,8 +14,8 @@ export default Fragment.extend({ failed: attr('boolean'), isActive: none('finishedAt'), + isRunning: alias('allocation.isRunning'), - allocation: fragmentOwner(), task: computed('allocation.taskGroup.tasks.[]', function() { const tasks = this.get('allocation.taskGroup.tasks'); return tasks && tasks.findBy('name', this.get('name')); From c1c40af236cf97a47849db78060f9e70a034adfa Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 10 Dec 2018 15:24:39 -0800 Subject: [PATCH 2/6] Task isRunning is based on both the task state and the allocation state --- ui/app/models/task-state.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ui/app/models/task-state.js b/ui/app/models/task-state.js index c8e25912b..eb1dcd534 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 } from '@ember/object/computed'; +import { alias, none, and } from '@ember/object/computed'; import Fragment from 'ember-data-model-fragments/fragment'; import attr from 'ember-data/attr'; import { fragment, fragmentOwner, fragmentArray } from 'ember-data-model-fragments/attributes'; @@ -14,7 +14,7 @@ export default Fragment.extend({ failed: attr('boolean'), isActive: none('finishedAt'), - isRunning: alias('allocation.isRunning'), + isRunning: and('isActive', 'allocation.isRunning'), task: computed('allocation.taskGroup.tasks.[]', function() { const tasks = this.get('allocation.taskGroup.tasks'); From 25a29cb4eee4a905b2c982662701a7c5f2627404 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 10 Dec 2018 15:25:48 -0800 Subject: [PATCH 3/6] Conditionally show utilization metrics on alloc and task rows --- ui/app/components/allocation-row.js | 17 +++-- ui/app/components/task-row.js | 22 +++--- .../templates/components/allocation-row.hbs | 68 +++++++++---------- ui/app/templates/components/task-row.hbs | 68 +++++++++---------- ui/tests/acceptance/client-detail-test.js | 10 ++- ui/tests/acceptance/task-group-detail-test.js | 3 + ui/tests/integration/allocation-row-test.js | 4 +- 7 files changed, 105 insertions(+), 87 deletions(-) diff --git a/ui/app/components/allocation-row.js b/ui/app/components/allocation-row.js index 5a85cb020..e720ac1b5 100644 --- a/ui/app/components/allocation-row.js +++ b/ui/app/components/allocation-row.js @@ -26,7 +26,9 @@ export default Component.extend({ enablePolling: computed(() => !Ember.testing), - stats: computed('allocation', function() { + stats: computed('allocation', 'allocation.isRunning', function() { + if (!this.get('allocation.isRunning')) return; + return AllocationStatsTracker.create({ fetch: url => this.get('token').authorizedRequest(url), allocation: this.get('allocation'), @@ -54,12 +56,15 @@ export default Component.extend({ fetchStats: task(function*() { do { - try { - yield this.get('stats.poll').perform(); - this.set('statsError', false); - } catch (error) { - this.set('statsError', true); + if (this.get('stats')) { + try { + yield this.get('stats.poll').perform(); + this.set('statsError', false); + } catch (error) { + this.set('statsError', true); + } } + yield timeout(500); } while (this.get('enablePolling')); }).drop(), diff --git a/ui/app/components/task-row.js b/ui/app/components/task-row.js index 970ba43df..2bb4ef7f5 100644 --- a/ui/app/components/task-row.js +++ b/ui/app/components/task-row.js @@ -22,13 +22,16 @@ export default Component.extend({ enablePolling: computed(() => !Ember.testing), // Since all tasks for an allocation share the same tracker, use the registry - stats: computed('task', function() { + stats: computed('task', 'task.isRunning', function() { + if (!this.get('task.isRunning')) return; + return this.get('statsTrackersRegistry').getTracker(this.get('task.allocation')); }), taskStats: computed('task.name', 'stats.tasks.[]', function() { - const ret = this.get('stats.tasks').findBy('task', this.get('task.name')); - return ret; + if (!this.get('stats')) return; + + return this.get('stats.tasks').findBy('task', this.get('task.name')); }), cpu: alias('taskStats.cpu.lastObject'), @@ -42,12 +45,15 @@ export default Component.extend({ fetchStats: task(function*() { do { - try { - yield this.get('stats.poll').perform(); - this.set('statsError', false); - } catch (error) { - this.set('statsError', true); + if (this.get('stats')) { + try { + yield this.get('stats.poll').perform(); + this.set('statsError', false); + } catch (error) { + this.set('statsError', true); + } } + yield timeout(500); } while (this.get('enablePolling')); }).drop(), diff --git a/ui/app/templates/components/allocation-row.hbs b/ui/app/templates/components/allocation-row.hbs index 229efef85..ffdfb9a5e 100644 --- a/ui/app/templates/components/allocation-row.hbs +++ b/ui/app/templates/components/allocation-row.hbs @@ -42,42 +42,42 @@ {{allocation.jobVersion}} {{/if}} - {{#if (and (not cpu) fetchStats.isRunning)}} - ... - {{else if (not allocation)}} - {{! nothing when there's no allocation}} - {{else if statsError}} - - {{x-icon "warning" class="is-warning"}} - - {{else}} - + {{#if allocation.isRunning}} + {{#if (and (not cpu) fetchStats.isRunning)}} + ... + {{else if statsError}} + + {{x-icon "warning" class="is-warning"}} + + {{else}} + + {{/if}} {{/if}} - {{#if (and (not memory) fetchStats.isRunning)}} - ... - {{else if (not allocation)}} - {{! nothing when there's no allocation}} - {{else if statsError}} - - {{x-icon "warning" class="is-warning"}} - - {{else}} - + {{#if allocation.isRunning}} + {{#if (and (not memory) fetchStats.isRunning)}} + ... + {{else if statsError}} + + {{x-icon "warning" class="is-warning"}} + + {{else}} + + {{/if}} {{/if}} diff --git a/ui/app/templates/components/task-row.hbs b/ui/app/templates/components/task-row.hbs index 54464ed91..55961a778 100644 --- a/ui/app/templates/components/task-row.hbs +++ b/ui/app/templates/components/task-row.hbs @@ -38,42 +38,42 @@ - {{#if (and (not cpu) fetchStats.isRunning)}} - ... - {{else if (not task)}} - {{! nothing when there's no task}} - {{else if statsError}} - - {{x-icon "warning" class="is-warning"}} - - {{else}} - + {{#if task.isRunning}} + {{#if (and (not cpu) fetchStats.isRunning)}} + ... + {{else if statsError}} + + {{x-icon "warning" class="is-warning"}} + + {{else}} + + {{/if}} {{/if}} - {{#if (and (not memory) fetchStats.isRunning)}} - ... - {{else if (not task)}} - {{! nothing when there's no task}} - {{else if statsError}} - - {{x-icon "warning" class="is-warning"}} - - {{else}} - + {{#if task.isRunning}} + {{#if (and (not memory) fetchStats.isRunning)}} + ... + {{else if statsError}} + + {{x-icon "warning" class="is-warning"}} + + {{else}} + + {{/if}} {{/if}} diff --git a/ui/tests/acceptance/client-detail-test.js b/ui/tests/acceptance/client-detail-test.js index b13ce5ade..28e960ad2 100644 --- a/ui/tests/acceptance/client-detail-test.js +++ b/ui/tests/acceptance/client-detail-test.js @@ -19,7 +19,7 @@ moduleForAcceptance('Acceptance | client detail', { // Related models server.create('agent'); server.create('job', { createAllocations: false }); - server.createList('allocation', 3, { nodeId: node.id }); + server.createList('allocation', 3, { nodeId: node.id, clientStatus: 'running' }); }, }); @@ -545,10 +545,14 @@ moduleForAcceptance('Acceptance | client detail (multi-namespace)', { // Make a job for each namespace, but have both scheduled on the same node server.create('job', { id: 'job-1', namespaceId: 'default', createAllocations: false }); - server.createList('allocation', 3, { nodeId: node.id }); + server.createList('allocation', 3, { nodeId: node.id, clientStatus: 'running' }); server.create('job', { id: 'job-2', namespaceId: 'other-namespace', createAllocations: false }); - server.createList('allocation', 3, { nodeId: node.id, jobId: 'job-2' }); + server.createList('allocation', 3, { + nodeId: node.id, + jobId: 'job-2', + clientStatus: 'running', + }); }, }); diff --git a/ui/tests/acceptance/task-group-detail-test.js b/ui/tests/acceptance/task-group-detail-test.js index 13f2d71ba..5f2efaab2 100644 --- a/ui/tests/acceptance/task-group-detail-test.js +++ b/ui/tests/acceptance/task-group-detail-test.js @@ -33,6 +33,7 @@ moduleForAcceptance('Acceptance | task group detail', { allocations = server.createList('allocation', 2, { jobId: job.id, taskGroup: taskGroup.name, + clientStatus: 'running', }); // Allocations associated to a different task group on the job to @@ -40,6 +41,7 @@ moduleForAcceptance('Acceptance | task group detail', { server.createList('allocation', 3, { jobId: job.id, taskGroup: taskGroups[1].name, + clientStatus: 'running', }); // Set a static name to make the search test deterministic @@ -118,6 +120,7 @@ test('/jobs/:id/:task-group should list one page of allocations for the task gro server.createList('allocation', TaskGroup.pageSize, { jobId: job.id, taskGroup: taskGroup.name, + clientStatus: 'running', }); JobsList.visit(); diff --git a/ui/tests/integration/allocation-row-test.js b/ui/tests/integration/allocation-row-test.js index a656732eb..60c9b0d1a 100644 --- a/ui/tests/integration/allocation-row-test.js +++ b/ui/tests/integration/allocation-row-test.js @@ -49,7 +49,7 @@ test('Allocation row polls for stats, even when it errors or has an invalid resp return new Response(500, {}, ''); }); - this.server.create('allocation'); + this.server.create('allocation', { clientStatus: 'running' }); this.store.findAll('allocation'); let allocation; @@ -93,7 +93,7 @@ test('Allocation row shows warning when it requires drivers that are unhealthy o }); node.update({ drivers }); - this.server.create('allocation'); + this.server.create('allocation', { clientStatus: 'running' }); this.store.findAll('job'); this.store.findAll('node'); this.store.findAll('allocation'); From e9ba939bd6329ef0ce33a106fc1a7c0c370ccbfa Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 10 Dec 2018 15:26:27 -0800 Subject: [PATCH 4/6] Conditionally show the utilization graphs on the allocation and task detail pages --- .../allocations/allocation/index.hbs | 19 +++++++++++++------ .../allocations/allocation/task/index.hbs | 19 +++++++++++++------ 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/ui/app/templates/allocations/allocation/index.hbs b/ui/app/templates/allocations/allocation/index.hbs index 9ea4f650d..668094bd0 100644 --- a/ui/app/templates/allocations/allocation/index.hbs +++ b/ui/app/templates/allocations/allocation/index.hbs @@ -22,14 +22,21 @@ Resource Utilization
-
-
- {{primary-metric resource=model metric="cpu"}} + {{#if model.isRunning}} +
+
+ {{primary-metric resource=model metric="cpu"}} +
+
+ {{primary-metric resource=model metric="memory"}} +
-
- {{primary-metric resource=model metric="memory"}} + {{else}} +
+

Allocation isn't running

+

Only running allocations utilize resources.

-
+ {{/if}}
diff --git a/ui/app/templates/allocations/allocation/task/index.hbs b/ui/app/templates/allocations/allocation/task/index.hbs index dad7a4a71..4d4a46a08 100644 --- a/ui/app/templates/allocations/allocation/task/index.hbs +++ b/ui/app/templates/allocations/allocation/task/index.hbs @@ -30,14 +30,21 @@ Resource Utilization
-
-
- {{primary-metric resource=model metric="cpu"}} + {{#if model.isRunning}} +
+
+ {{primary-metric resource=model metric="cpu"}} +
+
+ {{primary-metric resource=model metric="memory"}} +
-
- {{primary-metric resource=model metric="memory"}} + {{else}} +
+

Task isn't running

+

Only running tasks utilize resources.

-
+ {{/if}}
From 453f11d0c60fb67f6fdc55e09d572e9901d355d7 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 10 Dec 2018 17:28:23 -0800 Subject: [PATCH 5/6] Test coverage for allocation rows --- ui/tests/integration/allocation-row-test.js | 65 +++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/ui/tests/integration/allocation-row-test.js b/ui/tests/integration/allocation-row-test.js index 60c9b0d1a..a2204f621 100644 --- a/ui/tests/integration/allocation-row-test.js +++ b/ui/tests/integration/allocation-row-test.js @@ -7,6 +7,7 @@ import { startMirage } from 'nomad-ui/initializers/ember-cli-mirage'; import { find } from 'ember-native-dom-helpers'; import Response from 'ember-cli-mirage/response'; import { initialize as fragmentSerializerInitializer } from 'nomad-ui/initializers/fragment-serializer'; +import { Promise, resolve } from 'rsvp'; moduleForComponent('allocation-row', 'Integration | Component | allocation row', { integration: true, @@ -120,3 +121,67 @@ test('Allocation row shows warning when it requires drivers that are unhealthy o assert.ok(find('[data-test-icon="unhealthy-driver"]'), 'Unhealthy driver icon is shown'); }); }); + +test('when an allocation is not running, the utilization graphs are omitted', function(assert) { + this.setProperties({ + context: 'job', + enablePolling: false, + }); + + // All non-running statuses need to be tested + ['pending', 'complete', 'failed', 'lost'].forEach(clientStatus => + this.server.create('allocation', { clientStatus }) + ); + + this.store.findAll('allocation'); + + return wait().then(() => { + const allocations = this.store.peekAll('allocation'); + return waitForEach( + allocations.map(allocation => () => { + this.set('allocation', allocation); + this.render(hbs` + {{allocation-row + allocation=allocation + context=context + enablePolling=enablePolling}} + `); + return wait().then(() => { + const status = allocation.get('clientStatus'); + assert.notOk(find('[data-test-cpu] .inline-chart'), `No CPU chart for ${status}`); + assert.notOk(find('[data-test-mem] .inline-chart'), `No Mem chart for ${status}`); + }); + }) + ); + }); +}); + +// A way to loop over asynchronous code. Can be replaced by async/await in the future. +const waitForEach = fns => { + let i = 0; + let done = () => {}; + + // This function is asynchronous and needs to return a promise + const pending = new Promise(resolve => { + done = resolve; + }); + + const step = () => { + // The waitForEach promise and this recursive loop are done once + // all functions have been called. + if (i >= fns.length) { + done(); + return; + } + // Call the current function + const promise = fns[i]() || resolve(true); + // Increment the function position + i++; + // Wait for async behaviors to settle and repeat + promise.then(() => wait()).then(step); + }; + + step(); + + return pending; +}; From bf9ff6ed8b19d6df7e545a5a28d07a746c32fb1f Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 10 Dec 2018 17:28:35 -0800 Subject: [PATCH 6/6] Test coverage for resource graph empty states --- .../allocations/allocation/index.hbs | 4 ++-- .../allocations/allocation/task/index.hbs | 4 ++-- ui/tests/acceptance/allocation-detail-test.js | 21 +++++++++++++++++++ ui/tests/acceptance/task-detail-test.js | 19 +++++++++++++++++ ui/tests/pages/allocations/detail.js | 2 ++ ui/tests/pages/allocations/task/detail.js | 2 ++ 6 files changed, 48 insertions(+), 4 deletions(-) diff --git a/ui/app/templates/allocations/allocation/index.hbs b/ui/app/templates/allocations/allocation/index.hbs index 668094bd0..19d499dc5 100644 --- a/ui/app/templates/allocations/allocation/index.hbs +++ b/ui/app/templates/allocations/allocation/index.hbs @@ -32,8 +32,8 @@
{{else}} -
-

Allocation isn't running

+
+

Allocation isn't running

Only running allocations utilize resources.

{{/if}} diff --git a/ui/app/templates/allocations/allocation/task/index.hbs b/ui/app/templates/allocations/allocation/task/index.hbs index 4d4a46a08..ae6523c01 100644 --- a/ui/app/templates/allocations/allocation/task/index.hbs +++ b/ui/app/templates/allocations/allocation/task/index.hbs @@ -40,8 +40,8 @@
{{else}} -
-

Task isn't running

+
+

Task isn't running

Only running tasks utilize resources.

{{/if}} diff --git a/ui/tests/acceptance/allocation-detail-test.js b/ui/tests/acceptance/allocation-detail-test.js index bb7854ac9..dba89d9b5 100644 --- a/ui/tests/acceptance/allocation-detail-test.js +++ b/ui/tests/acceptance/allocation-detail-test.js @@ -191,3 +191,24 @@ moduleForAcceptance('Acceptance | allocation detail (rescheduled)', { test('when the allocation has been rescheduled, the reschedule events section is rendered', function(assert) { assert.ok(Allocation.hasRescheduleEvents, 'Reschedule Events section exists'); }); + +moduleForAcceptance('Acceptance | allocation detail (not running)', { + beforeEach() { + server.create('agent'); + + node = server.create('node'); + job = server.create('job', { createAllocations: false }); + allocation = server.create('allocation', { clientStatus: 'pending' }); + + Allocation.visit({ id: allocation.id }); + }, +}); + +test('when the allocation is not running, the utilization graphs are replaced by an empty message', function(assert) { + assert.equal(Allocation.resourceCharts.length, 0, 'No resource charts'); + assert.equal( + Allocation.resourceEmptyMessage, + "Allocation isn't running", + 'Empty message is appropriate' + ); +}); diff --git a/ui/tests/acceptance/task-detail-test.js b/ui/tests/acceptance/task-detail-test.js index 177df255d..aab5c954c 100644 --- a/ui/tests/acceptance/task-detail-test.js +++ b/ui/tests/acceptance/task-detail-test.js @@ -271,3 +271,22 @@ test('breadcrumbs match jobs / job / task group / allocation / task', function(a ); }); }); + +moduleForAcceptance('Acceptance | task detail (not running)', { + beforeEach() { + server.create('agent'); + server.create('node'); + server.create('namespace'); + server.create('namespace', { id: 'other-namespace' }); + server.create('job', { createAllocations: false, namespaceId: 'other-namespace' }); + allocation = server.create('allocation', 'withTaskWithPorts', { clientStatus: 'complete' }); + task = server.db.taskStates.where({ allocationId: allocation.id })[0]; + + Task.visit({ id: allocation.id, name: task.name }); + }, +}); + +test('when the allocation for a task is not running, the resource utilization graphs are replaced by an empty message', function(assert) { + assert.equal(Task.resourceCharts.length, 0, 'No resource charts'); + assert.equal(Task.resourceEmptyMessage, "Task isn't running", 'Empty message is appropriate'); +}); diff --git a/ui/tests/pages/allocations/detail.js b/ui/tests/pages/allocations/detail.js index bc3bc165d..54ce5acca 100644 --- a/ui/tests/pages/allocations/detail.js +++ b/ui/tests/pages/allocations/detail.js @@ -28,6 +28,8 @@ export default create({ chartClass: attribute('class', '[data-test-percentage-chart] progress'), }), + resourceEmptyMessage: text('[data-test-resource-error-headline]'), + tasks: collection('[data-test-task-row]', { name: text('[data-test-name]'), state: text('[data-test-state]'), diff --git a/ui/tests/pages/allocations/task/detail.js b/ui/tests/pages/allocations/task/detail.js index 249986cfb..9e5c0a60b 100644 --- a/ui/tests/pages/allocations/task/detail.js +++ b/ui/tests/pages/allocations/task/detail.js @@ -30,6 +30,8 @@ export default create({ chartClass: attribute('class', '[data-test-percentage-chart] progress'), }), + resourceEmptyMessage: text('[data-test-resource-error-headline]'), + hasAddresses: isPresent('[data-test-task-addresses]'), addresses: collection('[data-test-task-address]', { name: text('[data-test-task-address-name]'),