8000 Reconsider mounting /tmp as a shared PersistentVolumeClaim · Issue #30 · Duke-GCB/calrissian · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Reconsider mounting /tmp as a shared PersistentVolumeClaim #30

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.

Sign up for GitHub

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

Jump to bottom
Closed
johnbradley opened this issue Jan 22, 2019 · 5 comments · Fixed by #62
Closed

Reconsider mounting /tmp as a shared PersistentVolumeClaim #30

johnbradley opened this issue Jan 22, 2019 · 5 comments · Fixed by #62
Assignees
@dleehr
Labels
enhancement New feature or request

Comments

@johnbradley
Copy link
Contributor
johnbradley commented Jan 22, 2019 •
edited by dleehr
Loading

Currently, /tmp in each container is mounted from a subPath of a shared PV. So each container would get its own /tmp but that would be mounted over a network. As I mentioned in #5 (comment), this is probably counter to what we want and should re-consider.

I suggest in #5 (comment) that tmp should simply be mounted as a kubernetes emptyDir.
@johnbradley johnbradley mentioned this issue Jan 22, 2019
Closed
@dleehr dleehr self-assigned this Jan 22, 2019
@dleehr
Copy link
Member
dleehr commented Jan 22, 2019

This won't be as simple as I thought.

The current implementation (adapted from cwltool) copies files /directories into self.tmpdir using shutil.copy().

It appears to be implementing the InitialWorkDirRequirement, which allows for writable disposable copies of files/directories.

When that feature is used, /tmp is not empty before the container runs, so it wouldn't make sense to replace it with emptyDir.

Worth further discussion, won't be implementing anything immediately.

@dleehr dleehr changed the title use emptyDir for /tmp volume Revisit mounting /tmp as a PersistentVolumeClaim Jan 22, 2019
@dleehr dleehr changed the title Revisit mounting /tmp as a PersistentVolumeClaim Reconsider mounting /tmp as a shared PersistentVolumeClaim Jan 22, 2019
@dleehr
Copy link
Member
dleehr commented Jan 22, 2019

When I was porting this code, the shutil.copy() and tempfile.mkdtemp() calls jumped out as something that tightly couples the workflow engine to the local filesystem. I added debug logging around those calls to be able to see when it's used.

I think the best solution to this (and also relates to #31) would be to remove that coupling wherever possible. At some level, the engine does need access to the files to calculate checksums and sizes, but I don't think we should be tightening that coupling.

@dleehr dleehr removed their assignment Jan 22, 2019
@dleehr dleehr added the enhancement New feature or request label Mar 7, 2019
@dleehr
Copy link
Member
dleehr commented Mar 7, 2019

We'll want some data to determine how important this is. While we could (in most cases) just use a node-local /tmp directory from an emptyDir volume, that would not work (as stated above) for some CWL features used by other disciplines.

@dleehr
Copy link
Member
dleehr commented Mar 7, 2019

On a recent CWL call, I heard about the genesis of this feature (writable files put into tmp) and that it's for large data sets that need to be mutable but perhaps disposable.

That doesn't come up in our use cases. Again we'd still need some performance data, but I think the best plan for this feature is to default to using a local emptyDir for tmp, and if a case arises where that wouldn't work (the shutil.copy lines above), then to either raise an exception or fall back to the current behavior of using the shared PVC tmpdir.

@dleehr
Copy link
Member
dleehr commented Mar 7, 2019

Related to Duke-GCB/lando#141

@dleehr dleehr mentioned this issue Mar 7, 2019
Open
@dleehr dleehr self-assigned this Mar 19, 2019
dleehr added a commit that referenced this issue Mar 20, 2019
@dleehr
Use emptyDir for '/tmp' inside containers
77855a3 
- Updates KubernetesVolumeBuilder to support a list of named emptyDir volumes
- Updates CalrissianCommandLineJob.create_kubernetes_runtime to use an emptyDir volume for '/tmp' inside the container

Upon implementation, I don't think this will conflict with any of the special cases mentioned in #30 (writable files being copied into /tmp). These files are copied on the calrissian host and mounted later (via `add_volumes()`). This change will mount the base `/tmp` as an `emptyDir` but additional direct mounts to files placed by calrissian shouldn't be impacted.

Fixes #30
This was referenced Mar 20, 2019
Merged
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dleehr dleehr

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use emptyDir for '/tmp' inside containers Duke-GCB/calrissian
2 participants
@johnbradley @dleehr
0