-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix(xunix): also mount shared symlinked shared object files #123
Conversation
out, err = integrationtest.ExecInnerContainer(t, pool, integrationtest.ExecConfig{ | ||
ContainerID: resource.Container.ID, | ||
Cmd: []string{"cat", "/sys/fs/cgroup/memory/memory.limit_in_bytes"}, | ||
}) | ||
require.NoError(t, err) | ||
require.Equal(t, expectedMemoryLimit, strings.TrimSpace(string(out))) |
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.
This only gets run when cgroup v2 fails or is unavailable, do we ever expect that to be the case with our supported versions? Since this test code only gets run when cgroup v2 fails, will it ever get run in CI or on our dev machines?
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.
will it ever get run in CI or on our dev machines?
It ran on my dev machine ;-)
All jokes aside, it's a code path that won't ever really work inside a sysbox container on our dogfood boxen. But I think we do need to think about cgroupv2 given the following:
- Ubuntu 22.04 is supported by sysbox
- Ubuntu enables cgroupv2 by default from 22.04 onwards
- Kubernetes made cgroupv2 support GA from 1.25 onwards
It would be a good idea to add CI to test across multiple distros where you get cgroupv1 and cgroupv2 by default. I'd prefer to keep that out of the scope of this PR though.
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.
Nice job getting to the bottom of this, LGTM!
|
||
.PHONY: test | ||
test: | ||
go test -v -count=1 ./... |
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.
Note: setting -count ignores test caching, if that's intended then 👍🏻
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.
yep, intended
f9a87ea
to
6862e15
Compare
Signed-off-by: Cian Johnston <[email protected]>
Signed-off-by: Cian Johnston <[email protected]>
Signed-off-by: Cian Johnston <[email protected]>
Signed-off-by: Cian Johnston <[email protected]>
Co-authored-by: Dean Sheather <[email protected]> Signed-off-by: Cian Johnston <[email protected]>
6862e15
to
7d91c01
Compare
Summary
Relates to #111
Container runtimes like the NVIDIA container toolkit will inject library dependencies inside containers they create. More recent versions of these runtimes / operators appear to mount the libraries with
$library.so.$NVIDIA_DRIVER_VERISON
and symlink$library.so
to those locations.Example:
For more details, see here: https://gist.github.com/johnstcn/1caae2e79eea8788dc7f31e3db4326eb
Changes made:
*.so
were mounted inside the inner container. Now, all files matching.*.so(\.[0-9\.]+)?
are included.cgroupns=private
)Environment
shiftfs
installedNVIDIA-Linux-x86_64-550.90.07.run
)Before
After