Skip to content

SAN-5817 - Store container logs in s3 - #1903

Merged
Myztiq merged 10 commits into
masterfrom
SAN-5817-s3-logs
Mar 8, 2017
Merged

SAN-5817 - Store container logs in s3#1903
Myztiq merged 10 commits into
masterfrom
SAN-5817-s3-logs

Conversation

@Myztiq

@MyztiqMyztiq commented Feb 28, 2017

Copy link
Copy Markdown
Contributor

We need to store logs of containers when they die so we can show them for testing environments. We're going to need to add more logic and store container histories so we can fetch old versions etc but this is the first step.

Store them in s3 by container ID, then eventually we'll be able to stream directly to the client from s3 bypassing API sockets.

  • Configure production bucket and permissions

Myztiqand others added 2 commits March 1, 2017 11:55
Updated binding
Updated binding
Added task
Added tail limit
Updated to use aws.s3
s -> S
Updated to use .then instead of .tap
Create instance of S3
Updated to only tail if a tail limit is set
Re-working container log storing
Updated logging
Updated to clean stream
Pass end event through
Updated trace
Updated data log
Updated data length
Re-ordered code
Updated order
WIP
Being silly
Being silly
Being silly
Meh
Push raw data through
Go back in time
fix all the stream stuff so we can start saving logs to s3
@MyztiqMyztiq changed the title WIP - SAN-5817 - Store container logs in s3SAN-5817 - Store container logs in s3Mar 1, 2017
@Myztiq

Copy link
Copy Markdown
ContributorAuthor

return Promise.try(this._clearBuildResources.bind(this))
.bind(this)
.then(this._updateModelsAndGetUpdatedContextVersions)
.tap(rabbitMQ.publishContainerLogsStore.bind(rabbitMQ, {containerId: this.id}))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

() => rabbitMQ.publishContainerLogsStore({containerId: this.id})

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Why?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cuz it looks better

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I disagree. Not going to change it unless we've a style guide thing that prevents it.

Comment threadlib/workers/container.logs.store.js Outdated
.promise()
.catch((err) => {
log.error({err}, 'Error uploading logs to s3')
throw new WorkerStopError('Error uploading logs to s3', {originalError: err})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a bit aggressive, might just be network errors. Just add max retry count unless you have specific errors to workerStop

Comment threadlib/workers/container.logs.store.js Outdated
throw new WorkerStopError('Error uploading logs to s3', {originalError: err})
})
})
.then((uploadData) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: ponos should print out end of worker.

also what is uploadData ? is that the entire log string?!!?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

uploadData is the promise containing the upload results.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

just making sure, is upload results the entire log in memory?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

No, it's the response that we get from s3

@runnabot

runnabot commented Mar 7, 2017

Copy link
Copy Markdown

Deployed api/SAN-5817-s3-logs. View on Runnable.
From Runnable

@Myztiq

Copy link
Copy Markdown
ContributorAuthor

tests passed when I run them in small chunks of under 100 locally on bdd.

@Myztiq
Myztiq merged commit 88960f1 into masterMar 8, 2017
@Myztiq
Myztiq deleted the SAN-5817-s3-logs branch March 8, 2017 23:08
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Myztiq@runnabot@podviaznikov@anandkumarpatel@Nathan219