Move chroot from multus main process to its child processes (#1161)

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 <pliu@redhat.com>

deployments/multus-daemonset-thick.yml

@@ -166,6 +166,11 @@ spec:
           volumeMounts:
             - name: cni
               mountPath: /host/etc/cni/net.d
+            # multus-daemon expects that cni-bin path must be identical between pod and container host.
+            # e.g. if the cni bin is in '/opt/cni/bin' on the container host side, then it should be mount to '/opt/cni/bin' in multus-daemon,
+            # not to any other directory, like '/opt/bin' or '/usr/bin'.
+            - name: cni-bin
+              mountPath: /opt/cni/bin
             - name: host-run
               mountPath: /host/run
             - name: host-var-lib-cni-multus

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 <netname> 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

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)
 }

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())
-	})
-
 })

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)
 	}

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)