Merge pull request #1090 from squeed/gc-improvements

Spec, libcni: add disableGC flag

Author: squeed

SPEC.md

@@ -111,6 +111,7 @@ A network configuration consists of a JSON object with the following keys:
 - `cniVersions` (string list): List of all CNI versions which this configuration supports. See [version selection](#version-selection) below.
 - `name` (string): Network name. This should be unique across all network configurations on a host (or other administrative domain).  Must start with an alphanumeric character, optionally followed by any combination of one or more alphanumeric characters, underscore, dot (.) or hyphen (-).
 - `disableCheck` (boolean): Either `true` or `false`.  If `disableCheck` is `true`, runtimes must not call `CHECK` for this network configuration list.  This allows an administrator to prevent `CHECK`ing where a combination of plugins is known to return spurious errors.
+- `disableGC` (boolean): Either `true` or `false`.  If `disableGC` is `true`, runtimes must not call `GC` for this network configuration list.  This allows an administrator to prevent `GC`ing when it is known that garbage collection may have undesired effects (e.g. shared configuration between multiple runtimes).
 - `plugins` (list): A list of CNI plugins and their configuration, which is a list of plugin configuration objects.
 
 #### Plugin configuration objects:

libcni/api.go

@@ -76,6 +76,7 @@ type NetworkConfigList struct {
 	Name         string
 	CNIVersion   string
 	DisableCheck bool
+	DisableGC    bool
 	Plugins      []*NetworkConfig
 	Bytes        []byte
 }
@@ -425,6 +426,9 @@ func (c *CNIConfig) GetCachedAttachments(containerID string) ([]*NetworkAttachme
 	dirPath := filepath.Join(c.getCacheDir(&RuntimeConf{}), "results")
 	entries, err := os.ReadDir(dirPath)
 	if err != nil {
+		if os.IsNotExist(err) {
+			return nil, nil
+		}
 		return nil, err
 	}
 
@@ -759,6 +763,11 @@ func (c *CNIConfig) GetVersionInfo(ctx context.Context, pluginType string) (vers
 // - dump the list of cached attachments, and issue deletes as necessary
 // - issue a GC to the underlying plugins (if the version is high enough)
 func (c *CNIConfig) GCNetworkList(ctx context.Context, list *NetworkConfigList, args *GCArgs) error {
+	// If DisableGC is set, then don't bother GCing at all.
+	if list.DisableGC {
+		return nil
+	}
+
 	// First, get the list of cached attachments
 	cachedAttachments, err := c.GetCachedAttachments("")
 	if err != nil {

libcni/api_test.go

@@ -1511,8 +1511,19 @@ var _ = Describe("Invoking plugins", func() {
 				_, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig)
 				Expect(err).NotTo(HaveOccurred())
 
+				By("Issuing a GC with disableGC=true")
+				netConfigList.DisableGC = true
+				gcargs := &libcni.GCArgs{}
+				err = cniConfig.GCNetworkList(ctx, netConfigList, gcargs)
+				Expect(err).NotTo(HaveOccurred())
+
+				commands, err := noop_debug.ReadCommandLog(plugins[0].commandFilePath)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(commands).To(HaveLen(1)) // ADD
+
 				By("Issuing a GC with valid networks")
-				gcargs := &libcni.GCArgs{
+				netConfigList.DisableGC = false
+				gcargs = &libcni.GCArgs{
 					ValidAttachments: []types.GCAttachment{{
 						ContainerID: runtimeConfig.ContainerID,
 						IfName:      runtimeConfig.IfName,
@@ -1526,7 +1537,7 @@ var _ = Describe("Invoking plugins", func() {
 				err = cniConfig.GCNetworkList(ctx, netConfigList, gcargs)
 				Expect(err).NotTo(HaveOccurred())
 
-				commands, err := noop_debug.ReadCommandLog(plugins[0].commandFilePath)
+				commands, err = noop_debug.ReadCommandLog(plugins[0].commandFilePath)
 				Expect(err).NotTo(HaveOccurred())
 				Expect(commands).To(HaveLen(4))
 

libcni/conf.go

@@ -130,28 +130,43 @@ func ConfListFromBytes(bytes []byte) (*NetworkConfigList, error) {
 		}
 	}
 
-	disableCheck := false
-	if rawDisableCheck, ok := rawList["disableCheck"]; ok {
-		disableCheck, ok = rawDisableCheck.(bool)
+	readBool := func(key string) (bool, error) {
+		rawVal, ok := rawList[key]
 		if !ok {
-			disableCheckStr, ok := rawDisableCheck.(string)
-			if !ok {
-				return nil, fmt.Errorf("error parsing configuration list: invalid disableCheck type %T", rawDisableCheck)
-			}
-			switch {
-			case strings.ToLower(disableCheckStr) == "false":
-				disableCheck = false
-			case strings.ToLower(disableCheckStr) == "true":
-				disableCheck = true
-			default:
-				return nil, fmt.Errorf("error parsing configuration list: invalid disableCheck value %q", disableCheckStr)
-			}
+			return false, nil
+		}
+		if b, ok := rawVal.(bool); ok {
+			return b, nil
+		}
+
+		s, ok := rawVal.(string)
+		if !ok {
+			return false, fmt.Errorf("error parsing configuration list: invalid type %T for %s", rawVal, key)
+		}
+		s = strings.ToLower(s)
+		switch s {
+		case "false":
+			return false, nil
+		case "true":
+			return true, nil
 		}
+		return false, fmt.Errorf("error parsing configuration list: invalid value %q for %s", s, key)
+	}
+
+	disableCheck, err := readBool("disableCheck")
+	if err != nil {
+		return nil, err
+	}
+
+	disableGC, err := readBool("disableGC")
+	if err != nil {
+		return nil, err
 	}
 
 	list := &NetworkConfigList{
 		Name:         name,
 		DisableCheck: disableCheck,
+		DisableGC:    disableGC,
 		CNIVersion:   cniVersion,
 		Bytes:        bytes,
 	}

libcni/conf_test.go

@@ -386,7 +386,7 @@ var _ = Describe("Loading configuration from disk", func() {
 				Expect(os.WriteFile(filepath.Join(configDir, "50-whatever.conflist"), configList, 0o600)).To(Succeed())
 
 				_, err := libcni.LoadConfList(configDir, "some-list")
-				Expect(err).To(MatchError(fmt.Sprintf("error parsing configuration list: invalid disableCheck value \"%s\"", badValue)))
+				Expect(err).To(MatchError(`error parsing configuration list: invalid value "adsfasdfasf" for disableCheck`))
 			})
 		})
 	})

pkg/types/types.go

@@ -119,6 +119,7 @@ type NetConfList struct {
 
 	Name         string     `json:"name,omitempty"`
 	DisableCheck bool       `json:"disableCheck,omitempty"`
+	DisableGC    bool       `json:"disableGC,omitempty"`
 	Plugins      []*NetConf `json:"plugins,omitempty"`
 }