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

1842 nginx sendfile #37

Merged
merged 7 commits into from
Jun 2, 2018
Merged

1842 nginx sendfile #37

merged 7 commits into from
Jun 2, 2018

Conversation

alvacouch
Copy link

Changes necessary to utilize nginx sendfile.

@alvacouch
Copy link
Author

@aphelionz I have no review of this as yet and need to merge it in order to complete file download.

views.py Outdated
name=path.split('/')[-1])
response['X-Accel-Redirect'] = os.path.join(
getattr(settings, 'IRODS_USER_URI', '/irods-user'), path)
return response
Copy link

Choose a reason for hiding this comment

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

@alvacouch the path passed in by the download request does not include federation path, but instead only include resource uuid. The download view function get resource object from uuid and pre-concatenate federation path with the request path to create the right path for iget call. So I think instead to check the federation path here, you can put different processing logic into below around line 55-56, so there is no wasted processing of pre-concatenating federation path, followed by removing federation path with extra conditional checking.

Copy link
Author

@alvacouch alvacouch May 24, 2018

Choose a reason for hiding this comment

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

@hyi @aphelionz As far as I know, this is incorrect. That request includes only the resource id if the request is not federated. If it's federated by -- e.g. -- having a user federation path, that path is appended. I strip that off and then forward the request to nginx. This is the only way I know that the request is federated. This seems to work! I do not understand what you mean by modifying the code below because in this case, that code is not executed at all.

@alvacouch
Copy link
Author

alvacouch commented May 25, 2018

@hyi @aphelionz I think some context is in order. This PR intercepts the request after it has been redirected to the django_irods download_file URI. This request is unqualified if local, and qualified with the federation path if not local. The PR changes things so that nginx handles requests of two types:

  1. local requests
  2. requests that -- though federated -- are housed locally.

The way this works is that these two kinds of requests are translated into custom nginx calls, so that the file can be transferred via the operating system rather than via nginx. These paths are arbitrary and map to mount points in the nginx container.

Thus the function of this is very much dependent upon nginx configuration. In Nginx:

  • /irods_data is mapped in Nginx to the path of the irods local repository, as mounted on NFS in the nginx container.
  • /irods_user is mapped in Nginx to the path of the irods local user repository, as mounted on NFS in the nginx container.

These paths are only loosely related to the irods federation path.

@hyi
Copy link

hyi commented May 25, 2018

@alvacouch Thanks for the helpful context. Do you need the federation path to be able to do the mounting? If not, perhaps a flag that indicates whether the downloaded file comes from data or user zone would be sufficient?

@hyi
Copy link

hyi commented May 26, 2018

@alvacouch On second thought, a flag is most likely not sufficient since the resource could come from a third party federated zone. It looks like the federation path may be needed after all. The initial implementation of this download view function actually did use federation absolute logical path in the download URL as path parameter, then it was changed to remove the federation path since it was decided at the time it was not good to expose the whole federation path. Anyway, if the whole path is needed to mount the vault for nginx to send the file, I am fine with including the whole path in the download URL. However, I believe the url mapper view function also needs to be changed to accommodate this when mapping the download URL to the uniform URL used in resourcemap xml file.

@alvacouch alvacouch merged commit 257c945 into master Jun 2, 2018
Sign up for free to 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.

None yet

2 participants