From d21d0884aebf991b418481efab619f6dce95d945 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 31 Jan 2024 17:24:47 -0500 Subject: [PATCH] libnetwork: share a single datastore with drivers The bbolt library wants exclusive access to the boltdb file and uses file locking to assure that is the case. The controller and each network driver that needs persistent storage instantiates its own unique datastore instance, backed by the same boltdb file. The boltdb kvstore implementation works around multiple access to the same boltdb file by aggressively closing the boltdb file between each transaction. This is very inefficient. Have the controller pass its datastore instance into the drivers and enable the PersistConnection option to disable closing the boltdb between transactions. Set data-dir in unit tests which instantiate libnetwork controllers so they don't hang trying to lock the default boltdb database file. Signed-off-by: Cory Snider --- daemon/oci_linux_test.go | 3 ++- daemon/reload_test.go | 2 +- libnetwork/controller.go | 9 +------ libnetwork/datastore/datastore.go | 28 +-------------------- libnetwork/discoverapi/discoverapi.go | 2 ++ libnetwork/drivers/bridge/bridge_store.go | 11 +++----- libnetwork/drivers/ipvlan/ipvlan_store.go | 11 +++----- libnetwork/drivers/macvlan/macvlan_store.go | 11 +++----- libnetwork/drivers/windows/windows_store.go | 11 +++----- libnetwork/firewall_linux_test.go | 14 ++++++----- libnetwork/libnetwork_internal_test.go | 6 ++--- libnetwork/libnetwork_linux_test.go | 2 +- libnetwork/resolver_unix_test.go | 4 +-- libnetwork/sandbox_dns_unix_test.go | 2 +- libnetwork/service_common_unix_test.go | 2 +- libnetwork/store_linux_test.go | 12 +-------- 16 files changed, 36 insertions(+), 94 deletions(-) diff --git a/daemon/oci_linux_test.go b/daemon/oci_linux_test.go index b2008394df..b33fa365e9 100644 --- a/daemon/oci_linux_test.go +++ b/daemon/oci_linux_test.go @@ -11,6 +11,7 @@ import ( "github.com/docker/docker/daemon/config" "github.com/docker/docker/daemon/network" "github.com/docker/docker/libnetwork" + nwconfig "github.com/docker/docker/libnetwork/config" "github.com/google/go-cmp/cmp/cmpopts" "github.com/opencontainers/runtime-spec/specs-go" "golang.org/x/sys/unix" @@ -27,7 +28,7 @@ func setupFakeDaemon(t *testing.T, c *container.Container) *Daemon { err := os.MkdirAll(rootfs, 0o755) assert.NilError(t, err) - netController, err := libnetwork.New() + netController, err := libnetwork.New(nwconfig.OptionDataDir(t.TempDir())) assert.NilError(t, err) d := &Daemon{ diff --git a/daemon/reload_test.go b/daemon/reload_test.go index de606f3c60..985be05718 100644 --- a/daemon/reload_test.go +++ b/daemon/reload_test.go @@ -360,7 +360,7 @@ func TestDaemonReloadNetworkDiagnosticPort(t *testing.T) { }, } - netOptions, err := daemon.networkOptions(&config.Config{}, nil, nil) + netOptions, err := daemon.networkOptions(&config.Config{CommonConfig: config.CommonConfig{Root: t.TempDir()}}, nil, nil) if err != nil { t.Fatal(err) } diff --git a/libnetwork/controller.go b/libnetwork/controller.go index bcbbdd06ca..9a34a87e11 100644 --- a/libnetwork/controller.go +++ b/libnetwork/controller.go @@ -344,14 +344,7 @@ func (c *Controller) makeDriverConfig(ntype string) map[string]interface{} { } if c.cfg.Scope.IsValid() { - // FIXME: every driver instance constructs a new DataStore - // instance against the same database. Yikes! - cfg[netlabel.LocalKVClient] = discoverapi.DatastoreConfigData{ - Scope: scope.Local, - Provider: c.cfg.Scope.Client.Provider, - Address: c.cfg.Scope.Client.Address, - Config: c.cfg.Scope.Client.Config, - } + cfg[netlabel.LocalKVClient] = c.store } return cfg diff --git a/libnetwork/datastore/datastore.go b/libnetwork/datastore/datastore.go index b12fdba9e1..68e877b0a0 100644 --- a/libnetwork/datastore/datastore.go +++ b/libnetwork/datastore/datastore.go @@ -6,7 +6,6 @@ import ( "sync" "time" - "github.com/docker/docker/libnetwork/discoverapi" store "github.com/docker/docker/libnetwork/internal/kvstore" "github.com/docker/docker/libnetwork/internal/kvstore/boltdb" "github.com/docker/docker/libnetwork/types" @@ -93,6 +92,7 @@ func DefaultScope(dataDir string) ScopeCfg { Config: &store.Config{ Bucket: "libnetwork", ConnectionTimeout: time.Minute, + PersistConnection: true, }, }, } @@ -147,32 +147,6 @@ func New(cfg ScopeCfg) (*Store, error) { return newClient(cfg.Client.Provider, cfg.Client.Address, cfg.Client.Config) } -// FromConfig creates a new instance of LibKV data store starting from the datastore config data. -func FromConfig(dsc discoverapi.DatastoreConfigData) (*Store, error) { - var ( - ok bool - sCfgP *store.Config - ) - - sCfgP, ok = dsc.Config.(*store.Config) - if !ok && dsc.Config != nil { - return nil, fmt.Errorf("cannot parse store configuration: %v", dsc.Config) - } - - ds, err := New(ScopeCfg{ - Client: ScopeClientCfg{ - Address: dsc.Address, - Provider: dsc.Provider, - Config: sCfgP, - }, - }) - if err != nil { - return nil, fmt.Errorf("failed to construct datastore client from datastore configuration %v: %v", dsc, err) - } - - return ds, err -} - // Close closes the data store. func (ds *Store) Close() { ds.store.Close() diff --git a/libnetwork/discoverapi/discoverapi.go b/libnetwork/discoverapi/discoverapi.go index 8ee2f3edcb..f71013544e 100644 --- a/libnetwork/discoverapi/discoverapi.go +++ b/libnetwork/discoverapi/discoverapi.go @@ -30,6 +30,8 @@ type NodeDiscoveryData struct { } // DatastoreConfigData is the data for the datastore update event message +// +// Deprecated: no longer used. type DatastoreConfigData struct { Scope string Provider string diff --git a/libnetwork/drivers/bridge/bridge_store.go b/libnetwork/drivers/bridge/bridge_store.go index 2f13fafcba..e91f3cff51 100644 --- a/libnetwork/drivers/bridge/bridge_store.go +++ b/libnetwork/drivers/bridge/bridge_store.go @@ -10,7 +10,6 @@ import ( "github.com/containerd/log" "github.com/docker/docker/libnetwork/datastore" - "github.com/docker/docker/libnetwork/discoverapi" "github.com/docker/docker/libnetwork/netlabel" "github.com/docker/docker/libnetwork/types" ) @@ -25,17 +24,13 @@ const ( func (d *driver) initStore(option map[string]interface{}) error { if data, ok := option[netlabel.LocalKVClient]; ok { - var err error - dsc, ok := data.(discoverapi.DatastoreConfigData) + var ok bool + d.store, ok = data.(*datastore.Store) if !ok { return types.InternalErrorf("incorrect data in datastore configuration: %v", data) } - d.store, err = datastore.FromConfig(dsc) - if err != nil { - return types.InternalErrorf("bridge driver failed to initialize data store: %v", err) - } - err = d.populateNetworks() + err := d.populateNetworks() if err != nil { return err } diff --git a/libnetwork/drivers/ipvlan/ipvlan_store.go b/libnetwork/drivers/ipvlan/ipvlan_store.go index 068f10edec..476c7de26f 100644 --- a/libnetwork/drivers/ipvlan/ipvlan_store.go +++ b/libnetwork/drivers/ipvlan/ipvlan_store.go @@ -10,7 +10,6 @@ import ( "github.com/containerd/log" "github.com/docker/docker/libnetwork/datastore" - "github.com/docker/docker/libnetwork/discoverapi" "github.com/docker/docker/libnetwork/netlabel" "github.com/docker/docker/libnetwork/types" ) @@ -44,17 +43,13 @@ type ipSubnet struct { // initStore drivers are responsible for caching their own persistent state func (d *driver) initStore(option map[string]interface{}) error { if data, ok := option[netlabel.LocalKVClient]; ok { - var err error - dsc, ok := data.(discoverapi.DatastoreConfigData) + var ok bool + d.store, ok = data.(*datastore.Store) if !ok { return types.InternalErrorf("incorrect data in datastore configuration: %v", data) } - d.store, err = datastore.FromConfig(dsc) - if err != nil { - return types.InternalErrorf("ipvlan driver failed to initialize data store: %v", err) - } - err = d.populateNetworks() + err := d.populateNetworks() if err != nil { return err } diff --git a/libnetwork/drivers/macvlan/macvlan_store.go b/libnetwork/drivers/macvlan/macvlan_store.go index 3b84559126..1221c72ad1 100644 --- a/libnetwork/drivers/macvlan/macvlan_store.go +++ b/libnetwork/drivers/macvlan/macvlan_store.go @@ -10,7 +10,6 @@ import ( "github.com/containerd/log" "github.com/docker/docker/libnetwork/datastore" - "github.com/docker/docker/libnetwork/discoverapi" "github.com/docker/docker/libnetwork/netlabel" "github.com/docker/docker/libnetwork/types" ) @@ -43,17 +42,13 @@ type ipSubnet struct { // initStore drivers are responsible for caching their own persistent state func (d *driver) initStore(option map[string]interface{}) error { if data, ok := option[netlabel.LocalKVClient]; ok { - var err error - dsc, ok := data.(discoverapi.DatastoreConfigData) + var ok bool + d.store, ok = data.(*datastore.Store) if !ok { return types.InternalErrorf("incorrect data in datastore configuration: %v", data) } - d.store, err = datastore.FromConfig(dsc) - if err != nil { - return types.InternalErrorf("macvlan driver failed to initialize data store: %v", err) - } - err = d.populateNetworks() + err := d.populateNetworks() if err != nil { return err } diff --git a/libnetwork/drivers/windows/windows_store.go b/libnetwork/drivers/windows/windows_store.go index 7a43db7140..f7e83c2386 100644 --- a/libnetwork/drivers/windows/windows_store.go +++ b/libnetwork/drivers/windows/windows_store.go @@ -10,7 +10,6 @@ import ( "github.com/containerd/log" "github.com/docker/docker/libnetwork/datastore" - "github.com/docker/docker/libnetwork/discoverapi" "github.com/docker/docker/libnetwork/netlabel" "github.com/docker/docker/libnetwork/types" ) @@ -22,17 +21,13 @@ const ( func (d *driver) initStore(option map[string]interface{}) error { if data, ok := option[netlabel.LocalKVClient]; ok { - var err error - dsc, ok := data.(discoverapi.DatastoreConfigData) + var ok bool + d.store, ok = data.(*datastore.Store) if !ok { return types.InternalErrorf("incorrect data in datastore configuration: %v", data) } - d.store, err = datastore.FromConfig(dsc) - if err != nil { - return types.InternalErrorf("windows driver failed to initialize data store: %v", err) - } - err = d.populateNetworks() + err := d.populateNetworks() if err != nil { return err } diff --git a/libnetwork/firewall_linux_test.go b/libnetwork/firewall_linux_test.go index 7e43380b7c..79663376c7 100644 --- a/libnetwork/firewall_linux_test.go +++ b/libnetwork/firewall_linux_test.go @@ -54,12 +54,14 @@ func TestUserChain(t *testing.T) { defer netnsutils.SetupTestOSContext(t)() defer resetIptables(t) - c, err := New(config.OptionDriverConfig("bridge", map[string]any{ - netlabel.GenericData: options.Generic{ - "EnableIPTables": tc.iptables, - "EnableIP6Tables": tc.iptables, - }, - })) + c, err := New( + OptionBoltdbWithRandomDBFile(t), + config.OptionDriverConfig("bridge", map[string]any{ + netlabel.GenericData: options.Generic{ + "EnableIPTables": tc.iptables, + "EnableIP6Tables": tc.iptables, + }, + })) assert.NilError(t, err) defer c.Stop() diff --git a/libnetwork/libnetwork_internal_test.go b/libnetwork/libnetwork_internal_test.go index ef42dba7bf..96084c6ef0 100644 --- a/libnetwork/libnetwork_internal_test.go +++ b/libnetwork/libnetwork_internal_test.go @@ -314,7 +314,7 @@ func compareNwLists(a, b []*net.IPNet) bool { func TestAuxAddresses(t *testing.T) { defer netnsutils.SetupTestOSContext(t)() - c, err := New() + c, err := New(OptionBoltdbWithRandomDBFile(t)) if err != nil { t.Fatal(err) } @@ -353,7 +353,7 @@ func TestSRVServiceQuery(t *testing.T) { defer netnsutils.SetupTestOSContext(t)() - c, err := New() + c, err := New(OptionBoltdbWithRandomDBFile(t)) if err != nil { t.Fatal(err) } @@ -451,7 +451,7 @@ func TestServiceVIPReuse(t *testing.T) { defer netnsutils.SetupTestOSContext(t)() - c, err := New() + c, err := New(OptionBoltdbWithRandomDBFile(t)) if err != nil { t.Fatal(err) } diff --git a/libnetwork/libnetwork_linux_test.go b/libnetwork/libnetwork_linux_test.go index a228a657c7..47793dccf8 100644 --- a/libnetwork/libnetwork_linux_test.go +++ b/libnetwork/libnetwork_linux_test.go @@ -1197,7 +1197,7 @@ func TestInvalidRemoteDriver(t *testing.T) { t.Fatal(err) } - ctrlr, err := libnetwork.New() + ctrlr, err := libnetwork.New(libnetwork.OptionBoltdbWithRandomDBFile(t)) if err != nil { t.Fatal(err) } diff --git a/libnetwork/resolver_unix_test.go b/libnetwork/resolver_unix_test.go index 91b6891fe0..4a7c2b4d51 100644 --- a/libnetwork/resolver_unix_test.go +++ b/libnetwork/resolver_unix_test.go @@ -13,7 +13,7 @@ import ( // test only works on linux func TestDNSIPQuery(t *testing.T) { defer netnsutils.SetupTestOSContext(t)() - c, err := New() + c, err := New(OptionBoltdbWithRandomDBFile(t)) if err != nil { t.Fatal(err) } @@ -110,7 +110,7 @@ func TestDNSProxyServFail(t *testing.T) { osctx := netnsutils.SetupTestOSContextEx(t) defer osctx.Cleanup(t) - c, err := New() + c, err := New(OptionBoltdbWithRandomDBFile(t)) if err != nil { t.Fatal(err) } diff --git a/libnetwork/sandbox_dns_unix_test.go b/libnetwork/sandbox_dns_unix_test.go index 5a01a13d52..d092ab53fd 100644 --- a/libnetwork/sandbox_dns_unix_test.go +++ b/libnetwork/sandbox_dns_unix_test.go @@ -15,7 +15,7 @@ import ( func TestDNSOptions(t *testing.T) { skip.If(t, runtime.GOOS == "windows", "test only works on linux") - c, err := New() + c, err := New(OptionBoltdbWithRandomDBFile(t)) assert.NilError(t, err) sb, err := c.NewSandbox("cnt1", nil) diff --git a/libnetwork/service_common_unix_test.go b/libnetwork/service_common_unix_test.go index 4ea78ad917..e098d4180d 100644 --- a/libnetwork/service_common_unix_test.go +++ b/libnetwork/service_common_unix_test.go @@ -12,7 +12,7 @@ import ( func TestCleanupServiceDiscovery(t *testing.T) { defer netnsutils.SetupTestOSContext(t)() - c, err := New() + c, err := New(OptionBoltdbWithRandomDBFile(t)) assert.NilError(t, err) defer c.Stop() diff --git a/libnetwork/store_linux_test.go b/libnetwork/store_linux_test.go index d06c9631c7..0fc7db78e2 100644 --- a/libnetwork/store_linux_test.go +++ b/libnetwork/store_linux_test.go @@ -2,18 +2,13 @@ package libnetwork import ( "errors" - "os" "path/filepath" "testing" - "github.com/docker/docker/libnetwork/config" - "github.com/docker/docker/libnetwork/datastore" store "github.com/docker/docker/libnetwork/internal/kvstore" ) func TestBoltdbBackend(t *testing.T) { - defer os.Remove(datastore.DefaultScope("").Client.Address) - testLocalBackend(t, "", "", nil) tmpPath := filepath.Join(t.TempDir(), "boltdb.db") testLocalBackend(t, "boltdb", tmpPath, &store.Config{ Bucket: "testBackend", @@ -21,12 +16,7 @@ func TestBoltdbBackend(t *testing.T) { } func TestNoPersist(t *testing.T) { - dbFile := filepath.Join(t.TempDir(), "bolt.db") - configOption := func(c *config.Config) { - c.Scope.Client.Provider = "boltdb" - c.Scope.Client.Address = dbFile - c.Scope.Client.Config = &store.Config{Bucket: "testBackend"} - } + configOption := OptionBoltdbWithRandomDBFile(t) testController, err := New(configOption) if err != nil { t.Fatalf("Error creating new controller: %v", err)