CRI: Don't always close netConfMonitor channel

dcantah · dcantah · commit 62f98a1c1138 · 2023-03-15T20:01:17.000-07:00
In the CRI server initialization a syncgroup is setup that adds to the
counter for every cni config found/registered. This functions on platforms
where CNI is supported/theres an assumption that there will always be
the loopback config. However, on platforms like Darwin where there's generally
nothing registered the Wait() on the syncgroup returns immediately and the
channel used to return any Network config sync errors is closed. This channel
is one of three that's used to monitor if we should Close the CRI service in
containerd, so it's not great if this happens.

Signed-off-by: Danny Canter &lt;danny@dcantah.dev&gt;
diff --git a/pkg/cri/sbserver/service.go b/pkg/cri/sbserver/service.go
@@ -260,10 +260,17 @@ func (c *criService) Run() error {
 			netSyncGroup.Done()
 		}(h)
 	}
-	go func() {
-		netSyncGroup.Wait()
-		close(cniNetConfMonitorErrCh)
-	}()
+	// For platforms that may not support CNI (darwin etc.) there's no
+	// use in launching this as `Wait` will return immediately. Further
+	// down we select on this channel along with some others to determine
+	// if we should Close() the CRI service, so closing this preemptively
+	// isn't good.
+	if len(c.cniNetConfMonitor) > 0 {
+		go func() {
+			netSyncGroup.Wait()
+			close(cniNetConfMonitorErrCh)
+		}()
+	}
 
 	// Start streaming server.
 	logrus.Info("Start streaming server")
diff --git a/pkg/cri/sbserver/service_other.go b/pkg/cri/sbserver/service_other.go
@@ -22,12 +22,14 @@ import (
 	"github.com/containerd/go-cni"
 )
 
-// initPlatform handles linux specific initialization for the CRI service.
+// initPlatform handles initialization of the CRI service for non-windows
+// and non-linux platforms.
 func (c *criService) initPlatform() error {
 	return nil
 }
 
-// cniLoadOptions returns cni load options for the linux.
+// cniLoadOptions returns cni load options for non-windows and non-linux
+// platforms.
 func (c *criService) cniLoadOptions() []cni.Opt {
 	return []cni.Opt{}
 }
diff --git a/pkg/cri/sbserver/service_windows.go b/pkg/cri/sbserver/service_windows.go
@@ -26,7 +26,7 @@ import (
 // attaches to
 const windowsNetworkAttachCount = 1
 
-// initPlatform handles linux specific initialization for the CRI service.
+// initPlatform handles windows specific initialization for the CRI service.
 func (c *criService) initPlatform() error {
 	pluginDirs := map[string]string{
 		defaultNetworkPlugin: c.config.NetworkPluginConfDir,
diff --git a/pkg/cri/server/service.go b/pkg/cri/server/service.go
@@ -237,10 +237,17 @@ func (c *criService) Run() error {
 			netSyncGroup.Done()
 		}(h)
 	}
-	go func() {
-		netSyncGroup.Wait()
-		close(cniNetConfMonitorErrCh)
-	}()
+	// For platforms that may not support CNI (darwin etc.) there's no
+	// use in launching this as `Wait` will return immediately. Further
+	// down we select on this channel along with some others to determine
+	// if we should Close() the CRI service, so closing this preemptively
+	// isn't good.
+	if len(c.cniNetConfMonitor) > 0 {
+		go func() {
+			netSyncGroup.Wait()
+			close(cniNetConfMonitorErrCh)
+		}()
+	}
 
 	// Start streaming server.
 	logrus.Info("Start streaming server")

Original file line number	Diff line number	Diff line change
`@@ -22,12 +22,14 @@ import (`
`22`	`22`	`"github.com/containerd/go-cni"`
`23`	`23`	`)`
`24`	`24`
`25`		`-// initPlatform handles linux specific initialization for the CRI service.`
	`25`	`+// initPlatform handles initialization of the CRI service for non-windows`
	`26`	`+// and non-linux platforms.`
`26`	`27`	`func (c *criService) initPlatform() error {`
`27`	`28`	`return nil`
`28`	`29`	`}`
`29`	`30`
`30`		`-// cniLoadOptions returns cni load options for the linux.`
	`31`	`+// cniLoadOptions returns cni load options for non-windows and non-linux`
	`32`	`+// platforms.`
`31`	`33`	`func (c *criService) cniLoadOptions() []cni.Opt {`
`32`	`34`	`return []cni.Opt{}`
`33`	`35`	`}`