Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jul 6, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 17 additions & 35 deletions ui/app/adapters/job.js
Original file line number Diff line number Diff line change
@@ -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 => {
Expand All @@ -24,53 +14,38 @@ 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 } : {};

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: {
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() });
const url = this.urlForFindRecord(job.get('id'), 'job');
return this.ajax(url, 'GET');
},

forcePeriodic(job) {
Expand All @@ -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}`;
Expand Down
11 changes: 1 addition & 10 deletions ui/app/adapters/node.js
Original file line number Diff line number Diff line change
@@ -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();
9 changes: 6 additions & 3 deletions ui/app/utils/properties/watch.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand All @@ -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')
Expand All @@ -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 } }),
Expand Down
3 changes: 1 addition & 2 deletions ui/config/environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
};

Expand Down
100 changes: 74 additions & 26 deletions ui/tests/unit/adapters/job-test.js
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -8,32 +9,87 @@ 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();
},
});

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';
const jobId = JSON.stringify([jobName, jobNamespace]);

this.subject().findRecord(null, { modelName: 'job' }, jobId);

assert.deepEqual(
pretender.handledRequests.mapBy('url'),
[`/v1/job/${jobName}`],
'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';
Expand All @@ -43,8 +99,8 @@ 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 request made is /job/:id with no namespace query param'
);
});

Expand All @@ -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}`],
'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'
);
});

Expand Down Expand Up @@ -103,19 +159,14 @@ 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'
);

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'
);
Expand All @@ -137,19 +188,14 @@ 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'
);

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'
);
Expand All @@ -164,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) {
Expand Down