From 95dc5da201d4a5370924593273d9cfccd1f35261 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 5 Jul 2018 10:17:56 -0700 Subject: [PATCH 1/6] Add an env var to toggle blockingQueries support Mostly helpful when working with mirage --- ui/app/utils/properties/watch.js | 9 ++++++--- ui/config/environment.js | 3 +-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/ui/app/utils/properties/watch.js b/ui/app/utils/properties/watch.js index a372d293d..aec2c6921 100644 --- a/ui/app/utils/properties/watch.js +++ b/ui/app/utils/properties/watch.js @@ -3,13 +3,16 @@ import { get } from '@ember/object'; import RSVP from 'rsvp'; import { task } from 'ember-concurrency'; import wait from 'nomad-ui/utils/wait'; +import config from 'nomad-ui/config/environment'; + +const isEnabled = config.APP.blockingQueries !== false; export function watchRecord(modelName) { return task(function*(id, throttle = 2000) { if (typeof id === 'object') { id = get(id, 'id'); } - while (!Ember.testing) { + while (isEnabled && !Ember.testing) { try { yield RSVP.all([ this.get('store').findRecord(modelName, id, { @@ -32,7 +35,7 @@ export function watchRecord(modelName) { export function watchRelationship(relationshipName) { return task(function*(model, throttle = 2000) { - while (!Ember.testing) { + while (isEnabled && !Ember.testing) { try { yield RSVP.all([ this.get('store') @@ -54,7 +57,7 @@ export function watchRelationship(relationshipName) { export function watchAll(modelName) { return task(function*(throttle = 2000) { - while (!Ember.testing) { + while (isEnabled && !Ember.testing) { try { yield RSVP.all([ this.get('store').findAll(modelName, { reload: true, adapterOptions: { watch: true } }), diff --git a/ui/config/environment.js b/ui/config/environment.js index fdceb4472..b97c3f183 100644 --- a/ui/config/environment.js +++ b/ui/config/environment.js @@ -19,8 +19,7 @@ module.exports = function(environment) { }, APP: { - // Here you can pass flags/options to your application instance - // when it is created + blockingQueries: true, }, }; From 38034733f7ac37d90cbb876e5bd8a2063947ea97 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 5 Jul 2018 11:25:12 -0700 Subject: [PATCH 2/6] Remove unused job adapter method --- ui/app/adapters/job.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/ui/app/adapters/job.js b/ui/app/adapters/job.js index eeba06522..2e47090c2 100644 --- a/ui/app/adapters/job.js +++ b/ui/app/adapters/job.js @@ -1,5 +1,4 @@ import { inject as service } from '@ember/service'; -import { assign } from '@ember/polyfills'; import Watchable from './watchable'; export default Watchable.extend({ @@ -24,12 +23,6 @@ export default Watchable.extend({ }); }, - findRecordSummary(modelName, name, snapshot, namespaceQuery) { - return this.ajax(`${this.buildURL(modelName, name, snapshot, 'findRecord')}/summary`, 'GET', { - data: assign(this.buildQuery() || {}, namespaceQuery), - }); - }, - findRecord(store, type, id, snapshot) { const [, namespace] = JSON.parse(id); const namespaceQuery = namespace && namespace !== 'default' ? { namespace } : {}; From 566c5843dec3dd048a9ddb540ca22fa061cf94f7 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 5 Jul 2018 11:24:14 -0700 Subject: [PATCH 3/6] Remove unused findAllocations method in both job and node This was replaced with a relationship at some point. --- ui/app/adapters/job.js | 11 +---------- ui/app/adapters/node.js | 11 +---------- 2 files changed, 2 insertions(+), 20 deletions(-) diff --git a/ui/app/adapters/job.js b/ui/app/adapters/job.js index 2e47090c2..1d5919195 100644 --- a/ui/app/adapters/job.js +++ b/ui/app/adapters/job.js @@ -52,18 +52,9 @@ export default Watchable.extend({ summary: '/summary', }, - findAllocations(job) { - const url = `${this.buildURL('job', job.get('id'), job, 'findRecord')}/allocations`; - return this.ajax(url, 'GET', { data: this.buildQuery() }).then(allocs => { - return this.store.pushPayload('allocation', { - allocations: allocs, - }); - }); - }, - fetchRawDefinition(job) { const url = this.buildURL('job', job.get('id'), job, 'findRecord'); - return this.ajax(url, 'GET', { data: this.buildQuery() }); + return this.ajax(url, 'GET'); }, forcePeriodic(job) { diff --git a/ui/app/adapters/node.js b/ui/app/adapters/node.js index 5d77bebd2..8aa4b662f 100644 --- a/ui/app/adapters/node.js +++ b/ui/app/adapters/node.js @@ -1,12 +1,3 @@ import Watchable from './watchable'; -export default Watchable.extend({ - findAllocations(node) { - const url = `${this.buildURL('node', node.get('id'), node, 'findRecord')}/allocations`; - return this.ajax(url, 'GET').then(allocs => { - return this.store.pushPayload('allocation', { - allocations: allocs, - }); - }); - }, -}); +export default Watchable.extend(); From 877910208f0773bf2660f47df4a148e7d44397cc Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 5 Jul 2018 12:29:29 -0700 Subject: [PATCH 4/6] Don't put namespace logic in the catch-all buildQuery method --- ui/app/adapters/job.js | 34 ++++++++++++++--------------- ui/tests/unit/adapters/job-test.js | 35 ++++++++++++++++++++---------- 2 files changed, 40 insertions(+), 29 deletions(-) diff --git a/ui/app/adapters/job.js b/ui/app/adapters/job.js index 1d5919195..e13047815 100644 --- a/ui/app/adapters/job.js +++ b/ui/app/adapters/job.js @@ -4,15 +4,6 @@ import Watchable from './watchable'; export default Watchable.extend({ system: service(), - buildQuery() { - const namespace = this.get('system.activeNamespace.id'); - - if (namespace && namespace !== 'default') { - return { namespace }; - } - return {}; - }, - findAll() { const namespace = this.get('system.activeNamespace'); return this._super(...arguments).then(data => { @@ -30,22 +21,22 @@ export default Watchable.extend({ return this._super(store, type, id, snapshot, namespaceQuery); }, + urlForFindAll() { + const url = this._super(...arguments); + const namespace = this.get('system.activeNamespace.id'); + return associateNamespace(url, namespace); + }, + urlForFindRecord(id, type, hash) { const [name, namespace] = JSON.parse(id); let url = this._super(name, type, hash); - if (namespace && namespace !== 'default') { - url += `?namespace=${namespace}`; - } - return url; + return associateNamespace(url, namespace); }, xhrKey(url, method, options = {}) { const plainKey = this._super(...arguments); const namespace = options.data && options.data.namespace; - if (namespace) { - return `${plainKey}?namespace=${namespace}`; - } - return plainKey; + return associateNamespace(plainKey, namespace); }, relationshipFallbackLinks: { @@ -53,7 +44,7 @@ export default Watchable.extend({ }, fetchRawDefinition(job) { - const url = this.buildURL('job', job.get('id'), job, 'findRecord'); + const url = this.urlForFindRecord(job.get('id'), 'job'); return this.ajax(url, 'GET'); }, @@ -70,6 +61,13 @@ export default Watchable.extend({ }, }); +function associateNamespace(url, namespace) { + if (namespace && namespace !== 'default') { + url += `?namespace=${namespace}`; + } + return url; +} + function addToPath(url, extension = '') { const [path, params] = url.split('?'); let newUrl = `${path}${extension}`; diff --git a/ui/tests/unit/adapters/job-test.js b/ui/tests/unit/adapters/job-test.js index aa6827d5a..c38d20036 100644 --- a/ui/tests/unit/adapters/job-test.js +++ b/ui/tests/unit/adapters/job-test.js @@ -22,6 +22,7 @@ moduleForAdapter('job', 'Unit | Adapter | Job', { ], beforeEach() { window.sessionStorage.clear(); + window.localStorage.clear(); this.server = startMirage(); this.server.create('node'); @@ -33,7 +34,7 @@ moduleForAdapter('job', 'Unit | Adapter | Job', { }, }); -test('The job summary is stitched into the job request', function(assert) { +test('The job endpoint is the only required endpoint for fetching a job', function(assert) { const { pretender } = this.server; const jobName = 'job-1'; const jobNamespace = 'default'; @@ -43,8 +44,25 @@ test('The job summary is stitched into the job request', function(assert) { assert.deepEqual( pretender.handledRequests.mapBy('url'), - ['/v1/namespaces', `/v1/job/${jobName}`], - 'The two requests made are /namespaces and /job/:id' + [`/v1/job/${jobName}`], + 'The only request made is /job/:id' + ); +}); + +test('When a namespace is in localStorage and the requested job is in the default namespace, the namespace query param is left out', function(assert) { + window.localStorage.nomadActiveNamespace = 'red-herring'; + + const { pretender } = this.server; + const jobName = 'job-1'; + const jobNamespace = 'default'; + const jobId = JSON.stringify([jobName, jobNamespace]); + + this.subject().findRecord(null, { modelName: 'job' }, jobId); + + assert.deepEqual( + pretender.handledRequests.mapBy('url'), + [`/v1/job/${jobName}`], + 'The request made is /job/:id with no namespace query param' ); }); @@ -58,8 +76,8 @@ test('When the job has a namespace other than default, it is in the URL', functi assert.deepEqual( pretender.handledRequests.mapBy('url'), - ['/v1/namespaces', `/v1/job/${jobName}?namespace=${jobNamespace}`], - 'The two requests made are /namespaces and /job/:id?namespace=:namespace' + [`/v1/job/${jobName}?namespace=${jobNamespace}`], + 'The only request made is /job/:id?namespace=:namespace' ); }); @@ -137,11 +155,6 @@ test('findRecord can be watched', function(assert) { request(); assert.equal( pretender.handledRequests[0].url, - '/v1/namespaces', - 'First request is for namespaces' - ); - assert.equal( - pretender.handledRequests[1].url, '/v1/job/job-1?index=1', 'Second request is a blocking request for job-1' ); @@ -149,7 +162,7 @@ test('findRecord can be watched', function(assert) { return wait().then(() => { request(); assert.equal( - pretender.handledRequests[2].url, + pretender.handledRequests[1].url, '/v1/job/job-1?index=2', 'Third request is a blocking request with an incremented index param' ); From fc0e723d75a1accc3fbdac0eb0966ffc57a5f851 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 5 Jul 2018 13:49:27 -0700 Subject: [PATCH 5/6] A test to assert the bug --- ui/tests/unit/adapters/job-test.js | 65 +++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 15 deletions(-) diff --git a/ui/tests/unit/adapters/job-test.js b/ui/tests/unit/adapters/job-test.js index c38d20036..1eca17e93 100644 --- a/ui/tests/unit/adapters/job-test.js +++ b/ui/tests/unit/adapters/job-test.js @@ -1,4 +1,5 @@ import EmberObject from '@ember/object'; +import { getOwner } from '@ember/application'; import { run } from '@ember/runloop'; import { assign } from '@ember/polyfills'; import { test } from 'ember-qunit'; @@ -8,26 +9,43 @@ import moduleForAdapter from '../../helpers/module-for-adapter'; moduleForAdapter('job', 'Unit | Adapter | Job', { needs: [ + 'adapter:application', 'adapter:job', - 'service:token', - 'service:system', + 'adapter:namespace', + 'model:task-group', 'model:allocation', 'model:deployment', 'model:evaluation', 'model:job-summary', 'model:job-version', 'model:namespace', - 'adapter:application', + 'model:task-group-summary', + 'serializer:namespace', + 'serializer:job', + 'serializer:job-summary', + 'service:token', + 'service:system', 'service:watchList', + 'transform:fragment', + 'transform:fragment-array', ], beforeEach() { window.sessionStorage.clear(); window.localStorage.clear(); this.server = startMirage(); + this.server.create('namespace'); + this.server.create('namespace', { id: 'some-namespace' }); this.server.create('node'); - this.server.create('job', { id: 'job-1' }); + this.server.create('job', { id: 'job-1', namespaceId: 'default' }); this.server.create('job', { id: 'job-2', namespaceId: 'some-namespace' }); + + this.system = getOwner(this).lookup('service:system'); + this.system.get('namespaces'); + + // Reset the handledRequests array to avoid accounting for this + // namespaces request everywhere. + this.server.pretender.handledRequests.length = 0; }, afterEach() { this.server.shutdown(); @@ -49,6 +67,26 @@ test('The job endpoint is the only required endpoint for fetching a job', functi ); }); +test('When a namespace is set in localStorage but a job in the default namespace is requested, the namespace query param is not present', function(assert) { + window.localStorage.nomadActiveNamespace = 'some-namespace'; + + const { pretender } = this.server; + const jobName = 'job-1'; + const jobNamespace = 'default'; + const jobId = JSON.stringify([jobName, jobNamespace]); + + this.system.get('namespaces'); + return wait().then(() => { + this.subject().findRecord(null, { modelName: 'job' }, jobId); + + assert.deepEqual( + pretender.handledRequests.mapBy('url'), + [`/v1/job/${jobName}`], + 'The one request made is /job/:id with no namespace query param' + ); + }); +}); + test('When a namespace is in localStorage and the requested job is in the default namespace, the namespace query param is left out', function(assert) { window.localStorage.nomadActiveNamespace = 'red-herring'; @@ -121,11 +159,6 @@ test('findAll can be watched', function(assert) { request(); assert.equal( pretender.handledRequests[0].url, - '/v1/namespaces', - 'First request is for namespaces' - ); - assert.equal( - pretender.handledRequests[1].url, '/v1/jobs?index=1', 'Second request is a blocking request for jobs' ); @@ -133,7 +166,7 @@ test('findAll can be watched', function(assert) { return wait().then(() => { request(); assert.equal( - pretender.handledRequests[2].url, + pretender.handledRequests[1].url, '/v1/jobs?index=2', 'Third request is a blocking request with an incremented index param' ); @@ -177,11 +210,13 @@ test('relationships can be reloaded', function(assert) { const mockModel = makeMockModel(plainId); this.subject().reloadRelationship(mockModel, 'summary'); - assert.equal( - pretender.handledRequests[0].url, - `/v1/job/${plainId}/summary`, - 'Relationship was reloaded' - ); + return wait().then(() => { + assert.equal( + pretender.handledRequests[0].url, + `/v1/job/${plainId}/summary`, + 'Relationship was reloaded' + ); + }); }); test('relationship reloads can be watched', function(assert) { From 313b5bc8a81b2d60f3cab33f1db0dffc5bc96a00 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Fri, 6 Jul 2018 10:50:22 -0700 Subject: [PATCH 6/6] Acceptance test for jobs from different namespaces on a single client --- ui/mirage/factories/allocation.js | 2 ++ ui/tests/acceptance/client-detail-test.js | 42 +++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/ui/mirage/factories/allocation.js b/ui/mirage/factories/allocation.js index 6c1c20393..3336a9c15 100644 --- a/ui/mirage/factories/allocation.js +++ b/ui/mirage/factories/allocation.js @@ -125,6 +125,7 @@ export default Factory.extend({ ); const job = allocation.jobId ? server.db.jobs.find(allocation.jobId) : pickOne(server.db.jobs); + const namespace = allocation.namespace || job.namespace; const node = allocation.nodeId ? server.db.nodes.find(allocation.nodeId) : pickOne(server.db.nodes); @@ -147,6 +148,7 @@ export default Factory.extend({ ); allocation.update({ + namespace, jobId: job.id, nodeId: node.id, taskStateIds: states.mapBy('id'), diff --git a/ui/tests/acceptance/client-detail-test.js b/ui/tests/acceptance/client-detail-test.js index f1860c610..72204b92b 100644 --- a/ui/tests/acceptance/client-detail-test.js +++ b/ui/tests/acceptance/client-detail-test.js @@ -654,3 +654,45 @@ test('when the node has a drain stategy with a negative deadline, the drain stra ); }); }); + +moduleForAcceptance('Acceptance | client detail (multi-namespace)', { + beforeEach() { + server.create('node', 'forceIPv4', { schedulingEligibility: 'eligible' }); + node = server.db.nodes[0]; + + // Related models + server.create('namespace'); + server.create('namespace', { id: 'other-namespace' }); + + server.create('agent'); + + // 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.create('job', { id: 'job-2', namespaceId: 'other-namespace', createAllocations: false }); + server.createList('allocation', 3, { nodeId: node.id, jobId: 'job-2' }); + }, +}); + +test('when the node has allocations on different namespaces, the associated jobs are fetched correctly', function(assert) { + window.localStorage.nomadActiveNamespace = 'other-namespace'; + + visit(`/clients/${node.id}`); + + andThen(() => { + assert.equal( + findAll('[data-test-allocation]').length, + server.db.allocations.length, + 'All allocations are scheduled on this node' + ); + assert.ok( + server.pretender.handledRequests.findBy('url', '/v1/job/job-1'), + 'Job One fetched correctly' + ); + assert.ok( + server.pretender.handledRequests.findBy('url', '/v1/job/job-2?namespace=other-namespace'), + 'Job Two fetched correctly' + ); + }); +});