-
Notifications
You must be signed in to change notification settings - Fork 86
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
Tell StorageProviders where to store data #3053
Conversation
3a14cbf
to
69b5317
Compare
pkg/compute/executor.go
Outdated
@@ -216,7 +221,13 @@ func (e *BaseExecutor) Start(ctx context.Context, execution *models.Execution) ( | |||
return | |||
} | |||
|
|||
args, cleanup, err := PrepareRunArguments(ctx, e.Storages, execution, resultFolder) | |||
executionStorage := filepath.Join(e.storageDirectory, fmt.Sprintf("%s/%s", execution.JobID, execution.ID)) |
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.
Why not just filepath.Join(e.StorageDirectory, execution.JobID, execution.ID)
?
pkg/compute/executor.go
Outdated
@@ -216,7 +221,13 @@ func (e *BaseExecutor) Start(ctx context.Context, execution *models.Execution) ( | |||
return | |||
} | |||
|
|||
args, cleanup, err := PrepareRunArguments(ctx, e.Storages, execution, resultFolder) | |||
executionStorage := filepath.Join(e.storageDirectory, fmt.Sprintf("%s/%s", execution.JobID, execution.ID)) | |||
if err := os.MkdirAll(executionStorage, os.ModePerm); err != nil { |
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.
This sets 777 permissions, is that what we want? Would we prefer 755 or 500?
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.
Have set to 0755 and a local constant. I've left the exec bit for now, meaning users can download a binary and run it if they wish (within the container theyre in). Is this something we want to allow?
cm.RegisterCallback(func() error { | ||
if err := os.RemoveAll(dir); err != nil { | ||
return fmt.Errorf("unable to clean up S3 storage directory: %w", err) | ||
} | ||
return nil | ||
}) |
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.
So where does cleanup happen now?
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.
Other than tests, PrepareStorage is only ever called from ParallelPrepareStorage, and ParallelPrepareStorage is only used by the base executor (and the wasm executor) and each time returns a function that will call ParallelCleanup. This is a step in the direction of us cleaning up the resources when they're no longer used so that storage is responsible for fetching the data, but the user is responsible for cleaning it up.
Currently storage providers create a temporary folder underneath the system configured storage path, where the folder include the provider's name. This means all of the data is indexed on the provider, rather than the execution. This PR switches to telling the provider where to store the data, so that it is keyed by job/execution instead (also within the configured data folder).
The parallel tests were expecting to cleanup, and at the same time not checking the existence of files it thinks it had downloaded
0c6879c
to
666fedf
Compare
Currently storage providers create a temporary folder underneath the system configured storage path, where the folder include the provider's name. This means all of the data is indexed on the provider, rather than the execution. Most of the StorageProvider implementations determine these paths when they are first created.
This PR switches to telling the provider where to store the data, so that it is keyed by job/execution instead (also within the configured data folder). This provides a single location for all inputs affecting an execution (or all local executions for a Job).