Merge pull request #4475 from hashicorp/b-ui-wrong-ns

UI: Jobs requested with the wrong namespace under the right conditions

ui/app/adapters/job.js

@@ -1,19 +1,9 @@
 import { inject as service } from '@ember/service';
-import { assign } from '@ember/polyfills';
 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 => {
@@ -24,12 +14,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 } : {};
@@ -37,40 +21,31 @@ 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') {
+    return associateNamespace(url, namespace);
-      url += `?namespace=${namespace}`;
-    }
-    return url;
   },
 
   xhrKey(url, method, options = {}) {
     const plainKey = this._super(...arguments);
     const namespace = options.data && options.data.namespace;
-    if (namespace) {
+    return associateNamespace(plainKey, namespace);
-      return `${plainKey}?namespace=${namespace}`;
-    }
-    return plainKey;
   },
 
   relationshipFallbackLinks: {
     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');
+    const url = this.urlForFindRecord(job.get('id'), 'job');
-    return this.ajax(url, 'GET', { data: this.buildQuery() });
+    return this.ajax(url, 'GET');
   },
 
   forcePeriodic(job) {
@@ -86,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}`;

ui/app/adapters/node.js

@@ -1,12 +1,3 @@
 import Watchable from './watchable';
 
-export default Watchable.extend({
+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,
-      });
-    });
-  },
-});

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 } }),

ui/config/environment.js

@@ -19,8 +19,7 @@ module.exports = function(environment) {
     },
 
     APP: {
-      // Here you can pass flags/options to your application instance
+      blockingQueries: true,
-      // when it is created
     },
   };
 

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'),

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'
+    );
+  });
+});

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,32 +9,50 @@ import moduleForAdapter from '../../helpers/module-for-adapter';
 
 moduleForAdapter('job', 'Unit | Adapter | Job', {
   needs: [
+    'adapter:application',
     'adapter:job',
-    'service:token',
+    'adapter:namespace',
-    'service:system',
+    '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();
   },
 });
 
-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 +62,45 @@ test('The job summary is stitched into the job request', function(assert) {
 
   assert.deepEqual(
     pretender.handledRequests.mapBy('url'),
-    ['/v1/namespaces', `/v1/job/${jobName}`],
+    [`/v1/job/${jobName}`],
-    'The two requests made are /namespaces and /job/:id'
+    'The only request made is /job/:id'
+  );
+});
+
+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';
+
+  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 +114,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}`],
+    [`/v1/job/${jobName}?namespace=${jobNamespace}`],
-    'The two requests made are /namespaces and /job/:id?namespace=:namespace'
+    'The only request made is /job/:id?namespace=:namespace'
   );
 });
 
@@ -103,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'
   );
@@ -115,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'
     );
@@ -137,11 +188,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 +195,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'
     );
@@ -164,12 +210,14 @@ test('relationships can be reloaded', function(assert) {
   const mockModel = makeMockModel(plainId);
 
   this.subject().reloadRelationship(mockModel, 'summary');
+  return wait().then(() => {
     assert.equal(
       pretender.handledRequests[0].url,
       `/v1/job/${plainId}/summary`,
       'Relationship was reloaded'
     );
   });
+});
 
 test('relationship reloads can be watched', function(assert) {
   const { pretender } = this.server;