From e0f36816160b23ec9a1585bb7fa2c8c0d8b325e4 Mon Sep 17 00:00:00 2001 From: Peng Liu Date: Thu, 21 Sep 2023 07:41:52 -0400 Subject: [PATCH] Move chroot from multus main process to its child processes We used to run chroot in multus main process when calling other CNI plugin binary. We also use a mutex to lock the access to pod files. But this causes performance issues when facing heavy CNI_ADD/CNI_DEL requests. With this patch, we do chroot in the child processes instead. So file operations in the main process will not be affected by chroot. This change requires the multus thick plugin pod to mount CNI bin directory to the same path in the container host. Signed-off-by: Peng Liu --- deployments/multus-daemonset-thick.yml | 2 + pkg/k8sclient/k8sclient.go | 17 ----- pkg/server/exec_chroot.go | 92 ++------------------------ pkg/server/exec_chroot_test.go | 11 --- pkg/server/server.go | 1 - pkg/types/conf.go | 4 -- 6 files changed, 9 insertions(+), 118 deletions(-) diff --git a/deployments/multus-daemonset-thick.yml b/deployments/multus-daemonset-thick.yml index 43aa33298..b15fcdcd8 100644 --- a/deployments/multus-daemonset-thick.yml +++ b/deployments/multus-daemonset-thick.yml @@ -167,6 +167,8 @@ spec: volumeMounts: - name: cni mountPath: /host/etc/cni/net.d + - name: cni-bin + mountPath: /opt/cni/bin - name: host-run mountPath: /host/run - name: host-var-lib-cni-multus diff --git a/pkg/k8sclient/k8sclient.go b/pkg/k8sclient/k8sclient.go index c1d1857a9..77ccc9fab 100644 --- a/pkg/k8sclient/k8sclient.go +++ b/pkg/k8sclient/k8sclient.go @@ -286,11 +286,6 @@ func getKubernetesDelegate(client *ClientInfo, net *types.NetworkSelectionElemen } } - // acquire lock to access file - if types.ChrootMutex != nil { - types.ChrootMutex.Lock() - defer types.ChrootMutex.Unlock() - } configBytes, err := netutils.GetCNIConfig(customResource, confdir) if err != nil { return nil, resourceMap, err @@ -466,12 +461,6 @@ func getNetDelegate(client *ClientInfo, pod *v1.Pod, netname, confdir, namespace // option2) search CNI json config file, which has as CNI name, from confDir - // acquire lock to access file - if types.ChrootMutex != nil { - types.ChrootMutex.Lock() - defer types.ChrootMutex.Unlock() - } - configBytes, err = netutils.GetCNIConfigFromFile(netname, confdir) if err == nil { delegate, err := types.LoadDelegateNetConf(configBytes, nil, "", "") @@ -481,12 +470,6 @@ func getNetDelegate(client *ClientInfo, pod *v1.Pod, netname, confdir, namespace return delegate, resourceMap, nil } } else { - // acquire lock to access file - if types.ChrootMutex != nil { - types.ChrootMutex.Lock() - defer types.ChrootMutex.Unlock() - } - fInfo, err := os.Stat(netname) if err != nil { return nil, resourceMap, err diff --git a/pkg/server/exec_chroot.go b/pkg/server/exec_chroot.go index 3f84c5ed8..61e2bc541 100644 --- a/pkg/server/exec_chroot.go +++ b/pkg/server/exec_chroot.go @@ -20,10 +20,8 @@ import ( "encoding/json" "fmt" "io" - "os" "os/exec" "strings" - "sync" "syscall" "time" @@ -34,91 +32,24 @@ import ( // ChrootExec implements invoke.Exec to execute CNI with chroot type ChrootExec struct { - Stderr io.Writer - chrootDir string - workingDir string // working directory in the outer root - outerRoot *os.File // outer root directory + Stderr io.Writer + chrootDir string version.PluginDecoder - mu sync.Mutex } var _ invoke.Exec = &ChrootExec{} -func (e *ChrootExec) chroot() error { - var err error - e.workingDir, err = os.Getwd() - if err != nil { - fmt.Fprintf(os.Stderr, "getwd before chroot failed: %v\n", err) - return fmt.Errorf("getwd before chroot failed: %v", err) - } - - e.outerRoot, err = os.Open("/") - if err != nil { - fmt.Fprintf(os.Stderr, "getwd before chroot failed: %v\n", err) - return fmt.Errorf("getwd before chroot failed: %v", err) - } - - if err := syscall.Chroot(e.chrootDir); err != nil { - fmt.Fprintf(os.Stderr, "chroot to %s failed: %v\n", e.chrootDir, err) - return fmt.Errorf("chroot to %s failed: %v", e.chrootDir, err) - } - - if err := os.Chdir("/"); err != nil { - fmt.Fprintf(os.Stderr, "chdir to \"/\" failed: %v\n", err) - return fmt.Errorf("chdir to \"/\" failed: %v", err) - } - - return nil -} - -func (e *ChrootExec) escape() error { - if e.outerRoot == nil || e.workingDir == "" { - return nil - } - - // change directory to outer root and close it - if err := syscall.Fchdir(int(e.outerRoot.Fd())); err != nil { - fmt.Fprintf(os.Stderr, "changing directory to outer root failed: %v\n", err) - return fmt.Errorf("changing directory to outer root failed: %v", err) - } - - if err := e.outerRoot.Close(); err != nil { - fmt.Fprintf(os.Stderr, "closing outer root failed: %v\n", err) - return fmt.Errorf("closing outer root failed: %v", err) - } - - // chroot to current directory aka "." being the outer root - if err := syscall.Chroot("."); err != nil { - fmt.Fprintf(os.Stderr, "chroot to current directory failed: %v\n", err) - return fmt.Errorf("chroot to current directory failed: %v", err) - } - - if err := os.Chdir(e.workingDir); err != nil { - fmt.Fprintf(os.Stderr, "chdir to working directory failed: %v\n", err) - return fmt.Errorf("chdir to working directory failed: %v", err) - } - e.outerRoot = nil - e.workingDir = "" - - return nil -} - // ExecPlugin executes CNI plugin with given environment/stdin data. func (e *ChrootExec) ExecPlugin(ctx context.Context, pluginPath string, stdinData []byte, environ []string) ([]byte, error) { - // lock and do chroot to execute plugin with host root - e.mu.Lock() - defer e.mu.Unlock() - err := e.chroot() - defer e.escape() - - if err != nil { - fmt.Fprintf(os.Stderr, "ExecPlugin failed at chroot: %v\n", err) - return nil, fmt.Errorf("ExecPlugin failed at chroot: %v", err) - } + var err error stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} c := exec.CommandContext(ctx, pluginPath) + // execute delegate CNI with host filesystem context. + c.SysProcAttr = &syscall.SysProcAttr{ + Chroot: e.chrootDir, + } c.Env = environ c.Stdin = bytes.NewBuffer(stdinData) c.Stdout = stdout @@ -169,14 +100,5 @@ func (e *ChrootExec) pluginErr(err error, stdout, stderr []byte) error { // FindInPath try to find CNI plugin based on given path func (e *ChrootExec) FindInPath(plugin string, paths []string) (string, error) { - e.mu.Lock() - defer e.mu.Unlock() - err := e.chroot() - defer e.escape() - if err != nil { - fmt.Fprintf(os.Stderr, "FindInPath failed at chroot: %v\n", err) - return "", fmt.Errorf("FindInPath failed at chroot: %v", err) - } - return invoke.FindInPath(plugin, paths) } diff --git a/pkg/server/exec_chroot_test.go b/pkg/server/exec_chroot_test.go index 80b3b2c50..3290355b1 100644 --- a/pkg/server/exec_chroot_test.go +++ b/pkg/server/exec_chroot_test.go @@ -43,15 +43,4 @@ var _ = Describe("exec_chroot", func() { _, err := chrootExec.ExecPlugin(context.Background(), "/bin/true", nil, nil) Expect(err).To(HaveOccurred()) }) - - It("Call ChrootExec.FindInPath with dummy", func() { - chrootExec := &ChrootExec{ - Stderr: os.Stderr, - chrootDir: "/usr/bin", - } - - _, err := chrootExec.FindInPath("true", []string{"/"}) - Expect(err).NotTo(HaveOccurred()) - }) - }) diff --git a/pkg/server/server.go b/pkg/server/server.go index 4bea6ffc5..a215758c7 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -210,7 +210,6 @@ func NewCNIServer(daemonConfig *ControllerNetConf, serverConfig []byte, ignoreRe Stderr: os.Stderr, chrootDir: daemonConfig.ChrootDir, } - types.ChrootMutex = &chrootExec.mu exec = chrootExec logging.Verbosef("server configured with chroot: %s", daemonConfig.ChrootDir) } diff --git a/pkg/types/conf.go b/pkg/types/conf.go index 8295bd9c6..4402b4ec7 100644 --- a/pkg/types/conf.go +++ b/pkg/types/conf.go @@ -21,7 +21,6 @@ import ( "net" "os" "strings" - "sync" "time" utilwait "k8s.io/apimachinery/pkg/util/wait" @@ -43,9 +42,6 @@ const ( defaultNonIsolatedNamespace = "default" ) -// ChrootMutex provides lock to access host filesystem -var ChrootMutex *sync.Mutex - // LoadDelegateNetConfList reads DelegateNetConf from bytes func LoadDelegateNetConfList(bytes []byte, delegateConf *DelegateNetConf) error { logging.Debugf("LoadDelegateNetConfList: %s, %v", string(bytes), delegateConf)