From 6af31fed7ab8ccf7686b1726d8ba2f0f977d2525 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 27 Apr 2020 21:00:22 -0700 Subject: [PATCH 1/5] Always pass credential in fetch requests, but also treat options reasonably Now options can be provided without also having to remember to pass credentials. This is convenient for abort controller signals. --- ui/app/services/token.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ui/app/services/token.js b/ui/app/services/token.js index 5c251bd68..256c2646f 100644 --- a/ui/app/services/token.js +++ b/ui/app/services/token.js @@ -72,7 +72,8 @@ export default Service.extend({ // This authorizedRawRequest is necessary in order to fetch data // with the guarantee of a token but without the automatic region // param since the region cannot be known at this point. - authorizedRawRequest(url, options = { credentials: 'include' }) { + authorizedRawRequest(url, options = {}) { + const credentials = 'include'; const headers = {}; const token = this.secret; @@ -80,7 +81,7 @@ export default Service.extend({ headers['X-Nomad-Token'] = token; } - return fetch(url, assign(options, { headers })); + return fetch(url, assign(options, { headers, credentials })); }, authorizedRequest(url, options) { From fab6fcbd88f8682e94e674a0c33823e7cd8c0b5e Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 27 Apr 2020 21:03:33 -0700 Subject: [PATCH 2/5] Abort log fetch request when failing over from client to server Typically a failover means that the client can't be reached. However, if the client does eventually return after the timeout period, the log will stream indefinitely. This fixes that using an API that wasn't broadly available at the time this was first written. --- ui/app/components/task-log.js | 19 +++++++++++++++++-- ui/tests/integration/task-log-test.js | 5 +++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/ui/app/components/task-log.js b/ui/app/components/task-log.js index b5cda3eed..43a5df328 100644 --- a/ui/app/components/task-log.js +++ b/ui/app/components/task-log.js @@ -5,6 +5,12 @@ import RSVP from 'rsvp'; import { logger } from 'nomad-ui/utils/classes/log'; import timeout from 'nomad-ui/utils/timeout'; +class MockAbortController { + abort() { + /* noop */ + } +} + export default Component.extend({ token: service(), @@ -45,11 +51,20 @@ export default Component.extend({ 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 + + // AbortControllers don't exist in IE11, so provide a mock if it doesn't exist + const aborter = window.AbortController ? new AbortController() : new MockAbortController(); const timing = this.useServer ? this.serverTimeout : this.clientTimeout; return url => - RSVP.race([this.token.authorizedRequest(url), timeout(timing)]).then( - response => response, + RSVP.race([ + this.token.authorizedRequest(url, { signal: aborter.signal }), + timeout(timing), + ]).then( + response => { + return response; + }, error => { + aborter.abort(); if (this.useServer) { this.set('noConnection', true); } else { diff --git a/ui/tests/integration/task-log-test.js b/ui/tests/integration/task-log-test.js index 67f66786e..e84855e3f 100644 --- a/ui/tests/integration/task-log-test.js +++ b/ui/tests/integration/task-log-test.js @@ -219,6 +219,11 @@ module('Integration | Component | task log', function(hooks) { this.server.handledRequests.filter(req => req.url.startsWith(serverUrl)).length, 'Log request was later made to the server' ); + + assert.ok( + this.server.handledRequests.filter(req => clientUrlRegex.test(req.url))[0].aborted, + 'Client log request was aborted' + ); }); test('When both the client and the server are inaccessible, an error message is shown', async function(assert) { From e18655465184bc464861e5935f925b2ba534c4c1 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 27 Apr 2020 22:18:49 -0700 Subject: [PATCH 3/5] Clicking stdout/stderr when already on that tab is now a noop --- ui/app/components/task-log.js | 1 + ui/tests/integration/task-log-test.js | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/ui/app/components/task-log.js b/ui/app/components/task-log.js index 43a5df328..3bd50605c 100644 --- a/ui/app/components/task-log.js +++ b/ui/app/components/task-log.js @@ -77,6 +77,7 @@ export default Component.extend({ actions: { setMode(mode) { + if (this.mode === mode) return; this.logger.stop(); this.set('mode', mode); }, diff --git a/ui/tests/integration/task-log-test.js b/ui/tests/integration/task-log-test.js index e84855e3f..7e85931ff 100644 --- a/ui/tests/integration/task-log-test.js +++ b/ui/tests/integration/task-log-test.js @@ -190,6 +190,25 @@ module('Integration | Component | task log', function(hooks) { ); }); + test('Clicking stderr/stdout mode buttons does nothing when the mode remains the same', async function(assert) { + const { interval } = commonProps; + + run.later(() => { + click('[data-test-log-action="stdout"]'); + run.later(run, run.cancelTimers, interval * 6); + }, interval * 2); + + this.setProperties(commonProps); + await render(hbs`{{task-log allocation=allocation task=task}}`); + + await settled(); + assert.equal( + find('[data-test-log-cli]').textContent, + streamFrames[0] + streamFrames[0] + streamFrames[1], + 'Now includes second frame' + ); + }); + test('When the client is inaccessible, task-log falls back to requesting logs through the server', async function(assert) { run.later(run, run.cancelTimers, allowedConnectionTime * 2); From e5a5fc77442c913b7a576332808d62249b63d486 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 27 Apr 2020 23:34:01 -0700 Subject: [PATCH 4/5] Fix race condition where stdout and stderr requests can cause a no connection error This would happen because a no connection error happens after the second request fails, but that's because it's assumed the second request is to a server node. However, if a user clicks stderr fast enough, the first and second requests are both to the client node. This changes the logic to check if the request is to the server before deeming log streaming a total failure. --- ui/app/components/task-log.js | 6 +++- ui/tests/integration/task-log-test.js | 45 +++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/ui/app/components/task-log.js b/ui/app/components/task-log.js index 3bd50605c..773ef1072 100644 --- a/ui/app/components/task-log.js +++ b/ui/app/components/task-log.js @@ -55,6 +55,10 @@ export default Component.extend({ // AbortControllers don't exist in IE11, so provide a mock if it doesn't exist const aborter = window.AbortController ? new AbortController() : new MockAbortController(); const timing = this.useServer ? this.serverTimeout : this.clientTimeout; + + // Capture the state of useServer at logger create time to avoid a race + // between the stdout logger and stderr logger running at once. + const useServer = this.useServer; return url => RSVP.race([ this.token.authorizedRequest(url, { signal: aborter.signal }), @@ -65,7 +69,7 @@ export default Component.extend({ }, error => { aborter.abort(); - if (this.useServer) { + if (useServer) { this.set('noConnection', true); } else { this.send('failoverToServer'); diff --git a/ui/tests/integration/task-log-test.js b/ui/tests/integration/task-log-test.js index 7e85931ff..de17aab35 100644 --- a/ui/tests/integration/task-log-test.js +++ b/ui/tests/integration/task-log-test.js @@ -280,4 +280,49 @@ module('Integration | Component | task log', function(hooks) { ); assert.ok(find('[data-test-connection-error]'), 'An error message is shown'); }); + + test('When the client is inaccessible, the server is accessible, and stderr is pressed before the client timeout occurs, the no connection error is not shown', async function(assert) { + // override client response to timeout + this.server.get( + `http://${HOST}/v1/client/fs/logs/:allocation_id`, + () => [400, {}, ''], + allowedConnectionTime * 2 + ); + + // Click stderr before the client request responds + run.later(() => { + click('[data-test-log-action="stderr"]'); + run.later(run, run.cancelTimers, commonProps.interval * 5); + }, allowedConnectionTime / 2); + + this.setProperties(commonProps); + await render(hbs`{{task-log + allocation=allocation + task=task + clientTimeout=clientTimeout + serverTimeout=serverTimeout}}`); + + await settled(); + + const clientUrlRegex = new RegExp(`${HOST}/v1/client/fs/logs/${commonProps.allocation.id}`); + const clientRequests = this.server.handledRequests.filter(req => clientUrlRegex.test(req.url)); + assert.ok( + clientRequests.find(req => req.queryParams.type === 'stdout'), + 'Client request for stdout' + ); + assert.ok( + clientRequests.find(req => req.queryParams.type === 'stderr'), + 'Client request for stderr' + ); + + const serverUrl = `/v1/client/fs/logs/${commonProps.allocation.id}`; + assert.ok( + this.server.handledRequests + .filter(req => req.url.startsWith(serverUrl)) + .find(req => req.queryParams.type === 'stderr'), + 'Server request for stderr' + ); + + assert.notOk(find('[data-test-connection-error]'), 'An error message is not shown'); + }); }); From 9557475233f5897d904fb8b8fc1aa9882138596f Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Mon, 27 Apr 2020 23:47:55 -0700 Subject: [PATCH 5/5] Make the no connection error on the logs page dismissable --- ui/app/templates/components/task-log.hbs | 11 +++++++++-- ui/tests/integration/task-log-test.js | 3 +++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/ui/app/templates/components/task-log.hbs b/ui/app/templates/components/task-log.hbs index 89a363747..1dc5caa79 100644 --- a/ui/app/templates/components/task-log.hbs +++ b/ui/app/templates/components/task-log.hbs @@ -1,7 +1,14 @@ {{#if noConnection}}
-

Cannot fetch logs

-

The logs for this task are inaccessible. Check the condition of the node the allocation is on.

+
+
+

Cannot fetch logs

+

The logs for this task are inaccessible. Check the condition of the node the allocation is on.

+
+
+ +
+
{{/if}}
diff --git a/ui/tests/integration/task-log-test.js b/ui/tests/integration/task-log-test.js index de17aab35..bea9b342b 100644 --- a/ui/tests/integration/task-log-test.js +++ b/ui/tests/integration/task-log-test.js @@ -279,6 +279,9 @@ module('Integration | Component | task log', function(hooks) { 'Log request was later made to the server' ); assert.ok(find('[data-test-connection-error]'), 'An error message is shown'); + + await click('[data-test-connection-error-dismiss]'); + assert.notOk(find('[data-test-connection-error]'), 'The error message is dismissable'); }); test('When the client is inaccessible, the server is accessible, and stderr is pressed before the client timeout occurs, the no connection error is not shown', async function(assert) {