Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cni/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhowardmsft - I know its not you but typically no punctuation across other golang projects.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also. Typically the "[cni]" portion would be done via a logrus context or equivalent. (Again just FYI)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but I'm not changing the world here :)

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
}

Expand All @@ -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
}
}
Expand Down
2 changes: 2 additions & 0 deletions netlink/ip.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright 2017 Microsoft. All rights reserved.
// MIT License

// +build linux

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this work? I thought it had to be above header?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly does in 1.11

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...that lists the conditions under which a file should be included in the package. Constraints may appear in any kind of source file (not just Go), but they must appear near the top of the file, preceded only by blank lines and other line comments. These rules mean that in Go files a build constraint must appear before the package clause.

https://golang.org/pkg/go/build/


package netlink

import (
Expand Down
2 changes: 2 additions & 0 deletions netlink/link.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright 2017 Microsoft. All rights reserved.
// MIT License

// +build linux

package netlink

import (
Expand Down
2 changes: 2 additions & 0 deletions netlink/netlink.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright 2017 Microsoft. All rights reserved.
// MIT License

// +build linux

package netlink

import (
Expand Down
2 changes: 2 additions & 0 deletions netlink/netlink_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright 2017 Microsoft. All rights reserved.
// MIT License

// +build linux

package netlink

import (
Expand Down
4 changes: 4 additions & 0 deletions netlink/netlink_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Copyright 2017 Microsoft. All rights reserved.
// MIT License

package netlink
2 changes: 2 additions & 0 deletions netlink/protocol.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright 2017 Microsoft. All rights reserved.
// MIT License

// +build linux

package netlink

import (
Expand Down
2 changes: 2 additions & 0 deletions netlink/socket.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright 2017 Microsoft. All rights reserved.
// MIT License

// +build linux

package netlink

import (
Expand Down
2 changes: 0 additions & 2 deletions network/endpoint_linux.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright 2017 Microsoft. All rights reserved.
// MIT License

// +build linux

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename? endpoint_linux.go specifically implies to me "// +build linux" by convention.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you are just doing this because its already included by default via the naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just being consistent with what's there.


package network

import (
Expand Down
2 changes: 0 additions & 2 deletions network/endpoint_windows.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright 2017 Microsoft. All rights reserved.
// MIT License

// +build windows

package network

import (
Expand Down
1 change: 1 addition & 0 deletions network/epcommon/endpoint_common_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package epcommon
2 changes: 0 additions & 2 deletions network/namespace.go → network/namespace_linux.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright 2017 Microsoft. All rights reserved.
// MIT License

// +build linux

package network

import (
Expand Down
2 changes: 0 additions & 2 deletions network/network_linux.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright 2017 Microsoft. All rights reserved.
// MIT License

// +build linux

package network

import (
Expand Down
2 changes: 0 additions & 2 deletions network/network_windows.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
// Copyright 2017 Microsoft. All rights reserved.
// MIT License

// +build windows

package network

import (
Expand Down
50 changes: 23 additions & 27 deletions store/json.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@ const (
lockExtension = ".lock"

// Maximum number of retries before failing a lock call.
lockMaxRetries = 20
lockMaxRetries = 200
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason changing this to 200?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timeout (2s) was woefully small. 20 seconds is still reasonably low. Arguably, there should be no need whatsoever for a timeout in a correctly operating system. In fact, in a subsequent change I have which removes the store entirely and moves this to boltdb, there is no timeout.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok


// Delay between lock retries.
lockRetryDelayInMilliseconds = 100
lockRetryDelay = 100 * time.Millisecond
)

// jsonFileStore is an implementation of KeyValueStore using a local JSON file.
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not breaking anything right when CNI/CNM called first time on boot up when there is no state file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but it's a question of correctness, not obfuscation. The store package should behave and return errors as expected - a read of something which doesn't exist should return a not-found error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I got it. I'm just making sure we tested that scenario?

}
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
}

Expand Down Expand Up @@ -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.
Expand All @@ -153,21 +147,20 @@ func (kvs *jsonFileStore) Lock(block bool) error {
break
}

if !block || i == lockMaxRetries {
return ErrStoreLocked
if !block {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again not your code. But a better pattern would be to have:

func TryLock() bool -> Non-Blocking return value indicates wasLocked
func Lock() -> Blocking
func Unlock() -> Same for both

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how defer works in a loop . Just clarifying if open called multiple times and is close getting called for each open?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The defer of close is outside the loop.... It's only called at function exit, and only in the case that the open succeeded.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry! I missed it


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
}

Expand Down Expand Up @@ -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
}

Expand Down
8 changes: 5 additions & 3 deletions store/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
)