server: don't set CNI config readinessindicatorfile when using Config…

…Manager For whatever reason calling os.Stat() on the readiness indicator file from CmdAdd()/CmdDel() when multus is running in server mode and is containerized often returns "file not found", which triggers the polling behavior of GetReadinessIndicatorFile(). This greatly delays CNI operations that should be pretty quick. Even if an exponential backoff is used, os.Stat() can still return "file not found" multiple times, even though the file clearly exists. But it turns out we don't need to check the readiness file in server mode when running with MultusConfigFile == "auto". In this mode the server starts the ConfigManager which (a) waits until the file exists and (b) fsnotify watches the readiness and (c) exits the daemon immediately if the file is deleted or moved. This means we can assume that while the daemon is running and the server is handling CNI requests that the readiness file exists; otherwise the daemon would have exited. Thus CmdAdd/CmdDel don't need to run a lot of possibly failing os.Stat() calls in the CNI hot paths. Signed-off-by: Dan Williams <[email protected]>
k8snetworkplumbingwg · Sep 13, 2023 · 8212539 · 8212539
1 parent 3205f93
commit 8212539
Show file tree

Hide file tree

Showing 6 changed files with 179 additions and 49 deletions.
diff --git a/cmd/multus-daemon/main.go b/cmd/multus-daemon/main.go
@@ -83,38 +83,42 @@ func main() {
 		logging.Verbosef("Readiness Indicator file check done!")
 	}
 
-	if err := startMultusDaemon(ctx, daemonConf); err != nil {
-		logging.Panicf("failed start the multus thick-plugin listener: %v", err)
-		os.Exit(3)
-	}
-
-	// Wait until daemon ready
-	logging.Verbosef("API readiness check")
-	if waitUntilAPIReady(daemonConf.SocketDir) != nil {
-		logging.Panicf("failed to ready multus-daemon socket: %v", err)
-		os.Exit(1)
-	}
-	logging.Verbosef("API readiness check done!")
-
 	var configManager *config.Manager
-
-	// Generate multus CNI config from current CNI config
+	var ignoreReadinessIndicator bool
 	if multusConf.MultusConfigFile == "auto" {
 		if multusConf.CNIVersion == "" {
 			_ = logging.Errorf("the CNI version is a mandatory parameter when the '-multus-config-file=auto' option is used")
 		}
 
+		// Generate multus CNI config from current CNI config
 		configManager, err = config.NewManager(*multusConf)
 		if err != nil {
 			_ = logging.Errorf("failed to create the configuration manager for the primary CNI plugin: %v", err)
 			os.Exit(2)
 		}
+		// ConfigManager watches the readiness indicator file (if configured)
+		// and exits the daemon when that is removed. The CNIServer does
+		// not need to re-do that check every CNI operation
+		ignoreReadinessIndicator = true
 	} else {
 		if err := copyUserProvidedConfig(multusConf.MultusConfigFile, multusConf.CniConfigDir); err != nil {
 			logging.Errorf("failed to copy the user provided configuration %s: %v", multusConf.MultusConfigFile, err)
 		}
 	}
 
+	if err := startMultusDaemon(ctx, daemonConf, ignoreReadinessIndicator); err != nil {
+		logging.Panicf("failed start the multus thick-plugin listener: %v", err)
+		os.Exit(3)
+	}
+
+	// Wait until daemon ready
+	logging.Verbosef("API readiness check")
+	if waitUntilAPIReady(daemonConf.SocketDir) != nil {
+		logging.Panicf("failed to ready multus-daemon socket: %v", err)
+		os.Exit(1)
+	}
+	logging.Verbosef("API readiness check done!")
+
 	signalCh := make(chan os.Signal, 16)
 	signal.Notify(signalCh, syscall.SIGINT, syscall.SIGTERM)
 	go func() {
@@ -146,7 +150,7 @@ func waitUntilAPIReady(socketPath string) error {
 	})
 }
 
-func startMultusDaemon(ctx context.Context, daemonConfig *srv.ControllerNetConf) error {
+func startMultusDaemon(ctx context.Context, daemonConfig *srv.ControllerNetConf, ignoreReadinessIndicator bool) error {
 	if user, err := user.Current(); err != nil || user.Uid != "0" {
 		return fmt.Errorf("failed to run multus-daemon with root: %v, now running in uid: %s", err, user.Uid)
 	}
@@ -155,7 +159,7 @@ func startMultusDaemon(ctx context.Context, daemonConfig *srv.ControllerNetConf)
 		return fmt.Errorf("failed to prepare the cni-socket for communicating with the shim: %w", err)
 	}
 
-	server, err := srv.NewCNIServer(daemonConfig, daemonConfig.ConfigFileContents)
+	server, err := srv.NewCNIServer(daemonConfig, daemonConfig.ConfigFileContents, ignoreReadinessIndicator)
 	if err != nil {
 		return fmt.Errorf("failed to create the server: %v", err)
 	}

diff --git a/pkg/server/config/generator.go b/pkg/server/config/generator.go
@@ -128,6 +128,9 @@ func (mc *MultusConf) Generate() (string, error) {
 	mc.MultusAutoconfigDir = ""
 	mc.MultusMasterCni = ""
 	mc.ForceCNIVersion = false
+	// Readiness indicator file existence is already handled by the
+	// ConfigManager via an fsnotify watch, so CmdAdd/CmdDel don't need to.
+	mc.ReadinessIndicatorFile = ""
 
 	data, err := json.Marshal(mc)
 	return string(data), err

diff --git a/pkg/server/server.go b/pkg/server/server.go
@@ -15,7 +15,6 @@
 package server
 
 import (
-	"bytes"
 	"context"
 	"encoding/json"
 	"fmt"
@@ -45,6 +44,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/fields"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+	"k8s.io/apimachinery/pkg/util/sets"
 	utilwait "k8s.io/apimachinery/pkg/util/wait"
 	informerfactory "k8s.io/client-go/informers"
 	v1coreinformers "k8s.io/client-go/informers/core/v1"
@@ -105,11 +105,9 @@ func (s *Server) HandleCNIRequest(cmd string, k8sArgs *types.K8sArgs, cniCmdArgs
 func (s *Server) HandleDelegateRequest(cmd string, k8sArgs *types.K8sArgs, cniCmdArgs *skel.CmdArgs, interfaceAttributes *api.DelegateInterfaceAttributes) ([]byte, error) {
 	var result []byte
 	var err error
-	var multusConfByte []byte
 
-	multusConfByte = bytes.Replace(s.serverConfig, []byte(","), []byte("{"), 1)
 	multusConfig := types.GetDefaultNetConf()
-	if err = json.Unmarshal(multusConfByte, multusConfig); err != nil {
+	if err = json.Unmarshal(s.serverConfig, multusConfig); err != nil {
 		return nil, err
 	}
 
@@ -173,7 +171,7 @@ func newPodInformer(kubeClient kubernetes.Interface, nodeName string) (internali
 }
 
 // NewCNIServer creates and returns a new Server object which will listen on a socket in the given path
-func NewCNIServer(daemonConfig *ControllerNetConf, serverConfig []byte) (*Server, error) {
+func NewCNIServer(daemonConfig *ControllerNetConf, serverConfig []byte, ignoreReadinessIndicator bool) (*Server, error) {
 	kubeClient, err := k8s.InClusterK8sClient()
 	if err != nil {
 		return nil, fmt.Errorf("error getting k8s client: %v", err)
@@ -190,17 +188,10 @@ func NewCNIServer(daemonConfig *ControllerNetConf, serverConfig []byte) (*Server
 		logging.Verbosef("server configured with chroot: %s", daemonConfig.ChrootDir)
 	}
 
-	return newCNIServer(daemonConfig.SocketDir, kubeClient, exec, serverConfig)
+	return newCNIServer(daemonConfig.SocketDir, kubeClient, exec, serverConfig, ignoreReadinessIndicator)
 }
 
-func newCNIServer(rundir string, kubeClient *k8s.ClientInfo, exec invoke.Exec, servConfig []byte) (*Server, error) {
-
-	// preprocess server config to be used to override multus CNI config
-	// see extractCniData() for the detail
-	if servConfig != nil {
-		servConfig = bytes.Replace(servConfig, []byte("{"), []byte(","), 1)
-	}
-
+func newCNIServer(rundir string, kubeClient *k8s.ClientInfo, exec invoke.Exec, servConfig []byte, ignoreReadinessIndicator bool) (*Server, error) {
 	informerFactory, podInformer := newPodInformer(kubeClient.Client, os.Getenv("MULTUS_NODE_NAME"))
 
 	router := http.NewServeMux()
@@ -221,8 +212,9 @@ func newCNIServer(rundir string, kubeClient *k8s.ClientInfo, exec invoke.Exec, s
 				[]string{"handler", "code", "method"},
 			),
 		},
-		informerFactory: informerFactory,
-		podInformer:     podInformer,
+		informerFactory:          informerFactory,
+		podInformer:              podInformer,
+		ignoreReadinessIndicator: ignoreReadinessIndicator,
 	}
 	s.SetKeepAlivesEnabled(false)
 
@@ -326,7 +318,7 @@ func (s *Server) handleCNIRequest(r *http.Request) ([]byte, error) {
 	if err := json.Unmarshal(b, &cr); err != nil {
 		return nil, err
 	}
-	cmdType, cniCmdArgs, err := extractCniData(&cr, s.serverConfig)
+	cmdType, cniCmdArgs, err := s.extractCniData(&cr, s.serverConfig)
 	if err != nil {
 		return nil, fmt.Errorf("could not extract the CNI command args: %w", err)
 	}
@@ -353,7 +345,7 @@ func (s *Server) handleDelegateRequest(r *http.Request) ([]byte, error) {
 	if err := json.Unmarshal(b, &cr); err != nil {
 		return nil, err
 	}
-	cmdType, cniCmdArgs, err := extractCniData(&cr, s.serverConfig)
+	cmdType, cniCmdArgs, err := s.extractCniData(&cr, s.serverConfig)
 	if err != nil {
 		return nil, fmt.Errorf("could not extract the CNI command args: %w", err)
 	}
@@ -371,7 +363,42 @@ func (s *Server) handleDelegateRequest(r *http.Request) ([]byte, error) {
 	return result, nil
 }
 
-func extractCniData(cniRequest *api.Request, overrideConf []byte) (string, *skel.CmdArgs, error) {
+func overrideCNIConfigWithServerConfig(cniConf []byte, overrideConf []byte, ignoreReadinessIndicator bool) ([]byte, error) {
+	if len(overrideConf) == 0 {
+		return cniConf, nil
+	}
+
+	var cni map[string]interface{}
+	if err := json.Unmarshal(cniConf, &cni); err != nil {
+		return nil, fmt.Errorf("failed to unmarshall CNI config: %w", err)
+	}
+
+	var override map[string]interface{}
+	if err := json.Unmarshal(overrideConf, &override); err != nil {
+		return nil, fmt.Errorf("failed to unmarshall CNI override config: %w", err)
+	}
+
+	// Copy each key of the override config into the CNI config except for
+	// a few specific keys
+	ignoreKeys := sets.NewString()
+	if ignoreReadinessIndicator {
+		ignoreKeys.Insert("readinessindicatorfile")
+	}
+	for overrideKey, overrideVal := range override {
+		if !ignoreKeys.Has(overrideKey) {
+			cni[overrideKey] = overrideVal
+		}
+	}
+
+	newBytes, err := json.Marshal(cni)
+	if err != nil {
+		return nil, fmt.Errorf("failed ot marshall new CNI config with overrides: %w", err)
+	}
+
+	return newBytes, nil
+}
+
+func (s *Server) extractCniData(cniRequest *api.Request, overrideConf []byte) (string, *skel.CmdArgs, error) {
 	cmd, ok := cniRequest.Env["CNI_COMMAND"]
 	if !ok {
 		return "", nil, fmt.Errorf("unexpected or missing CNI_COMMAND")
@@ -398,18 +425,10 @@ func extractCniData(cniRequest *api.Request, overrideConf []byte) (string, *skel
 	}
 	cniCmdArgs.Args = cniArgs
 
-	if overrideConf != nil {
-		// trim the close bracket from multus CNI config and put the server config
-		// to override CNI config with server config.
-		// note: if there are two or more value in same key, then the
-		// latest one is used at golang json implementation
-		idx := bytes.LastIndex(cniRequest.Config, []byte("}"))
-		if idx == -1 {
-			return "", nil, fmt.Errorf("invalid CNI config")
-		}
-		cniCmdArgs.StdinData = append(cniRequest.Config[:idx], overrideConf...)
-	} else {
-		cniCmdArgs.StdinData = cniRequest.Config
+	var err error
+	cniCmdArgs.StdinData, err = overrideCNIConfigWithServerConfig(cniRequest.Config, overrideConf, s.ignoreReadinessIndicator)
+	if err != nil {
+		return "", nil, err
 	}
 
 	return cmd, cniCmdArgs, nil

diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go
@@ -0,0 +1,102 @@
+// Copyright (c) 2022 Multus Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package server
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Server", func() {
+	cniConf := []byte(`{
+	"binDir": "/var/lib/cni/bin",
+	"clusterNetwork": "/host/run/multus/cni/net.d/10-ovn-kubernetes.conf",
+	"cniVersion": "0.3.1",
+	"daemonSocketDir": "/run/multus/socket",
+	"globalNamespaces": "default,openshift-multus,openshift-sriov-network-operator",
+	"logLevel": "verbose",
+	"logToStderr": true,
+	"name": "multus-cni-network",
+	"namespaceIsolation": true,
+	"type": "multus-shim"
+}`)
+
+	serverConf := []byte(`{
+	"cniVersion": "0.4.0",
+	"chrootDir": "/hostroot",
+	"logToStderr": false,
+	"logLevel": "debug",
+	"binDir": "/foo/bar",
+	"cniConfigDir": "/host/etc/cni/net.d",
+	"multusConfigFile": "auto",
+	"multusAutoconfigDir": "/host/run/multus/cni/net.d",
+	"namespaceIsolation": false,
+	"globalNamespaces": "other,namespace",
+	"readinessindicatorfile": "/host/run/multus/cni/net.d/10-ovn-kubernetes.conf",
+	"daemonSocketDir": "/somewhere/socket",
+	"socketDir": "/host/run/multus/socket"
+}`)
+
+	Context("correctly overrides incoming CNI config with server config", func() {
+		newConf, err := overrideCNIConfigWithServerConfig(cniConf, serverConf, false)
+		Expect(err).ToNot(HaveOccurred())
+
+		// All server options except readinessindicatorfile should exist
+		// in the returned config
+		Expect(newConf).To(MatchJSON(`{
+	"clusterNetwork": "/host/run/multus/cni/net.d/10-ovn-kubernetes.conf",
+	"name": "multus-cni-network",
+	"type": "multus-shim",
+	"cniVersion": "0.4.0",
+	"chrootDir": "/hostroot",
+	"logToStderr": false,
+	"logLevel": "debug",
+	"binDir": "/foo/bar",
+	"cniConfigDir": "/host/etc/cni/net.d",
+	"multusConfigFile": "auto",
+	"multusAutoconfigDir": "/host/run/multus/cni/net.d",
+	"namespaceIsolation": false,
+	"globalNamespaces": "other,namespace",
+	"readinessindicatorfile": "/host/run/multus/cni/net.d/10-ovn-kubernetes.conf",
+	"daemonSocketDir": "/somewhere/socket",
+	"socketDir": "/host/run/multus/socket"
+}`))
+	})
+
+	Context("correctly overrides incoming CNI config with server config and ignores readinessindicatorfile", func() {
+		newConf, err := overrideCNIConfigWithServerConfig(cniConf, serverConf, true)
+		Expect(err).ToNot(HaveOccurred())
+
+		// All server options except readinessindicatorfile should exist
+		// in the returned config
+		Expect(newConf).To(MatchJSON(`{
+	"clusterNetwork": "/host/run/multus/cni/net.d/10-ovn-kubernetes.conf",
+	"name": "multus-cni-network",
+	"type": "multus-shim",
+	"cniVersion": "0.4.0",
+	"chrootDir": "/hostroot",
+	"logToStderr": false,
+	"logLevel": "debug",
+	"binDir": "/foo/bar",
+	"cniConfigDir": "/host/etc/cni/net.d",
+	"multusConfigFile": "auto",
+	"multusAutoconfigDir": "/host/run/multus/cni/net.d",
+	"namespaceIsolation": false,
+	"globalNamespaces": "other,namespace",
+	"daemonSocketDir": "/somewhere/socket",
+	"socketDir": "/host/run/multus/socket"
+}`))
+	})
+})
diff --git a/pkg/server/thick_cni_test.go b/pkg/server/thick_cni_test.go
@@ -256,7 +256,7 @@ func createFakePod(k8sClient *k8s.ClientInfo, podName string) error {
 func startCNIServer(ctx context.Context, runDir string, k8sClient *k8s.ClientInfo, servConfig []byte) (*Server, error) {
 	const period = 0
 
-	cniServer, err := newCNIServer(runDir, k8sClient, &fakeExec{}, servConfig)
+	cniServer, err := newCNIServer(runDir, k8sClient, &fakeExec{}, servConfig, true)
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/server/types.go b/pkg/server/types.go
@@ -52,6 +52,8 @@ type Server struct {
 	metrics         *Metrics
 	informerFactory internalinterfaces.SharedInformerFactory
 	podInformer     cache.SharedIndexInformer
+
+	ignoreReadinessIndicator bool
 }
 
 // ControllerNetConf for the controller cni configuration