diff --git a/Makefile b/Makefile index c532a340fcc..c7c4780caea 100644 --- a/Makefile +++ b/Makefile @@ -311,6 +311,7 @@ testunit-bin: done mockgen: \ + mock-cmdrunner \ mock-containerstorage \ mock-criostorage \ mock-lib-config \ diff --git a/internal/config/conmonmgr/conmonmgr.go b/internal/config/conmonmgr/conmonmgr.go index 094b4c818f4..857437c3f54 100644 --- a/internal/config/conmonmgr/conmonmgr.go +++ b/internal/config/conmonmgr/conmonmgr.go @@ -19,14 +19,10 @@ type ConmonManager struct { // this function is heavily based on github.com/containers/common#probeConmon func New(conmonPath string) (*ConmonManager, error) { - return newWithCommandRunner(conmonPath, &cmdrunner.RealCommandRunner{}) -} - -func newWithCommandRunner(conmonPath string, runner cmdrunner.CommandRunner) (*ConmonManager, error) { if !path.IsAbs(conmonPath) { return nil, errors.Errorf("conmon path is not absolute: %s", conmonPath) } - out, err := runner.CombinedOutput(conmonPath, "--version") + out, err := cmdrunner.CombinedOutput(conmonPath, "--version") if err != nil { return nil, errors.Wrapf(err, "get conmon version") } diff --git a/internal/config/conmonmgr/conmonmgr_test.go b/internal/config/conmonmgr/conmonmgr_test.go index d8e2b5fa389..a09731281af 100644 --- a/internal/config/conmonmgr/conmonmgr_test.go +++ b/internal/config/conmonmgr/conmonmgr_test.go @@ -2,6 +2,7 @@ package conmonmgr import ( runnerMock "github.com/cri-o/cri-o/test/mocks/cmdrunner" + "github.com/cri-o/cri-o/utils/cmdrunner" "github.com/golang/mock/gomock" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -16,6 +17,7 @@ var _ = t.Describe("ConmonManager", func() { t.Describe("New", func() { BeforeEach(func() { runner = runnerMock.NewMockCommandRunner(mockCtrl) + cmdrunner.SetMocked(runner) }) It("should fail when path not absolute", func() { // Given @@ -24,7 +26,7 @@ var _ = t.Describe("ConmonManager", func() { ) // When - mgr, err := newWithCommandRunner("", runner) + mgr, err := New("") // Then Expect(err).NotTo(BeNil()) @@ -37,7 +39,7 @@ var _ = t.Describe("ConmonManager", func() { ) // When - mgr, err := newWithCommandRunner(validPath, runner) + mgr, err := New(validPath) // Then Expect(err).NotTo(BeNil()) @@ -50,7 +52,7 @@ var _ = t.Describe("ConmonManager", func() { ) // When - mgr, err := newWithCommandRunner(validPath, runner) + mgr, err := New(validPath) // Then Expect(err).NotTo(BeNil()) @@ -63,7 +65,7 @@ var _ = t.Describe("ConmonManager", func() { ) // When - mgr, err := newWithCommandRunner(validPath, runner) + mgr, err := New(validPath) // Then Expect(err).To(BeNil()) @@ -76,7 +78,7 @@ var _ = t.Describe("ConmonManager", func() { ) // When - mgr, err := newWithCommandRunner(validPath, runner) + mgr, err := New(validPath) // Then Expect(err).To(BeNil()) diff --git a/internal/config/node/systemd.go b/internal/config/node/systemd.go index fdd9ec049ba..3d0eacdb4dc 100644 --- a/internal/config/node/systemd.go +++ b/internal/config/node/systemd.go @@ -4,9 +4,9 @@ package node import ( - "os/exec" "sync" + "github.com/cri-o/cri-o/utils/cmdrunner" "github.com/pkg/errors" ) @@ -37,7 +37,7 @@ func SystemdHasAllowedCPUs() bool { // systemdSupportsProperty checks whether systemd supports a property // It returns an error if it does not. func systemdSupportsProperty(property string) (bool, error) { - output, err := exec.Command("systemctl", "show", "-p", property, "systemd").Output() + output, err := cmdrunner.Command("systemctl", "show", "-p", property, "systemd").Output() if err != nil { return false, errors.Wrapf(err, "check systemd %s", property) } diff --git a/internal/config/nsmgr/nsmgr.go b/internal/config/nsmgr/nsmgr.go index dc64cbc939c..829858d794b 100644 --- a/internal/config/nsmgr/nsmgr.go +++ b/internal/config/nsmgr/nsmgr.go @@ -4,7 +4,6 @@ import ( "bytes" "fmt" "os" - "os/exec" "path/filepath" "strings" "syscall" @@ -12,6 +11,7 @@ import ( nspkg "github.com/containernetworking/plugins/pkg/ns" "github.com/containers/storage/pkg/idtools" "github.com/cri-o/cri-o/utils" + "github.com/cri-o/cri-o/utils/cmdrunner" "github.com/google/uuid" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -120,7 +120,7 @@ func (mgr *NamespaceManager) NewPodNamespaces(cfg *PodNamespacesConfig) ([]Names } logrus.Debugf("Calling pinns with %v", pinnsArgs) - output, err := exec.Command(mgr.pinnsPath, pinnsArgs...).CombinedOutput() + output, err := cmdrunner.Command(mgr.pinnsPath, pinnsArgs...).CombinedOutput() if err != nil { logrus.Warnf("Pinns %v failed: %s (%v)", pinnsArgs, string(output), err) // cleanup the mounts diff --git a/internal/dbusmgr/user.go b/internal/dbusmgr/user.go index 0e0520e9708..98100742f68 100644 --- a/internal/dbusmgr/user.go +++ b/internal/dbusmgr/user.go @@ -6,12 +6,12 @@ import ( "bufio" "bytes" "os" - "os/exec" "path/filepath" "strconv" "strings" systemdDbus "github.com/coreos/go-systemd/v22/dbus" + "github.com/cri-o/cri-o/utils/cmdrunner" dbus "github.com/godbus/dbus/v5" "github.com/opencontainers/runc/libcontainer/userns" "github.com/pkg/errors" @@ -55,7 +55,7 @@ func DetectUID() (int, error) { if !userns.RunningInUserNS() { return os.Getuid(), nil } - b, err := exec.Command("busctl", "--user", "--no-pager", "status").CombinedOutput() + b, err := cmdrunner.Command("busctl", "--user", "--no-pager", "status").CombinedOutput() if err != nil { return -1, errors.Wrapf(err, "could not execute `busctl --user --no-pager status`: %q", string(b)) } @@ -91,7 +91,7 @@ func DetectUserDbusSessionBusAddress() (string, error) { return busAddress, nil } } - b, err := exec.Command("systemctl", "--user", "--no-pager", "show-environment").CombinedOutput() + b, err := cmdrunner.Command("systemctl", "--user", "--no-pager", "show-environment").CombinedOutput() if err != nil { return "", errors.Wrapf(err, "could not execute `systemctl --user --no-pager show-environment`, output=%q", string(b)) } diff --git a/internal/oci/runtime_oci.go b/internal/oci/runtime_oci.go index 9758526071b..0930eea5076 100644 --- a/internal/oci/runtime_oci.go +++ b/internal/oci/runtime_oci.go @@ -20,6 +20,7 @@ import ( "github.com/cri-o/cri-o/pkg/config" "github.com/cri-o/cri-o/server/metrics" "github.com/cri-o/cri-o/utils" + "github.com/cri-o/cri-o/utils/cmdrunner" "github.com/fsnotify/fsnotify" json "github.com/json-iterator/go" rspec "github.com/opencontainers/runtime-spec/specs-go" @@ -137,7 +138,7 @@ func (r *runtimeOCI) CreateContainer(ctx context.Context, c *Container, cgroupPa "args": args, }).Debugf("running conmon: %s", r.config.Conmon) - cmd := exec.Command(r.config.Conmon, args...) // nolint: gosec + cmd := cmdrunner.Command(r.config.Conmon, args...) // nolint: gosec cmd.Dir = c.bundlePath cmd.SysProcAttr = sysProcAttrPlatform() cmd.Stdin = os.Stdin @@ -353,7 +354,7 @@ func (r *runtimeOCI) ExecContainer(ctx context.Context, c *Container, cmd []stri args := []string{rootFlag, r.root, "exec"} args = append(args, "--process", processFile, c.ID()) - execCmd := exec.Command(r.path, args...) // nolint: gosec + execCmd := cmdrunner.Command(r.path, args...) // nolint: gosec if v, found := os.LookupEnv("XDG_RUNTIME_DIR"); found { execCmd.Env = append(execCmd.Env, fmt.Sprintf("XDG_RUNTIME_DIR=%s", v)) } @@ -487,7 +488,7 @@ func (r *runtimeOCI) ExecSyncContainer(ctx context.Context, c *Container, comman "--exec-process-spec", processFile, "--runtime-arg", fmt.Sprintf("%s=%s", rootFlag, r.root)) - cmd := exec.Command(r.config.Conmon, args...) // nolint: gosec + cmd := cmdrunner.Command(r.config.Conmon, args...) // nolint: gosec var stdoutBuf, stderrBuf bytes.Buffer cmd.Stdout = &stdoutBuf @@ -598,7 +599,7 @@ func (r *runtimeOCI) UpdateContainer(ctx context.Context, c *Container, res *rsp return nil } - cmd := exec.Command(r.path, rootFlag, r.root, "update", "--resources", "-", c.ID()) // nolint: gosec + cmd := cmdrunner.Command(r.path, rootFlag, r.root, "update", "--resources", "-", c.ID()) // nolint: gosec var stdout bytes.Buffer var stderr bytes.Buffer cmd.Stdout = &stdout @@ -811,7 +812,7 @@ func (r *runtimeOCI) UpdateContainerStatus(ctx context.Context, c *Container) er } stateCmd := func() (*ContainerState, bool, error) { - cmd := exec.Command(r.path, rootFlag, r.root, "state", c.ID()) // nolint: gosec + cmd := cmdrunner.Command(r.path, rootFlag, r.root, "state", c.ID()) // nolint: gosec if v, found := os.LookupEnv("XDG_RUNTIME_DIR"); found { cmd.Env = append(cmd.Env, fmt.Sprintf("XDG_RUNTIME_DIR=%s", v)) } diff --git a/internal/runtimehandlerhooks/high_performance_hooks.go b/internal/runtimehandlerhooks/high_performance_hooks.go index 209ff4f3de4..f270cd722b0 100644 --- a/internal/runtimehandlerhooks/high_performance_hooks.go +++ b/internal/runtimehandlerhooks/high_performance_hooks.go @@ -15,6 +15,7 @@ import ( "github.com/cri-o/cri-o/internal/log" "github.com/cri-o/cri-o/internal/oci" crioannotations "github.com/cri-o/cri-o/pkg/annotations" + "github.com/cri-o/cri-o/utils/cmdrunner" "github.com/opencontainers/runc/libcontainer/cgroups" "github.com/opencontainers/runc/libcontainer/cgroups/systemd" "github.com/pkg/errors" @@ -263,7 +264,7 @@ func setIRQLoadBalancing(c *oci.Container, enable bool, irqSmpAffinityFile, irqB return nil } // run irqbalance in daemon mode, so this won't cause delay - cmd := exec.Command(irqBalancedName, "--oneshot") + cmd := cmdrunner.Command(irqBalancedName, "--oneshot") additionalEnv := irqBalanceBannedCpus + "=" + newIRQBalanceSetting cmd.Env = append(os.Environ(), additionalEnv) return cmd.Run() diff --git a/internal/runtimehandlerhooks/utils.go b/internal/runtimehandlerhooks/utils.go index 3728db25e20..e2fd6c6f43f 100644 --- a/internal/runtimehandlerhooks/utils.go +++ b/internal/runtimehandlerhooks/utils.go @@ -5,10 +5,10 @@ import ( "fmt" "io/ioutil" "os" - "os/exec" "strings" "unicode" + "github.com/cri-o/cri-o/utils/cmdrunner" "github.com/sirupsen/logrus" "k8s.io/kubernetes/pkg/kubelet/cm/cpuset" ) @@ -138,11 +138,11 @@ func UpdateIRQSmpAffinityMask(cpus, current string, set bool) (cpuMask, bannedCP } func restartIrqBalanceService() error { - return exec.Command("systemctl", "restart", "irqbalance").Run() + return cmdrunner.Command("systemctl", "restart", "irqbalance").Run() } func isServiceEnabled(serviceName string) bool { - cmd := exec.Command("systemctl", "is-enabled", serviceName) + cmd := cmdrunner.Command("systemctl", "is-enabled", serviceName) status, err := cmd.CombinedOutput() if err != nil { logrus.Infof("Service %s is-enabled check returned with: %v", serviceName, err) diff --git a/pkg/config/config.go b/pkg/config/config.go index 6e7308bed07..c932db02fe1 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -34,6 +34,7 @@ import ( "github.com/cri-o/cri-o/server/metrics/collectors" "github.com/cri-o/cri-o/server/useragent" "github.com/cri-o/cri-o/utils" + "github.com/cri-o/cri-o/utils/cmdrunner" "github.com/cri-o/ocicni/pkg/ocicni" selinux "github.com/opencontainers/selinux/go-selinux" "github.com/pkg/errors" @@ -53,6 +54,7 @@ const ( defaultCtrStopTimeout = 30 // seconds defaultNamespacesDir = "/var/run" RuntimeTypeVMBinaryPattern = "containerd-shim-([a-zA-Z0-9\\-\\+])+-v2" + tasksetBinary = "taskset" ) // Config represents the entire set of configuration values that can be set for @@ -969,9 +971,16 @@ func (c *RuntimeConfig) Validate(systemContext *types.SystemContext, onExecution } if c.InfraCtrCPUSet != "" { - if _, err := cpuset.Parse(c.InfraCtrCPUSet); err != nil { + set, err := cpuset.Parse(c.InfraCtrCPUSet) + if err != nil { return errors.Wrap(err, "invalid infra_ctr_cpuset") } + + executable, err := exec.LookPath(tasksetBinary) + if err != nil { + return errors.Wrapf(err, "%q not found in $PATH", tasksetBinary) + } + cmdrunner.PrependCommandsWith(executable, "--cpu-list", set.String()) } if err := c.Workloads.Validate(); err != nil { diff --git a/test/mocks/cmdrunner/cmdrunner.go b/test/mocks/cmdrunner/cmdrunner.go index 4d62ceeb76a..48e22c50458 100644 --- a/test/mocks/cmdrunner/cmdrunner.go +++ b/test/mocks/cmdrunner/cmdrunner.go @@ -5,6 +5,7 @@ package cmdrunnermock import ( + exec "os/exec" reflect "reflect" gomock "github.com/golang/mock/gomock" @@ -52,3 +53,22 @@ func (mr *MockCommandRunnerMockRecorder) CombinedOutput(arg0 interface{}, arg1 . varargs := append([]interface{}{arg0}, arg1...) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CombinedOutput", reflect.TypeOf((*MockCommandRunner)(nil).CombinedOutput), varargs...) } + +// Command mocks base method. +func (m *MockCommandRunner) Command(arg0 string, arg1 ...string) *exec.Cmd { + m.ctrl.T.Helper() + varargs := []interface{}{arg0} + for _, a := range arg1 { + varargs = append(varargs, a) + } + ret := m.ctrl.Call(m, "Command", varargs...) + ret0, _ := ret[0].(*exec.Cmd) + return ret0 +} + +// Command indicates an expected call of Command. +func (mr *MockCommandRunnerMockRecorder) Command(arg0 interface{}, arg1 ...interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + varargs := append([]interface{}{arg0}, arg1...) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Command", reflect.TypeOf((*MockCommandRunner)(nil).Command), varargs...) +} diff --git a/utils/cmdrunner/cmdrunner.go b/utils/cmdrunner/cmdrunner.go index eebb37c1eb3..6a1d1536d8b 100644 --- a/utils/cmdrunner/cmdrunner.go +++ b/utils/cmdrunner/cmdrunner.go @@ -4,12 +4,68 @@ import ( "os/exec" ) +// Use a singleton instance because there are many modules that may want access +// and having it all go through a shared object like the config or server would +// add a lot of complexity. +var commandRunner CommandRunner + +// CommandRunner is an interface for executing commands. +// It gives the option to change the way commands are run server-wide. type CommandRunner interface { + Command(string, ...string) *exec.Cmd CombinedOutput(string, ...string) ([]byte, error) } -type RealCommandRunner struct{} +// prependableCommandRunner is an implementation of CommandRunner. +// It gives the option for all commands that are run to be prepended by another command +// and arguments. +type prependableCommandRunner struct { + prependCmd string + prependArgs []string +} + +// PrependCommandsWith updates the commandRunner singleton to have the configured prepended args and command. +func PrependCommandsWith(prependCmd string, prependArgs ...string) { + commandRunner = &prependableCommandRunner{ + prependCmd: prependCmd, + prependArgs: prependArgs, + } +} + +// CombinedOutput calls CombinedOutput on the defined commandRunner, +// or the default implementation in the exec package if there's no commandRunner defined. +func CombinedOutput(command string, args ...string) ([]byte, error) { + if commandRunner == nil { + return exec.Command(command, args...).CombinedOutput() + } + return commandRunner.CombinedOutput(command, args...) +} + +// CombinedOutput returns the combined output of the command, given the prepended cmd/args that were defined. +func (c *prependableCommandRunner) CombinedOutput(command string, args ...string) ([]byte, error) { + return c.Command(command, args...).CombinedOutput() +} + +// Command calls Command on the defined commandRunner, +// or the default implementation in the exec package if there's no commandRunner defined. +func Command(cmd string, args ...string) *exec.Cmd { + if commandRunner == nil { + return exec.Command(cmd, args...) + } + return commandRunner.Command(cmd, args...) +} -func (c *RealCommandRunner) CombinedOutput(command string, args ...string) ([]byte, error) { - return exec.Command(command, args...).CombinedOutput() +// Command creates an exec.Cmd object. If prependCmd is defined, the command will be prependCmd +// and the args will be prependArgs + cmd + args. +// Otherwise, cmd and args will be as inputted. +func (c *prependableCommandRunner) Command(cmd string, args ...string) *exec.Cmd { + realCmd := cmd + realArgs := args + if c.prependCmd != "" { + realCmd = c.prependCmd + realArgs = c.prependArgs + realArgs = append(realArgs, cmd) + realArgs = append(realArgs, args...) + } + return exec.Command(realCmd, realArgs...) } diff --git a/utils/cmdrunner/cmdrunner_test.go b/utils/cmdrunner/cmdrunner_test.go new file mode 100644 index 00000000000..c83e9391cb2 --- /dev/null +++ b/utils/cmdrunner/cmdrunner_test.go @@ -0,0 +1,53 @@ +package cmdrunner_test + +import ( + "os/exec" + + "github.com/cri-o/cri-o/utils/cmdrunner" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = t.Describe("CommandRunner", func() { + It("command should not prepend if not configured", func() { + // Given + cmd := "ls" + baseline, err := exec.Command(cmd).CombinedOutput() + Expect(err).To(BeNil()) + + // When + output, err := cmdrunner.CombinedOutput(cmd) + Expect(err).To(BeNil()) + + // Then + Expect(output).To(Equal(baseline)) + }) + It("command should prepend if configured", func() { + // Given + cmd := "ls" + cmdrunner.PrependCommandsWith("which") + baseline, err := exec.Command(cmd).CombinedOutput() + Expect(err).To(BeNil()) + + // When + output, err := cmdrunner.CombinedOutput(cmd) + Expect(err).To(BeNil()) + + // Then + Expect(output).NotTo(Equal(baseline)) + }) + It("command should not prepend if only args are configured", func() { + // Given + cmd := "ls" + cmdrunner.PrependCommandsWith("", "-l") + baseline, err := exec.Command(cmd).CombinedOutput() + Expect(err).To(BeNil()) + + // When + output, err := cmdrunner.CombinedOutput(cmd) + Expect(err).To(BeNil()) + + // Then + Expect(output).To(Equal(baseline)) + }) +}) diff --git a/utils/cmdrunner/cmdrunner_test_inject.go b/utils/cmdrunner/cmdrunner_test_inject.go new file mode 100644 index 00000000000..f056d8d653e --- /dev/null +++ b/utils/cmdrunner/cmdrunner_test_inject.go @@ -0,0 +1,15 @@ +//go:build test +// +build test + +// All *_inject.go files are meant to be used by tests only. Purpose of this +// files is to provide a way to inject mocked data into the current setup. + +package cmdrunner + +import ( + runnerMock "github.com/cri-o/cri-o/test/mocks/cmdrunner" +) + +func SetMocked(runner *runnerMock.MockCommandRunner) { + commandRunner = runner +} diff --git a/utils/cmdrunner/suite_test.go b/utils/cmdrunner/suite_test.go new file mode 100644 index 00000000000..55d950677e9 --- /dev/null +++ b/utils/cmdrunner/suite_test.go @@ -0,0 +1,26 @@ +package cmdrunner_test + +import ( + "testing" + + . "github.com/cri-o/cri-o/test/framework" + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +// TestCommandRunner runs the created specs +func TestCommandRunner(t *testing.T) { + RegisterFailHandler(Fail) + RunFrameworkSpecs(t, "CommandRunner") +} + +var t *TestFramework + +var _ = BeforeSuite(func() { + t = NewTestFramework(NilFunc, NilFunc) + t.Setup() +}) + +var _ = AfterSuite(func() { + t.Teardown() +}) diff --git a/utils/utils.go b/utils/utils.go index 73f22298ab1..0ef5c19c455 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -8,7 +8,6 @@ import ( "io" "io/ioutil" "os" - "os/exec" "path/filepath" "runtime" "strconv" @@ -18,6 +17,7 @@ import ( "github.com/containers/podman/v3/pkg/lookup" "github.com/cri-o/cri-o/internal/dbusmgr" + "github.com/cri-o/cri-o/utils/cmdrunner" securejoin "github.com/cyphar/filepath-securejoin" "github.com/opencontainers/runc/libcontainer/user" "github.com/pkg/errors" @@ -32,7 +32,7 @@ import ( // ExecCmd executes a command with args and returns its output as a string along // with an error, if any func ExecCmd(name string, args ...string) (string, error) { - cmd := exec.Command(name, args...) + cmd := cmdrunner.Command(name, args...) var stdout bytes.Buffer var stderr bytes.Buffer cmd.Stdout = &stdout