Add support for cache_path option

The change is required to be able to configure persistent dir for cache files. The default path "/tmp/ovscache" is ephemeral in some distros, and this can cause CmdDel to exit without proper cleanup after the host reboot (because the cache is empty). Signed-off-by: Yury Kulazhenkov <[email protected]>
k8snetworkplumbingwg · Apr 15, 2024 · 03424be · 03424be
1 parent 10d2594
commit 03424be
Show file tree

Hide file tree

Showing 8 changed files with 93 additions and 26 deletions.
diff --git a/docs/cni-plugin.md b/docs/cni-plugin.md
@@ -60,6 +60,7 @@ Another example with a port which has an interface of type system:
 * `interface_type` (string, optional): type of the interface belongs to ports. if value is "", ovs will use default interface of type 'internal'
 * `configuration_path` (optional): configuration file containing ovsdb
   socket file path, etc.
+* `cache_path` (string, optional): path that ovs-cni uses to store temporary state, defaults to /tmp/ovscache
 
 ### Flatfile Configuation
 

diff --git a/pkg/config/config.go b/pkg/config/config.go
@@ -78,9 +78,9 @@ func LoadMirrorConf(data []byte) (*types.MirrorNetConf, error) {
 }
 
 // LoadPrevResultConfFromCache retrieve preResult config from cache
-func LoadPrevResultConfFromCache(cRef string) (*types.CachedPrevResultNetConf, error) {
+func LoadPrevResultConfFromCache(cacheDir, cRef string) (*types.CachedPrevResultNetConf, error) {
 	netCache := &types.CachedPrevResultNetConf{}
-	netConfBytes, err := utils.ReadCache(cRef)
+	netConfBytes, err := utils.ReadCache(cacheDir, cRef)
 	if err != nil {
 		return nil, fmt.Errorf("error reading cached prevResult conf with name %s: %v", cRef, err)
 	}
@@ -93,9 +93,9 @@ func LoadPrevResultConfFromCache(cRef string) (*types.CachedPrevResultNetConf, e
 }
 
 // LoadConfFromCache retrieve net config from cache
-func LoadConfFromCache(cRef string) (*types.CachedNetConf, error) {
+func LoadConfFromCache(cacheDir, cRef string) (*types.CachedNetConf, error) {
 	netCache := &types.CachedNetConf{}
-	netConfBytes, err := utils.ReadCache(cRef)
+	netConfBytes, err := utils.ReadCache(cacheDir, cRef)
 	if err != nil {
 		return nil, fmt.Errorf("error reading cached NetConf with name %s: %v", cRef, err)
 	}

diff --git a/pkg/mirror-consumer/consumer.go b/pkg/mirror-consumer/consumer.go
@@ -98,7 +98,7 @@ func CmdAdd(args *skel.CmdArgs) error {
 	}
 
 	// Cache PrevResult for CmdDel
-	if err = utils.SaveCache(config.GetCRef(args.ContainerID, args.IfName)+"_cons",
+	if err = utils.SaveCache(netconf.CachePath, config.GetCRef(args.ContainerID, args.IfName)+"_cons",
 		&types.CachedPrevResultNetConf{PrevResult: netconf.PrevResult}); err != nil {
 		return fmt.Errorf("error saving NetConf %q", err)
 	}
@@ -138,9 +138,12 @@ func CmdAdd(args *skel.CmdArgs) error {
 // CmdDel remove handler for deleting container from network
 func CmdDel(args *skel.CmdArgs) error {
 	logCall("DEL", args)
-
+	stdinConf, err := config.LoadConf(args.StdinData)
+	if err != nil {
+		return err
+	}
 	cRef := config.GetCRef(args.ContainerID, args.IfName)
-	cache, err := config.LoadPrevResultConfFromCache(cRef + "_cons")
+	cache, err := config.LoadPrevResultConfFromCache(stdinConf.CachePath, cRef+"_cons")
 	if err != nil {
 		// If cmdDel() fails, cached prevResult is cleaned up by
 		// the followed defer call. However, subsequence calls
@@ -154,7 +157,7 @@ func CmdDel(args *skel.CmdArgs) error {
 
 	defer func() {
 		if err == nil {
-			if err := utils.CleanCache(cRef + "_cons"); err != nil {
+			if err := utils.CleanCache(stdinConf.CachePath, cRef+"_cons"); err != nil {
 				log.Printf("Failed cleaning up cache: %v", err)
 			}
 		}

diff --git a/pkg/mirror-producer/producer.go b/pkg/mirror-producer/producer.go
@@ -98,7 +98,7 @@ func CmdAdd(args *skel.CmdArgs) error {
 	}
 
 	// Cache PrevResult for CmdDel
-	if err = utils.SaveCache(config.GetCRef(args.ContainerID, args.IfName)+"_prod",
+	if err = utils.SaveCache(netconf.CachePath, config.GetCRef(args.ContainerID, args.IfName)+"_prod",
 		&types.CachedPrevResultNetConf{PrevResult: netconf.PrevResult}); err != nil {
 		return fmt.Errorf("error saving NetConf %q", err)
 	}
@@ -130,9 +130,13 @@ func CmdAdd(args *skel.CmdArgs) error {
 // CmdDel remove handler for deleting container from network
 func CmdDel(args *skel.CmdArgs) error {
 	logCall("DEL", args)
+	stdinConf, err := config.LoadConf(args.StdinData)
+	if err != nil {
+		return err
+	}
 
 	cRef := config.GetCRef(args.ContainerID, args.IfName)
-	cache, err := config.LoadPrevResultConfFromCache(cRef + "_prod")
+	cache, err := config.LoadPrevResultConfFromCache(stdinConf.CachePath, cRef+"_prod")
 	if err != nil {
 		// If cmdDel() fails, cached prevResult is cleaned up by
 		// the followed defer call. However, subsequence calls
@@ -146,7 +150,7 @@ func CmdDel(args *skel.CmdArgs) error {
 
 	defer func() {
 		if err == nil {
-			if err := utils.CleanCache(cRef + "_prod"); err != nil {
+			if err := utils.CleanCache(stdinConf.CachePath, cRef+"_prod"); err != nil {
 				log.Printf("Failed cleaning up cache: %v", err)
 			}
 		}

diff --git a/pkg/plugin/plugin.go b/pkg/plugin/plugin.go
@@ -287,7 +287,7 @@ func CmdAdd(args *skel.CmdArgs) error {
 	}
 
 	// Cache NetConf for CmdDel
-	if err = utils.SaveCache(config.GetCRef(args.ContainerID, args.IfName),
+	if err = utils.SaveCache(netconf.CachePath, config.GetCRef(args.ContainerID, args.IfName),
 		&types.CachedNetConf{Netconf: netconf, OrigIfName: origIfName}); err != nil {
 		return fmt.Errorf("error saving NetConf %q", err)
 	}
@@ -472,8 +472,13 @@ func removeOvsPort(ovsDriver *ovsdb.OvsBridgeDriver, portName string) error {
 func CmdDel(args *skel.CmdArgs) error {
 	logCall("DEL", args)
 
+	stdinConf, err := config.LoadConf(args.StdinData)
+	if err != nil {
+		return err
+	}
+
 	cRef := config.GetCRef(args.ContainerID, args.IfName)
-	cache, err := config.LoadConfFromCache(cRef)
+	cache, err := config.LoadConfFromCache(stdinConf.CachePath, cRef)
 	if err != nil {
 		// If cmdDel() fails, cached netconf is cleaned up by
 		// the followed defer call. However, subsequence calls
@@ -487,7 +492,7 @@ func CmdDel(args *skel.CmdArgs) error {
 
 	defer func() {
 		if err == nil {
-			if err := utils.CleanCache(cRef); err != nil {
+			if err := utils.CleanCache(stdinConf.CachePath, cRef); err != nil {
 				log.Printf("Failed cleaning up cache: %v", err)
 			}
 		}
@@ -616,7 +621,7 @@ func CmdCheck(args *skel.CmdArgs) error {
 
 	// check cache
 	cRef := config.GetCRef(args.ContainerID, args.IfName)
-	cache, err := config.LoadConfFromCache(cRef)
+	cache, err := config.LoadConfFromCache(netconf.CachePath, cRef)
 	if err != nil {
 		return err
 	}

diff --git a/pkg/plugin/plugin_test.go b/pkg/plugin/plugin_test.go
@@ -19,9 +19,9 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"github.com/k8snetworkplumbingwg/ovs-cni/pkg/config"
 	"math/rand"
 	"net"
+	"os"
 	"os/exec"
 	"strconv"
 	"strings"
@@ -41,6 +41,7 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
+	"github.com/k8snetworkplumbingwg/ovs-cni/pkg/config"
 	"github.com/k8snetworkplumbingwg/ovs-cni/pkg/types"
 )
 
@@ -73,6 +74,7 @@ type Net040 struct {
 	VlanTag       *uint                  `json:"vlan"`
 	Trunk         []*types.Trunk         `json:"trunk,omitempty"`
 	InterfaceType string                 `json:"interface_type"`
+	CachePath     string                 `json:"cache_path"`
 	RawPrevResult map[string]interface{} `json:"prevResult,omitempty"`
 	PrevResult    types040.Result        `json:"-"`
 }
@@ -86,6 +88,7 @@ type NetCurrent struct {
 	VlanTag       *uint                  `json:"vlan"`
 	Trunk         []*types.Trunk         `json:"trunk,omitempty"`
 	InterfaceType string                 `json:"interface_type"`
+	CachePath     string                 `json:"cache_path"`
 	RawPrevResult map[string]interface{} `json:"prevResult,omitempty"`
 	PrevResult    current.Result         `json:"-"`
 }
@@ -904,6 +907,47 @@ var testFunc = func(version string) {
 					ContainSubstring(secondHostIface.Name), "OVS port with healthy interface should have been kept")
 			})
 		})
+		Context("cache path configuration", func() {
+			var (
+				err     error
+				tempDir string
+			)
+			BeforeEach(func() {
+				tempDir, err = os.MkdirTemp("", "ovs-cni-test-*")
+				Expect(err).NotTo(HaveOccurred())
+			})
+			AfterEach(func() {
+				err = os.RemoveAll(tempDir)
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("should use provided path", func() {
+				conf := fmt.Sprintf(`{
+						"cniVersion": "%s",
+						"name": "mynet",
+						"type": "ovs",
+						"bridge": "%s",
+						"cache_path": "%s"
+					}`, version, bridgeName, tempDir)
+				targetNs := newNS()
+				defer func() {
+					closeNS(targetNs)
+				}()
+				hostIfName, result := testAdd(conf, false, false, "", targetNs)
+
+				// cmdAdd should create file in the configured cache dir
+				cacheFiles, err := os.ReadDir(tempDir)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(cacheFiles).To(HaveLen(1))
+
+				testCheck(conf, result, targetNs)
+				testDel(conf, hostIfName, targetNs, true)
+				// cmdDel should remove file from the cache dir
+				cacheFiles, err = os.ReadDir(tempDir)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(cacheFiles).To(BeEmpty())
+			})
+		})
 	})
 }
 

diff --git a/pkg/types/types.go b/pkg/types/types.go
@@ -40,6 +40,7 @@ type NetConf struct {
 	SocketFile             string   `json:"socket_file"`
 	LinkStateCheckRetries  int      `json:"link_state_check_retries"`
 	LinkStateCheckInterval int      `json:"link_state_check_interval"`
+	CachePath              string   `json:"cache_path"` // path to the cache folder, defaults to /tmp/ovscache
 }
 
 // MirrorNetConf extends types.NetConf for ovs-mirrors
@@ -55,6 +56,7 @@ type MirrorNetConf struct {
 	ConfigurationPath string    `json:"configuration_path"`
 	SocketFile        string    `json:"socket_file"`
 	Mirrors           []*Mirror `json:"mirrors"`
+	CachePath         string    `json:"cache_path"` // path to the cache folder, defaults to /tmp/ovscache
 }
 
 // Mirror configuration

diff --git a/pkg/utils/cache.go b/pkg/utils/cache.go
@@ -30,17 +30,18 @@ var (
 )
 
 // SaveCache takes in key as string and a json encoded struct Conf and save this Conf in cache dir
-func SaveCache(key string, conf interface{}) error {
+func SaveCache(cacheDir, key string, conf interface{}) error {
+	cacheDir = getCacheDirPath(cacheDir)
 	confBytes, err := json.Marshal(conf)
 	if err != nil {
 		return fmt.Errorf("error serializing delegate conf: %v", err)
 	}
 
 	// save the rendered conf for cmdDel
-	if err = os.MkdirAll(DefaultCacheDir, 0700); err != nil {
-		return fmt.Errorf("failed to create the sriov data directory(%q): %v", DefaultCacheDir, err)
+	if err = os.MkdirAll(cacheDir, 0700); err != nil {
+		return fmt.Errorf("failed to create the sriov data directory(%q): %v", cacheDir, err)
 	}
-	path := getKeyPath(key)
+	path := getKeyPath(cacheDir, key)
 	err = ioutil.WriteFile(path, confBytes, 0600)
 	if err != nil {
 		return fmt.Errorf("failed to write container data in the path(%q): %v", path, err)
@@ -49,8 +50,8 @@ func SaveCache(key string, conf interface{}) error {
 }
 
 // ReadCache read cached conf from disk for the given key and returns data in byte array
-func ReadCache(key string) ([]byte, error) {
-	path := getKeyPath(key)
+func ReadCache(cacheDir, key string) ([]byte, error) {
+	path := getKeyPath(getCacheDirPath(cacheDir), key)
 	data, err := ioutil.ReadFile(path)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read container data in the path(%q): %v", path, err)
@@ -59,14 +60,21 @@ func ReadCache(key string) ([]byte, error) {
 }
 
 // CleanCache removes cached conf from disk for the given key
-func CleanCache(key string) error {
-	path := getKeyPath(key)
+func CleanCache(cacheDir, key string) error {
+	path := getKeyPath(getCacheDirPath(cacheDir), key)
 	if err := os.Remove(path); err != nil {
 		return fmt.Errorf("error removing Conf file %s: %q", path, err)
 	}
 	return nil
 }
 
-func getKeyPath(key string) string {
-	return filepath.Join(DefaultCacheDir, key)
+func getKeyPath(cacheDir, key string) string {
+	return filepath.Join(getCacheDirPath(cacheDir), key)
+}
+
+func getCacheDirPath(cacheDir string) string {
+	if cacheDir == "" {
+		cacheDir = DefaultCacheDir
+	}
+	return cacheDir
 }