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

a few more bind/copy fixes #41

Merged
merged 9 commits into from
Mar 16, 2023
Merged

a few more bind/copy fixes #41

merged 9 commits into from
Mar 16, 2023

Conversation

lastephey
Copy link
Collaborator

Changes bind to rbind to address #40

Another lingering change needed to fix the wildcard bind-mount logic in #36

There are some situations where we might always have something to copy or bind (like in the NCCL module), so I added logic to handle this via if mod["copy"] is not None: etc

@lastephey lastephey requested review from scanon and danfulton March 15, 2023 01:51
@danfulton
Copy link
Contributor

WIldcards in the target path are not being handled and mapped. To illustrate, I added some print statements to do_plugin() and passed the following values:

rp = "/root/path"
mod = {
    "copy": [
        "../01-mpich.conf:/etc/ld.so.conf.d/02-mpich.conf",
        "/global/homes/d/dfulton/tmp/opt/*/foo/libbar:/libs/*/libbbar"
    ],
    "bind": [
        "/dev/xpmem:/dev/xpmem",
        "/dev/cxi*:/dev/cxi*",
        "/opt/cray/libfabric/*/lib64/libfabric:/opt/cray/libfabric/*/lib64/libfabric",
    ]
}
modulesd = "/etc/podman_hpc/modules.d"
do_plugin(rp, mod, modulesd)

which has the output

copy :
	../01-mpich.conf:/etc/ld.so.conf.d/02-mpich.conf
		/etc/podman_hpc/01-mpich.conf	--->	/root/path/etc/ld.so.conf.d/02-mpich.conf

	/global/homes/d/dfulton/tmp/opt/*/foo/libbar:/libs/*/libbbar
		/global/homes/d/dfulton/tmp/opt/dir2/foo/libbar	--->	/root/path/libs/*/libbbar/libbar
		/global/homes/d/dfulton/tmp/opt/dir1/foo/libbar	--->	/root/path/libs/*/libbbar/libbar

bind :
	/dev/xpmem:/dev/xpmem
		/dev/xpmem	--->	/root/path/dev/xpmem

	/dev/cxi*:/dev/cxi*
		/dev/cxi1	--->	/root/path/dev/cxi*/cxi1
		/dev/cxi0	--->	/root/path/dev/cxi*/cxi0

	/opt/cray/libfabric/*/lib64/libfabric:/opt/cray/libfabric/*/lib64/libfabric
		/opt/cray/libfabric/1.15.2.0/lib64/libfabric	--->	/root/path/opt/cray/libfabric/*/lib64/libfabric/libfabric

Since copy and bind use the same mapping rules, I am going to put the wildcard parsing logic into the resolve_src routine (and probably rename it). Will update shortly.

@danfulton
Copy link
Contributor

To summarize the changes here:

  1. Copy/Bind rules should now take the format source[:destination].
  2. Copy can now copy directory trees.
  3. Environment variables as well as ~ and ~user shortcuts are now allowed in copy/bind rules. They will always be evaluated on the host, regardless of if they are in the source or destination part of the rule.
  4. The source may be an absolute or relative path. If it is relative it will be interpreted relative to the modules.d.
  5. If destination is not present, it will be the same as the source path.
  6. A trailing path separator / in destination will always means it is treated as a directory.
  7. The destination may use the glob * pattern captured from source. In this case, the source must contain exactly one glob *. The destination may use the glob * pattern any number of times.
  8. If multiple files match source glob and the destination does not use the glob * pattern, then the destination will be treated as a directory. The unique object name will be the source path relative to the longest common prefix shared by all the glob matches.
  9. In all other cases, the destination will be treated as including the destination object name.

@danfulton danfulton merged commit cc9789a into main Mar 16, 2023
# determine how to construct absolute path to destination object
# if no destination rule is given, use the path from source rule
if rd == "":
dest = lambda src: os.path.join(root_path, src[1:])
Copy link
Member

Choose a reason for hiding this comment

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

where are lambdas being used?

Copy link
Contributor

@danfulton danfulton Mar 17, 2023

Choose a reason for hiding this comment

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

The dictionary comprehension in the return line on 139.

return {src: os.path.normpath(dest(src)) for src in iglob(rs)}

If you don't like that, we could either wrap the whole if block in a loop (but then the if block would be executed unnecessarily for every file in the glob), or the if block could set a enum parameter, and the dest function could be defined with a def and take the enum as an argument.

We could also use def inside the if block, but I thought that would be too verbose.

Copy link
Member

@scanon scanon left a comment

Choose a reason for hiding this comment

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

I had a few questions

@danfulton
Copy link
Contributor

@scanon if there are changes you want, let me know, and I'm happy to make a revision PR

This was referenced Mar 17, 2023
@scanon scanon deleted the lastephey/change-bind branch March 17, 2023 23:18
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.

3 participants