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

Allow modes z and Z for docker volumes #18998

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

bernt-matthias
Copy link
Contributor

Fixes #18989

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@github-actions github-actions bot added this to the 24.2 milestone Oct 15, 2024
@bernt-matthias bernt-matthias changed the title allow z and Z for docker volumes Allow z and Z for docker volumes Oct 15, 2024
@bernt-matthias bernt-matthias changed the title Allow z and Z for docker volumes Allow modes z and Z for docker volumes Oct 15, 2024
@hexylena
Copy link
Member

  1. thanks so much for the quick fix @bernt-matthias , much appreciated! 2) It looked like there was some special handling around singularity, I'm not sure if that's needed as well or not

@bernt-matthias
Copy link
Contributor Author

special handling around singularity

Will see what I can do.

@bernt-matthias
Copy link
Contributor Author

Is it correct that one can comma separate bind options, e.g. rw,z?

https://docs.docker.com/engine/storage/volumes/#choose-the--v-or---mount-flag: "The third field is optional, and is a comma-separated list of options, such as ro. These options are discussed below." (unfortunately I can't find the "below"...

I was also asking myself if it might be an alternative to hardcode the corresponding binds as additional docker options?

@hexylena
Copy link
Member

Is it correct that one can comma separate bind options, e.g. rw,z?

testing locally i can confirm, rw,z and z,rw are both valid

I also cannot find the below in their docs 🤔

I was also asking myself if it might be an alternative to hardcode the corresponding binds as additional docker options?

I don't believe $default_file_path and $working_directory are available in the additional options field? if they were it would indeed be an option to put it there once you've set <param id="docker_volumes"></param> to be intentionally empty.

@hexylena
Copy link
Member

I ran this patch today and noted the following:

            Error: z: duplicate mount destination

it seems the mounts are being interpreted as a literal z

-v $dir/galaxy/database/jobs_directory/000/503/working:z -v $dir/galaxy/database/objects:z 

rather than being reduplicated like happens for rw/ro

@hexylena
Copy link
Member

Judging by

        >>> str(Volume('A:A:rw', 'docker'))
        'A:rw'

below in the code I would've expected that that was the intended behaviour. However for every :ro mount I have they're duplicated, A:A:ro which I guess also needs to be done for :z?

@bernt-matthias
Copy link
Contributor Author

Guess we could simplify the code by dropping

# /host_path:mode is not (or is no longer?) valid Docker volume syntax
..

@hexylena
Copy link
Member

In my testing it's still generating single element volumes, I'm not sure why. My reading of the code suggests that shouldn't be done anymore but, I'll need to look at it further.

@hexylena
Copy link
Member

with 2d7cd6f, -v /home/user/arbeit/galaxy/galaxy/database/jobs_directory/000/505:ro:rw -v /home/user/arbeit/galaxy/galaxy/database/objects:z:rw

The combination

- preprocess_volumes which creates Volume objects and creates strings
  again in the end and
- creation of DockerVolumes which are only temporary and are soon
  converted to str again

confused me and needed testing IMO. Also code was redundant.
@bernt-matthias
Copy link
Contributor Author

My last commit should (I hope) make it testable. Could you add you test case? Then we may be able to understand what is going on.

@hexylena
Copy link
Member

hexylena commented Nov 4, 2024

No, that's got it completely!

/usr/bin/podman run -e "GALAXY_SLOTS=$GALAXY_SLOTS" -e "GALAXY_MEMORY_MB=$GALAXY_MEMORY_MB" -e "GALAXY_MEMORY_MB_PER_SLOT=$GALAXY_MEMORY_MB_PER_SLOT" -e "HOME=$HOME" -e "_GALAXY_JOB_HOME_DIR=$_GALAXY_JOB_HOME_DIR" -e "_GALAXY_JOB_TMP_DIR=$_GALAXY_JOB_TMP_DIR" -e "TMPDIR=$TMPDIR" -e "TMP=$TMP" -e "TEMP=$TEMP" --name a21df026676e41b38646c0fe14511ef4 \
-v /home/user/arbeit/galaxy/galaxy:/home/user/arbeit/galaxy/galaxy:ro \
-v /home/user/arbeit/emc/infrastructure/files/emc-tools:/home/user/arbeit/emc/infrastructure/files/emc-tools:ro \
-v /home/user/arbeit/galaxy/galaxy/database/jobs_directory/000/506:/home/user/arbeit/galaxy/galaxy/database/jobs_directory/000/506:ro \
-v /home/user/arbeit/galaxy/galaxy/database/jobs_directory/000/506/working:/home/user/arbeit/galaxy/galaxy/database/jobs_directory/000/506/working:z \
-v /home/user/arbeit/galaxy/galaxy/database/objects:/home/user/arbeit/galaxy/galaxy/database/objects:z \
--cpus ${GALAXY_SLOTS:-1} -w /home/user/arbeit/galaxy/galaxy/database/jobs_directory/000/506/working --rm --userns

yeah that looks completely correct now.

Copy link
Member

@hexylena hexylena left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @bernt-matthias

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

Successfully merging this pull request may close these issues.

z option is mishandled when adding custom docker_volumes
2 participants