From ed08486ec79b1d79dd6408d57086883501ffae52 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Fri, 14 Jun 2024 11:09:52 +0200 Subject: [PATCH] libnet/ds: simplify datastore.New() That function was needlessly complex. Instead of relying on a struct and a sub-struct, it now just takes two string params: a path and a bucket name. Libnetwork config is now initialized with default values. A new struct is introduced in libnetwork/config to let tests customize the path and bucket name. Signed-off-by: Albin Kerouanton --- libnetwork/config/config.go | 9 +-- libnetwork/controller.go | 10 ++- libnetwork/datastore/datastore.go | 74 +++----------------- libnetwork/datastore/datastore_test.go | 10 --- libnetwork/internal/kvstore/boltdb/boltdb.go | 21 ++---- libnetwork/internal/kvstore/kvstore.go | 7 -- libnetwork/libnetwork_linux_test.go | 3 +- libnetwork/store_linux_test.go | 4 +- libnetwork/store_test.go | 19 +++-- 9 files changed, 35 insertions(+), 122 deletions(-) diff --git a/libnetwork/config/config.go b/libnetwork/config/config.go index 2f40f08577..abab6e6593 100644 --- a/libnetwork/config/config.go +++ b/libnetwork/config/config.go @@ -39,7 +39,7 @@ type Config struct { ClusterProvider cluster.Provider NetworkControlPlaneMTU int DefaultAddressPool []*ipamutils.NetworkToSplit - Scope datastore.ScopeCfg + DatastoreBucket string ActiveSandboxes map[string]interface{} PluginGetter plugingetter.PluginGetter } @@ -47,7 +47,8 @@ type Config struct { // New creates a new Config and initializes it with the given Options. func New(opts ...Option) *Config { cfg := &Config{ - driverCfg: make(map[string]map[string]any), + driverCfg: make(map[string]map[string]any), + DatastoreBucket: datastore.DefaultBucket, } for _, opt := range opts { @@ -56,10 +57,6 @@ func New(opts ...Option) *Config { } } - // load default scope configs which don't have explicit user specified configs. - if cfg.Scope == (datastore.ScopeCfg{}) { - cfg.Scope = datastore.DefaultScope(cfg.DataDir) - } return cfg } diff --git a/libnetwork/controller.go b/libnetwork/controller.go index 6b8901b507..65bfd268a6 100644 --- a/libnetwork/controller.go +++ b/libnetwork/controller.go @@ -108,7 +108,7 @@ type Controller struct { // New creates a new instance of network controller. func New(cfgOptions ...config.Option) (*Controller, error) { cfg := config.New(cfgOptions...) - store, err := datastore.New(cfg.Scope) + store, err := datastore.New(cfg.DataDir, cfg.DatastoreBucket) if err != nil { return nil, fmt.Errorf("libnet controller initialization: %w", err) } @@ -333,7 +333,9 @@ func (c *Controller) makeDriverConfig(ntype string) map[string]interface{} { return nil } - cfg := map[string]interface{}{} + cfg := map[string]interface{}{ + netlabel.LocalKVClient: c.store, + } for _, label := range c.cfg.Labels { key, val, _ := strings.Cut(label, "=") if !strings.HasPrefix(key, netlabel.DriverPrefix+"."+ntype) { @@ -348,10 +350,6 @@ func (c *Controller) makeDriverConfig(ntype string) map[string]interface{} { cfg[k] = v } - if c.cfg.Scope.IsValid() { - cfg[netlabel.LocalKVClient] = c.store - } - return cfg } diff --git a/libnetwork/datastore/datastore.go b/libnetwork/datastore/datastore.go index 0e38358abb..3e5da5637a 100644 --- a/libnetwork/datastore/datastore.go +++ b/libnetwork/datastore/datastore.go @@ -1,10 +1,10 @@ package datastore import ( - "fmt" + "errors" + "path" "strings" "sync" - "time" store "github.com/docker/docker/libnetwork/internal/kvstore" "github.com/docker/docker/libnetwork/internal/kvstore/boltdb" @@ -50,18 +50,6 @@ type KVObject interface { CopyTo(KVObject) error } -// ScopeCfg represents Datastore configuration. -type ScopeCfg struct { - Client ScopeClientCfg -} - -// ScopeClientCfg represents Datastore Client-only mode configuration -type ScopeClientCfg struct { - Provider string - Address string - Config *store.Config -} - const ( // NetworkKeyPrefix is the prefix for network key in the kv store NetworkKeyPrefix = "network" @@ -74,37 +62,7 @@ var ( rootChain = defaultRootChain ) -const defaultPrefix = "/var/lib/docker/network/files" - -// DefaultScope returns a default scope config for clients to use. -func DefaultScope(dataDir string) ScopeCfg { - var dbpath string - if dataDir == "" { - dbpath = defaultPrefix + "/local-kv.db" - } else { - dbpath = dataDir + "/network/files/local-kv.db" - } - - return ScopeCfg{ - Client: ScopeClientCfg{ - Provider: string(store.BOLTDB), - Address: dbpath, - Config: &store.Config{ - Bucket: "libnetwork", - ConnectionTimeout: time.Minute, - }, - }, - } -} - -// IsValid checks if the scope config has valid configuration. -func (cfg *ScopeCfg) IsValid() bool { - if cfg == nil || strings.TrimSpace(cfg.Client.Provider) == "" || strings.TrimSpace(cfg.Client.Address) == "" { - return false - } - - return true -} +const DefaultBucket = "libnetwork" // Key provides convenient method to create a Key func Key(key ...string) string { @@ -118,17 +76,16 @@ func Key(key ...string) string { return b.String() } -// newClient used to connect to KV Store -func newClient(kv string, addr string, config *store.Config) (*Store, error) { - if kv != string(store.BOLTDB) { - return nil, fmt.Errorf("unsupported KV store") +// New creates a new Store instance. +func New(dir, bucket string) (*Store, error) { + if dir == "" { + return nil, errors.New("empty dir") + } + if bucket == "" { + return nil, errors.New("empty bucket") } - if config == nil { - config = &store.Config{} - } - - s, err := boltdb.New(addr, config) + s, err := boltdb.New(path.Join(dir, "local-kv.db"), bucket) if err != nil { return nil, err } @@ -136,15 +93,6 @@ func newClient(kv string, addr string, config *store.Config) (*Store, error) { return &Store{store: s, cache: newCache(s)}, nil } -// New creates a new Store instance. -func New(cfg ScopeCfg) (*Store, error) { - if cfg.Client.Provider == "" || cfg.Client.Address == "" { - cfg = DefaultScope("") - } - - return newClient(cfg.Client.Provider, cfg.Client.Address, cfg.Client.Config) -} - // Close closes the data store. func (ds *Store) Close() { ds.store.Close() diff --git a/libnetwork/datastore/datastore_test.go b/libnetwork/datastore/datastore_test.go index 01b816c411..175a200594 100644 --- a/libnetwork/datastore/datastore_test.go +++ b/libnetwork/datastore/datastore_test.go @@ -23,16 +23,6 @@ func TestKey(t *testing.T) { assert.Check(t, is.Equal(sKey, expected)) } -func TestInvalidDataStore(t *testing.T) { - _, err := New(ScopeCfg{ - Client: ScopeClientCfg{ - Provider: "invalid", - Address: "localhost:8500", - }, - }) - assert.Check(t, is.Error(err, "unsupported KV store")) -} - func TestKVObjectFlatKey(t *testing.T) { store := NewTestDataStore() expected := dummyKVObject("1000", true) diff --git a/libnetwork/internal/kvstore/boltdb/boltdb.go b/libnetwork/internal/kvstore/boltdb/boltdb.go index bf3e68649d..d79c6155c9 100644 --- a/libnetwork/internal/kvstore/boltdb/boltdb.go +++ b/libnetwork/internal/kvstore/boltdb/boltdb.go @@ -3,19 +3,16 @@ package boltdb import ( "bytes" "encoding/binary" - "errors" "os" "path/filepath" "sync" "sync/atomic" + "time" store "github.com/docker/docker/libnetwork/internal/kvstore" bolt "go.etcd.io/bbolt" ) -// ErrBoltBucketOptionMissing is thrown when boltBucket config option is missing -var ErrBoltBucketOptionMissing = errors.New("boltBucket config option missing") - const filePerm = 0o644 // BoltDB type implements the Store interface @@ -30,18 +27,14 @@ type BoltDB struct { const libkvmetadatalen = 8 // New opens a new BoltDB connection to the specified path and bucket -func New(endpoint string, options *store.Config) (store.Store, error) { - if (options == nil) || (len(options.Bucket) == 0) { - return nil, ErrBoltBucketOptionMissing - } - - dir, _ := filepath.Split(endpoint) +func New(path, bucket string) (store.Store, error) { + dir, _ := filepath.Split(path) if err := os.MkdirAll(dir, 0o750); err != nil { return nil, err } - db, err := bolt.Open(endpoint, filePerm, &bolt.Options{ - Timeout: options.ConnectionTimeout, + db, err := bolt.Open(path, filePerm, &bolt.Options{ + Timeout: time.Minute, }) if err != nil { return nil, err @@ -49,8 +42,8 @@ func New(endpoint string, options *store.Config) (store.Store, error) { b := &BoltDB{ client: db, - path: endpoint, - boltBucket: []byte(options.Bucket), + path: path, + boltBucket: []byte(bucket), } return b, nil diff --git a/libnetwork/internal/kvstore/kvstore.go b/libnetwork/internal/kvstore/kvstore.go index 84e7300b5c..2314dc2b37 100644 --- a/libnetwork/internal/kvstore/kvstore.go +++ b/libnetwork/internal/kvstore/kvstore.go @@ -2,7 +2,6 @@ package kvstore import ( "errors" - "time" ) // Backend represents a KV Store Backend @@ -24,12 +23,6 @@ var ( ErrKeyExists = errors.New("Previous K/V pair exists, cannot complete Atomic operation") ) -// Config contains the options for a storage client -type Config struct { - ConnectionTimeout time.Duration - Bucket string -} - // Store represents the backend K/V storage // Each store should support every call listed // here. Or it couldn't be implemented as a K/V diff --git a/libnetwork/libnetwork_linux_test.go b/libnetwork/libnetwork_linux_test.go index b814513545..7f628f097c 100644 --- a/libnetwork/libnetwork_linux_test.go +++ b/libnetwork/libnetwork_linux_test.go @@ -21,7 +21,6 @@ import ( "github.com/docker/docker/internal/testutils/netnsutils" "github.com/docker/docker/libnetwork" "github.com/docker/docker/libnetwork/config" - "github.com/docker/docker/libnetwork/datastore" "github.com/docker/docker/libnetwork/driverapi" "github.com/docker/docker/libnetwork/ipams/defaultipam" "github.com/docker/docker/libnetwork/ipams/null" @@ -45,7 +44,7 @@ const ( func TestMain(m *testing.M) { // Cleanup local datastore file - _ = os.Remove(datastore.DefaultScope("").Client.Address) + _ = os.Remove("/var/lib/docker/network/files/local-kv.db") os.Exit(m.Run()) } diff --git a/libnetwork/store_linux_test.go b/libnetwork/store_linux_test.go index fe8fa378d2..f024d058b5 100644 --- a/libnetwork/store_linux_test.go +++ b/libnetwork/store_linux_test.go @@ -12,9 +12,7 @@ import ( func TestBoltdbBackend(t *testing.T) { tmpPath := filepath.Join(t.TempDir(), "boltdb.db") - testLocalBackend(t, "boltdb", tmpPath, &store.Config{ - Bucket: "testBackend", - }) + testLocalBackend(t, tmpPath, "testBackend") } func TestNoPersist(t *testing.T) { diff --git a/libnetwork/store_test.go b/libnetwork/store_test.go index 4fa55ec727..c9341e2b12 100644 --- a/libnetwork/store_test.go +++ b/libnetwork/store_test.go @@ -5,21 +5,18 @@ import ( "testing" "github.com/docker/docker/libnetwork/config" - store "github.com/docker/docker/libnetwork/internal/kvstore" "github.com/docker/docker/libnetwork/netlabel" "github.com/docker/docker/libnetwork/options" ) -func testLocalBackend(t *testing.T, provider, url string, storeConfig *store.Config) { - cfgOptions := []config.Option{func(c *config.Config) { - c.Scope.Client.Provider = provider - c.Scope.Client.Address = url - c.Scope.Client.Config = storeConfig - }} - - cfgOptions = append(cfgOptions, config.OptionDriverConfig("host", map[string]interface{}{ - netlabel.GenericData: options.Generic{}, - })) +func testLocalBackend(t *testing.T, path, bucket string) { + cfgOptions := []config.Option{ + config.OptionDataDir(path), + func(c *config.Config) { c.DatastoreBucket = bucket }, + config.OptionDriverConfig("host", map[string]interface{}{ + netlabel.GenericData: options.Generic{}, + }), + } testController, err := New(cfgOptions...) if err != nil {