-
Notifications
You must be signed in to change notification settings - Fork 429
Filter already tracked directories from ldcache update #1403
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
base: main
Are you sure you want to change the base?
Conversation
|
/cherry-pick release-1.18 |
0b4a83b to
d0a1221
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
2 non Blocking comments
internal/ldconfig/ldconfig.go
Outdated
| return SafeExec(ldconfigPath, args, nil) | ||
| } | ||
|
|
||
| func (l *Ldconfig) filterDirectories(configFilePath string, directories ...string) ([]string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: If the function doesn't need to access or modify the state of an Ldconfig object, why not to be a regular standalone function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was originally accessing a member, but I can remove it.
d0a1221 to
2585da9
Compare
Signed-off-by: Evan Lezar <[email protected]>
2585da9 to
5b1d918
Compare
| // Explicitly specify using /etc/ld.so.conf since the host's ldconfig may | ||
| // be configured to use a different config file by default. | ||
| configFilePath := "/etc/ld.so.conf" | ||
| filteredDirectories, err := filterDirectories(configFilePath, directories...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Jason from Uber AI Infra here - we filed the ticket) What will be passed in as directories?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directories contains the container paths of parent directories for any CUDA user-mode driver libraries that are mounted from the host. Note that at present the contianer paths of the libraries are the same as the host path.
On an ubuntu based system the list is typically:
/usr/lib/${ARCH_SPECIFIC_LIB_DIR}/
/usr/lib/${ARCH_SPECIFIC_LIB_DIR}/vdpau
which results in the following CDI hook:
- hookName: createContainer
path: /usr/bin/nvidia-cdi-hook
args:
- nvidia-cdi-hook
- update-ldcache
- --folder
- /lib/x86_64-linux-gnu
- --folder
- /lib/x86_64-linux-gnu/vdpau
env:
- NVIDIA_CTK_DEBUG=false
If you wanted to know what the list is on your system, you could run the nvidia-ctk cdi generate command and check the output for the generated update-ldcache hook.
|
@jasonzlai do you have a simple reproducer Dockerfile that we could add to our test suite to address this? |
This change ensures that directories that are already included in the ld.conf are not added to the NVCR-specific config files. The intent is to not inadventently promote existing directories to a higher priority.
See #123 where this was already an issue in the libnvidia-container-based implementation.