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

Wire up overrides for the S3 uploader path. #1692

Merged
merged 4 commits into from Jan 17, 2018

Conversation

Projects
None yet
2 participants
@baconmania
Contributor

baconmania commented Jan 8, 2018

No description provided.

@@ -40,6 +42,7 @@ public SingularityRunNowRequest(@JsonProperty("message") Optional<String> messag
@JsonProperty("runId") Optional<String> runId,
@JsonProperty("commandLineArgs") Optional<List<String>> commandLineArgs,
@JsonProperty("resources") Optional<Resources> resources,
@JsonProperty("s3UploaderAdditionalFiles") List<SingularityS3UploaderFile> s3UploaderAdditionalFiles,

This comment has been minimized.

@ssalinas

ssalinas Jan 9, 2018

Member

remember this can be used from the client, so adding args will break the build for folks

This comment has been minimized.

@baconmania

baconmania Jan 10, 2018

Contributor

👍 good catch. Restored the constructor from the most recent release.

if (configuration.getS3ConfigurationOptional().isPresent()) {
uploaderAdditionalFiles.addAll(configuration.getS3ConfigurationOptional().get().getS3UploaderAdditionalFiles());
}
uploaderAdditionalFiles.addAll(task.getPendingTask().getS3UploaderAdditionalFiles());

This comment has been minimized.

@ssalinas

ssalinas Jan 9, 2018

Member

is it possible for two of these to overlap? The original use for this was to override where a specific file was being sent. Maybe we need to dedupe on the directory/glob in the additional file settings? If the uploader tries to upload it twice, one instance of that will fail when the first one done tries to delete it

This comment has been minimized.

@baconmania

baconmania Jan 10, 2018

Contributor

Hmm, that's a tricky one. Since we don't have a priori information about the actual set of files in the sandbox, it's nontrivial to determine if the glob patterns overlap. We could just compare the pattern strings directly, but that would of course not cover all potential overlaps.

This comment has been minimized.

@ssalinas

ssalinas Jan 10, 2018

Member

I would argue that some protection is better than none. Maybe we at least check for exact matches and call it good? That or another way to ensure it is to only take the override if it is set and ignore the executor settings

This comment has been minimized.

@baconmania

baconmania Jan 10, 2018

Contributor

After digging on this, it looks like a ConcurrentMap is used to ensure that two uploaders don't simultaneously try to handle the same file.

This means we won't have uploader errors due to conflicts, although it does leave a race condition in place. The way to solve this in the future would be to ensure that upload overrides for a file use a matching directory and glob strings as the corresponding global config, and then just compare those fields directly instead of trying to compute glob overlaps.

@ssalinas ssalinas added this to the 0.19.0 milestone Jan 17, 2018

@ssalinas

This comment has been minimized.

Member

ssalinas commented Jan 17, 2018

🚢

@ssalinas ssalinas merged commit 9d5145d into master Jan 17, 2018

1 of 2 checks passed

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

@ssalinas ssalinas deleted the s3-uploader-path-overrides branch Jan 17, 2018

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