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

Create an artifact plugin #55

Merged
merged 13 commits into from Mar 31, 2016

Conversation

Projects
None yet
5 participants
@imbstack
Member

imbstack commented Mar 11, 2016

Does what it says on the tin.

@imbstack

This comment has been minimized.

Show comment
Hide comment
@imbstack

imbstack Mar 11, 2016

Member

@jonasfj, @gregarndt: How does this payload schema look to you? Let's change it to be what we want.

Member

imbstack commented Mar 11, 2016

@jonasfj, @gregarndt: How does this payload schema look to you? Let's change it to be what we want.

Show outdated Hide outdated plugins/artifacts/config-schema.yml
title: Local artifact location
description: Filesystem path of artifact
type: string
remotePath:

This comment has been minimized.

@jonasfj

jonasfj Mar 11, 2016

Contributor

I propose path and name...

@jonasfj

jonasfj Mar 11, 2016

Contributor

I propose path and name...

This comment has been minimized.

@jonasfj

jonasfj Mar 11, 2016

Contributor

remotePath/name should not start with /...

If type is folder it should end with /, and if type is file it should not ever end with /...

Also it should never contain any double slashes...

@jonasfj

jonasfj Mar 11, 2016

Contributor

remotePath/name should not start with /...

If type is folder it should end with /, and if type is file it should not ever end with /...

Also it should never contain any double slashes...

@gregarndt

This comment has been minimized.

Show comment
Hide comment
@gregarndt

gregarndt Mar 11, 2016

Collaborator

Looks good, we just need to get a consensus on the naming of the source and destination stuff.

Collaborator

gregarndt commented Mar 11, 2016

Looks good, we just need to get a consensus on the naming of the source and destination stuff.

@selenamarie selenamarie changed the title from [WIP] Create and artifact plugin (Don't merge yet) to [WIP] Create an artifact plugin (Don't merge yet) Mar 11, 2016

@jonasfj

This comment has been minimized.

Show comment
Hide comment
@jonasfj

jonasfj Mar 13, 2016

Contributor

I propose path and name... I see the benefit of going with artifactName, the only downside is that everywhere else in artifact end-point documentation it is referred to as <name>..

Honestly, I can't care much... As long as we don't call it remotePath because it is not a path.
Whatever we do, the JSON schema should document it with title: "Artifact Name".


Regarding source I would prefer sourcePath as it's always going to be a path. Just path works fine too... But I care less about that since it doesn't related to queue docs...

Contributor

jonasfj commented Mar 13, 2016

I propose path and name... I see the benefit of going with artifactName, the only downside is that everywhere else in artifact end-point documentation it is referred to as <name>..

Honestly, I can't care much... As long as we don't call it remotePath because it is not a path.
Whatever we do, the JSON schema should document it with title: "Artifact Name".


Regarding source I would prefer sourcePath as it's always going to be a path. Just path works fine too... But I care less about that since it doesn't related to queue docs...

@imbstack

This comment has been minimized.

Show comment
Hide comment
@imbstack

imbstack Mar 18, 2016

Member

I submit a further WIP diff to check and see if this is a good direction to be going in. I feel fairly happy with how this is shaping up. If this is a generally acceptable way of going about this, most of the changes from here on out will be filling out error handling in the plugin itself, and using the client lib stuff (#60) that you all are working on now in the runtime bits.

Currently the tests for this look like

$ go test -v ./plugins/artifacts/
=== RUN   TestArtifactsEmpty
--- PASS: TestArtifactsEmpty (0.00s)
=== RUN   TestArtifactsFile
/public/blah.txt
Hello World
--- PASS: TestArtifactsFile (0.00s)
=== RUN   TestArtifactsDirectory
/public
Hello World
/public
Hello World
/public
Hello World
--- PASS: TestArtifactsDirectory (0.00s)
PASS
ok      github.com/taskcluster/taskcluster-worker/plugins/artifacts     0.017s
Member

imbstack commented Mar 18, 2016

I submit a further WIP diff to check and see if this is a good direction to be going in. I feel fairly happy with how this is shaping up. If this is a generally acceptable way of going about this, most of the changes from here on out will be filling out error handling in the plugin itself, and using the client lib stuff (#60) that you all are working on now in the runtime bits.

Currently the tests for this look like

$ go test -v ./plugins/artifacts/
=== RUN   TestArtifactsEmpty
--- PASS: TestArtifactsEmpty (0.00s)
=== RUN   TestArtifactsFile
/public/blah.txt
Hello World
--- PASS: TestArtifactsFile (0.00s)
=== RUN   TestArtifactsDirectory
/public
Hello World
/public
Hello World
/public
Hello World
--- PASS: TestArtifactsDirectory (0.00s)
PASS
ok      github.com/taskcluster/taskcluster-worker/plugins/artifacts     0.017s
@imbstack

This comment has been minimized.

Show comment
Hide comment
@imbstack

imbstack Mar 18, 2016

Member

A note for @gregarndt: We decided to move ReadSeekCloser to runtime to avoid dependency loops between runtime and engine.

Member

imbstack commented Mar 18, 2016

A note for @gregarndt: We decided to move ReadSeekCloser to runtime to avoid dependency loops between runtime and engine.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 18, 2016

Coverage Status

Coverage increased (+0.1%) to 70.511% when pulling 069cf17 on artifacts into 197638a on master.

Coverage Status

Coverage increased (+0.1%) to 70.511% when pulling 069cf17 on artifacts into 197638a on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Mar 22, 2016

Coverage Status

Coverage increased (+0.1%) to 75.214% when pulling c7f400b on artifacts into ea81ad1 on master.

Coverage Status

Coverage increased (+0.1%) to 75.214% when pulling c7f400b on artifacts into ea81ad1 on master.

@imbstack

This comment has been minimized.

Show comment
Hide comment
@imbstack

imbstack Mar 25, 2016

Member

I'd love if I could get a 30% review on this at some point today, just so that we all feel like this is moving in the right direction. As a caveat, nothing is done and the points don't matter. If this might be better done as a guided excercise at this point, I'd be happy to hop on vidyo and talk it through with someone.

@jonasfj, @gregarndt: any takers?

Member

imbstack commented Mar 25, 2016

I'd love if I could get a 30% review on this at some point today, just so that we all feel like this is moving in the right direction. As a caveat, nothing is done and the points don't matter. If this might be better done as a guided excercise at this point, I'd be happy to hop on vidyo and talk it through with someone.

@jonasfj, @gregarndt: any takers?

Show outdated Hide outdated plugins/artifacts/artifacts.go
func (plugin) NewTaskPlugin(options plugins.TaskPluginOptions) (plugins.TaskPlugin, error) {
return &taskPlugin{
TaskPluginBase: plugins.TaskPluginBase{},

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

No need for this line... the zero-value is assigned by default...

@jonasfj

jonasfj Mar 25, 2016

Contributor

No need for this line... the zero-value is assigned by default...

Show outdated Hide outdated plugins/artifacts/artifacts.go
@@ -0,0 +1,110 @@
//go:generate go-composite-schema --unexported --required artifacts config-schema.yml generated_configschema.go

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

this isn't config, it's payload... let's rename...

@jonasfj

jonasfj Mar 25, 2016

Contributor

this isn't config, it's payload... let's rename...

Show outdated Hide outdated plugins/artifacts/config-schema.yml
@@ -0,0 +1,28 @@
$schema: http://json-schema.org/draft-04/schema#
title: config

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

this is payload not config.

@jonasfj

jonasfj Mar 25, 2016

Contributor

this is payload not config.

Show outdated Hide outdated plugins/artifacts/artifacts.go
}
}
// TODO: Don't always return true?
return true, nil

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

If there was errors uploading artifacts (and we used all retries)... we return the http error... or wrap it... this will be a fatal error, no way around... Maybe later we'll make that an internal error.

If there was an illegal filename, feature not supported, MalformedPayloadError, then we create a MalformedPayloadError explaining what when wrong...

In the face of: ErrResourceNotFound
We return false, nil, and we print to log that the task failed because the file or folder was missing.

@jonasfj

jonasfj Mar 25, 2016

Contributor

If there was errors uploading artifacts (and we used all retries)... we return the http error... or wrap it... this will be a fatal error, no way around... Maybe later we'll make that an internal error.

If there was an illegal filename, feature not supported, MalformedPayloadError, then we create a MalformedPayloadError explaining what when wrong...

In the face of: ErrResourceNotFound
We return false, nil, and we print to log that the task failed because the file or folder was missing.

Show outdated Hide outdated plugins/artifacts/artifacts.go
func (tp *taskPlugin) Stopped(result engines.ResultSet) (bool, error) {
for _, artifact := range tp.payload {
switch artifact.Type {
case "directory":

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

I suspect splitting this into two methods will help..

@jonasfj

jonasfj Mar 25, 2016

Contributor

I suspect splitting this into two methods will help..

Show outdated Hide outdated plugins/artifacts/artifacts.go
switch artifact.Type {
case "directory":
err := result.ExtractFolder(artifact.Path, tp.createUploadHandler(artifact.Name, artifact.Expires))
if err != nil {

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

other errors could happen, but you are required to handle some of them... See:

// Non-fatal erorrs: ErrFeatureNotSupported, ErrResourceNotFound,
// MalformedPayloadError, ErrNonFatalInternalError, ErrHandlerInterrupt

I would say that ErrHandlerInterrupt only happens if you do it...
and ErrNonFatalInternalError probably can't be handled...

If you can't handle errors you forward them... Random errors inside a engine should be fatal.. We do this by returning them...

@jonasfj

jonasfj Mar 25, 2016

Contributor

other errors could happen, but you are required to handle some of them... See:

// Non-fatal erorrs: ErrFeatureNotSupported, ErrResourceNotFound,
// MalformedPayloadError, ErrNonFatalInternalError, ErrHandlerInterrupt

I would say that ErrHandlerInterrupt only happens if you do it...
and ErrNonFatalInternalError probably can't be handled...

If you can't handle errors you forward them... Random errors inside a engine should be fatal.. We do this by returning them...

Show outdated Hide outdated plugins/artifacts/artifacts.go
case "file":
fileReader, err := result.ExtractFile(artifact.Path)
if err != nil {
runtime.CreateErrorArtifact(runtime.ErrorArtifact{

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

Only, in case of ErrResourceNotFound

@jonasfj

jonasfj Mar 25, 2016

Contributor

Only, in case of ErrResourceNotFound

Show outdated Hide outdated plugins/artifacts/artifacts.go
// application/octet-stream is the mime type for "unknown"
mimeType = "application/octet-stream"
}
runtime.UploadS3Artifact(runtime.S3Artifact{

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

I assume UploadS3Artifact can return an error

@jonasfj

jonasfj Mar 25, 2016

Contributor

I assume UploadS3Artifact can return an error

Show outdated Hide outdated plugins/artifacts/artifacts.go
for _, artifact := range tp.payload {
switch artifact.Type {
case "directory":
err := result.ExtractFolder(artifact.Path, tp.createUploadHandler(artifact.Name, artifact.Expires))

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

This isn't enough... you need to handle ErrHandlerInterrupt which indicates that an upload failed... but the actual error returned by the handler is eaten, so you have store that in a separate variable... or something like that...

@jonasfj

jonasfj Mar 25, 2016

Contributor

This isn't enough... you need to handle ErrHandlerInterrupt which indicates that an upload failed... but the actual error returned by the handler is eaten, so you have store that in a separate variable... or something like that...

Show outdated Hide outdated runtime/artifact.go
// TODO: Do something with all of these errors
}
putArtifact(resp.PutURL, artifact.Mimetype, artifact.Stream)

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

this can return an error

@jonasfj

jonasfj Mar 25, 2016

Contributor

this can return an error

Body: stream,
ContentLength: contentLength,
}
resp, err := client.Do(req)

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

@imbstack, this is fine short term...

@gregarndt, @imbstack, long term I think we need TaskContext to implement the Context interface from:
https://godoc.org/golang.org/x/net/context

Then we can use: https://godoc.org/golang.org/x/net/context/ctxhttp
Here and ensure that the http request stops uploading if the task is called, or some other plugin returns an error while we are uploading artifacts... then the plugin manager can cancel...(okay for plugin manager to cancel it would have to wrap it somehow)...

So for plugin manager to cancel we would have to pass a Context object with every request. And this would have to be a separate Context object, not just TaskContext...

So maybe:

  1. TaskContext implmenets the Context interface
  2. Every hooks on plugin takes a Context interface as first parameter

Then pluginManager will call plugin.Prepare(Context, TaskContext), and plugin.Stopped(Context, ResultSet), where Context is TaskContext.WithCancel()

@jonasfj

jonasfj Mar 25, 2016

Contributor

@imbstack, this is fine short term...

@gregarndt, @imbstack, long term I think we need TaskContext to implement the Context interface from:
https://godoc.org/golang.org/x/net/context

Then we can use: https://godoc.org/golang.org/x/net/context/ctxhttp
Here and ensure that the http request stops uploading if the task is called, or some other plugin returns an error while we are uploading artifacts... then the plugin manager can cancel...(okay for plugin manager to cancel it would have to wrap it somehow)...

So for plugin manager to cancel we would have to pass a Context object with every request. And this would have to be a separate Context object, not just TaskContext...

So maybe:

  1. TaskContext implmenets the Context interface
  2. Every hooks on plugin takes a Context interface as first parameter

Then pluginManager will call plugin.Prepare(Context, TaskContext), and plugin.Stopped(Context, ResultSet), where Context is TaskContext.WithCancel()

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

maybe this is something we figure out later... and we just don't support aborting uploads due to cancel... which seems okay,

@jonasfj

jonasfj Mar 25, 2016

Contributor

maybe this is something we figure out later... and we just don't support aborting uploads due to cancel... which seems okay,

This comment has been minimized.

@gregarndt

gregarndt Mar 25, 2016

Collaborator

I think that if we are in the middle of canceling the most time consuming resource intensive thing has already been done and canceling during this might be overkill.

@gregarndt

gregarndt Mar 25, 2016

Collaborator

I think that if we are in the middle of canceling the most time consuming resource intensive thing has already been done and canceling during this might be overkill.

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

it certainly could be overkill... maybe even undesired... Since the artifacts are ready to upload..

@jonasfj

jonasfj Mar 25, 2016

Contributor

it certainly could be overkill... maybe even undesired... Since the artifacts are ready to upload..

Show outdated Hide outdated runtime/ioext/ioext.go
return ReadSeekNopCloser{r.(io.ReadSeeker)}
}
type ReadSeekNopCloser struct {

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

This type should not be exported

@jonasfj

jonasfj Mar 25, 2016

Contributor

This type should not be exported

This comment has been minimized.

@imbstack

imbstack Mar 29, 2016

Member

It is used outside of runtime, we end up needing to export it.

@imbstack

imbstack Mar 29, 2016

Member

It is used outside of runtime, we end up needing to export it.

Show outdated Hide outdated runtime/ioext/ioext.go
io.Closer
}
func NopCloser(r io.Reader) ReadSeekNopCloser {

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

Exported functions should have a documentation comment

@jonasfj

jonasfj Mar 25, 2016

Contributor

Exported functions should have a documentation comment

This comment has been minimized.

@imbstack

imbstack Mar 25, 2016

Member

Haha, yes. I'll circle back and get these things before we do a real review. Should've defined "30% review" before I asked for one. I just use it to mean something between a design review and checking to see if everything is documented correctly 😄. There's still a great bit of work to do before this is ready to be landed.

@imbstack

imbstack Mar 25, 2016

Member

Haha, yes. I'll circle back and get these things before we do a real review. Should've defined "30% review" before I asked for one. I just use it to mean something between a design review and checking to see if everything is documented correctly 😄. There's still a great bit of work to do before this is ready to be landed.

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

Sure, I was just commenting things I saw :)
bad habit... Should have focused on high-level stuff.. Which seems okay,
well, there is some in error handling.

Regards Jonas Finnemann Jensen.

2016-03-25 11:45 GMT-07:00 Brian Stack notifications@github.com:

In runtime/ioext/ioext.go
#55 (comment)
:

@@ -0,0 +1,24 @@
+package ioext
+
+import (

  • "io"
    +)

+// ReadSeekCloser implements io.Reader, io.Seeker, and io.Closer. It is trivially implemented by os.File.
+type ReadSeekCloser interface {

  • io.Reader
  • io.Seeker
  • io.Closer
    +}

+func NopCloser(r io.Reader) ReadSeekNopCloser {

Haha, yes. I'll circle back and get these things before we do a real
review. Should've defined "30% review" before I asked for one. I just use
it to mean something between a design review and checking to see if
everything is documented correctly [image: 😄]. There's still a
great bit of work to do before this is ready to be landed.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/taskcluster/taskcluster-worker/pull/55/files/641d4358cbd8bfb1d63a428002d6cd29f2372f18#r57476233

@jonasfj

jonasfj Mar 25, 2016

Contributor

Sure, I was just commenting things I saw :)
bad habit... Should have focused on high-level stuff.. Which seems okay,
well, there is some in error handling.

Regards Jonas Finnemann Jensen.

2016-03-25 11:45 GMT-07:00 Brian Stack notifications@github.com:

In runtime/ioext/ioext.go
#55 (comment)
:

@@ -0,0 +1,24 @@
+package ioext
+
+import (

  • "io"
    +)

+// ReadSeekCloser implements io.Reader, io.Seeker, and io.Closer. It is trivially implemented by os.File.
+type ReadSeekCloser interface {

  • io.Reader
  • io.Seeker
  • io.Closer
    +}

+func NopCloser(r io.Reader) ReadSeekNopCloser {

Haha, yes. I'll circle back and get these things before we do a real
review. Should've defined "30% review" before I asked for one. I just use
it to mean something between a design review and checking to see if
everything is documented correctly [image: 😄]. There's still a
great bit of work to do before this is ready to be landed.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/taskcluster/taskcluster-worker/pull/55/files/641d4358cbd8bfb1d63a428002d6cd29f2372f18#r57476233

This comment has been minimized.

@imbstack

imbstack Mar 25, 2016

Member

No worries! I just didn't want you to have to do all of this work this soon.

@imbstack

imbstack Mar 25, 2016

Member

No worries! I just didn't want you to have to do all of this work this soon.

@@ -0,0 +1,24 @@
package ioext

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

Package needs a doc comment... You can do it here, or create a doc.go file with this...

@jonasfj

jonasfj Mar 25, 2016

Contributor

Package needs a doc comment... You can do it here, or create a doc.go file with this...

@@ -173,7 +173,10 @@ func (t *TaskRun) parsePayload() error {
// createTaskPlugins will create a new task plugin to be used during the task lifecycle.
func (t *TaskRun) createTaskPlugins() error {
var err error
popts := plugins.TaskPluginOptions{TaskInfo: &runtime.TaskInfo{}, Payload: t.pluginPayload}
popts := plugins.TaskPluginOptions{TaskInfo: &runtime.TaskInfo{

This comment has been minimized.

@gregarndt

gregarndt Mar 25, 2016

Collaborator

Ah, good catch on this, thanks!

@gregarndt

gregarndt Mar 25, 2016

Collaborator

Ah, good catch on this, thanks!

Show outdated Hide outdated plugins/artifacts/artifacts.go
payload config
}
func (tp *taskPlugin) Prepare(context *runtime.TaskContext) error {

This comment has been minimized.

@gregarndt

gregarndt Mar 25, 2016

Collaborator

I'm not sure if you want to do some validation here about the artifacts. Like making sure artifact expires is <= task.expires otherwise when you go to create the artifact the queue will complain.

@gregarndt

gregarndt Mar 25, 2016

Collaborator

I'm not sure if you want to do some validation here about the artifacts. Like making sure artifact expires is <= task.expires otherwise when you go to create the artifact the queue will complain.

Show outdated Hide outdated plugins/artifacts/config-schema.yml
type: string
format: date-time
# TODO: Should expires be required?

This comment has been minimized.

@gregarndt

gregarndt Mar 25, 2016

Collaborator

right now what we do is default it to task.expires (which is set by the queue if not explicitly declared (default is 1 year from creation I think))

@gregarndt

gregarndt Mar 25, 2016

Collaborator

right now what we do is default it to task.expires (which is set by the queue if not explicitly declared (default is 1 year from creation I think))

This comment has been minimized.

@gregarndt

gregarndt Mar 25, 2016

Collaborator

Note that artifact expiration is required when creating an artifact on the queue, so either we make it standard here as well that's it's required (likely another breaking change in this worker) or we accept that people might not specify it and if that's the care we should default it when creating the artifact.

@gregarndt

gregarndt Mar 25, 2016

Collaborator

Note that artifact expiration is required when creating an artifact on the queue, so either we make it standard here as well that's it's required (likely another breaking change in this worker) or we accept that people might not specify it and if that's the care we should default it when creating the artifact.

This comment has been minimized.

@jonasfj

jonasfj Mar 25, 2016

Contributor

I would love of expires to be optional... and default to task.expires. That seems sane...

@jonasfj

jonasfj Mar 25, 2016

Contributor

I would love of expires to be optional... and default to task.expires. That seems sane...

@imbstack imbstack changed the title from [WIP] Create an artifact plugin (Don't merge yet) to Create an artifact plugin Mar 29, 2016

@imbstack

This comment has been minimized.

Show comment
Hide comment
@imbstack

imbstack Mar 29, 2016

Member

@gregarndt, @jonasfj: Alright, I think this is pretty much ready (barring inevitable review comments). In particular, I'm pretty sure the error handling I have in here is still overly simplistic, but it's at least moving in the right direction I think.

Example of created artifacts: https://tools.taskcluster.net/task-inspector/#WzLXbfiVTf2MHmaCEJFRmg/0
Example of error artifact: https://tools.taskcluster.net/task-inspector/#ObSgu516SnOhJdOuu3EMsQ/0

Member

imbstack commented Mar 29, 2016

@gregarndt, @jonasfj: Alright, I think this is pretty much ready (barring inevitable review comments). In particular, I'm pretty sure the error handling I have in here is still overly simplistic, but it's at least moving in the right direction I think.

Example of created artifacts: https://tools.taskcluster.net/task-inspector/#WzLXbfiVTf2MHmaCEJFRmg/0
Example of error artifact: https://tools.taskcluster.net/task-inspector/#ObSgu516SnOhJdOuu3EMsQ/0

Show outdated Hide outdated runtime/ioext/ioext.go
}
// ReadSeekNopCloser is an implementation of ReadSeekCloser that wraps io.ReadSeekers
type ReadSeekNopCloser struct {

This comment has been minimized.

@jonasfj

jonasfj Mar 29, 2016

Contributor

You don't need to export this type...

You create it with NopCloser whose return type should ReadSeekCloser

@jonasfj

jonasfj Mar 29, 2016

Contributor

You don't need to export this type...

You create it with NopCloser whose return type should ReadSeekCloser

Show outdated Hide outdated runtime/ioext/ioext.go
// NopCloser is useful in testing where something that implements ReadSeekCloser is needed
// If something that implements io.ReadSeeker is passed in, it will give it a noop close function.
func NopCloser(r io.Reader) ReadSeekNopCloser {

This comment has been minimized.

@jonasfj

jonasfj Mar 29, 2016

Contributor

return type should be ReadSeekCloser

@jonasfj

jonasfj Mar 29, 2016

Contributor

return type should be ReadSeekCloser

func (plugin) NewTaskPlugin(options plugins.TaskPluginOptions) (plugins.TaskPlugin, error) {
if options.Payload == nil {
return plugins.TaskPluginBase{}, nil

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

nice trick, because payload is optional right?

@jonasfj

jonasfj Mar 30, 2016

Contributor

nice trick, because payload is optional right?

This comment has been minimized.

@imbstack

imbstack Mar 30, 2016

Member

Yeah, without this if the artifacts plugin was enabled, and no artifacts were specified, the whole thing would come to a grinding halt!

@imbstack

imbstack Mar 30, 2016

Member

Yeah, without this if the artifacts plugin was enabled, and no artifacts were specified, the whole thing would come to a grinding halt!

This comment has been minimized.

@jonasfj

jonasfj Mar 31, 2016

Contributor

maybe we should modify the plugin manager to ignore a plugin that returns nil, just so it'll be easier to disable things...

On the other hand this is really elegant :)

@jonasfj

jonasfj Mar 31, 2016

Contributor

maybe we should modify the plugin manager to ignore a plugin that returns nil, just so it'll be easier to disable things...

On the other hand this is really elegant :)

Show outdated Hide outdated plugins/artifacts/artifacts.go
func (tp taskPlugin) errorHandled(name string, expires tcclient.Time, err error) bool {
if err == engines.ErrFeatureNotSupported || err == engines.ErrResourceNotFound ||
err == engines.ErrNonFatalInternalError || err == engines.ErrHandlerInterrupt ||
reflect.TypeOf(err).String() == "engines.MalformedPayloadError" {

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

Avoid using reflect...

Just make different if statement on the form:

if e, ok := err.(*engines.MalformedPayloadError); ok {
  // Now is e is a MalformedPayloadError type
}

Don't for forget the ok part, otherwise it'll panic on failure.

@jonasfj

jonasfj Mar 30, 2016

Contributor

Avoid using reflect...

Just make different if statement on the form:

if e, ok := err.(*engines.MalformedPayloadError); ok {
  // Now is e is a MalformedPayloadError type
}

Don't for forget the ok part, otherwise it'll panic on failure.

artifact.Expires = tp.context.TaskInfo.Expires
}
switch artifact.Type {
case "directory":

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

hmm, How not to be a pain... But how about we do a method for each case...
Then do all uploads in parallel? Or is that insane?

Take a look at how PluginManager does it with waitgroup and atomicBool

@jonasfj

jonasfj Mar 30, 2016

Contributor

hmm, How not to be a pain... But how about we do a method for each case...
Then do all uploads in parallel? Or is that insane?

Take a look at how PluginManager does it with waitgroup and atomicBool

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

Since, the only error we're allowed to return is a MalformedPayloadError.
Perhaps all errors we get that aren't fatal should be combined into a single MalformedPayloadError.

Like a bullet point for each thing that was wrong :)

@jonasfj

jonasfj Mar 30, 2016

Contributor

Since, the only error we're allowed to return is a MalformedPayloadError.
Perhaps all errors we get that aren't fatal should be combined into a single MalformedPayloadError.

Like a bullet point for each thing that was wrong :)

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

In which case Atomic bool isn't much use to you, as you'll need a lock when appending the string anyways.

@jonasfj

jonasfj Mar 30, 2016

Contributor

In which case Atomic bool isn't much use to you, as you'll need a lock when appending the string anyways.

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

note, this might be better left for a follow up patch...

@jonasfj

jonasfj Mar 30, 2016

Contributor

note, this might be better left for a follow up patch...

This comment has been minimized.

@gregarndt

gregarndt Mar 30, 2016

Collaborator

if there was an upload error because of s3/queue/ why would we only be allowed to return a malformed payload error?

@gregarndt

gregarndt Mar 30, 2016

Collaborator

if there was an upload error because of s3/queue/ why would we only be allowed to return a malformed payload error?

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

Any other error is fatal as per the interface defintion...

Sorry, you are allowed to return other errors, as long as you keep in mind that such errors are fatal.

@jonasfj

jonasfj Mar 30, 2016

Contributor

Any other error is fatal as per the interface defintion...

Sorry, you are allowed to return other errors, as long as you keep in mind that such errors are fatal.

This comment has been minimized.

@imbstack

imbstack Mar 30, 2016

Member

Perhaps we should update the interface definition to allow more non-fatal errors?

@imbstack

imbstack Mar 30, 2016

Member

Perhaps we should update the interface definition to allow more non-fatal errors?

This comment has been minimized.

@jonasfj

jonasfj Mar 31, 2016

Contributor

Perhaps we should update the interface definition to allow more non-fatal errors?
What errors do you propose, and how do you propose we handle them?

Yeah, maybe we should allow: InternalError:
https://github.com/taskcluster/taskcluster-worker/blob/master/engines/errors.go#L117

With the assumption that this must only be used for errors that:
A) Cannot be trigger by the task payload, or behavior of the code running inside the sandbox,
B) We are confident will not affect the next task the worker runs

Most internal errors like out-of-disk-space, out-of-memory, cannot contact docker socket, container suddenly disappeared, file that I know I created don't exist anymore, missing hardware device, file system is corrupted, docker daemon died, do not satisfy condition B.

In cases like above, the sane thing is to report the task exception worker-shutdown, and then polity crash, probably terminate the instance.

In cases like: can't reach the internet, hostname doesn't resolve, credentials are expired or don't work, etc.
The worker may be able to report the task exception internal-error (have the task retried, we probably need to change queue semantics for that), and then try with the next task.

However, if we see a lot of these can't reach the internet, can't resolve DNS, can't upload to S3, credentials expired unexpectedly, etc... Then we should very much die..

@imbstack, We don't want tasks to retry too much, and we definitely don't want a corrupted worker to just churn through the queue, claim tasks and the report them exception... So we have to be extremely careful about what is a non-fatal internal error. Note: errors that potentially leaves something in a dirty state cannot be non-fatal.

If we want to pursue this, rather than just saying everything is fatal. Then we should probably rename InternalError to NonFatalInternalError or CleanInternalError, suggestions?
Then specify in doc string what it can be used for, and what it absolutely cannot be used for. Before we expand what the interfaces can return...

This could be a follow-up, please file a bug for those... :)

@jonasfj

jonasfj Mar 31, 2016

Contributor

Perhaps we should update the interface definition to allow more non-fatal errors?
What errors do you propose, and how do you propose we handle them?

Yeah, maybe we should allow: InternalError:
https://github.com/taskcluster/taskcluster-worker/blob/master/engines/errors.go#L117

With the assumption that this must only be used for errors that:
A) Cannot be trigger by the task payload, or behavior of the code running inside the sandbox,
B) We are confident will not affect the next task the worker runs

Most internal errors like out-of-disk-space, out-of-memory, cannot contact docker socket, container suddenly disappeared, file that I know I created don't exist anymore, missing hardware device, file system is corrupted, docker daemon died, do not satisfy condition B.

In cases like above, the sane thing is to report the task exception worker-shutdown, and then polity crash, probably terminate the instance.

In cases like: can't reach the internet, hostname doesn't resolve, credentials are expired or don't work, etc.
The worker may be able to report the task exception internal-error (have the task retried, we probably need to change queue semantics for that), and then try with the next task.

However, if we see a lot of these can't reach the internet, can't resolve DNS, can't upload to S3, credentials expired unexpectedly, etc... Then we should very much die..

@imbstack, We don't want tasks to retry too much, and we definitely don't want a corrupted worker to just churn through the queue, claim tasks and the report them exception... So we have to be extremely careful about what is a non-fatal internal error. Note: errors that potentially leaves something in a dirty state cannot be non-fatal.

If we want to pursue this, rather than just saying everything is fatal. Then we should probably rename InternalError to NonFatalInternalError or CleanInternalError, suggestions?
Then specify in doc string what it can be used for, and what it absolutely cannot be used for. Before we expand what the interfaces can return...

This could be a follow-up, please file a bug for those... :)

This comment has been minimized.

@gregarndt

gregarndt Mar 31, 2016

Collaborator

We've discussed before about the use of internal-error and the semantics around retrying. I'm till on the fence with it. I think in the case of failure to upload, most of the time it's a transient issue that I think could be reported as internal-error and the next task that worker tries to complete will not fail for the same reason.

I definitely think in the cases of resource exhaustion or other issues that the worker knows about the host, the worker should shut itself down and resolve tasks as 'worker-shutdown'

@gregarndt

gregarndt Mar 31, 2016

Collaborator

We've discussed before about the use of internal-error and the semantics around retrying. I'm till on the fence with it. I think in the case of failure to upload, most of the time it's a transient issue that I think could be reported as internal-error and the next task that worker tries to complete will not fail for the same reason.

I definitely think in the cases of resource exhaustion or other issues that the worker knows about the host, the worker should shut itself down and resolve tasks as 'worker-shutdown'

This comment has been minimized.

@gregarndt

gregarndt Mar 31, 2016

Collaborator

I don't see the need to label it as "NonFatalXXX" if there is not also a "FatalXXX".

Also, with fatal errors, because the shutdown logic is tied into the worker process, we need to make sure those that are treated as fatal are handled appropriately and the shutdown logic (when it exists) will get called rather than just panicking

@gregarndt

gregarndt Mar 31, 2016

Collaborator

I don't see the need to label it as "NonFatalXXX" if there is not also a "FatalXXX".

Also, with fatal errors, because the shutdown logic is tied into the worker process, we need to make sure those that are treated as fatal are handled appropriately and the shutdown logic (when it exists) will get called rather than just panicking

Show outdated Hide outdated plugins/artifacts/artifacts.go
}
}
}
return true, nil

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

if you created an error artifact you should return false, MalformedPayloadError

@jonasfj

jonasfj Mar 30, 2016

Contributor

if you created an error artifact you should return false, MalformedPayloadError

Show outdated Hide outdated plugins/artifacts/artifacts.go
Reason: "invalid-resource-on-worker",
Expires: expires,
}, tp.context)
return true

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

We should also log these errors to the task log... tp.context has a method for this...

@jonasfj

jonasfj Mar 30, 2016

Contributor

We should also log these errors to the task log... tp.context has a method for this...

func (tp taskPlugin) createUploadHandler(name, prefix string, expires tcclient.Time) func(string, ioext.ReadSeekCloser) error {
return func(path string, stream ioext.ReadSeekCloser) error {
return tp.attemptUpload(stream, path, strings.Replace(path, prefix, name, 1), expires)

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

Why strings.Replace? what does it do?

@jonasfj

jonasfj Mar 30, 2016

Contributor

Why strings.Replace? what does it do?

This comment has been minimized.

@imbstack

imbstack Mar 30, 2016

Member

It's just the way to make the following do the right thing.

"artifacts": [
    {
        "type": "directory",
        "path": "/artifacts",
        "name": "public"
    }
] 

strings.Replace is just turning the path /artifacts/foo.txt given to the UploadHandler into public/foo.txt.

@imbstack

imbstack Mar 30, 2016

Member

It's just the way to make the following do the right thing.

"artifacts": [
    {
        "type": "directory",
        "path": "/artifacts",
        "name": "public"
    }
] 

strings.Replace is just turning the path /artifacts/foo.txt given to the UploadHandler into public/foo.txt.

Show outdated Hide outdated plugins/artifacts/artifacts_test.go
)
type artifactTestCase struct {
TestCase *plugintest.Case

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

This is a neat trick!!! :)

You can probably just embed plugintest.Case removing some unneeded intentation...

You can still call Test() by using a.plugintest.Case.Test() or a.Case.Test() I think...

@jonasfj

jonasfj Mar 30, 2016

Contributor

This is a neat trick!!! :)

You can probably just embed plugintest.Case removing some unneeded intentation...

You can still call Test() by using a.plugintest.Case.Test() or a.Case.Test() I think...

EngineSuccess: true,
},
}.Test()
}

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

In a later PR let's add more test cases... for all the error cases... Might might have to do some tricks, but I think mockEngine can be made to return all sorts of errors.

@jonasfj

jonasfj Mar 30, 2016

Contributor

In a later PR let's add more test cases... for all the error cases... Might might have to do some tricks, but I think mockEngine can be made to return all sorts of errors.

Show outdated Hide outdated plugins/artifacts/payload-schema.yml
title: Artifact Name
description: This will be the leading path to directories and the full name for files that are uploaded to s3
type: string
pattern: "^[^/].*[^/]$"

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

@djmitche, @gregarndt If we ever want to enforce artifact naming... Restrict characters to printable ascii or something like that, now is the time...
We can't really make this change on queue.taskcluster.net before we've made sure that all uploaders don't violate this requirement...

However, given that the scope 'queue:get-artifact:<name>' is required to get an artifact... We are already in a situation where any non-public artifact can't be downloaded if name contains characters that isn't ascii printable.

This might be the wrong the place for this discussion, but perhaps we should take the first step in disallowing non-printable ascii chars in artifact names...
I doubt any are in use, but we should probably do a table scan and check before we change the queue. However, locking it down here is a good start.

@jonasfj

jonasfj Mar 30, 2016

Contributor

@djmitche, @gregarndt If we ever want to enforce artifact naming... Restrict characters to printable ascii or something like that, now is the time...
We can't really make this change on queue.taskcluster.net before we've made sure that all uploaders don't violate this requirement...

However, given that the scope 'queue:get-artifact:<name>' is required to get an artifact... We are already in a situation where any non-public artifact can't be downloaded if name contains characters that isn't ascii printable.

This might be the wrong the place for this discussion, but perhaps we should take the first step in disallowing non-printable ascii chars in artifact names...
I doubt any are in use, but we should probably do a table scan and check before we change the queue. However, locking it down here is a good start.

This comment has been minimized.

@gregarndt

gregarndt Mar 31, 2016

Collaborator

I agree that if we're going to start making this a requirement (we should) then we should start with this new worker to set the precedent and make sure at least this worker wouldn't allow it.

@gregarndt

gregarndt Mar 31, 2016

Collaborator

I agree that if we're going to start making this a requirement (we should) then we should start with this new worker to set the precedent and make sure at least this worker wouldn't allow it.

This comment has been minimized.

@djmitche

djmitche Mar 31, 2016

Contributor

I think that limits similar to scopes make a lot of sense. These are basically filenames, and encodings + filenames are an absolute nightmare, so let's avoid it by sticking to printable ascii

@djmitche

djmitche Mar 31, 2016

Contributor

I think that limits similar to scopes make a lot of sense. These are basically filenames, and encodings + filenames are an absolute nightmare, so let's avoid it by sticking to printable ascii

Show outdated Hide outdated plugins/artifacts/payload-schema.yml
type: string
format: date-time
# TODO: Should expires be required?

This comment has been minimized.

@jonasfj

jonasfj Mar 30, 2016

Contributor

nope, remove comment

@jonasfj

jonasfj Mar 30, 2016

Contributor

nope, remove comment

@imbstack imbstack merged commit 37dd9ea into master Mar 31, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@imbstack imbstack deleted the artifacts branch Mar 31, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment