From 4ff8a738eb990e7dcdd83f570f1d1eb1ce6fa852 Mon Sep 17 00:00:00 2001 From: Ashvin Deodhar Date: Tue, 9 Feb 2021 14:15:43 -0800 Subject: [PATCH 1/3] Remove unreadable CNS state file upon CNS service start --- cns/restserver/api_test.go | 33 ++++++++++++++++++++++++------ cns/restserver/internalapi_test.go | 6 +++++- cns/restserver/restserver.go | 1 - cns/restserver/util.go | 7 ++++--- cns/service/main.go | 2 +- store/json.go | 9 +++++++- store/store.go | 1 + testutils/store_mock.go | 4 ++++ 8 files changed, 50 insertions(+), 13 deletions(-) diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index e266a9b457..280130b860 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -8,6 +8,7 @@ import ( "encoding/json" "encoding/xml" "fmt" + "github.com/Azure/azure-container-networking/store" "net/http" "net/http/httptest" "net/url" @@ -26,6 +27,7 @@ import ( const ( defaultCnsURL = "http://localhost:10090" contentTypeJSON = "application/json" + cnsJsonFileName = "azure-cns.json" ) type IPAddress struct { @@ -125,7 +127,10 @@ func TestMain(m *testing.M) { logger.InitLogger("testlogs", 0, 0, "./") // Create the service. - startService() + if err = startService(); err != nil { + fmt.Printf("Failed to start CNS Service. Error: %v", err) + os.Exit(1) + } // Setup mock nmagent server u, err := url.Parse("tcp://" + nmagentEndpoint) @@ -908,34 +913,50 @@ func setEnv(t *testing.T) *httptest.ResponseRecorder { return w } -func startService() { +func startService() error { var err error // Create the service. config := common.ServiceConfig{} + // Create the key value store. + if config.Store, err = store.NewJsonFileStore(cnsJsonFileName); err != nil { + logger.Errorf("Failed to create store file: %s, due to error %v\n", cnsJsonFileName, err) + return err + } + service, err = NewHTTPRestService(&config, fakes.NewFakeImdsClient(), fakes.NewFakeNMAgentClient()) if err != nil { - fmt.Printf("Failed to create CNS object %v\n", err) - os.Exit(1) + return err } svc = service.(*HTTPRestService) svc.Name = "cns-test-server" if err != nil { logger.Errorf("Failed to create CNS object, err:%v.\n", err) - return + return err } svc.IPAMPoolMonitor = fakes.NewIPAMPoolMonitorFake() if service != nil { + // Create empty azure-cns.json. CNS should start successfully by deleting this file + file, _ := os.Create(cnsJsonFileName) + file.Close() + err = service.Start(&config) if err != nil { logger.Errorf("Failed to start CNS, err:%v.\n", err) - return + return err + } + + if _, err := os.Stat(cnsJsonFileName); err == nil || !os.IsNotExist(err) { + logger.Errorf("Failed to remove empty CNS state file: %s, err:%v", cnsJsonFileName, err) + return err } } // Get the internal http mux as test hook. mux = service.(*HTTPRestService).Listener.GetMux() + + return nil } // IGNORE TEST AS IT IS FAILING. TODO:- Fix it https://msazure.visualstudio.com/One/_workitems/edit/7720083 diff --git a/cns/restserver/internalapi_test.go b/cns/restserver/internalapi_test.go index 1b43fb4826..5b30b89064 100644 --- a/cns/restserver/internalapi_test.go +++ b/cns/restserver/internalapi_test.go @@ -7,6 +7,7 @@ import ( "context" "encoding/json" "fmt" + "os" "reflect" "strconv" "testing" @@ -545,5 +546,8 @@ func restartService() { fmt.Println("Restart Service") service.Stop() - startService() + if err := startService(); err != nil { + fmt.Printf("Failed to restart CNS Service. Error: %v", err) + os.Exit(1) + } } diff --git a/cns/restserver/restserver.go b/cns/restserver/restserver.go index ca325db8b2..6b79fe508a 100644 --- a/cns/restserver/restserver.go +++ b/cns/restserver/restserver.go @@ -132,7 +132,6 @@ func NewHTTPRestService(config *common.ServiceConfig, imdsClientInterface imdscl // Start starts the CNS listener. func (service *HTTPRestService) Start(config *common.ServiceConfig) error { - err := service.Initialize(config) if err != nil { logger.Errorf("[Azure CNS] Failed to initialize base service, err:%v.", err) diff --git a/cns/restserver/util.go b/cns/restserver/util.go index a8d24fff9b..77b2b1eca5 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -89,11 +89,12 @@ func (service *HTTPRestService) restoreState() error { if err == store.ErrKeyNotFound { // Nothing to restore. logger.Printf("[Azure CNS] No state to restore.\n") - return nil + } else { + logger.Errorf("[Azure CNS] Failed to restore state, err:%v. Removing azure-cns.json", err) + service.store.Remove() } - logger.Errorf("[Azure CNS] Failed to restore state, err:%v\n", err) - return err + return nil } logger.Printf("[Azure CNS] Restored state, %+v\n", service.state) diff --git a/cns/service/main.go b/cns/service/main.go index 3657a578a6..f3406176b4 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -395,7 +395,7 @@ func main() { if !telemetryEnabled { logger.Errorf("[Azure CNS] Cannot disable telemetry via cmdline. Update cns_config.json to disable telemetry.") } - + cnsconfig, err := configuration.ReadConfig() if err != nil { logger.Errorf("[Azure CNS] Error reading cns config: %v", err) diff --git a/store/json.go b/store/json.go index ee55f37da0..b19f4a1394 100644 --- a/store/json.go +++ b/store/json.go @@ -148,7 +148,6 @@ func (kvs *jsonFileStore) flush() error { return fmt.Errorf("temp file close failed with: %v", err) } - log.Printf("renaming temp file %v to state file", tmpFileName) // atomic replace if err = platform.ReplaceFile(tmpFileName, kvs.fileName); err != nil { return fmt.Errorf("rename temp file to state file failed:%v", err) @@ -276,3 +275,11 @@ func (kvs *jsonFileStore) GetLockFileModificationTime() (time.Time, error) { func (kvs *jsonFileStore) GetLockFileName() string { return kvs.fileName + lockExtension } + +func (kvs *jsonFileStore) Remove() { + kvs.Mutex.Lock() + if err := os.Remove(kvs.fileName); err != nil { + log.Errorf("could not remove file %s. Error: %v", kvs.fileName, err) + } + kvs.Mutex.Unlock() +} diff --git a/store/store.go b/store/store.go index ef648a161c..019f51245b 100644 --- a/store/store.go +++ b/store/store.go @@ -18,6 +18,7 @@ type KeyValueStore interface { GetModificationTime() (time.Time, error) GetLockFileModificationTime() (time.Time, error) GetLockFileName() string + Remove() } var ( diff --git a/testutils/store_mock.go b/testutils/store_mock.go index 88cce08ea3..51f6dbd9df 100644 --- a/testutils/store_mock.go +++ b/testutils/store_mock.go @@ -47,3 +47,7 @@ func (store *KeyValueStoreMock) GetLockFileName() string { return "" } +func (store *KeyValueStoreMock) Remove() { + return +} + From 5b9c5ffa391c1a79e4f24e00ceb3bd647ad22910 Mon Sep 17 00:00:00 2001 From: Ashvin Deodhar Date: Tue, 9 Feb 2021 14:21:48 -0800 Subject: [PATCH 2/3] remove white space --- cns/service/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cns/service/main.go b/cns/service/main.go index f3406176b4..3657a578a6 100644 --- a/cns/service/main.go +++ b/cns/service/main.go @@ -395,7 +395,7 @@ func main() { if !telemetryEnabled { logger.Errorf("[Azure CNS] Cannot disable telemetry via cmdline. Update cns_config.json to disable telemetry.") } - + cnsconfig, err := configuration.ReadConfig() if err != nil { logger.Errorf("[Azure CNS] Error reading cns config: %v", err) From e75ac7b5da6a5a8b8d7d2e8b829a23deb0aa3d04 Mon Sep 17 00:00:00 2001 From: Ashvin Deodhar Date: Tue, 9 Feb 2021 14:54:08 -0800 Subject: [PATCH 3/3] address review comment --- cns/restserver/restserver.go | 7 +------ cns/restserver/util.go | 8 ++++---- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/cns/restserver/restserver.go b/cns/restserver/restserver.go index 6b79fe508a..c2fd6649a2 100644 --- a/cns/restserver/restserver.go +++ b/cns/restserver/restserver.go @@ -138,12 +138,7 @@ func (service *HTTPRestService) Start(config *common.ServiceConfig) error { return err } - err = service.restoreState() - if err != nil { - logger.Errorf("[Azure CNS] Failed to restore service state, err:%v.", err) - return err - } - + service.restoreState() err = service.restoreNetworkState() if err != nil { logger.Errorf("[Azure CNS] Failed to restore network state, err:%v.", err) diff --git a/cns/restserver/util.go b/cns/restserver/util.go index 77b2b1eca5..7d26f9ba99 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -74,13 +74,13 @@ func (service *HTTPRestService) saveState() error { } // restoreState restores CNS state from persistent store. -func (service *HTTPRestService) restoreState() error { +func (service *HTTPRestService) restoreState() { logger.Printf("[Azure CNS] restoreState") // Skip if a store is not provided. if service.store == nil { logger.Printf("[Azure CNS] store not initialized.") - return nil + return } // Read any persisted state. @@ -94,11 +94,11 @@ func (service *HTTPRestService) restoreState() error { service.store.Remove() } - return nil + return } logger.Printf("[Azure CNS] Restored state, %+v\n", service.state) - return nil + return } func (service *HTTPRestService) saveNetworkContainerGoalState(req cns.CreateNetworkContainerRequest) (int, string) {