Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1
SAN-5848 - Stream logs from s3 to the frontend#1906
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
428c10cb27ec3c186df4784eef6df53e98de079560df62edf5a44a1e9c543ca1773539805f6f9e596eeef323e75dba65c8d273b0a3108b081810920fdca6aad39db46d81fd09428a90e3a0e81bFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| const aws = require('aws-sdk') | ||
| const logger = require('logger') | ||
| const s3 = new aws.S3() | ||
| const Promise = require('bluebird') | ||
| const pipeLogsToClient = (targetStream, containerId) => { | ||
| const log = logger.child({ | ||
| containerId, | ||
| method: 'pipeLogsToClient' | ||
| }) | ||
| log.trace('called') | ||
| const s3Object = s3.getObject({ | ||
| Bucket: process.env.S3_LOG_BUCKET, | ||
| Key: containerId | ||
| }) | ||
| return Promise.fromCallback((cb) => { | ||
| s3Object | ||
| .createReadStream() | ||
| .on('data', (data) => { | ||
| targetStream.write(data.toString()) | ||
| }) | ||
| .on('error', (error) => { | ||
| log.trace({error}, 'Error while fetching logs from s3') | ||
| cb(error) | ||
| }) | ||
| .on('end', () => { | ||
| log.trace('Finished serving logs from s3') | ||
| targetStream.end() | ||
| cb(null) | ||
| }) | ||
| }) | ||
| } | ||
| module.exports = { | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: we have ES6 now, you can make this a class if you want 😉 ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ehh, a little late to the party for streaming. | ||
| pipeLogsToClient | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -8,6 +8,7 @@ const keypather = require('keypather')() | ||
| const monitorDog = require('monitor-dog') | ||
| const commonStream = require('./common-stream') | ||
| const commonS3 = require('./common-s3') | ||
| const Instance = require('models/mongo/instance') | ||
| const logger = require('logger') | ||
| const PermissionService = require('models/services/permission-service') | ||
| @@ -91,7 +92,22 @@ function setupLogs (socket, id, data, instance, tags) { | ||
| } | ||
| } | ||
| rabbitMQ.publishLogStreamConnected(eventData) | ||
| return commonStream.pipeLogsToClient(destLogStream, baseDataName, tags, data.containerId, { tailLimit }) | ||
| let streamPromise | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NIT: seems like isolated logic, mind moving it to helper function? ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Specifically the check if the container is running? Or the method to pipe logs to a client? I'm unsure if its needed to be separated. | ||
| if (keypather.get(instance, 'container.inspect.State.Running')) { | ||
| streamPromise = commonStream.pipeLogsToClient(destLogStream, baseDataName, tags, data.containerId, { tailLimit }) | ||
| } else { | ||
| streamPromise = commonS3.pipeLogsToClient(destLogStream, data.containerId) | ||
| .catch((err) => { | ||
| log.error({error: err}, 'Error piping logs from s3 to client') | ||
| if (err.code === 'NoSuchKey') { | ||
| // fallback on no file exists to go against docker directly | ||
| return commonStream.pipeLogsToClient(destLogStream, baseDataName, tags, data.containerId, { tailLimit }) | ||
| } | ||
| throw err | ||
| }) | ||
| } | ||
| return streamPromise | ||
| .catch(function (err) { | ||
| tags.result = 'failure' | ||
| monitorDog.increment(baseDataName + '.err.getting_logs', tags) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,26 +1,27 @@ | ||
| 'use strict' | ||
| var Lab = require('lab') | ||
| var lab = exports.lab = Lab.script() | ||
| var describe = lab.describe | ||
| var it = lab.it | ||
| var beforeEach = lab.beforeEach | ||
| // var after = lab.after | ||
| var afterEach = lab.afterEach | ||
| var Code = require('code') | ||
| var expect = Code.expect | ||
| const Lab = require('lab') | ||
| const lab = exports.lab = Lab.script() | ||
| const describe = lab.describe | ||
| const it = lab.it | ||
| const beforeEach = lab.beforeEach | ||
| // const after = lab.after | ||
| const afterEach = lab.afterEach | ||
| const Code = require('code') | ||
| const expect = Code.expect | ||
| var sinon = require('sinon') | ||
| var EventEmitter = require('events').EventEmitter | ||
| var util = require('util') | ||
| const sinon = require('sinon') | ||
| const EventEmitter = require('events').EventEmitter | ||
| const util = require('util') | ||
| var logStream = require('socket/log-stream') | ||
| var Instance = require('models/mongo/instance') | ||
| const logStream = require('socket/log-stream') | ||
| const Instance = require('models/mongo/instance') | ||
| var Promise = require('bluebird') | ||
| const Promise = require('bluebird') | ||
| require('sinon-as-promised')(Promise) | ||
| var commonStream = require('socket/common-stream') | ||
| var PermissionService = require('models/services/permission-service') | ||
| const commonStream = require('socket/common-stream') | ||
| const commonS3 = require('socket/common-s3') | ||
| const PermissionService = require('models/services/permission-service') | ||
| function ClientStream () { | ||
| EventEmitter.call(this) | ||
| @@ -80,7 +81,12 @@ describe('log stream: ' + moduleName, function () { | ||
| github: 123 | ||
| }, | ||
| container: { | ||
| dockerContainer: ctx.data.containerId | ||
| dockerContainer: ctx.data.containerId, | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does unit test cover both cases? where Running is true and false? ContributorAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope, just when running is true Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you make test for both cases. and the fallback case? we dont know it works unless its tested :) | ||
| inspect: { | ||
| State: { | ||
| Running: true | ||
| } | ||
| } | ||
| } | ||
| } | ||
| writeStream = new ClientStream() | ||
| @@ -220,6 +226,62 @@ describe('log stream: ' + moduleName, function () { | ||
| }) | ||
| .catch(done) | ||
| }) | ||
| describe('When container is stopped', () => { | ||
| beforeEach((done) => { | ||
| ctx.instance.container.inspect.State.Running = false | ||
| sinon.stub(commonS3, 'pipeLogsToClient').resolves({}) | ||
| done() | ||
| }) | ||
| afterEach((done) => { | ||
| commonS3.pipeLogsToClient.restore() | ||
| done() | ||
| }) | ||
| describe('file exists in s3', () => { | ||
| it('should stream logs from s3 file', (done) => { | ||
| logStream.logStreamHandler(ctx.socket, ctx.id, ctx.data) | ||
| .then(() => { | ||
| sinon.assert.notCalled(commonStream.pipeLogsToClient) | ||
| sinon.assert.calledOnce(commonS3.pipeLogsToClient) | ||
| sinon.assert.calledWith( | ||
| commonS3.pipeLogsToClient, | ||
| substream, | ||
| ctx.data.containerId | ||
| ) | ||
| }) | ||
| .asCallback(done) | ||
| }) | ||
| }) | ||
| describe('and file does not exist in s3', () => { | ||
| beforeEach((done) => { | ||
| commonS3.pipeLogsToClient.rejects({code: 'NoSuchKey'}) | ||
| done() | ||
| }) | ||
| it('should stream logs from docker file', (done) => { | ||
| logStream.logStreamHandler(ctx.socket, ctx.id, ctx.data) | ||
| .then(() => { | ||
| sinon.assert.calledOnce(commonS3.pipeLogsToClient) | ||
| sinon.assert.calledWith( | ||
| commonS3.pipeLogsToClient, | ||
| substream, | ||
| ctx.data.containerId | ||
| ) | ||
| sinon.assert.calledOnce(commonStream.pipeLogsToClient) | ||
| sinon.assert.calledWith( | ||
| commonStream.pipeLogsToClient, | ||
| substream, | ||
| 'api.socket.log', | ||
| sinon.match.object, | ||
| ctx.data.containerId, | ||
| { | ||
| tailLimit: process.env.DOCKER_LOG_TAIL_LIMIT | ||
| } | ||
| ) | ||
| }) | ||
| .asCallback(done) | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to close
createReadStreamhere? it might be automatic but just making sure no call is neededThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, the stream is closed when end is called on it already.