As of previous commit, this is implied in a particular scenario. In
fact, this is the one and only scenario that justifies the use of -a.
Drop the option from the documentation. For backward compatibility, do
recognize it, and retain the feature of ignoring the "container is
stopped" error when set.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
By default, the container has its own PID namespace, and killing (with
SIGKILL) its init process from the parent PID namespace also kills all
the other processes.
Obviously, it does not work that way when the container is sharing its
PID namespace with the host or another container, since init is no
longer special (it's not PID 1). In this case, killing container's init
will result in a bunch of other processes left running (and thus the
inability to remove the cgroup).
The solution to the above problem is killing all the container
processes, not just init.
The problem with the current implementation is, the killing logic is
implemented in libcontainer's initProcess.wait, and thus only available
to libcontainer users, but not the runc kill command (which uses
nonChildProcess.kill and does not use wait at all). So, some workarounds
exist:
- func destroy(c *Container) calls signalAllProcesses;
- runc kill implements -a flag.
This code became very tangled over time. Let's simplify things by moving
the killing all processes from initProcess.wait to container.Signal,
and documents the new behavior.
In essence, this also makes `runc kill` to automatically kill all container
processes when the container does not have its own PID namespace.
Document that as well.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When someone is using libcontainer to start and kill containers from a
long lived process (i.e. the same process creates and removes the
container), initProcess.wait method is used, which has a kludge to work
around killing containers that do not have their own PID namespace.
The code that checks for own PID namespace is not entirely correct.
To be exact, it does not set sharePidns flag when the host/caller PID
namespace is implicitly used. As a result, the above mentioned kludge
does not work.
Fix the issue, add a test case (which fails without the fix).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There are two very distinct usage scenarios for signalAllProcesses:
* when used from the runc binary ("runc kill" command), the processes
that it kills are not the children of "runc kill", and so calling
wait(2) on each process is totally useless, as it will return ECHLD;
* when used from a program that have created the container (such as
libcontainer/integration test suite), that program can and should call
wait(2), not the signalling code.
So, the child reaping code is totally useless in the first case, and
should be implemented by the program using libcontainer in the second
case. I was not able to track down how this code was added, my best
guess is it happened when this code was part of dockerd, which did not
have a proper child reaper implemented at that time.
Remove it, and add a proper documentation piece.
Change the integration test accordingly.
PS the first attempt to disable the child reaping code in
signalAllProcesses was made in commit bb912eb00c, which used a
questionable heuristic to figure out whether wait(2) should be called.
This heuristic worked for a particular use case, but is not correct in
general.
While at it:
- simplify signalAllProcesses to use unix.Kill;
- document (container).Signal.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is roughly the same as TestPIDHostInitProcessWait in libct/int,
except that here we use separate processes to create and to kill a
container, so the processes inside a container are not children of "runc kill", and
also we hit different codepaths (nonChildProcess.signal rather than
initProcess.signal).
One other thing is, rootless is also tested.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It seems that set -x was temporarily added as a debug measure, but
slipped into the final commit.
Remove it, for the sake of test logs brevity.
Fixes: 9f656dbb11
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test is occasionally failing on CS9.
The test case always takes about 7 seconds on my laptop (decreasing
memory, using a different memory eater in shell etc. doesn't help).
Increase the number of iterations from 10 to 30 to make sure we don't
see any flakes.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Fix some missing punctuation, use proper case.
2. Remove "runc init" (previously removed from "runc --help" by commit
7a0302f0d7).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As of today, vagrant stopped working, my best guess is due to bad
caching. Here's an excerpt from logs:
...
vagrant plugin install vagrant-libvirt
Installing the 'vagrant-libvirt' plugin. This can take a few minutes...
Building native extensions. This could take a while...
Installed the plugin 'vagrant-libvirt (0.12.1)'!
...
uname -s ; cat Vagrantfile.$DISTRO
Linux
...
Downloaded 481Mb in 4.096201s.
Cache hit for vagrant-8be35383dc00f23d080ff00b2a724c938d650254861f26b67624c28e3fe5e6ae!
...
Vagrant failed to initialize at a very early stage:
The plugins failed to initialize correctly. This may be due to manual
modifications made within the Vagrant home directory.
...
Error message given during initialization: Unable to resolve dependency:
user requested 'vagrant-libvirt (= 0.12.0)'
...
The problem is, vagrant cache overwrites newer plugin with an older one.
Let's only cache the downloaded image.
Also, change the cache fingerprint script (remove "Linux").
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently, TestInit sets up logrus, and init uses it to log an error
from StartInitialization(). This is solely used by TestExecInError
to check that error returned from StartInitialization is the one it
expects.
Note that the very same error is communicated to the runc init parent
and is ultimately returned by container.Run(), so checking what
StartInitialization returned is redundant.
Remove logrus setup and use from TestMain/init.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is a cosmetic change to improve code readability, making it easier
to distinguish between a local error and the error being returned.
While at it, rename e to err (it was originally called e to not clash
with returned error named err) and ee to err2.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Instead of having newContainerInit return an interface, and let its
caller call Init(), it is easier to call Init directly.
Do that, and rename newContainerInit to containerInit.
I think it makes the code more readable and straightforward.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit e3cf217cf1 actions/setup-go@v4 uses caching
implicitly, and olangci/golangci-lint-action also uses caching.
These two caches clash, resulting in multiple warnings in CI logs.
The official golangci-lint-action solution is to disable caching
for setup-go job (see [1]). Do the same.
[1] https://github.com/golangci/golangci-lint-action/pull/704
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As of today, installing fails with
> libc6:i386 : Depends: libgcc-s1:i386 but it is not going to be installed
Add the package explicitly to work around that.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The first kludge is not needed since the switch to Ubuntu 22.04 in
commit 953e1cc48.
The second one is not needed since Go 1.20.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Adding fd field to mountConfig was not a good thing since mountConfig
contains data that is not specific to a particular mount, while fd is
a mount entry attribute.
Introduce mountEntry structure, which embeds configs.Mount and adds
srcFd to replace the removed mountConfig.fd.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Simplify mount call by removing the procfd argument, and use the new
mount() where procfd is not used. Now, the mount() arguments are the
same as for unix.Mount.
2. Introduce a new mountViaFDs function, which is similar to the old
mount(), except it can take procfd for both source and target.
The new arguments are called srcFD and dstFD.
3. Modify the mount error to show both srcFD and dstFD so it's clear
which one is used for which purpose. This fixes the issue of having
a somewhat cryptic errors like this:
> mount /proc/self/fd/11:/sys/fs/cgroup/systemd (via /proc/self/fd/12), flags: 0x20502f: operation not permitted
(in which fd 11 is actually the source, and fd 12 is the target).
After this change, it looks like
> mount src=/proc/self/fd/11, dst=/sys/fs/cgroup/systemd, dstFD=/proc/self/fd/12, flags=0x20502f: operation not permitted
so it's clear that 12 is a destination fd.
4. Fix the mountViaFDs callers to use dstFD (rather than procfd) for the
variable name.
5. Use srcFD where mountFd is set.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When given an environment variable that is invalid, it's not a good idea
to output the contents in case they are supposed to be private (though
such a container wouldn't start anyway so it seems unlikely there's a
real way to use this to exfiltrate environment variables you didn't
already know).
Reported-by: Carl Henrik Lunde <chlunde@ifi.uio.no>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
1. Use strings.TrimPrefix instead of fmt.Sscanf and simplify the code.
2. Add a test case and a benchmark.
The benchmark shows some improvement, compared to the old
implementation:
name old time/op new time/op delta
FindDeviceGroup-4 39.7µs ± 2% 26.8µs ± 2% -32.63% (p=0.008 n=5+5)
name old alloc/op new alloc/op delta
FindDeviceGroup-4 6.08kB ± 0% 4.23kB ± 0% -30.39% (p=0.008 n=5+5)
name old allocs/op new allocs/op delta
FindDeviceGroup-4 117 ± 0% 6 ± 0% -94.87% (p=0.008 n=5+5)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
A version of vagrant available from the stock repos (2.2.19) is too old
and contains a bug that prevents downloading Fedora 38 image (see [1]).
Use packages from hashicorp repo, which currently has vagrant 2.3.4.
This resolves the problem of downloading the latest Fedora image.
Also, vagrant-libvirt plugin from Ubuntu repos is not working with
vagrant from hashicorp, so switch to using "vagrant plugin install".
The downside it, this takes extra 4 minutes or so in our CI, and I
am not sure how to cache it or speed it up.
[1] https://github.com/opencontainers/runc/pull/3835#issuecomment-1519321619
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin