Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CNI] zap logger migration for store package #2231

Merged
merged 9 commits into from
Sep 25, 2023
Merged

[CNI] zap logger migration for store package #2231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1d3c1e0
zap logger migration for store package
paulyufan2 Sep 20, 2023
a2d17c6
fix an issue
paulyufan2 Sep 20, 2023
d0ea07a
fix if condition
paulyufan2 Sep 20, 2023
53a6592
fix if condition
paulyufan2 Sep 21, 2023
b53bdcc
fix comments
paulyufan2 Sep 21, 2023
7e87535
comment fix
paulyufan2 Sep 21, 2023
685c2bf
fix comment
paulyufan2 Sep 21, 2023
39539f8
fix comments
paulyufan2 Sep 25, 2023
038c035
fix comment
paulyufan2 Sep 25, 2023
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
2 changes: 1 addition & 1 deletion aitelemetry/telemetrywrapper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func getMetadata(th *telemetryHandle) {
}

// Save metadata retrieved from wireserver to a file
kvs, err := store.NewJsonFileStore(metadataFile, lockclient)
kvs, err := store.NewJsonFileStore(metadataFile, lockclient, nil)
if err != nil {
debugLog("[AppInsights] Error initializing kvs store: %v", err)
return
Expand Down
3 changes: 2 additions & 1 deletion cni/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
)

var logger = log.CNILogger.With(zap.String("component", "cni-plugin"))
var storeLogger = log.CNILogger.With(zap.String("component", "cni-store"))

var errEmptyContent = errors.New("read content is zero bytes")

Expand Down Expand Up @@ -188,7 +189,7 @@ func (plugin *Plugin) InitializeKeyValueStore(config *common.PluginConfig) error
return errors.Wrap(err, "error creating new filelock")
}

plugin.Store, err = store.NewJsonFileStore(platform.CNIRuntimePath+plugin.Name+".json", lockclient)
plugin.Store, err = store.NewJsonFileStore(platform.CNIRuntimePath+plugin.Name+".json", lockclient, storeLogger)
if err != nil {
logger.Error("Failed to create store", zap.Error(err))
return err
Expand Down
2 changes: 1 addition & 1 deletion cnm/plugin/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ func main() {

// Create the key value store.
storeFileName := storeFileLocation + name + ".json"
config.Store, err = store.NewJsonFileStore(storeFileName, lockclient)
config.Store, err = store.NewJsonFileStore(storeFileName, lockclient, nil)
if err != nil {
log.Errorf("Failed to create store file: %s, due to error %v\n", storeFileName, err)
return
Expand Down
2 changes: 1 addition & 1 deletion cnms/service/networkmonitor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func main() {
return
}

config.Store, err = store.NewJsonFileStore(platform.CNIRuntimePath+pluginName+".json", lockclient)
config.Store, err = store.NewJsonFileStore(platform.CNIRuntimePath+pluginName+".json", lockclient, nil)
if err != nil {
fmt.Printf("[monitor] Failed to create store: %v\n", err)
return
Expand Down
2 changes: 1 addition & 1 deletion cns/restserver/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1671,7 +1671,7 @@ func startService() error {
config := common.ServiceConfig{}

// Create the key value fileStore.
fileStore, err := store.NewJsonFileStore(cnsJsonFileName, processlock.NewMockFileLock(false))
fileStore, err := store.NewJsonFileStore(cnsJsonFileName, processlock.NewMockFileLock(false), nil)
if err != nil {
logger.Errorf("Failed to create store file: %s, due to error %v\n", cnsJsonFileName, err)
return err
Expand Down
6 changes: 3 additions & 3 deletions cns/service/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@ func main() {

// Create the key value store.
storeFileName := storeFileLocation + name + ".json"
config.Store, err = store.NewJsonFileStore(storeFileName, lockclient)
config.Store, err = store.NewJsonFileStore(storeFileName, lockclient, nil)
if err != nil {
logger.Errorf("Failed to create store file: %s, due to error %v\n", storeFileName, err)
return
Expand All @@ -664,7 +664,7 @@ func main() {
}
// Create the key value store.
storeFileName := endpointStoreLocation + endpointStoreName + ".json"
endpointStateStore, err = store.NewJsonFileStore(storeFileName, endpointStoreLock)
endpointStateStore, err = store.NewJsonFileStore(storeFileName, endpointStoreLock, nil)
if err != nil {
logger.Errorf("Failed to create endpoint state store file: %s, due to error %v\n", storeFileName, err)
return
Expand Down Expand Up @@ -903,7 +903,7 @@ func main() {

// Create the key value store.
pluginStoreFile := storeFileLocation + pluginName + ".json"
pluginConfig.Store, err = store.NewJsonFileStore(pluginStoreFile, lockclientCnm)
pluginConfig.Store, err = store.NewJsonFileStore(pluginStoreFile, lockclientCnm, nil)
if err != nil {
logger.Errorf("Failed to create plugin store file %s, due to error : %v\n", pluginStoreFile, err)
return
Expand Down
40 changes: 34 additions & 6 deletions store/json.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/Azure/azure-container-networking/platform"
"github.com/Azure/azure-container-networking/processlock"
"github.com/pkg/errors"
"go.uber.org/zap"
)

const (
Expand All @@ -35,19 +36,21 @@ type jsonFileStore struct {
inSync bool
processLock processlock.Interface
sync.Mutex
logger *zap.Logger
}

// NewJsonFileStore creates a new jsonFileStore object, accessed as a KeyValueStore.
//
//nolint:revive // ignoring name change
func NewJsonFileStore(fileName string, lockclient processlock.Interface) (KeyValueStore, error) {
func NewJsonFileStore(fileName string, lockclient processlock.Interface, logger *zap.Logger) (KeyValueStore, error) {
if fileName == "" {
return &jsonFileStore{}, errors.New("need to pass in a json file path")
}
kvs := &jsonFileStore{
fileName: fileName,
processLock: lockclient,
data: make(map[string]*json.RawMessage),
logger: logger,
}

return kvs, nil
Expand Down Expand Up @@ -83,7 +86,12 @@ func (kvs *jsonFileStore) Read(key string, value interface{}) error {
}

if len(b) == 0 {
log.Printf("Unable to read file %s, was empty", kvs.fileName)
if kvs.logger != nil {
kvs.logger.Info("Unable to read empty file", zap.String("fileName", kvs.fileName))
} else {
log.Printf("Unable to read file %s, was empty", kvs.fileName)
}

return ErrStoreEmpty
}

Expand Down Expand Up @@ -184,7 +192,12 @@ func (kvs *jsonFileStore) Lock(timeout time.Duration) error {
afterTime := time.After(timeout)
status := make(chan error)

log.Printf("Acquiring process lock")
if kvs.logger != nil {
kvs.logger.Info("Acquiring process lock")
} else {
log.Printf("Acquiring process lock")
}

go kvs.lockUtil(status)

var err error
Expand All @@ -198,7 +211,12 @@ func (kvs *jsonFileStore) Lock(timeout time.Duration) error {
return errors.Wrap(err, "processLock acquire error")
}

log.Printf("Acquired process lock with timeout value of %v ", timeout)
if kvs.logger != nil {
kvs.logger.Info("Acquired process lock with timeout value of", zap.Any("timeout", timeout))
} else {
log.Printf("Acquired process lock with timeout value of %v", timeout)
}

return nil
}

Expand All @@ -212,7 +230,12 @@ func (kvs *jsonFileStore) Unlock() error {
return errors.Wrap(err, "unlock error")
}

log.Printf("Released process lock")
if kvs.logger != nil {
kvs.logger.Info("Released process lock")
} else {
log.Printf("Released process lock")
}

return nil
}

Expand All @@ -223,7 +246,12 @@ func (kvs *jsonFileStore) GetModificationTime() (time.Time, error) {

info, err := os.Stat(kvs.fileName)
if err != nil {
log.Printf("os.stat() for file %v failed: %v", kvs.fileName, err)
if kvs.logger != nil {
kvs.logger.Info("os.stat() for file", zap.String("fileName", kvs.fileName), zap.Error(err))
} else {
log.Printf("os.stat() for file %v failed: %v", kvs.fileName, err)
}

return time.Time{}.UTC(), err
}

Expand Down
24 changes: 14 additions & 10 deletions store/json_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"testing"
"time"

"github.com/Azure/azure-container-networking/cni/log"
"github.com/Azure/azure-container-networking/processlock"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -49,7 +50,7 @@ func TestKeyValuePairsAreReinstantiatedFromJSONFile(t *testing.T) {
defer os.Remove(testFileName)

// Create the store, initialized using the JSON file.
kvs, err := NewJsonFileStore(testFileName, processlock.NewMockFileLock(false))
kvs, err := NewJsonFileStore(testFileName, processlock.NewMockFileLock(false), nil)
if err != nil {
t.Fatalf("Failed to create KeyValueStore %v\n", err)
}
Expand All @@ -74,7 +75,7 @@ func TestKeyValuePairsArePersistedToJSONFile(t *testing.T) {
var actualPair string

// Create the store.
kvs, err := NewJsonFileStore(testFileName, processlock.NewMockFileLock(false))
kvs, err := NewJsonFileStore(testFileName, processlock.NewMockFileLock(false), nil)
if err != nil {
t.Fatalf("Failed to create KeyValueStore %v\n", err)
}
Expand Down Expand Up @@ -119,8 +120,11 @@ func TestKeyValuePairsAreWrittenAndReadCorrectly(t *testing.T) {
anotherValue := testType1{"any", 14}
var readValue testType1

// Test when passing zap logger obj to NewJsonFileStore
logger := log.CNILogger

// Create the store.
kvs, err := NewJsonFileStore(testFileName, processlock.NewMockFileLock(false))
kvs, err := NewJsonFileStore(testFileName, processlock.NewMockFileLock(false), logger)
if err != nil {
t.Fatalf("Failed to create KeyValueStore %v\n", err)
}
Expand Down Expand Up @@ -155,12 +159,12 @@ func TestKeyValuePairsAreWrittenAndReadCorrectly(t *testing.T) {

// test case for testing newjsonfilestore idempotent
func TestNewJsonFileStoreIdempotent(t *testing.T) {
_, err := NewJsonFileStore(testLockFileName, processlock.NewMockFileLock(false))
_, err := NewJsonFileStore(testLockFileName, processlock.NewMockFileLock(false), nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be testing the condition where it is not nil. In that case, zap.logger should be called.

Copy link
Member

Choose a reason for hiding this comment

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

@paulyufan2 was this addressed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it's fixed

if err != nil {
t.Errorf("Failed to initialize store: %v", err)
}

_, err = NewJsonFileStore(testLockFileName, processlock.NewMockFileLock(false))
_, err = NewJsonFileStore(testLockFileName, processlock.NewMockFileLock(false), nil)
if err != nil {
t.Errorf("Failed to initialize same store second time: %v", err)
}
Expand All @@ -177,7 +181,7 @@ func TestLock(t *testing.T) {
{
name: "Acquire Lock happy path",
store: func() KeyValueStore {
st, _ := NewJsonFileStore(testFileName, processlock.NewMockFileLock(false))
st, _ := NewJsonFileStore(testFileName, processlock.NewMockFileLock(false), nil)
return st
}(),
timeoutms: 10000,
Expand All @@ -186,7 +190,7 @@ func TestLock(t *testing.T) {
{
name: "Acquire Lock Fail",
store: func() KeyValueStore {
st, _ := NewJsonFileStore(testFileName, processlock.NewMockFileLock(true))
st, _ := NewJsonFileStore(testFileName, processlock.NewMockFileLock(true), nil)
return st
}(),
timeoutms: 10000,
Expand All @@ -196,7 +200,7 @@ func TestLock(t *testing.T) {
{
name: "Acquire Lock timeout error",
store: func() KeyValueStore {
st, _ := NewJsonFileStore(testFileName, processlock.NewMockFileLock(false))
st, _ := NewJsonFileStore(testFileName, processlock.NewMockFileLock(false), nil)
return st
}(),
timeoutms: 0,
Expand All @@ -223,12 +227,12 @@ func TestLock(t *testing.T) {

// test case for testing newjsonfilestore idempotent
func TestFileName(t *testing.T) {
_, err := NewJsonFileStore("", processlock.NewMockFileLock(false))
_, err := NewJsonFileStore("", processlock.NewMockFileLock(false), nil)
if err == nil {
t.Errorf("This should have failed for empty file name")
}

_, err = NewJsonFileStore("test.json", processlock.NewMockFileLock(false))
_, err = NewJsonFileStore("test.json", processlock.NewMockFileLock(false), nil)
if err != nil {
t.Fatalf("This should not fail for a non-empty file %v", err)
}
Expand Down