Store fixes; Windows compile

John Howard · John Howard · commit a3868463451a · 2018-09-19T15:30:36.000-07:00
Signed-off-by: John Howard <jhoward@microsoft.com> - First, the store timeout is woefully low. Bumped to 20 seconds from 2 seconds. This may fix #242 (comment) IMO, as only test code calls it non-blocked, why even have a block parameter to Lock()? IMO also, why a timeout at all? They're always fraught with error and machine timing. - Presence of a key should be checked using `raw, ok := hvs.data[key]`, not the current nil checked - ErrKeyNotFound should be returned if the store file does not exist. It shouldn't ignore that error. - Actually now reports if a timeout occurred correctly, along with non-block lock attempt when already locked. - Serial pattern abuse in not always closing the lock file. - Some golang correctness (errors should be lower case) - go build ./... actually passes on Windows now - various compile errors previously. - golang pattern conformance `if err:=<test>; err!=nil {....` - Simplified timeout duration (no need for time.Duration(...))
diff --git a/cni/plugin.go b/cni/plugin.go
@@ -161,14 +161,14 @@ func (plugin *Plugin) InitializeKeyValueStore(config *common.PluginConfig) error
 		var err error
 		plugin.Store, err = store.NewJsonFileStore(platform.CNIRuntimePath + plugin.Name + ".json")
 		if err != nil {
-			log.Printf("[cni] Failed to create store, err:%v.", err)
+			log.Printf("[cni] Failed to create store: %v.", err)
 			return err
 		}
 	}
 
 	// Acquire store lock.
 	if err := plugin.Store.Lock(true); err != nil {
-		log.Printf("[cni] Timed out on locking store, err:%v.", err)
+		log.Printf("[cni] Failed to lock store: %v.", err)
 		return err
 	}
 
@@ -182,7 +182,7 @@ func (plugin *Plugin) UninitializeKeyValueStore() error {
 	if plugin.Store != nil {
 		err := plugin.Store.Unlock()
 		if err != nil {
-			log.Printf("[cni] Failed to unlock store, err:%v.", err)
+			log.Printf("[cni] Failed to unlock store: %v.", err)
 			return err
 		}
 	}
diff --git a/netlink/ip.go b/netlink/ip.go
@@ -1,6 +1,8 @@
 // Copyright 2017 Microsoft. All rights reserved.
 // MIT License
 
+// +build linux
+
 package netlink
 
 import (
diff --git a/netlink/link.go b/netlink/link.go
@@ -1,6 +1,8 @@
 // Copyright 2017 Microsoft. All rights reserved.
 // MIT License
 
+// +build linux
+
 package netlink
 
 import (
diff --git a/netlink/netlink.go b/netlink/netlink.go
@@ -1,6 +1,8 @@
 // Copyright 2017 Microsoft. All rights reserved.
 // MIT License
 
+// +build linux
+
 package netlink
 
 import (
diff --git a/netlink/netlink_test.go b/netlink/netlink_test.go
@@ -1,6 +1,8 @@
 // Copyright 2017 Microsoft. All rights reserved.
 // MIT License
 
+// +build linux
+
 package netlink
 
 import (
diff --git a/netlink/netlink_windows.go b/netlink/netlink_windows.go
@@ -0,0 +1,4 @@
+// Copyright 2017 Microsoft. All rights reserved.
+// MIT License
+
+package netlink
diff --git a/netlink/protocol.go b/netlink/protocol.go
@@ -1,6 +1,8 @@
 // Copyright 2017 Microsoft. All rights reserved.
 // MIT License
 
+// +build linux
+
 package netlink
 
 import (
diff --git a/netlink/socket.go b/netlink/socket.go
@@ -1,6 +1,8 @@
 // Copyright 2017 Microsoft. All rights reserved.
 // MIT License
 
+// +build linux
+
 package netlink
 
 import (
diff --git a/network/endpoint_linux.go b/network/endpoint_linux.go
@@ -1,8 +1,6 @@
 // Copyright 2017 Microsoft. All rights reserved.
 // MIT License
 
-// +build linux
-
 package network
 
 import (
diff --git a/network/endpoint_windows.go b/network/endpoint_windows.go
@@ -1,8 +1,6 @@
 // Copyright 2017 Microsoft. All rights reserved.
 // MIT License
 
-// +build windows
-
 package network
 
 import (
diff --git a/network/epcommon/endpoint_common_windows.go b/network/epcommon/endpoint_common_windows.go
@@ -0,0 +1 @@
+package epcommon
diff --git a/network/namespace_linux.go b/network/namespace_linux.go
@@ -1,8 +1,6 @@
 // Copyright 2017 Microsoft. All rights reserved.
 // MIT License
 
-// +build linux
-
 package network
 
 import (
diff --git a/network/network_linux.go b/network/network_linux.go
@@ -1,8 +1,6 @@
 // Copyright 2017 Microsoft. All rights reserved.
 // MIT License
 
-// +build linux
-
 package network
 
 import (
diff --git a/network/network_windows.go b/network/network_windows.go
@@ -1,8 +1,6 @@
 // Copyright 2017 Microsoft. All rights reserved.
 // MIT License
 
-// +build windows
-
 package network
 
 import (
diff --git a/store/json.go b/store/json.go
@@ -21,10 +21,10 @@ const (
 	lockExtension = ".lock"
 
 	// Maximum number of retries before failing a lock call.
-	lockMaxRetries = 20
+	lockMaxRetries = 200
 
 	// Delay between lock retries.
-	lockRetryDelayInMilliseconds = 100
+	lockRetryDelay = 100 * time.Millisecond
 )
 
 // jsonFileStore is an implementation of KeyValueStore using a local JSON file.
@@ -61,27 +61,22 @@ func (kvs *jsonFileStore) Read(key string, value interface{}) error {
 		file, err := os.Open(kvs.fileName)
 		if err != nil {
 			if os.IsNotExist(err) {
-				return nil
+				return ErrKeyNotFound
 			}
 			return err
 		}
+		defer file.Close()
 
 		// Decode to raw JSON messages.
-		err = json.NewDecoder(file).Decode(&kvs.data)
-		if err != nil {
-			return err
-		}
-
-		err = file.Close()
-		if err != nil {
+		if err := json.NewDecoder(file).Decode(&kvs.data); err != nil {
 			return err
 		}
 
 		kvs.inSync = true
 	}
 
-	raw := kvs.data[key]
-	if raw == nil {
+	raw, ok := kvs.data[key]
+	if !ok {
 		return ErrKeyNotFound
 	}
 
@@ -118,18 +113,17 @@ func (kvs *jsonFileStore) flush() error {
 	if err != nil {
 		return err
 	}
+	defer file.Close()
 
 	buf, err := json.MarshalIndent(&kvs.data, "", "\t")
 	if err != nil {
 		return err
 	}
 
-	_, err = file.Write(buf)
-	if err != nil {
+	if _, err := file.Write(buf); err != nil {
 		return err
 	}
-
-	return file.Close()
+	return nil
 }
 
 // Lock locks the store for exclusive access.
@@ -153,21 +147,20 @@ func (kvs *jsonFileStore) Lock(block bool) error {
 			break
 		}
 
-		if !block || i == lockMaxRetries {
-			return ErrStoreLocked
+		if !block {
+			return ErrNonBlockingLockIsAlreadyLocked
 		}
 
-		time.Sleep(time.Duration(lockRetryDelayInMilliseconds) * time.Millisecond)
-	}
+		if i == lockMaxRetries {
+			return ErrTimeoutLockingStore
+		}
 
-	// Write the process ID for easy identification.
-	_, err = lockFile.WriteString(strconv.Itoa(os.Getpid()))
-	if err != nil {
-		return err
+		time.Sleep(lockRetryDelay)
 	}
+	defer lockFile.Close()
 
-	err = lockFile.Close()
-	if err != nil {
+	// Write the process ID for easy identification.
+	if _, err = lockFile.WriteString(strconv.Itoa(os.Getpid())); err != nil {
 		return err
 	}
 
@@ -198,9 +191,12 @@ func (kvs *jsonFileStore) Unlock() error {
 
 // GetModificationTime returns the modification time of the persistent store.
 func (kvs *jsonFileStore) GetModificationTime() (time.Time, error) {
+	kvs.Mutex.Lock()
+	defer kvs.Mutex.Unlock()
+
 	info, err := os.Stat(kvs.fileName)
 	if err != nil {
-		log.Printf("os.stat() for file %v failed with error %v", kvs.fileName, err)
+		log.Printf("os.stat() for file %v failed: %v", kvs.fileName, err)
 		return time.Time{}.UTC(), err
 	}
 
diff --git a/store/store.go b/store/store.go
@@ -20,7 +20,9 @@ type KeyValueStore interface {
 
 var (
 	// Errors returned by KeyValueStore methods.
-	ErrKeyNotFound    = fmt.Errorf("Key not found")
-	ErrStoreLocked    = fmt.Errorf("Store is locked")
-	ErrStoreNotLocked = fmt.Errorf("Store is not locked")
+	ErrKeyNotFound                    = fmt.Errorf("key not found")
+	ErrStoreLocked                    = fmt.Errorf("store is already locked")
+	ErrStoreNotLocked                 = fmt.Errorf("store is not locked")
+	ErrTimeoutLockingStore            = fmt.Errorf("timed out locking store")
+	ErrNonBlockingLockIsAlreadyLocked = fmt.Errorf("attempted to perform non-blocking lock on an already locked store")
 )

Original file line number	Diff line number	Diff line change
`@@ -161,14 +161,14 @@ func (plugin Plugin) InitializeKeyValueStore(config common.PluginConfig) error`
`161`	`161`	`var err error`
`162`	`162`	`plugin.Store, err = store.NewJsonFileStore(platform.CNIRuntimePath + plugin.Name + ".json")`
`163`	`163`	`if err != nil {`
`164`		`- log.Printf("[cni] Failed to create store, err:%v.", err)`
	`164`	`+ log.Printf("[cni] Failed to create store: %v.", err)`
`165`	`165`	`return err`
`166`	`166`	`}`
`167`	`167`	`}`
`168`	`168`
`169`	`169`	`// Acquire store lock.`
`170`	`170`	`if err := plugin.Store.Lock(true); err != nil {`
`171`		`- log.Printf("[cni] Timed out on locking store, err:%v.", err)`
	`171`	`+ log.Printf("[cni] Failed to lock store: %v.", err)`
`172`	`172`	`return err`
`173`	`173`	`}`
`174`	`174`
`@@ -182,7 +182,7 @@ func (plugin *Plugin) UninitializeKeyValueStore() error {`
`182`	`182`	`if plugin.Store != nil {`
`183`	`183`	`err := plugin.Store.Unlock()`
`184`	`184`	`if err != nil {`
`185`		`- log.Printf("[cni] Failed to unlock store, err:%v.", err)`
	`185`	`+ log.Printf("[cni] Failed to unlock store: %v.", err)`
`186`	`186`	`return err`
`187`	`187`	`}`
`188`	`188`	`}`