From 5fe3bd97d58b96b11e9c2cc99401dbb11a15fb45 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Fri, 24 Sep 2021 11:40:15 -0700 Subject: [PATCH 1/7] lock fix changes --- cni/plugin.go | 48 +++++++++++++++------ cni/plugin_test.go | 83 +++++++++++++++++++++++++++++++++++++ cni/testfiles/testlock.lock | 1 + store/json.go | 2 +- store/mockstore.go | 51 +++++++++++++++++++++++ 5 files changed, 171 insertions(+), 14 deletions(-) create mode 100644 cni/plugin_test.go create mode 100644 cni/testfiles/testlock.lock create mode 100644 store/mockstore.go diff --git a/cni/plugin.go b/cni/plugin.go index d811155234..0c52c2fea1 100644 --- a/cni/plugin.go +++ b/cni/plugin.go @@ -20,8 +20,11 @@ import ( cniTypes "github.com/containernetworking/cni/pkg/types" cniTypesCurr "github.com/containernetworking/cni/pkg/types/current" cniVers "github.com/containernetworking/cni/pkg/version" + "github.com/pkg/errors" ) +var errEmptyContent = errors.New("Read content is zero bytes") + // Plugin is the parent class for CNI plugins. type Plugin struct { *common.Plugin @@ -197,26 +200,30 @@ func (plugin *Plugin) IsSafeToRemoveLock(processName string) (bool, error) { return false, cmdErr } - // Read pid from lockfile lockFileName := plugin.Store.GetLockFileName() - content, err := ioutil.ReadFile(lockFileName) + // Read pid from lockfile + lockFilePid, err := plugin.readLockFile(lockFileName) if err != nil { - log.Errorf("Failed to read lock file :%v, ", err) - return false, err - } - - if len(content) <= 0 { - log.Errorf("Num bytes read from lock file is 0") - return false, fmt.Errorf("Num bytes read from lock file is 0") + return false, errors.Wrap(err, "IsSafeToRemoveLock lockfile read failed") } - log.Printf("Read from Lock file:%s", content) + log.Printf("Read from Lock file:%s", lockFilePid) // Get the process name if running and // check if that matches with our expected process - pName, err := platform.GetProcessNameByID(string(content)) + pName, err := platform.GetProcessNameByID(lockFilePid) if err != nil { - return true, nil - // if process id changed. read lockfile? + var content string + content, err = plugin.readLockFile(lockFileName) + if err != nil { + return false, errors.Wrap(err, "IsSafeToRemoveLock lockfile 2nd read failed") + } + + if string(content) == lockFilePid { + return true, nil + } + + log.Printf("Lockfile content changed from %s to %s. So not safe to clean lock file", lockFilePid, content) + return false, nil } log.Printf("[CNI] Process name is %s", pName) @@ -229,3 +236,18 @@ func (plugin *Plugin) IsSafeToRemoveLock(processName string) (bool, error) { log.Errorf("Plugin store is nil") return false, fmt.Errorf("plugin store nil") } + +func (plugin *Plugin) readLockFile(filename string) (string, error) { + content, err := ioutil.ReadFile(filename) + if err != nil { + log.Errorf("Failed to read lock file :%v, ", err) + return "", fmt.Errorf("readLockFile error:%w", err) + } + + if len(content) == 0 { + log.Errorf("Num bytes read from lock file is 0") + return "", errEmptyContent + } + + return string(content), nil +} diff --git a/cni/plugin_test.go b/cni/plugin_test.go new file mode 100644 index 0000000000..2d41d56493 --- /dev/null +++ b/cni/plugin_test.go @@ -0,0 +1,83 @@ +package cni + +import ( + "github.com/Azure/azure-container-networking/common" + "github.com/Azure/azure-container-networking/store" + "github.com/stretchr/testify/require" + "os" + "testing" +) + +func TestMain(m *testing.M) { + // Run tests. + exitCode := m.Run() + os.Exit(exitCode) +} + +func TestPluginSafeToRemoveLock(t *testing.T) { + tests := []struct { + name string + plugin Plugin + processName string + wantIsSafe bool + wantErr bool + wantErrMsg string + }{ + { + name: "Safe to remove lock-true", + plugin: Plugin{ + Plugin: &common.Plugin{ + Name: "cni", + Version: "0.3.0", + Store: store.NewMockStore("testfiles/processinit.lock"), + }, + version: "0.3.0", + }, + processName: "azure-vnet", + wantIsSafe: true, + wantErr: false, + }, + { + name: "Safe to remove lock-true", + plugin: Plugin{ + Plugin: &common.Plugin{ + Name: "cni", + Version: "0.3.0", + Store: store.NewMockStore("testfiles/processnotfound.lock"), + }, + version: "0.3.0", + }, + processName: "azure-vnet", + wantIsSafe: true, + wantErr: false, + }, + { + name: "Safe to remove lock-false", + plugin: Plugin{ + Plugin: &common.Plugin{ + Name: "cni", + Version: "0.3.0", + Store: store.NewMockStore("testfiles/processinit.lock"), + }, + version: "0.3.0", + }, + processName: "init", + wantIsSafe: false, + wantErr: true, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + isSafe, err := tt.plugin.IsSafeToRemoveLock(tt.processName) + if tt.wantErr { + require.Error(t, err) + require.Equal(t, tt.wantIsSafe, isSafe) + } else { + require.NoError(t, err) + require.Equal(t, tt.wantIsSafe, isSafe) + } + }) + } +} diff --git a/cni/testfiles/testlock.lock b/cni/testfiles/testlock.lock new file mode 100644 index 0000000000..56a6051ca2 --- /dev/null +++ b/cni/testfiles/testlock.lock @@ -0,0 +1 @@ +1 \ No newline at end of file diff --git a/store/json.go b/store/json.go index 3181f371a9..7c51299fb8 100644 --- a/store/json.go +++ b/store/json.go @@ -25,7 +25,7 @@ const ( lockExtension = ".lock" // Maximum number of retries before failing a lock call. - lockMaxRetries = 200 + lockMaxRetries = 100 // Delay between lock retries. lockRetryDelay = 100 * time.Millisecond diff --git a/store/mockstore.go b/store/mockstore.go new file mode 100644 index 0000000000..85b242a5af --- /dev/null +++ b/store/mockstore.go @@ -0,0 +1,51 @@ +package store + +import ( + "time" +) + +type mockStore struct { + lockFilePath string +} + +// NewMockStore creates a new jsonFileStore object, accessed as a KeyValueStore. +func NewMockStore(lockFilePath string) KeyValueStore { + return &mockStore{ + lockFilePath: lockFilePath, + } +} + +// Read restores the value for the given key from persistent store. +func (ms *mockStore) Read(key string, value interface{}) error { + return nil +} + +func (ms *mockStore) Write(key string, value interface{}) error { + return nil +} +func (ms *mockStore) Flush() error { + return nil +} + +func (ms *mockStore) Lock(block bool) error { + return nil +} + +func (ms *mockStore) Unlock(forceUnlock bool) error { + return nil +} + +func (ms *mockStore) GetModificationTime() (time.Time, error) { + return time.Time{}, nil +} + +func (ms *mockStore) GetLockFileModificationTime() (time.Time, error) { + return time.Time{}, nil +} +func (ms *mockStore) GetLockFileName() string { + return ms.lockFilePath +} + +func (ms *mockStore) Remove() { + +} From 9aba9aa2a4c28b0e68fe8d90e6d1d2acfb1533ec Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Tue, 28 Sep 2021 12:19:06 -0700 Subject: [PATCH 2/7] added test files --- cni/testfiles/processinit.lock | 1 + cni/testfiles/processnotfound.lock | 1 + 2 files changed, 2 insertions(+) create mode 100644 cni/testfiles/processinit.lock create mode 100644 cni/testfiles/processnotfound.lock diff --git a/cni/testfiles/processinit.lock b/cni/testfiles/processinit.lock new file mode 100644 index 0000000000..56a6051ca2 --- /dev/null +++ b/cni/testfiles/processinit.lock @@ -0,0 +1 @@ +1 \ No newline at end of file diff --git a/cni/testfiles/processnotfound.lock b/cni/testfiles/processnotfound.lock new file mode 100644 index 0000000000..7d105a7b45 --- /dev/null +++ b/cni/testfiles/processnotfound.lock @@ -0,0 +1 @@ +-3 \ No newline at end of file From 8908fa92f2df18557501dacb86343bfa55bb5ccd Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Tue, 28 Sep 2021 12:44:25 -0700 Subject: [PATCH 3/7] removing unused file --- cni/testfiles/testlock.lock | 1 - 1 file changed, 1 deletion(-) delete mode 100644 cni/testfiles/testlock.lock diff --git a/cni/testfiles/testlock.lock b/cni/testfiles/testlock.lock deleted file mode 100644 index 56a6051ca2..0000000000 --- a/cni/testfiles/testlock.lock +++ /dev/null @@ -1 +0,0 @@ -1 \ No newline at end of file From 274dfd6a237fe8688b22451d4714782bd8a9c1e2 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Tue, 28 Sep 2021 12:53:43 -0700 Subject: [PATCH 4/7] lint fixes --- cni/plugin_test.go | 25 +++++-------------- .../{processinit.lock => processfound.lock} | 0 store/mockstore.go | 6 ++--- 3 files changed, 9 insertions(+), 22 deletions(-) rename cni/testfiles/{processinit.lock => processfound.lock} (100%) diff --git a/cni/plugin_test.go b/cni/plugin_test.go index 2d41d56493..1af38f6fa0 100644 --- a/cni/plugin_test.go +++ b/cni/plugin_test.go @@ -1,11 +1,12 @@ package cni import ( + "os" + "testing" + "github.com/Azure/azure-container-networking/common" "github.com/Azure/azure-container-networking/store" "github.com/stretchr/testify/require" - "os" - "testing" ) func TestMain(m *testing.M) { @@ -24,12 +25,12 @@ func TestPluginSafeToRemoveLock(t *testing.T) { wantErrMsg string }{ { - name: "Safe to remove lock-true", + name: "Safe to remove lock-true. Process name not match", plugin: Plugin{ Plugin: &common.Plugin{ Name: "cni", Version: "0.3.0", - Store: store.NewMockStore("testfiles/processinit.lock"), + Store: store.NewMockStore("testfiles/processfound.lock"), }, version: "0.3.0", }, @@ -38,7 +39,7 @@ func TestPluginSafeToRemoveLock(t *testing.T) { wantErr: false, }, { - name: "Safe to remove lock-true", + name: "Safe to remove lock-true. Process not running", plugin: Plugin{ Plugin: &common.Plugin{ Name: "cni", @@ -51,20 +52,6 @@ func TestPluginSafeToRemoveLock(t *testing.T) { wantIsSafe: true, wantErr: false, }, - { - name: "Safe to remove lock-false", - plugin: Plugin{ - Plugin: &common.Plugin{ - Name: "cni", - Version: "0.3.0", - Store: store.NewMockStore("testfiles/processinit.lock"), - }, - version: "0.3.0", - }, - processName: "init", - wantIsSafe: false, - wantErr: true, - }, } for _, tt := range tests { diff --git a/cni/testfiles/processinit.lock b/cni/testfiles/processfound.lock similarity index 100% rename from cni/testfiles/processinit.lock rename to cni/testfiles/processfound.lock diff --git a/store/mockstore.go b/store/mockstore.go index 85b242a5af..fde8659656 100644 --- a/store/mockstore.go +++ b/store/mockstore.go @@ -23,6 +23,7 @@ func (ms *mockStore) Read(key string, value interface{}) error { func (ms *mockStore) Write(key string, value interface{}) error { return nil } + func (ms *mockStore) Flush() error { return nil } @@ -42,10 +43,9 @@ func (ms *mockStore) GetModificationTime() (time.Time, error) { func (ms *mockStore) GetLockFileModificationTime() (time.Time, error) { return time.Time{}, nil } + func (ms *mockStore) GetLockFileName() string { return ms.lockFilePath } -func (ms *mockStore) Remove() { - -} +func (ms *mockStore) Remove() {} From 6bcc09b338678bf16695f1ab5ac6e6c007115f58 Mon Sep 17 00:00:00 2001 From: tamilmani1989 Date: Tue, 28 Sep 2021 20:17:13 -0700 Subject: [PATCH 5/7] Update cni/plugin.go Co-authored-by: Evan Baker --- cni/plugin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cni/plugin.go b/cni/plugin.go index 0c52c2fea1..3f0144f856 100644 --- a/cni/plugin.go +++ b/cni/plugin.go @@ -23,7 +23,7 @@ import ( "github.com/pkg/errors" ) -var errEmptyContent = errors.New("Read content is zero bytes") +var errEmptyContent = errors.New("read content is zero bytes") // Plugin is the parent class for CNI plugins. type Plugin struct { From afa08482699cc917aa38d292e605fed8ad5fbf28 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Thu, 30 Sep 2021 12:04:07 -0700 Subject: [PATCH 6/7] addressed comments --- cni/plugin.go | 17 ++++++++++------- cni/plugin_test.go | 3 +-- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/cni/plugin.go b/cni/plugin.go index 0c52c2fea1..18de5284be 100644 --- a/cni/plugin.go +++ b/cni/plugin.go @@ -207,9 +207,10 @@ func (plugin *Plugin) IsSafeToRemoveLock(processName string) (bool, error) { return false, errors.Wrap(err, "IsSafeToRemoveLock lockfile read failed") } - log.Printf("Read from Lock file:%s", lockFilePid) + log.Printf("Read from lockfile:%s", lockFilePid) // Get the process name if running and // check if that matches with our expected process + // if it runs non-nil error then process is not running pName, err := platform.GetProcessNameByID(lockFilePid) if err != nil { var content string @@ -218,12 +219,14 @@ func (plugin *Plugin) IsSafeToRemoveLock(processName string) (bool, error) { return false, errors.Wrap(err, "IsSafeToRemoveLock lockfile 2nd read failed") } - if string(content) == lockFilePid { - return true, nil + // pid in lockfile changed after getprocessnamebyid call. so some other process acquired lockfile in between. + // so its not safe to remove lockfile + if string(content) != lockFilePid { + log.Printf("Lockfile content changed from %s to %s. So not safe to remove lockfile", lockFilePid, content) + return false, nil } - log.Printf("Lockfile content changed from %s to %s. So not safe to clean lock file", lockFilePid, content) - return false, nil + return true, nil } log.Printf("[CNI] Process name is %s", pName) @@ -240,12 +243,12 @@ func (plugin *Plugin) IsSafeToRemoveLock(processName string) (bool, error) { func (plugin *Plugin) readLockFile(filename string) (string, error) { content, err := ioutil.ReadFile(filename) if err != nil { - log.Errorf("Failed to read lock file :%v, ", err) + log.Errorf("Failed to read lockfile :%v", err) return "", fmt.Errorf("readLockFile error:%w", err) } if len(content) == 0 { - log.Errorf("Num bytes read from lock file is 0") + log.Errorf("Num bytes read from lockfile is 0") return "", errEmptyContent } diff --git a/cni/plugin_test.go b/cni/plugin_test.go index 1af38f6fa0..7c60cfaf69 100644 --- a/cni/plugin_test.go +++ b/cni/plugin_test.go @@ -22,10 +22,9 @@ func TestPluginSafeToRemoveLock(t *testing.T) { processName string wantIsSafe bool wantErr bool - wantErrMsg string }{ { - name: "Safe to remove lock-true. Process name not match", + name: "Safe to remove lock-true. Process name does not match", plugin: Plugin{ Plugin: &common.Plugin{ Name: "cni", From 8e735b7768518aac872db8e183754de3782f4112 Mon Sep 17 00:00:00 2001 From: Tamilmani Manoharan Date: Thu, 30 Sep 2021 12:15:33 -0700 Subject: [PATCH 7/7] nit fix --- cni/plugin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cni/plugin.go b/cni/plugin.go index 895c3165e2..2f3873fecf 100644 --- a/cni/plugin.go +++ b/cni/plugin.go @@ -210,7 +210,7 @@ func (plugin *Plugin) IsSafeToRemoveLock(processName string) (bool, error) { log.Printf("Read from lockfile:%s", lockFilePid) // Get the process name if running and // check if that matches with our expected process - // if it runs non-nil error then process is not running + // if it returns non-nil error then process is not running pName, err := platform.GetProcessNameByID(lockFilePid) if err != nil { var content string