While doing the previous fix, I went over all the tests in this file and
made sure they were named correctly. This patch just adds a small
sentence to clarify the intent, and does some minor improvements to some
other test names.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
There was a typo and instead of "empty" we should have used "non-empty".
Let's add a small sentence explaining the intent (like other tests in
this file) and let's highlight what we expect to happen in this test (to
ignore the listenerPath).
Fixes: #3415
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
golangci-lint-action v3 no longer installs golang itself, and the
version that comes with Ubuntu is not new/good enough.
Install go 1.17.x explicitly.
Introduce GO_VERSION environment variable to avoid duplication,
and use it instead of 1.x in other places, so that implicit go update
won't bring some unexpected failures.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently, "systemctl --user --no-pager show-environment" is useless
without DBUS_SESSION_BUS_ADDRESS or XDG_RUNTIME_DIR set:
$ echo $DBUS_SESSION_BUS_ADDRESS, $XDG_RUNTIME_DIR
unix:path=/run/user/1000/bus, /run/user/1000
$ systemctl --user --no-pager show-environment | grep DBUS_SESS
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus
$ unset DBUS_SESSION_BUS_ADDRESS
$ systemctl --user --no-pager show-environment | grep DBUS_SESS
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus
$ unset XDG_RUNTIME_DIR
$ systemctl --user --no-pager show-environment | grep DBUS_SESS
Failed to connect to bus: $DBUS_SESSION_BUS_ADDRESS and $XDG_RUNTIME_DIR not defined (consider using --machine=<user>@.host --user to connect to bus of other user)
So, it does not make sense to try it to get the address.
Also, it does not make sense to suggest "systemctl --user start dbus"
either, for the same reason, so remove that suggestion from the error
message text.
Since DBUS_SESSION_BUS_ADDRESS environment variable, on which the code
relies, is et by dbus-run-session (or dbus-launch, or something similar
that is supposed to be run during the login process), add a suggestion
to re-login.
Finally, fix the following linter warning:
> error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently, not all files listed in /sys/kernel/cgroup/delegate must
exist in every cgroup, so we should ignore ENOENT.
Dot not ignore ENOENT on the directory itself though.
Change cgroupFilesToChown to not return ".", and refactor it to not do
any dynamic slice appending in case we're using the default built-in
list of files.
Fixes: 35d20c4e0
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It is questionable whether runc list should return an empty list of
containers when non-existent --root is specified or not.
The current behavior is the directory is always created and then the
empty list of container is shown.
To my mind, specifying a non-existent root is an error and should be
reported as such. This is what this patch does.
For backward compatibility, if --root is not set (i.e. a default is
used), ENOENT is not reported.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. In case --root option is not provided, do nothing.
2. Instead of checking if root value is empty string, check it after
filepath.Abs, and reject "/". Improve docstring while at it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Variable xdgRuntimeDir is only checked to be non-empty. Change it to
a boolean.
2. Refactor so that os.Getenv is not called twice.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There is a mix of styles when handling CLI commands. In most cases we
return an error, which is handled from app.Run in main.go (it calls
fatal if there is an error).
In a few cases, though, we call fatal(err) from random places.
Let's be consistent and always return an error. The only exception is
runc exec, which needs to exit with a particular exit code.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The value of root is already an absolute path since commit
ede8a86ec1, so it does not make sense to call filepath.Abs()
again.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Don't require CAT or MBA because we don't detect those correctly (we
don't support L2 or L3DATA/L3CODE for example, and in the future
possibly even more). With plain "ClosId mode" we don't really care: we
assign the container to a pre-configured CLOS without trying to do
anything smarter.
Moreover, this was a duplicate/redundant check anyway, as for CAT and
MBA there is another specific sanity check that is done if L3 or MB
is specified in the config.
Signed-off-by: Markus Lehtonen <markus.lehtonen@intel.com>
1. This valid warning is reported by shellcheck v0.8.0:
In tests/integration/helpers.bash line 38:
KERNEL_MINOR="${KERNEL_VERSION#$KERNEL_MAJOR.}"
^-----------^ SC2295 (info): Expansions inside ${..} need to be quoted separately, otherwise they match as patterns.
Did you mean:
KERNEL_MINOR="${KERNEL_VERSION#"$KERNEL_MAJOR".}"
Fix this.
2. These (invalid) warnings are also reported by the new version:
In tests/integration/events.bats line 13:
@test "events --stats" {
^-- SC2030 (info): Modification of status is local (to subshell caused by @bats test).
In tests/integration/events.bats line 41:
[ "$status" -eq 0 ]
^-----^ SC2031 (info): status was modified in a subshell. That change might be lost.
Basically, this is happening because shellcheck do not really track
the call tree and/or local variables. This is a known (and reported)
deficiency, and the alternative to disabling these warnings is moving
the code around, which is worse due to more changes in git history.
So we have to silence/disable these.
3. Update shellcheck to 0.8.0.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Foreground runc exec and runc run forwards all the signals (that it can)
to the process being run.
Since Go 1.14, go runtime uses SIGURG for async preemptive scheduling.
This means that runc regularly receives SIGURG and, in case of
foreground runc run/exec, it gets forwarded to the container process.
For example:
[kir@kir-rhat runc]$ sudo ./runc --debug exec xx67 sleep 1m
...
DEBU[0000] child process in init()
DEBU[0000] setns_init: about to exec
DEBU[0000]signals.go:102 main.(*signalHandler).forward() sending signal to process urgent I/O condition
DEBU[0000]signals.go:102 main.(*signalHandler).forward() sending signal to process urgent I/O condition
DEBU[0000]signals.go:102 main.(*signalHandler).forward() sending signal to process urgent I/O condition
...
Or, with slightly better debug messages from commit 58c1ff39a5:
DEBU[0000]signals.go:102 main.(*signalHandler).forward() forwarding SIGURG to 819784
DEBU[0000]signals.go:102 main.(*signalHandler).forward() forwarding SIGURG to 819784
Obviously, this signal is an internal implementation detail of Go
runtime, and should not be forwarded to the container process.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Remove intelrtd.Manager interface, since we only have a single
implementation, and do not expect another one.
Rename intelRdtManager to Manager, and modify its users accordingly.
Remove NewIntelRdtManager from factory.
Remove IntelRdtfs. Instead, make intelrdt.NewManager return nil if the
feature is not available.
Remove TestFactoryNewIntelRdt as it is now identical to TestFactoryNew.
Add internal function newManager to be used for tests (to make sure
some testing is done even when the feature is not available in
kernel/hardware).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
TestGetContainerStats test a function that is smaller than the test
itself, and only calls a couple of other functions (which are
represented by mocks). It does not make sense to have it.
mockIntelRdtManager is only needed for TestGetContainerStats
and TestGetContainerState, which basically tests that Path
is called. Also, it does not make much sense to have it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since we are looking up the path to newuidmap/newgidmap in one context,
and executing those in another (libct/nsenter), it might make sense to
use a stricter rules for looking up path to those binaries.
Practically it means that if someone wants to use custom newuidmap and
newgidmap binaries from $PATH, it would be impossible to use these from
the current directory by means of PATH=.:$PATH; instead one would have
to do something like PATH=$(pwd):$PATH.
See https://go.dev/blog/path-security for background.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These were introduced in commit d8b669400 back in 2017, with a TODO
of "make binary names configurable". Apparently, everyone is happy with
the hardcoded names. In fact, they *are* configurable (by prepending the
PATH with a directory containing own version of newuidmap/newgidmap).
Now, these binaries are only needed in a few specific cases (when
rootless is set etc.), so let's look them up only when needed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We only have one implementation of config validator, which is always
used. It makes no sense to have Validator interface.
Having validate.Validator field in Factory does not make sense for all
the same reasons.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Those are *always* /proc/self/exe init, and it does not make sense
to ever change these. More to say, if InitArgs option func (removed
by this commit) is used to change these parameters, it will break
things, since "init" is hardcoded elsewhere.
Remove this.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 029b73c1b replaced a regular expression with code checking the
characters. Despite what the comment said about ASCII, the check was
performed rune by rune, not byte by byte.
Note the check was still correct, basically comparing int32's, but the
byte by byte way is a tad faster and more straightforward. The change
also fixes the issue of a misleading comment.
Benchmark before/after shows a modest improvement:
name old time/op new time/op delta
CheckPropertyName-4 164ns ± 2% 123ns ± 2% -24.73% (p=0.029 n=4+4)
name old alloc/op new alloc/op delta
CheckPropertyName-4 96.0B ± 0% 64.0B ± 0% -33.33% (p=0.029 n=4+4)
name old allocs/op new allocs/op delta
CheckPropertyName-4 6.00 ± 0% 4.00 ± 0% -33.33% (p=0.029 n=4+4)
Fixes: 029b73c1b
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 643f8a2b40 renamed isValidName to checkPropertyName, but fell
short of renaming its test and benchmark. Fix that.
Fixes: 643f8a2b40
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Akihiro Suda