From 8fa4232148ebc0fa89bbfe7689a0f41c52e32b50 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Fri, 10 Apr 2020 12:25:55 +0200 Subject: [PATCH 1/4] fix arguments order Co-Authored-By: Jakob Ackermann --- test/unit/js/DockerRunnerTests.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/js/DockerRunnerTests.js b/test/unit/js/DockerRunnerTests.js index 8399283..3daded8 100644 --- a/test/unit/js/DockerRunnerTests.js +++ b/test/unit/js/DockerRunnerTests.js @@ -357,8 +357,8 @@ describe('DockerRunner', function() { return this.DockerRunner.startContainer( this.options, this.volumes, - this.callback, - () => {} + () => {}, + this.callback ) }) From e3da458b376871c3ce72d6984d14bf1ee668b04b Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Fri, 10 Apr 2020 12:28:11 +0200 Subject: [PATCH 2/4] retry once on EPIPE errors Co-Authored-By: Jakob Ackermann --- app/js/DockerRunner.js | 34 +++++++++++------- test/unit/js/DockerRunnerTests.js | 58 +++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 12 deletions(-) diff --git a/app/js/DockerRunner.js b/app/js/DockerRunner.js index 3594b3a..32bcf70 100644 --- a/app/js/DockerRunner.js +++ b/app/js/DockerRunner.js @@ -407,19 +407,29 @@ module.exports = DockerRunner = { }) } ) - return container.inspect(function(error, stats) { - if ((error != null ? error.statusCode : undefined) === 404) { - return createAndStartContainer() - } else if (error != null) { - logger.err( - { container_name: name, error }, - 'unable to inspect container to start' - ) - return callback(error) - } else { - return startExistingContainer() + + const callbackWithRetry = error => { + if (error.message.match(/EPIPE/)) { + return inspectContainer(container, callback) } - }) + callback(error) + } + + var inspectContainer = (container, innerCallback) => + container.inspect(function(error, stats) { + if ((error != null ? error.statusCode : undefined) === 404) { + return createAndStartContainer() + } else if (error != null) { + logger.err( + { container_name: name, error }, + 'unable to inspect container to start' + ) + return innerCallback(error) + } else { + return startExistingContainer() + } + }) + inspectContainer(container, callbackWithRetry) }, attachToContainer(containerId, attachStreamHandler, attachStartCallback) { diff --git a/test/unit/js/DockerRunnerTests.js b/test/unit/js/DockerRunnerTests.js index 3daded8..7284c6e 100644 --- a/test/unit/js/DockerRunnerTests.js +++ b/test/unit/js/DockerRunnerTests.js @@ -36,6 +36,7 @@ describe('DockerRunner', function() { 'logger-sharelatex': (this.logger = { log: sinon.stub(), error: sinon.stub(), + err: sinon.stub(), info: sinon.stub(), warn: sinon.stub() }), @@ -387,6 +388,63 @@ describe('DockerRunner', function() { }) }) + describe('when inspect always fails with EPIPE error', function() { + beforeEach(function() { + this.error = new Error('write EPIPE') + this.container.inspect = sinon.stub().yields(this.error) + this.container.start = sinon.stub().yields() + + this.DockerRunner.startContainer( + this.options, + this.volumes, + () => {}, + this.callback + ) + }) + + it('should retry once', function() { + sinon.assert.callOrder( + this.container.inspect, + this.container.inspect, + this.callback + ) + }) + + it('should call back with error', function() { + sinon.assert.calledWith(this.callback, this.error) + }) + }) + + describe('when inspect fails once with EPIPE error', function() { + beforeEach(function() { + this.container.inspect = sinon.stub() + this.container.inspect.onFirstCall().yields(new Error('write EPIPE')) + this.container.inspect.onSecondCall().yields() + this.container.start = sinon.stub().yields() + + this.DockerRunner.startContainer( + this.options, + this.volumes, + () => {}, + this.callback + ) + }) + + it('should retry once and start container', function() { + sinon.assert.callOrder( + this.container.inspect, + this.container.inspect, + this.DockerRunner.attachToContainer, + this.container.start, + this.callback + ) + }) + + it('should call back without error', function() { + sinon.assert.calledWith(this.callback, null) + }) + }) + describe('when the container does not exist', function() { beforeEach(function() { const exists = false From 9e82ab0890c5cc8c7fb95362c3f7edbcaad0cf29 Mon Sep 17 00:00:00 2001 From: Tim Alby Date: Fri, 10 Apr 2020 12:28:48 +0200 Subject: [PATCH 3/4] add metrics for EPIPE errors Co-Authored-By: Jakob Ackermann --- app/js/DockerRunner.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/js/DockerRunner.js b/app/js/DockerRunner.js index 32bcf70..552f65e 100644 --- a/app/js/DockerRunner.js +++ b/app/js/DockerRunner.js @@ -26,6 +26,7 @@ const LockManager = require('./DockerLockManager') const fs = require('fs') const Path = require('path') const _ = require('underscore') +const metrics = require('metrics-sharelatex') logger.info('using docker runner') @@ -410,6 +411,7 @@ module.exports = DockerRunner = { const callbackWithRetry = error => { if (error.message.match(/EPIPE/)) { + metrics.inc('container-inspect-epipe-retry') return inspectContainer(container, callback) } callback(error) @@ -420,6 +422,7 @@ module.exports = DockerRunner = { if ((error != null ? error.statusCode : undefined) === 404) { return createAndStartContainer() } else if (error != null) { + metrics.inc('container-inspect-epipe-error') logger.err( { container_name: name, error }, 'unable to inspect container to start' From 2b2fcca39ce8dee0fdc0c342aa0d6c822592bcec Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 10 Apr 2020 13:25:40 +0200 Subject: [PATCH 4/4] [DockerRunner] fix metric incrementing and error logging - do not log on first EPIPE - inc 'container-inspect-epipe-error' on permanent error only Co-Authored-By: Tim Alby --- app/js/DockerRunner.js | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/app/js/DockerRunner.js b/app/js/DockerRunner.js index 552f65e..6a603d8 100644 --- a/app/js/DockerRunner.js +++ b/app/js/DockerRunner.js @@ -408,31 +408,28 @@ module.exports = DockerRunner = { }) } ) - - const callbackWithRetry = error => { - if (error.message.match(/EPIPE/)) { - metrics.inc('container-inspect-epipe-retry') - return inspectContainer(container, callback) - } - callback(error) - } - - var inspectContainer = (container, innerCallback) => + var inspectContainer = (isRetry) => container.inspect(function(error, stats) { if ((error != null ? error.statusCode : undefined) === 404) { return createAndStartContainer() } else if (error != null) { - metrics.inc('container-inspect-epipe-error') + if (error.message.match(/EPIPE/)) { + if (!isRetry) { + metrics.inc('container-inspect-epipe-retry') + return inspectContainer(true) + } + metrics.inc('container-inspect-epipe-error') + } logger.err( { container_name: name, error }, 'unable to inspect container to start' ) - return innerCallback(error) + return callback(error) } else { return startExistingContainer() } }) - inspectContainer(container, callbackWithRetry) + inspectContainer(false) }, attachToContainer(containerId, attachStreamHandler, attachStartCallback) {