Skip to content
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

Proxy download from Mesos slave over Singularity. #1817

Merged
merged 6 commits into from Aug 1, 2018
Merged

Conversation

@kdorosh
Copy link
Contributor

@kdorosh kdorosh commented Jul 11, 2018

Solves problem of being on different network than Mesos slave. Directly proxying through a configurable load balancer can still be looked into.

@Path("/download/{httpPrefix}/{slaveHostname}/port/{port}/path/{path}")
@Produces(MediaType.APPLICATION_OCTET_STREAM)
@Operation(summary = "Proxy a file download from a Mesos Slave through Singularity")
@Consumes(MediaType.APPLICATION_FORM_URLENCODED)

This comment has been minimized.

@baconmania

baconmania Jul 11, 2018
Contributor

Since this is a GET, it shouldn't consume anything.

This comment has been minimized.

@kdorosh

kdorosh Jul 11, 2018
Author Contributor

-- whoops -- left this there from when I had it as a POST

@Parameter(required = true, description = "Full file path to file on Mesos slave to be downloaded") @PathParam("path") String fileFullPath
) {

Client client = ClientBuilder.newClient();

This comment has been minimized.

@baconmania

baconmania Jul 11, 2018
Contributor

I believe we want to inject & use the AsyncHttpClient here instead of using a JAX-RS client, similarly to

Response response = asyncHttpClient
.prepareGet(String.format("http://%s:5051/files/browse", slaveHostname))
.setPerRequestConfig(timeoutConfig)
.addQueryParameter("path", fullPath)
.execute()
.get();
.

This comment has been minimized.

@kdorosh

kdorosh Jul 11, 2018
Author Contributor

Ah neat, will do. We already have one being injected too which is awesome since creating these clients are expensive operations.


file.downloadLink = `${config.apiRoot}/tasks/download/${httpPrefix}/${files.slaveHostname}/port/${httpPort}/path/${file.fullPath}`;

This comment has been minimized.

@baconmania

baconmania Jul 11, 2018
Contributor

The value of httpPrefix is based on the Singularity configuration, so we shouldn't need to send this as a parameter to the backend (since the backend also has access to that configuration and knows whether the slave communicates over HTTP or HTTPS and which port it's listening on).

@@ -609,4 +613,30 @@ private SingularityTaskShellCommandRequest startShellCommand(SingularityTaskId t
SingularityTaskId taskIdObj = getTaskIdFromStr(taskId);
return taskManager.getTaskShellCommandUpdates(new SingularityTaskShellCommandRequestId(taskIdObj, commandName, commandTimestamp));
}

@GET
@Path("/download/{httpPrefix}/{slaveHostname}/port/{port}/path/{path}")

This comment has been minimized.

@baconmania

baconmania Jul 11, 2018
Contributor

Sort of a nit, but a better API design here could be something like GET /download?slaveHostname=x&path=y (since the actual resource being retrieved is a download). The rest of our API doesn't stick to these guidelines religiously, though, so probably not a huge deal. I'll let @ssalinas weigh in as well.

This comment has been minimized.

@kdorosh

kdorosh Jul 11, 2018
Author Contributor

Wouldn't it be better to have them both as PathParams instead of QueryParams since both the slave hostname and the path are both required fields? I did change the URL a bit to better match entity class hierarchy standards. See top answer and comments on this StackOverflow post.

kdorosh added 2 commits Jul 11, 2018
… passing params that were in Mesos config.
@kdorosh
Copy link
Contributor Author

@kdorosh kdorosh commented Jul 13, 2018

This was merged into hs_staging and hs_qa yesterday, 7/12/18.

@ssalinas
Copy link
Member

@ssalinas ssalinas commented Jul 16, 2018

🚢

@ssalinas ssalinas added the hs_stable label Jul 16, 2018
@ssalinas ssalinas added this to the 0.21.0 milestone Jul 16, 2018
@ssalinas ssalinas merged commit 4c119da into master Aug 1, 2018
1 of 2 checks passed
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 file_download_proxy branch Aug 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.