From 36b04ff46c29a451a11f65f496a66b2a206ac3e3 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Fri, 9 Jul 2021 18:00:56 -0700 Subject: [PATCH 1/6] removed lock for version command --- cni/network/network.go | 79 ++++++++++++++++++++++++++++++++++---- cni/network/plugin/main.go | 29 -------------- store/json.go | 9 ++++- 3 files changed, 80 insertions(+), 37 deletions(-) diff --git a/cni/network/network.go b/cni/network/network.go index 1da16a9efd..342731384f 100644 --- a/cni/network/network.go +++ b/cni/network/network.go @@ -41,6 +41,7 @@ const ( ipVersion = "4" ipamV6 = "azure-vnet-ipamv6" defaultRequestTimeout = 15 * time.Second + name = "azure-vnet" ) // CNI Operation Types @@ -145,12 +146,6 @@ func (plugin *netPlugin) Start(config *common.PluginConfig) error { common.LogNetworkInterfaces() // Initialize network manager. - err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot) - if err != nil { - log.Printf("[cni-net] Failed to initialize network manager, err:%v.", err) - return err - } - log.Printf("[cni-net] Plugin started.") return nil @@ -282,7 +277,6 @@ func (plugin *netPlugin) setCNIReportDetails(nwCfg *cni.NetworkConfig, opType st plugin.report.SubContext = fmt.Sprintf("%+v", nwCfg) plugin.report.EventMessage = msg plugin.report.BridgeDetails.NetworkMode = nwCfg.Mode - plugin.report.InterfaceDetails.SecondaryCAUsedCount = plugin.nm.GetNumberOfEndpoints("", nwCfg.Name) } func addNatIPV6SubnetInfo(nwCfg *cni.NetworkConfig, @@ -301,6 +295,30 @@ func addNatIPV6SubnetInfo(nwCfg *cni.NetworkConfig, } } +func acquireLockForStore(config *common.PluginConfig, plugin *netPlugin) error { + var err error + if err = plugin.InitializeKeyValueStore(config); err != nil { + log.Errorf("Failed to initialize key-value store of network plugin, err:%v.\n", err) + + if isSafe, _ := plugin.IsSafeToRemoveLock(name); isSafe { + log.Printf("[CNI] Removing lock file as process holding lock exited") + if err = plugin.UninitializeKeyValueStore(true); err != nil { + log.Errorf("Failed to uninitialize key-value store of network plugin, err:%v.\n", err) + } + } + } + + return err +} + +func releaseLockForStore(plugin *netPlugin) error { + var err error + if err = plugin.UninitializeKeyValueStore(false); err != nil { + log.Errorf("Failed to uninitialize key-value store of network plugin, err:%v.\n", err) + } + + return err +} // // CNI implementation // https://github.com/containernetworking/cni/blob/master/SPEC.md @@ -349,6 +367,18 @@ func (plugin *netPlugin) Add(args *cniSkel.CmdArgs) error { iptables.DisableIPTableLock = nwCfg.DisableIPTableLock plugin.setCNIReportDetails(nwCfg, CNI_ADD, "") + // acquire cni lock file + config := &common.PluginConfig{} + acquireLockForStore(config, plugin) + defer releaseLockForStore(plugin) + + // restore network state + err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot) + if err != nil { + log.Printf("[cni-net] Failed to initialize network manager, err:%v.", err) + return err + } + defer func() { operationTimeMs := time.Since(startTime).Milliseconds() cniMetric.Metric = aitelemetry.Metric{ @@ -774,6 +804,17 @@ func (plugin *netPlugin) Get(args *cniSkel.CmdArgs) error { iptables.DisableIPTableLock = nwCfg.DisableIPTableLock + config := &common.PluginConfig{} + acquireLockForStore(config, plugin) + defer releaseLockForStore(plugin) + + // restore network state + err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot) + if err != nil { + log.Printf("[cni-net] Failed to initialize network manager, err:%v.", err) + return err + } + // Parse Pod arguments. if k8sPodName, k8sNamespace, err = plugin.getPodInfo(args.Args); err != nil { return err @@ -859,6 +900,18 @@ func (plugin *netPlugin) Delete(args *cniSkel.CmdArgs) error { log.Printf("[cni-net] Read network configuration %+v.", nwCfg) + // acquire cni lock file + config := &common.PluginConfig{} + acquireLockForStore(config, plugin) + defer releaseLockForStore(plugin) + + // restore network state + err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot) + if err != nil { + log.Printf("[cni-net] Failed to initialize network manager, err:%v.", err) + return err + } + // Parse Pod arguments. if k8sPodName, k8sNamespace, err = plugin.getPodInfo(args.Args); err != nil { log.Printf("[cni-net] Failed to get POD info due to error: %v", err) @@ -1021,6 +1074,18 @@ func (plugin *netPlugin) Update(args *cniSkel.CmdArgs) error { iptables.DisableIPTableLock = nwCfg.DisableIPTableLock plugin.setCNIReportDetails(nwCfg, CNI_UPDATE, "") + // acquire cni lock file + config := &common.PluginConfig{} + acquireLockForStore(config, plugin) + defer releaseLockForStore(plugin) + + // restore network state + err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot) + if err != nil { + log.Printf("[cni-net] Failed to initialize network manager, err:%v.", err) + return err + } + defer func() { operationTimeMs := time.Since(startTime).Milliseconds() cniMetric.Metric = aitelemetry.Metric{ diff --git a/cni/network/plugin/main.go b/cni/network/plugin/main.go index 98b8ec6a9f..fe71d26a8a 100644 --- a/cni/network/plugin/main.go +++ b/cni/network/plugin/main.go @@ -181,35 +181,6 @@ func main() { return } - // CNI Acquires lock - if err = netPlugin.Plugin.InitializeKeyValueStore(&config); err != nil { - log.Errorf("Failed to initialize key-value store of network plugin, err:%v.\n", err) - tb := telemetry.NewTelemetryBuffer() - if tberr := tb.Connect(); tberr == nil { - reportPluginError(reportManager, tb, err) - tb.Close() - } - - if isSafe, _ := netPlugin.Plugin.IsSafeToRemoveLock(name); isSafe { - log.Printf("[CNI] Removing lock file as process holding lock exited") - if errUninit := netPlugin.Plugin.UninitializeKeyValueStore(true); errUninit != nil { - log.Errorf("Failed to uninitialize key-value store of network plugin, err:%v.\n", errUninit) - } - } - - return - } - - defer func() { - if errUninit := netPlugin.Plugin.UninitializeKeyValueStore(false); errUninit != nil { - log.Errorf("Failed to uninitialize key-value store of network plugin, err:%v.\n", errUninit) - } - - if recover() != nil { - os.Exit(1) - } - }() - // Start telemetry process if not already started. This should be done inside lock, otherwise multiple process // end up creating/killing telemetry process results in undesired state. tb := telemetry.NewTelemetryBuffer() diff --git a/store/json.go b/store/json.go index 8cf27affbc..ce12a7d406 100644 --- a/store/json.go +++ b/store/json.go @@ -221,8 +221,15 @@ func (kvs *jsonFileStore) Lock(block bool) error { defer lockFile.Close() + currentPid := os.Getpid() + log.Printf("Write pid %d to lockfile", currentPid) // Write the process ID for easy identification. - if _, err = lockFile.WriteString(strconv.Itoa(os.Getpid())); err != nil { + if _, err = lockFile.WriteString(strconv.Itoa(currentPid)); err != nil { + // remove lockfile + log.Errorf("Write to lockfile failed:%+v", err) + if errRem := os.Remove(kvs.lockFileName); errRem != nil { + log.Errorf("removing lockfile failed:%+v", errRem) + } return err } From 1651765651ccbf356ebf5f8b13f39285d1cc6680 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Fri, 9 Jul 2021 18:03:49 -0700 Subject: [PATCH 2/6] updated variable name --- cni/network/network.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cni/network/network.go b/cni/network/network.go index 342731384f..a57f388d21 100644 --- a/cni/network/network.go +++ b/cni/network/network.go @@ -41,7 +41,7 @@ const ( ipVersion = "4" ipamV6 = "azure-vnet-ipamv6" defaultRequestTimeout = 15 * time.Second - name = "azure-vnet" + azureCniName = "azure-vnet" ) // CNI Operation Types @@ -300,7 +300,7 @@ func acquireLockForStore(config *common.PluginConfig, plugin *netPlugin) error { if err = plugin.InitializeKeyValueStore(config); err != nil { log.Errorf("Failed to initialize key-value store of network plugin, err:%v.\n", err) - if isSafe, _ := plugin.IsSafeToRemoveLock(name); isSafe { + if isSafe, _ := plugin.IsSafeToRemoveLock(azureCniName); isSafe { log.Printf("[CNI] Removing lock file as process holding lock exited") if err = plugin.UninitializeKeyValueStore(true); err != nil { log.Errorf("Failed to uninitialize key-value store of network plugin, err:%v.\n", err) From 96c9ba96c22a447247395f052805ec123f9c2f32 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Mon, 12 Jul 2021 11:31:33 -0700 Subject: [PATCH 3/6] catch and return error on lock --- cni/network/network.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/cni/network/network.go b/cni/network/network.go index a57f388d21..79fa5194fb 100644 --- a/cni/network/network.go +++ b/cni/network/network.go @@ -369,7 +369,11 @@ func (plugin *netPlugin) Add(args *cniSkel.CmdArgs) error { // acquire cni lock file config := &common.PluginConfig{} - acquireLockForStore(config, plugin) + if err := acquireLockForStore(config, plugin); err != nil { + log.Errorf("Couldn't acquire lock: %+v", err) + return err + } + defer releaseLockForStore(plugin) // restore network state @@ -805,7 +809,11 @@ func (plugin *netPlugin) Get(args *cniSkel.CmdArgs) error { iptables.DisableIPTableLock = nwCfg.DisableIPTableLock config := &common.PluginConfig{} - acquireLockForStore(config, plugin) + if err := acquireLockForStore(config, plugin); err != nil { + log.Errorf("Couldn't acquire lock: %+v", err) + return err + } + defer releaseLockForStore(plugin) // restore network state @@ -902,7 +910,11 @@ func (plugin *netPlugin) Delete(args *cniSkel.CmdArgs) error { // acquire cni lock file config := &common.PluginConfig{} - acquireLockForStore(config, plugin) + if err := acquireLockForStore(config, plugin); err != nil { + log.Errorf("Couldn't acquire lock: %+v", err) + return err + } + defer releaseLockForStore(plugin) // restore network state @@ -1076,7 +1088,11 @@ func (plugin *netPlugin) Update(args *cniSkel.CmdArgs) error { // acquire cni lock file config := &common.PluginConfig{} - acquireLockForStore(config, plugin) + if err := acquireLockForStore(config, plugin); err != nil { + log.Errorf("Couldn't acquire lock: %+v", err) + return err + } + defer releaseLockForStore(plugin) // restore network state From 0a802d957cd3fa0f2076db65030527d0ec486e3e Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Mon, 12 Jul 2021 12:16:06 -0700 Subject: [PATCH 4/6] added log for release lock file --- cni/network/network.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cni/network/network.go b/cni/network/network.go index 79fa5194fb..62133d3bba 100644 --- a/cni/network/network.go +++ b/cni/network/network.go @@ -315,9 +315,11 @@ func releaseLockForStore(plugin *netPlugin) error { var err error if err = plugin.UninitializeKeyValueStore(false); err != nil { log.Errorf("Failed to uninitialize key-value store of network plugin, err:%v.\n", err) + return err } - return err + log.Printf("Released lock file") + return nil } // // CNI implementation From 91bf31d341e898a19b19bc881bc8a725b715d473 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Mon, 12 Jul 2021 14:00:57 -0700 Subject: [PATCH 5/6] separated out lock and store initialization --- cni/network/network.go | 30 +++++++++++++++++------------- cni/network/plugin/main.go | 21 +++++++++++++++++++++ cni/plugin.go | 15 --------------- 3 files changed, 38 insertions(+), 28 deletions(-) diff --git a/cni/network/network.go b/cni/network/network.go index 62133d3bba..181a9354d0 100644 --- a/cni/network/network.go +++ b/cni/network/network.go @@ -297,13 +297,14 @@ func addNatIPV6SubnetInfo(nwCfg *cni.NetworkConfig, func acquireLockForStore(config *common.PluginConfig, plugin *netPlugin) error { var err error - if err = plugin.InitializeKeyValueStore(config); err != nil { - log.Errorf("Failed to initialize key-value store of network plugin, err:%v.\n", err) - + if err = plugin.Store.Lock(true); err != nil { + log.Printf("[CNI] Failed to lock store: %v. check if process running", err) if isSafe, _ := plugin.IsSafeToRemoveLock(azureCniName); isSafe { log.Printf("[CNI] Removing lock file as process holding lock exited") - if err = plugin.UninitializeKeyValueStore(true); err != nil { - log.Errorf("Failed to uninitialize key-value store of network plugin, err:%v.\n", err) + if errUninit := releaseLockForStore(plugin); errUninit != nil { + log.Errorf("Failed to release lock file, err:%v.\n", errUninit) + } else { + err = nil } } } @@ -312,10 +313,12 @@ func acquireLockForStore(config *common.PluginConfig, plugin *netPlugin) error { } func releaseLockForStore(plugin *netPlugin) error { - var err error - if err = plugin.UninitializeKeyValueStore(false); err != nil { - log.Errorf("Failed to uninitialize key-value store of network plugin, err:%v.\n", err) - return err + if plugin.Store != nil { + err := plugin.Store.Unlock(false) + if err != nil { + log.Printf("[cni] Failed to unlock store: %v.", err) + return err + } } log.Printf("Released lock file") @@ -370,7 +373,8 @@ func (plugin *netPlugin) Add(args *cniSkel.CmdArgs) error { plugin.setCNIReportDetails(nwCfg, CNI_ADD, "") // acquire cni lock file - config := &common.PluginConfig{} + // TODO: check if we need pluginconfig and if not remove it + config := &common.PluginConfig{Store: plugin.Store} if err := acquireLockForStore(config, plugin); err != nil { log.Errorf("Couldn't acquire lock: %+v", err) return err @@ -810,7 +814,7 @@ func (plugin *netPlugin) Get(args *cniSkel.CmdArgs) error { iptables.DisableIPTableLock = nwCfg.DisableIPTableLock - config := &common.PluginConfig{} + config := &common.PluginConfig{Store: plugin.Store} if err := acquireLockForStore(config, plugin); err != nil { log.Errorf("Couldn't acquire lock: %+v", err) return err @@ -911,7 +915,7 @@ func (plugin *netPlugin) Delete(args *cniSkel.CmdArgs) error { log.Printf("[cni-net] Read network configuration %+v.", nwCfg) // acquire cni lock file - config := &common.PluginConfig{} + config := &common.PluginConfig{Store: plugin.Store} if err := acquireLockForStore(config, plugin); err != nil { log.Errorf("Couldn't acquire lock: %+v", err) return err @@ -1089,7 +1093,7 @@ func (plugin *netPlugin) Update(args *cniSkel.CmdArgs) error { plugin.setCNIReportDetails(nwCfg, CNI_UPDATE, "") // acquire cni lock file - config := &common.PluginConfig{} + config := &common.PluginConfig{Store: plugin.Store} if err := acquireLockForStore(config, plugin); err != nil { log.Errorf("Couldn't acquire lock: %+v", err) return err diff --git a/cni/network/plugin/main.go b/cni/network/plugin/main.go index fe71d26a8a..d5486d90a2 100644 --- a/cni/network/plugin/main.go +++ b/cni/network/plugin/main.go @@ -180,6 +180,27 @@ func main() { log.Printf("Failed to create network plugin, err:%v.\n", err) return } + // CNI initializes store + if err = netPlugin.Plugin.InitializeKeyValueStore(&config); err != nil { + log.Errorf("Failed to initialize key-value store of network plugin, err:%v.\n", err) + tb := telemetry.NewTelemetryBuffer() + if tberr := tb.Connect(); tberr == nil { + reportPluginError(reportManager, tb, err) + tb.Close() + } + + return + } + + defer func() { + if errUninit := netPlugin.Plugin.UninitializeKeyValueStore(false); errUninit != nil { + log.Errorf("Failed to uninitialize key-value store of network plugin, err:%v.\n", errUninit) + } + + if recover() != nil { + os.Exit(1) + } + }() // Start telemetry process if not already started. This should be done inside lock, otherwise multiple process // end up creating/killing telemetry process results in undesired state. diff --git a/cni/plugin.go b/cni/plugin.go index 855ddb2012..373c14c939 100644 --- a/cni/plugin.go +++ b/cni/plugin.go @@ -163,28 +163,13 @@ func (plugin *Plugin) InitializeKeyValueStore(config *common.PluginConfig) error removeLockFileAfterReboot(plugin) } - // Acquire store lock. - if err := plugin.Store.Lock(true); err != nil { - log.Printf("[cni] Failed to lock store: %v.", err) - return err - } - config.Store = plugin.Store - return nil } // Uninitialize key-value store func (plugin *Plugin) UninitializeKeyValueStore(force bool) error { - if plugin.Store != nil { - err := plugin.Store.Unlock(force) - if err != nil { - log.Printf("[cni] Failed to unlock store: %v.", err) - return err - } - } plugin.Store = nil - return nil } From a342c91cac07f3dde3b046f262d5c2410d0a7b44 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Mon, 12 Jul 2021 14:54:14 -0700 Subject: [PATCH 6/6] addressed comments --- cni/network/network.go | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/cni/network/network.go b/cni/network/network.go index 181a9354d0..69d8b9dd15 100644 --- a/cni/network/network.go +++ b/cni/network/network.go @@ -301,10 +301,8 @@ func acquireLockForStore(config *common.PluginConfig, plugin *netPlugin) error { log.Printf("[CNI] Failed to lock store: %v. check if process running", err) if isSafe, _ := plugin.IsSafeToRemoveLock(azureCniName); isSafe { log.Printf("[CNI] Removing lock file as process holding lock exited") - if errUninit := releaseLockForStore(plugin); errUninit != nil { - log.Errorf("Failed to release lock file, err:%v.\n", errUninit) - } else { - err = nil + if err = releaseLockForStore(plugin); err != nil { + log.Errorf("Failed to release lock file, err:%v.\n", err) } } } @@ -383,9 +381,8 @@ func (plugin *netPlugin) Add(args *cniSkel.CmdArgs) error { defer releaseLockForStore(plugin) // restore network state - err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot) - if err != nil { - log.Printf("[cni-net] Failed to initialize network manager, err:%v.", err) + if err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot); err != nil { + log.Printf("[cni-net] Failed to initialize network manager, err:%+v.", err) return err } @@ -823,9 +820,8 @@ func (plugin *netPlugin) Get(args *cniSkel.CmdArgs) error { defer releaseLockForStore(plugin) // restore network state - err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot) - if err != nil { - log.Printf("[cni-net] Failed to initialize network manager, err:%v.", err) + if err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot); err != nil { + log.Printf("[cni-net] Failed to initialize network manager, err:%+v.", err) return err } @@ -924,9 +920,8 @@ func (plugin *netPlugin) Delete(args *cniSkel.CmdArgs) error { defer releaseLockForStore(plugin) // restore network state - err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot) - if err != nil { - log.Printf("[cni-net] Failed to initialize network manager, err:%v.", err) + if err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot); err != nil { + log.Printf("[cni-net] Failed to initialize network manager, err:%+v.", err) return err } @@ -1102,9 +1097,8 @@ func (plugin *netPlugin) Update(args *cniSkel.CmdArgs) error { defer releaseLockForStore(plugin) // restore network state - err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot) - if err != nil { - log.Printf("[cni-net] Failed to initialize network manager, err:%v.", err) + if err = plugin.nm.Initialize(config, rehydrateNetworkInfoOnReboot); err != nil { + log.Printf("[cni-net] Failed to initialize network manager, err:%+v.", err) return err }