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: Take advantage of client request tunneling #3908

Merged
merged 3 commits into from
Mar 2, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
39 changes: 34 additions & 5 deletions ui/app/components/task-log.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ import { inject as service } from '@ember/service';
import Component from '@ember/component';
import { computed } from '@ember/object';
import { run } from '@ember/runloop';
import RSVP from 'rsvp';
import { task } from 'ember-concurrency';
import { logger } from 'nomad-ui/utils/classes/log';
import WindowResizable from 'nomad-ui/mixins/window-resizable';
import timeout from 'nomad-ui/utils/timeout';

export default Component.extend(WindowResizable, {
token: service(),
Expand All @@ -14,6 +16,15 @@ export default Component.extend(WindowResizable, {
allocation: null,
task: null,

// When true, request logs from the server agent
useServer: false,

// When true, logs cannot be fetched from either the client or the server
noConnection: false,

clientTimeout: 1000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think 1s is ambitious (networking! ¯_(ツ)_/¯ ), but most likely not a huge deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Especially if the browser attempting the connection is on a slow network or is far away from the server, but it's a careful balance between timing out too early and wasting time before just using the server. It's easy to change for 0.8.1 if need be.

serverTimeout: 5000,

didReceiveAttrs() {
if (this.get('allocation') && this.get('task')) {
this.send('toggleStream');
Expand All @@ -37,11 +48,12 @@ export default Component.extend(WindowResizable, {

mode: 'stdout',

logUrl: computed('allocation.id', 'allocation.node.httpAddr', function() {
logUrl: computed('allocation.id', 'allocation.node.httpAddr', 'useServer', function() {
const address = this.get('allocation.node.httpAddr');
const allocation = this.get('allocation.id');

return `//${address}/v1/client/fs/logs/${allocation}`;
const url = `/v1/client/fs/logs/${allocation}`;
return this.get('useServer') ? url : `//${address}${url}`;
}),

logParams: computed('task', 'mode', function() {
Expand All @@ -51,9 +63,23 @@ export default Component.extend(WindowResizable, {
};
}),

logger: logger('logUrl', 'logParams', function() {
const token = this.get('token');
return token.authorizedRequest.bind(token);
logger: logger('logUrl', 'logParams', function logFetch() {
// If the log request can't settle in one second, the client
// must be unavailable and the server should be used instead
const timing = this.get('useServer') ? this.get('serverTimeout') : this.get('clientTimeout');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the added flip flopping is nice 🙆‍♂️

return url =>
RSVP.race([this.get('token').authorizedRequest(url), timeout(timing)]).then(
response => response,
error => {
if (this.get('useServer')) {
this.set('noConnection', true);
} else {
this.send('failoverToServer');
this.get('stream').perform();
}
throw error;
}
);
}),

head: task(function*() {
Expand Down Expand Up @@ -100,5 +126,8 @@ export default Component.extend(WindowResizable, {
this.get('stream').perform();
}
},
failoverToServer() {
this.set('useServer', true);
},
},
});
12 changes: 4 additions & 8 deletions ui/app/models/allocation.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import attr from 'ember-data/attr';
import { belongsTo } from 'ember-data/relationships';
import { fragment, fragmentArray } from 'ember-data-model-fragments/attributes';
import PromiseObject from '../utils/classes/promise-object';
import timeout from '../utils/timeout';
import shortUUIDProperty from '../utils/properties/short-uuid';

const STATUS_ORDER = {
Expand Down Expand Up @@ -92,14 +91,11 @@ export default Model.extend({
});
}

const url = `//${this.get('node.httpAddr')}/v1/client/allocation/${this.get('id')}/stats`;
const url = `/v1/client/allocation/${this.get('id')}/stats`;
return PromiseObject.create({
promise: RSVP.Promise.race([
this.get('token')
.authorizedRequest(url)
.then(res => res.json()),
timeout(2000),
]),
promise: this.get('token')
.authorizedRequest(url)
.then(res => res.json()),
});
}),

Expand Down
6 changes: 6 additions & 0 deletions ui/app/templates/components/task-log.hbs
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
{{#if noConnection}}
<div data-test-connection-error class="notification is-error">
<h3 class="title is-4">Cannot fetch logs</h3>
<p>The logs for this task are inaccessible. Check the condition of the node the allocation is on.</p>
</div>
{{/if}}
<div class="boxed-section-head">
<span>
<button data-test-log-action="stdout" class="button {{if (eq mode "stdout") "is-info"}}" {{action "setMode" "stdout"}}>stdout</button>
Expand Down
9 changes: 6 additions & 3 deletions ui/app/utils/classes/log.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import Ember from 'ember';
import { alias } from '@ember/object/computed';
import { assert } from '@ember/debug';
import Evented from '@ember/object/evented';
Expand All @@ -10,6 +11,8 @@ import PollLogger from 'nomad-ui/utils/classes/poll-logger';

const MAX_OUTPUT_LENGTH = 50000;

export const fetchFailure = url => () => Ember.Logger.warn(`LOG FETCH: Couldn't connect to ${url}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ember.Logger's is being deprecated and removed (not that it matters here or will happen soon), but console.warn is likely fine here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah... Not gonna lie, I just wanted to avoid adding an eslint-ignore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, fair


const Log = EmberObject.extend(Evented, {
// Parameters

Expand Down Expand Up @@ -74,9 +77,9 @@ const Log = EmberObject.extend(Evented, {
const url = `${this.get('url')}?${queryParams}`;

this.stop();
let text = yield logFetch(url).then(res => res.text());
let text = yield logFetch(url).then(res => res.text(), fetchFailure(url));

if (text.length > MAX_OUTPUT_LENGTH) {
if (text && text.length > MAX_OUTPUT_LENGTH) {
text = text.substr(0, MAX_OUTPUT_LENGTH);
text += '\n\n---------- TRUNCATED: Click "tail" to view the bottom of the log ----------';
}
Expand All @@ -96,7 +99,7 @@ const Log = EmberObject.extend(Evented, {
const url = `${this.get('url')}?${queryParams}`;

this.stop();
let text = yield logFetch(url).then(res => res.text());
let text = yield logFetch(url).then(res => res.text(), fetchFailure(url));

this.set('tail', text);
this.set('logPointer', 'tail');
Expand Down
10 changes: 9 additions & 1 deletion ui/app/utils/classes/poll-logger.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import EmberObject from '@ember/object';
import { task, timeout } from 'ember-concurrency';
import AbstractLogger from './abstract-logger';
import { fetchFailure } from './log';

export default EmberObject.extend(AbstractLogger, {
interval: 1000,
Expand All @@ -18,7 +19,14 @@ export default EmberObject.extend(AbstractLogger, {
poll: task(function*() {
const { interval, logFetch } = this.getProperties('interval', 'logFetch');
while (true) {
let text = yield logFetch(this.get('fullUrl')).then(res => res.text());
const url = this.get('fullUrl');
let response = yield logFetch(url).then(res => res, fetchFailure(url));

if (!response) {
return;
}

let text = yield response.text();

if (text) {
const lines = text.replace(/\}\{/g, '}\n{').split('\n');
Expand Down
7 changes: 6 additions & 1 deletion ui/app/utils/classes/stream-logger.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import EmberObject, { computed } from '@ember/object';
import { task } from 'ember-concurrency';
import TextDecoder from 'nomad-ui/utils/classes/text-decoder';
import AbstractLogger from './abstract-logger';
import { fetchFailure } from './log';

export default EmberObject.extend(AbstractLogger, {
reader: null,
Expand Down Expand Up @@ -30,7 +31,11 @@ export default EmberObject.extend(AbstractLogger, {
let buffer = '';

const decoder = new TextDecoder();
const reader = yield logFetch(url).then(res => res.body.getReader());
const reader = yield logFetch(url).then(res => res.body.getReader(), fetchFailure(url));

if (!reader) {
return;
}

this.set('reader', reader);

Expand Down
53 changes: 29 additions & 24 deletions ui/mirage/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,37 +178,42 @@ export default function() {
return new Response(403, {}, null);
});

const clientAllocationStatsHandler = function({ clientAllocationStats }, { params }) {
return this.serialize(clientAllocationStats.find(params.id));
};

const clientAllocationLog = function(server, { params, queryParams }) {
const allocation = server.allocations.find(params.allocation_id);
const tasks = allocation.taskStateIds.map(id => server.taskStates.find(id));

if (!tasks.mapBy('name').includes(queryParams.task)) {
return new Response(400, {}, 'must include task name');
}

if (queryParams.plain) {
return logFrames.join('');
}

return logEncode(logFrames, logFrames.length - 1);
};

// Client requests are available on the server and the client
this.get('/client/allocation/:id/stats', clientAllocationStatsHandler);
this.get('/client/fs/logs/:allocation_id', clientAllocationLog);

this.get('/client/v1/client/stats', function({ clientStats }, { queryParams }) {
return this.serialize(clientStats.find(queryParams.node_id));
});

// TODO: in the future, this hack may be replaceable with dynamic host name
// support in pretender: https://github.com/pretenderjs/pretender/issues/210
HOSTS.forEach(host => {
this.get(`http://${host}/v1/client/allocation/:id/stats`, function(
{ clientAllocationStats },
{ params }
) {
return this.serialize(clientAllocationStats.find(params.id));
});
this.get(`http://${host}/v1/client/allocation/:id/stats`, clientAllocationStatsHandler);
this.get(`http://${host}/v1/client/fs/logs/:allocation_id`, clientAllocationLog);

this.get(`http://${host}/v1/client/stats`, function({ clientStats }) {
return this.serialize(clientStats.find(host));
});

this.get(`http://${host}/v1/client/fs/logs/:allocation_id`, function(
server,
{ params, queryParams }
) {
const allocation = server.allocations.find(params.allocation_id);
const tasks = allocation.taskStateIds.map(id => server.taskStates.find(id));

if (!tasks.mapBy('name').includes(queryParams.task)) {
return new Response(400, {}, 'must include task name');
}

if (queryParams.plain) {
return logFrames.join('');
}

return logEncode(logFrames, logFrames.length - 1);
});
});
}

Expand Down
20 changes: 3 additions & 17 deletions ui/tests/acceptance/task-group-detail-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,7 @@ test('/jobs/:id/:task-group first breadcrumb should link to jobs', function(asse
});
});

test('/jobs/:id/:task-group second breadcrumb should link to the job for the task group', function(
assert
) {
test('/jobs/:id/:task-group second breadcrumb should link to the job for the task group', function(assert) {
click(`[data-test-breadcrumb="${job.name}"]`);
andThen(() => {
assert.equal(
Expand All @@ -114,9 +112,7 @@ test('/jobs/:id/:task-group second breadcrumb should link to the job for the tas
});
});

test('/jobs/:id/:task-group should list one page of allocations for the task group', function(
assert
) {
test('/jobs/:id/:task-group should list one page of allocations for the task group', function(assert) {
const pageSize = 10;

server.createList('allocation', 10, {
Expand Down Expand Up @@ -185,9 +181,7 @@ test('each allocation should show basic information about the allocation', funct
});
});

test('each allocation should show stats about the allocation, retrieved directly from the node', function(
assert
) {
test('each allocation should show stats about the allocation', function(assert) {
const allocation = allocations.sortBy('name')[0];
const allocationRow = find('[data-test-allocation]');
const allocStats = server.db.clientAllocationStats.find(allocation.id);
Expand Down Expand Up @@ -219,14 +213,6 @@ test('each allocation should show stats about the allocation, retrieved directly
`${formatBytes([allocStats.resourceUsage.MemoryStats.RSS])} / ${memoryUsed} MiB`,
'Detailed memory information is in a tooltip'
);

const node = server.db.nodes.find(allocation.nodeId);
const nodeStatsUrl = `//${node.httpAddr}/v1/client/allocation/${allocation.id}/stats`;

assert.ok(
server.pretender.handledRequests.some(req => req.url === nodeStatsUrl),
`Requests ${nodeStatsUrl}`
);
});

test('when the allocation search has no matches, there is an empty message', function(assert) {
Expand Down
Loading