From e509965c487bc497dc80d3912985534537613d98 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/4] 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 d9d022d..24053b3 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 e261fae..229b349 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 9cac227c05f84802f706d68143c9ef294cb3409a 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/4] Allow cgroup arguments to be configurable Signed-off-by: Gudmundur Bjarni Olafsson --- jailer.go | 18 +++++++++++++++++- jailer_test.go | 25 ++++--------------------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/jailer.go b/jailer.go index 229b349..14b83ac 100644 --- a/jailer.go +++ b/jailer.go @@ -86,6 +86,9 @@ 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. Stdout io.Writer // Stderr specifies the IO writer for STDERR to use when spawning the jailer. @@ -109,6 +112,7 @@ type JailerCommandBuilder struct { daemonize bool firecrackerArgs []string cgroupVersion string + cgroupArgs []string node *int stdin io.Reader @@ -139,9 +143,13 @@ 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)) + args = append(args, "--cgroup", fmt.Sprintf("cpuset.mems=%d", *b.node)) args = append(args, "--cgroup", fmt.Sprintf("cpuset.cpus=%s", cpulist)) } } @@ -214,6 +222,13 @@ 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 +} + // 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 +365,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) diff --git a/jailer_test.go b/jailer_test.go index 7c7017b..2a67877 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 44c27c7866ba5c0e0b1b26b8c9b26622db8075e4 Mon Sep 17 00:00:00 2001 From: Aditya Maru Date: Sun, 14 Jul 2024 17:02:07 -0400 Subject: [PATCH 3/4] *: add more logging to setup step --- cni/vmconf/vmconf.go | 53 ++++++++++++++++++++++++++------------------ network.go | 19 +++++++++++++--- 2 files changed, 48 insertions(+), 24 deletions(-) diff --git a/cni/vmconf/vmconf.go b/cni/vmconf/vmconf.go index f0f7e2a..7336a2b 100644 --- a/cni/vmconf/vmconf.go +++ b/cni/vmconf/vmconf.go @@ -12,29 +12,31 @@ // permissions and limitations under the License. /* - Package vmconf defines an interface for converting particular CNI invocation - results to networking configuration usable by a VM. It expects the CNI result - to have the following properties: - * The results should contain an interface for a tap device, which will be used - as the VM's tap device. - * The results should contain an interface with the same name as the tap device - but with sandbox ID set to the containerID provided during CNI invocation. - This should be a "pseudo-interface", not one that has actually been created. - It represents the configuration that should be applied to the VM internally. - The CNI "containerID" is, in this case, used more as a "vmID" to represent - the VM's internal network interface. - * If the CNI results specify an IP associated with this interface, that IP - should be used to statically configure the VM's internal network interface. +Package vmconf defines an interface for converting particular CNI invocation +results to networking configuration usable by a VM. It expects the CNI result +to have the following properties: + - The results should contain an interface for a tap device, which will be used + as the VM's tap device. + - The results should contain an interface with the same name as the tap device + but with sandbox ID set to the containerID provided during CNI invocation. + This should be a "pseudo-interface", not one that has actually been created. + It represents the configuration that should be applied to the VM internally. + The CNI "containerID" is, in this case, used more as a "vmID" to represent + the VM's internal network interface. + - If the CNI results specify an IP associated with this interface, that IP + should be used to statically configure the VM's internal network interface. */ package vmconf import ( "fmt" "strings" + "time" "github.com/containernetworking/cni/pkg/types" current "github.com/containernetworking/cni/pkg/types/100" "github.com/containernetworking/plugins/pkg/ns" + log "github.com/sirupsen/logrus" "github.com/firecracker-microvm/firecracker-go-sdk/cni/internal" ) @@ -88,13 +90,13 @@ type StaticNetworkConf struct { // // Due to the limitation of "ip=", not all configuration specified in StaticNetworkConf can be // applied automatically. In particular: -// * The MacAddr and MTU cannot be applied -// * The only routes created will match what's specified in VMIPConfig; VMRoutes will be ignored. -// * Only up to two namesevers can be supplied. If VMNameservers is has more than 2 entries, only -// the first two in the slice will be applied in the VM. -// * VMDomain, VMSearchDomains and VMResolverOptions will be ignored -// * Nameserver settings are also only set in /proc/net/pnp. Most applications will thus require -// /etc/resolv.conf to be a symlink to /proc/net/pnp in order to resolve names as expected. +// - The MacAddr and MTU cannot be applied +// - The only routes created will match what's specified in VMIPConfig; VMRoutes will be ignored. +// - Only up to two namesevers can be supplied. If VMNameservers is has more than 2 entries, only +// the first two in the slice will be applied in the VM. +// - VMDomain, VMSearchDomains and VMResolverOptions will be ignored +// - Nameserver settings are also only set in /proc/net/pnp. Most applications will thus require +// /etc/resolv.conf to be a symlink to /proc/net/pnp in order to resolve names as expected. func (c StaticNetworkConf) IPBootParam() string { // See "ip=" section of kernel linked above for details on each field listed below. @@ -149,7 +151,7 @@ func (c StaticNetworkConf) IPBootParam() string { // StaticNetworkConfFrom takes the result of a CNI invocation that conforms to the specification // in this package's docstring and converts it to a StaticNetworkConf object that the caller // can use to configure their VM with. -func StaticNetworkConfFrom(result types.Result, containerID string) (*StaticNetworkConf, error) { +func StaticNetworkConfFrom(result types.Result, containerID string, logger *log.Entry) (*StaticNetworkConf, error) { currentResult, err := current.NewResultFromResult(result) if err != nil { return nil, fmt.Errorf("failed to parse cni result: %w", err) @@ -161,28 +163,37 @@ func StaticNetworkConfFrom(result types.Result, containerID string) (*StaticNetw // internal network device. vmIfaceSandbox := containerID + beforeVMTapPair := time.Now() vmIface, tapIface, err := internal.VMTapPair(currentResult, vmIfaceSandbox) if err != nil { return nil, err } + logger.Infof("VMTapPair took %s", time.Since(beforeVMTapPair)) // find the IP associated with the VM iface + beforeInterfaceIPs := time.Now() vmIPs := internal.InterfaceIPs(currentResult, vmIface.Name, vmIface.Sandbox) if len(vmIPs) != 1 { return nil, fmt.Errorf("expected to find 1 IP for vm interface %q, but instead found %+v", vmIface.Name, vmIPs) } + logger.Infof("InterfaceIPs took %s", time.Since(beforeInterfaceIPs)) + vmIP := vmIPs[0] + beforeGetNS := time.Now() netNS, err := ns.GetNS(tapIface.Sandbox) if err != nil { return nil, fmt.Errorf("failed to find netns at path %q: %w", tapIface.Sandbox, err) } + logger.Infof("GetNS took %s", time.Since(beforeGetNS)) + beforeMTU := time.Now() tapMTU, err := mtuOf(tapIface.Name, netNS, internal.DefaultNetlinkOps()) if err != nil { return nil, err } + logger.Infof("MTUOf took %s", time.Since(beforeMTU)) return &StaticNetworkConf{ TapName: tapIface.Name, diff --git a/network.go b/network.go index 3535299..828161e 100644 --- a/network.go +++ b/network.go @@ -20,6 +20,7 @@ import ( "os" "path/filepath" "runtime" + "time" "github.com/containernetworking/cni/libcni" "github.com/containernetworking/cni/pkg/types" @@ -103,10 +104,12 @@ func (networkInterfaces NetworkInterfaces) setupNetwork( // Get the network interface with CNI configuration or, if there is none, // just return right away. + beforeCNIInterface := time.Now() cniNetworkInterface := networkInterfaces.cniInterface() if cniNetworkInterface == nil { return nil, cleanupFuncs } + logger.Infof("CNI interface found in %s", time.Since(beforeCNIInterface)) cniNetworkInterface.CNIConfiguration.containerID = vmID cniNetworkInterface.CNIConfiguration.netNSPath = netNSPath @@ -114,23 +117,28 @@ func (networkInterfaces NetworkInterfaces) setupNetwork( // Make sure the netns is setup. If the path doesn't yet exist, it will be // initialized with a new empty netns. - err, netnsCleanupFuncs := cniNetworkInterface.CNIConfiguration.initializeNetNS() + beforeInitializeNetNS := time.Now() + err, netnsCleanupFuncs := cniNetworkInterface.CNIConfiguration.initializeNetNS(logger) cleanupFuncs = append(cleanupFuncs, netnsCleanupFuncs...) if err != nil { return fmt.Errorf("failed to initialize netns: %w", err), cleanupFuncs } + logger.Infof("NetNS initialized in %s", time.Since(beforeInitializeNetNS)) + beforeInvokeCNI := time.Now() cniResult, err, cniCleanupFuncs := cniNetworkInterface.CNIConfiguration.invokeCNI(ctx, logger) cleanupFuncs = append(cleanupFuncs, cniCleanupFuncs...) if err != nil { return fmt.Errorf("failure when invoking CNI: %w", err), cleanupFuncs } + logger.Infof("CNI invoked in %s", time.Since(beforeInvokeCNI)) // If static configuration is not already set for the network device, fill it out // by parsing the CNI result object according to the specifications detailed in the // vmconf package docs. + beforeStaticConf := time.Now() if cniNetworkInterface.StaticConfiguration == nil { - vmNetConf, err := vmconf.StaticNetworkConfFrom(*cniResult, cniNetworkInterface.CNIConfiguration.containerID) + vmNetConf, err := vmconf.StaticNetworkConfFrom(*cniResult, cniNetworkInterface.CNIConfiguration.containerID, logger) if err != nil { return fmt.Errorf("failed to parse VM network configuration from CNI output, ensure CNI is configured with a plugin "+ "that supports automatic VM network configuration such as tc-redirect-tap"+": %w", err), cleanupFuncs @@ -156,6 +164,7 @@ func (networkInterfaces NetworkInterfaces) setupNetwork( } } } + logger.Infof("Static configuration filled out in %s", time.Since(beforeStaticConf)) return nil, cleanupFuncs } @@ -377,9 +386,10 @@ func (cniConf CNIConfiguration) invokeCNI(ctx context.Context, logger *log.Entry // initializeNetNS checks to see if the netNSPath already exists, if it doesn't it will create // a new one mounted at that path. -func (cniConf CNIConfiguration) initializeNetNS() (error, []func() error) { +func (cniConf CNIConfiguration) initializeNetNS(logger *log.Entry) (error, []func() error) { var cleanupFuncs []func() error + beforeIsNSorErr := time.Now() err := ns.IsNSorErr(cniConf.netNSPath) switch err.(type) { case nil: @@ -394,6 +404,7 @@ func (cniConf CNIConfiguration) initializeNetNS() (error, []func() error) { // if something else bad happened return the error return fmt.Errorf("failure checking if %q is a mounted netns: %w", cniConf.netNSPath, err), cleanupFuncs } + logger.Infof("IsNSorErr took %s", time.Since(beforeIsNSorErr)) // the path doesn't exist, so we need to create a new netns and mount it at the path @@ -439,6 +450,7 @@ func (cniConf CNIConfiguration) initializeNetNS() (error, []func() error) { // separate OS thread that's discarded at the end of the call is the // simplest way to prevent the namespace from leaking to other goroutines. doneCh := make(chan error) + beforeUnshare := time.Now() go func() { defer close(doneCh) // Lock the goroutine to the OS thread but don't ever unlock it. When @@ -468,6 +480,7 @@ func (cniConf CNIConfiguration) initializeNetNS() (error, []func() error) { }) }() + logger.Infof("Unshare and mount took %s", time.Since(beforeUnshare)) err = <-doneCh return err, cleanupFuncs } From 0ab40db042b1138f7537e45a43579972f7b50db0 Mon Sep 17 00:00:00 2001 From: Aditya Maru Date: Sun, 14 Jul 2024 19:30:26 -0400 Subject: [PATCH 4/4] add some more logging --- network.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/network.go b/network.go index 828161e..2185eb9 100644 --- a/network.go +++ b/network.go @@ -336,20 +336,24 @@ func (cniConf CNIConfiguration) invokeCNI(ctx context.Context, logger *log.Entry var err error if networkConf == nil { + beforeLoadConf := time.Now() networkConf, err = libcni.LoadConfList(cniConf.ConfDir, cniConf.NetworkName) if err != nil { return nil, fmt.Errorf("failed to load CNI configuration from dir %q for network %q: %w", cniConf.ConfDir, cniConf.NetworkName, err), cleanupFuncs } + logger.Infof("LoadConfList took %s", time.Since(beforeLoadConf)) } runtimeConf := cniConf.asCNIRuntimeConf() delNetworkFunc := func() error { + beforeDelNetwork := time.Now() err := cniPlugin.DelNetworkList(ctx, networkConf, runtimeConf) if err != nil { return fmt.Errorf("failed to delete CNI network list %q: %w", cniConf.NetworkName, err) } + logger.Infof("DelNetworkList took %s", time.Since(beforeDelNetwork)) return nil } @@ -376,10 +380,15 @@ func (cniConf CNIConfiguration) invokeCNI(ctx context.Context, logger *log.Entry // case where AddNetworkList fails but leaves intermediate resources around like // devices and ip allocations. cleanupFuncs = append(cleanupFuncs, delNetworkFunc) + for _, net := range networkConf.Plugins { + logger.Infof("Adding network %s", net.Network.Name) + } + beforeAddNetwork := time.Now() cniResult, err := cniPlugin.AddNetworkList(ctx, networkConf, runtimeConf) if err != nil { return nil, fmt.Errorf("failed to create CNI network: %w", err), cleanupFuncs } + logger.Infof("AddNetworkList took %s", time.Since(beforeAddNetwork)) return &cniResult, nil, cleanupFuncs }