From 62f98a1c11385f1f20d1696edcd11e1defc9bb09 Mon Sep 17 00:00:00 2001 From: Danny Canter Date: Wed, 15 Mar 2023 19:50:11 -0700 Subject: [PATCH] CRI: Don't always close netConfMonitor channel 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 --- pkg/cri/sbserver/service.go | 15 +++++++++++---- pkg/cri/sbserver/service_other.go | 6 ++++-- pkg/cri/sbserver/service_windows.go | 2 +- pkg/cri/server/service.go | 15 +++++++++++---- 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/pkg/cri/sbserver/service.go b/pkg/cri/sbserver/service.go index 9154ddb6c..1967d430e 100644 --- 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 index 6118a4d60..053178600 100644 --- 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 index f46ee2f5c..2f73d248a 100644 --- 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 index 1e2d7a31d..7937a2358 100644 --- 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")