From 001a3a23342faebea92624deabe98d319b7bdc8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gu=C3=B0mundur=20Bjarni=20=C3=93lafsson?= Date: Tue, 16 May 2023 15:55:25 +0000 Subject: [PATCH 1/5] Make NUMA node to be optional Signed-off-by: Gudmundur Bjarni Olafsson --- handlers.go | 4 ---- jailer.go | 17 +++++++++++------ 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/handlers.go b/handlers.go index d9d022d8..24053b36 100644 --- a/handlers.go +++ b/handlers.go @@ -109,10 +109,6 @@ var JailerConfigValidationHandler = Handler{ return fmt.Errorf("UID must be specified when using jailer mode") } - if m.Cfg.JailerCfg.NumaNode == nil { - return fmt.Errorf("ID must be specified when using jailer mode") - } - return nil }, } diff --git a/jailer.go b/jailer.go index e261fae6..229b349d 100644 --- a/jailer.go +++ b/jailer.go @@ -102,7 +102,6 @@ type JailerCommandBuilder struct { uid int gid int execFile string - node int // optional params chrootBaseDir string @@ -110,6 +109,7 @@ type JailerCommandBuilder struct { daemonize bool firecrackerArgs []string cgroupVersion string + node *int stdin io.Reader stdout io.Writer @@ -139,9 +139,11 @@ func (b JailerCommandBuilder) Args() []string { args = append(args, "--gid", strconv.Itoa(b.gid)) args = append(args, "--exec-file", b.execFile) - if cpulist := getNumaCpuset(b.node); len(cpulist) > 0 { - args = append(args, "--cgroup", fmt.Sprintf("cpuset.mems=%d", b.node)) - args = append(args, "--cgroup", fmt.Sprintf("cpuset.cpus=%s", cpulist)) + if b.node != nil { + if cpulist := getNumaCpuset(*b.node); len(cpulist) > 0 { + args = append(args, "--cgroup", fmt.Sprintf("cpuset.mems=%d", b.node)) + args = append(args, "--cgroup", fmt.Sprintf("cpuset.cpus=%s", cpulist)) + } } if len(b.cgroupVersion) > 0 { @@ -208,7 +210,7 @@ func (b JailerCommandBuilder) WithExecFile(path string) JailerCommandBuilder { // WithNumaNode uses the specfied node for the jailer. This represents the numa // node that the process will get assigned to. func (b JailerCommandBuilder) WithNumaNode(node int) JailerCommandBuilder { - b.node = node + b.node = &node return b } @@ -344,7 +346,6 @@ func jail(ctx context.Context, m *Machine, cfg *Config) error { WithID(cfg.JailerCfg.ID). WithUID(*cfg.JailerCfg.UID). WithGID(*cfg.JailerCfg.GID). - WithNumaNode(*cfg.JailerCfg.NumaNode). WithExecFile(cfg.JailerCfg.ExecFile). WithChrootBaseDir(cfg.JailerCfg.ChrootBaseDir). WithDaemonize(cfg.JailerCfg.Daemonize). @@ -365,6 +366,10 @@ func jail(ctx context.Context, m *Machine, cfg *Config) error { builder = builder.WithStdin(stdin) } + if cfg.JailerCfg.NumaNode != nil { + builder = builder.WithNumaNode(*cfg.JailerCfg.NumaNode) + } + m.cmd = builder.Build(ctx) if err := cfg.JailerCfg.ChrootStrategy.AdaptHandlers(&m.Handlers); err != nil { From bf6a083237b190bfc88caecce76ce785c3fcc53f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gu=C3=B0mundur=20Bjarni=20=C3=93lafsson?= Date: Tue, 16 May 2023 15:56:21 +0000 Subject: [PATCH 2/5] Allow cgroup arguments to be configurable Signed-off-by: Gudmundur Bjarni Olafsson --- jailer.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/jailer.go b/jailer.go index 229b349d..1f46e9ad 100644 --- a/jailer.go +++ b/jailer.go @@ -86,6 +86,8 @@ type JailerConfig struct { // CgroupVersion is the version of the cgroup filesystem to use. CgroupVersion string + CgroupArgs []string + // Stdout specifies the IO writer for STDOUT to use when spawning the jailer. Stdout io.Writer // Stderr specifies the IO writer for STDERR to use when spawning the jailer. @@ -109,6 +111,7 @@ type JailerCommandBuilder struct { daemonize bool firecrackerArgs []string cgroupVersion string + cgroupArgs []string node *int stdin io.Reader @@ -139,6 +142,10 @@ func (b JailerCommandBuilder) Args() []string { args = append(args, "--gid", strconv.Itoa(b.gid)) args = append(args, "--exec-file", b.execFile) + for _, cgroupArg := range b.cgroupArgs { + args = append(args, "--cgroup", cgroupArg) + } + if b.node != nil { if cpulist := getNumaCpuset(*b.node); len(cpulist) > 0 { args = append(args, "--cgroup", fmt.Sprintf("cpuset.mems=%d", b.node)) @@ -214,6 +221,11 @@ func (b JailerCommandBuilder) WithNumaNode(node int) JailerCommandBuilder { return b } +func (b JailerCommandBuilder) WithCgroupArgs(cgroupArgs ...string) JailerCommandBuilder { + b.cgroupArgs = cgroupArgs + return b +} + // WithChrootBaseDir will set the given path as the chroot base directory. This // specifies where chroot jails are built and defaults to /srv/jailer. func (b JailerCommandBuilder) WithChrootBaseDir(path string) JailerCommandBuilder { @@ -350,6 +362,7 @@ func jail(ctx context.Context, m *Machine, cfg *Config) error { WithChrootBaseDir(cfg.JailerCfg.ChrootBaseDir). WithDaemonize(cfg.JailerCfg.Daemonize). WithCgroupVersion(cfg.JailerCfg.CgroupVersion). + WithCgroupArgs(cfg.JailerCfg.CgroupArgs...). WithFirecrackerArgs(fcArgs...). WithStdout(stdout). WithStderr(stderr) From a25434145a7459608819de25b0a44b456d82bba5 Mon Sep 17 00:00:00 2001 From: Gudmundur Bjarni Olafsson Date: Wed, 17 May 2023 10:32:54 +0200 Subject: [PATCH 3/5] Add comments for CgroupArgs Signed-off-by: Gudmundur Bjarni Olafsson --- jailer.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/jailer.go b/jailer.go index 1f46e9ad..a59a26d9 100644 --- a/jailer.go +++ b/jailer.go @@ -86,6 +86,7 @@ type JailerConfig struct { // CgroupVersion is the version of the cgroup filesystem to use. CgroupVersion string + // CgroupArgs are the cgroup arguments to pass to the jailer. CgroupArgs []string // Stdout specifies the IO writer for STDOUT to use when spawning the jailer. @@ -221,6 +222,8 @@ func (b JailerCommandBuilder) WithNumaNode(node int) JailerCommandBuilder { return b } +// WithCgroupArgs will set the specified cgroup args to the builder. This +// represents the cgroup settings that the process will get assigned. func (b JailerCommandBuilder) WithCgroupArgs(cgroupArgs ...string) JailerCommandBuilder { b.cgroupArgs = cgroupArgs return b From b0d4dc615d00ca73f56c6279f10a7ed6d7c19162 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gu=C3=B0mundur=20Bjarni=20=C3=93lafsson?= Date: Wed, 17 May 2023 09:10:42 +0000 Subject: [PATCH 4/5] Fix tests and pass value of b.node not reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Guðmundur Bjarni Ólafsson --- jailer.go | 2 +- jailer_test.go | 25 ++++--------------------- 2 files changed, 5 insertions(+), 22 deletions(-) diff --git a/jailer.go b/jailer.go index a59a26d9..14b83acf 100644 --- a/jailer.go +++ b/jailer.go @@ -149,7 +149,7 @@ func (b JailerCommandBuilder) Args() []string { if b.node != nil { if cpulist := getNumaCpuset(*b.node); len(cpulist) > 0 { - args = append(args, "--cgroup", fmt.Sprintf("cpuset.mems=%d", b.node)) + args = append(args, "--cgroup", fmt.Sprintf("cpuset.mems=%d", *b.node)) args = append(args, "--cgroup", fmt.Sprintf("cpuset.cpus=%s", cpulist)) } } diff --git a/jailer_test.go b/jailer_test.go index 7c7017bc..2a678773 100644 --- a/jailer_test.go +++ b/jailer_test.go @@ -34,7 +34,6 @@ func TestJailerBuilder(t *testing.T) { ID: "my-test-id", UID: Int(123), GID: Int(100), - NumaNode: Int(0), ChrootStrategy: NewNaiveChrootStrategy("kernel-image-path"), ExecFile: "/path/to/firecracker", }, @@ -48,10 +47,6 @@ func TestJailerBuilder(t *testing.T) { "100", "--exec-file", "/path/to/firecracker", - "--cgroup", - "cpuset.mems=0", - "--cgroup", - fmt.Sprintf("cpuset.cpus=%s", getNumaCpuset(0)), }, expectedSockPath: filepath.Join( defaultJailerPath, @@ -67,7 +62,6 @@ func TestJailerBuilder(t *testing.T) { ID: "my-test-id", UID: Int(123), GID: Int(100), - NumaNode: Int(0), ChrootStrategy: NewNaiveChrootStrategy("kernel-image-path"), ExecFile: "/path/to/firecracker", JailerBinary: "imprisoner", @@ -82,10 +76,6 @@ func TestJailerBuilder(t *testing.T) { "100", "--exec-file", "/path/to/firecracker", - "--cgroup", - "cpuset.mems=0", - "--cgroup", - fmt.Sprintf("cpuset.cpus=%s", getNumaCpuset(0)), }, expectedSockPath: filepath.Join( defaultJailerPath, @@ -145,10 +135,13 @@ func TestJailerBuilder(t *testing.T) { WithID(c.jailerCfg.ID). WithUID(IntValue(c.jailerCfg.UID)). WithGID(IntValue(c.jailerCfg.GID)). - WithNumaNode(IntValue(c.jailerCfg.NumaNode)). WithCgroupVersion(c.jailerCfg.CgroupVersion). WithExecFile(c.jailerCfg.ExecFile) + if c.jailerCfg.NumaNode != nil { + b = b.WithNumaNode(IntValue(c.jailerCfg.NumaNode)) + } + if len(c.jailerCfg.JailerBinary) > 0 { b = b.WithBin(c.jailerCfg.JailerBinary) } @@ -188,7 +181,6 @@ func TestJail(t *testing.T) { ID: "my-test-id", UID: Int(123), GID: Int(100), - NumaNode: Int(0), ChrootStrategy: NewNaiveChrootStrategy("kernel-image-path"), ExecFile: "/path/to/firecracker", }, @@ -202,10 +194,6 @@ func TestJail(t *testing.T) { "100", "--exec-file", "/path/to/firecracker", - "--cgroup", - "cpuset.mems=0", - "--cgroup", - fmt.Sprintf("cpuset.cpus=%s", getNumaCpuset(0)), "--", "--no-seccomp", "--api-sock", @@ -225,7 +213,6 @@ func TestJail(t *testing.T) { ID: "my-test-id", UID: Int(123), GID: Int(100), - NumaNode: Int(0), ChrootStrategy: NewNaiveChrootStrategy("kernel-image-path"), ExecFile: "/path/to/firecracker", JailerBinary: "imprisoner", @@ -240,10 +227,6 @@ func TestJail(t *testing.T) { "100", "--exec-file", "/path/to/firecracker", - "--cgroup", - "cpuset.mems=0", - "--cgroup", - fmt.Sprintf("cpuset.cpus=%s", getNumaCpuset(0)), "--", "--no-seccomp", "--api-sock", From 607a694d6e836c88cbe73f657661eff2899a8ce2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gu=C3=B0mundur=20Bjarni=20=C3=93lafsson?= Date: Wed, 17 May 2023 09:22:41 +0000 Subject: [PATCH 5/5] Add test for CgroupArgs in jailer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Guðmundur Bjarni Ólafsson --- jailer_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/jailer_test.go b/jailer_test.go index 2a678773..ce323475 100644 --- a/jailer_test.go +++ b/jailer_test.go @@ -98,6 +98,7 @@ func TestJailerBuilder(t *testing.T) { ChrootBaseDir: "/tmp", JailerBinary: "/path/to/the/jailer", CgroupVersion: "2", + CgroupArgs: []string{"cpu.weight=200"}, }, expectedArgs: []string{ "/path/to/the/jailer", @@ -110,6 +111,8 @@ func TestJailerBuilder(t *testing.T) { "--exec-file", "/path/to/firecracker", "--cgroup", + "cpu.weight=200", + "--cgroup", "cpuset.mems=0", "--cgroup", fmt.Sprintf("cpuset.cpus=%s", getNumaCpuset(0)), @@ -142,6 +145,10 @@ func TestJailerBuilder(t *testing.T) { b = b.WithNumaNode(IntValue(c.jailerCfg.NumaNode)) } + if len(c.jailerCfg.CgroupArgs) > 0 { + b = b.WithCgroupArgs(c.jailerCfg.CgroupArgs...) + } + if len(c.jailerCfg.JailerBinary) > 0 { b = b.WithBin(c.jailerCfg.JailerBinary) } @@ -253,6 +260,7 @@ func TestJail(t *testing.T) { ChrootBaseDir: "/tmp", JailerBinary: "/path/to/the/jailer", CgroupVersion: "2", + CgroupArgs: []string{"cpu.weight=200"}, }, expectedArgs: []string{ "/path/to/the/jailer", @@ -265,6 +273,8 @@ func TestJail(t *testing.T) { "--exec-file", "/path/to/firecracker", "--cgroup", + "cpu.weight=200", + "--cgroup", "cpuset.mems=0", "--cgroup", fmt.Sprintf("cpuset.cpus=%s", getNumaCpuset(0)),