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.

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

Allow pass global datastore config after boot #908

Merged
merged 2 commits into from
Feb 17, 2016
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
16 changes: 16 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,22 @@ func ParseConfig(tomlCfgFile string) (*Config, error) {
return cfg, nil
}

// ParseConfigOptions parses the configuration options and returns
// a reference to the corresponding Config structure
func ParseConfigOptions(cfgOptions ...Option) *Config {
cfg := &Config{
Daemon: DaemonCfg{
DriverCfg: make(map[string]interface{}),
},
Scopes: make(map[string]*datastore.ScopeCfg),
}

cfg.ProcessOptions(cfgOptions...)
cfg.LoadDefaultScopes(cfg.Daemon.DataDir)

return cfg
}

// Option is an option setter function type used to pass various configurations
// to the controller
type Option func(c *Config)
Expand Down
120 changes: 103 additions & 17 deletions controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ type NetworkController interface {

// Stop network controller
Stop()

// ReloadCondfiguration updates the controller configuration
ReloadConfiguration(cfgOptions ...config.Option) error
}

// NetworkWalker is a client provided function which will be used to walk the Networks.
Expand All @@ -129,7 +132,6 @@ type ipamData struct {
}

type driverTable map[string]*driverData

type ipamTable map[string]*ipamData
type sandboxTable map[string]*sandbox

Expand All @@ -153,22 +155,9 @@ type controller struct {

// New creates a new instance of network controller.
func New(cfgOptions ...config.Option) (NetworkController, error) {
var cfg *config.Config
cfg = &config.Config{
Daemon: config.DaemonCfg{
DriverCfg: make(map[string]interface{}),
},
Scopes: make(map[string]*datastore.ScopeCfg),
}

if len(cfgOptions) > 0 {
cfg.ProcessOptions(cfgOptions...)
}
cfg.LoadDefaultScopes(cfg.Daemon.DataDir)

c := &controller{
id: stringid.GenerateRandomID(),
cfg: cfg,
cfg: config.ParseConfigOptions(cfgOptions...),
sandboxes: sandboxTable{},
drivers: driverTable{},
ipamDrivers: ipamTable{},
Expand All @@ -179,8 +168,8 @@ func New(cfgOptions ...config.Option) (NetworkController, error) {
return nil, err
}

if cfg != nil && cfg.Cluster.Watcher != nil {
if err := c.initDiscovery(cfg.Cluster.Watcher); err != nil {
if c.cfg != nil && c.cfg.Cluster.Watcher != nil {
if err := c.initDiscovery(c.cfg.Cluster.Watcher); err != nil {
// Failing to initalize discovery is a bad situation to be in.
// But it cannot fail creating the Controller
log.Errorf("Failed to Initialize Discovery : %v", err)
Expand All @@ -206,6 +195,83 @@ func New(cfgOptions ...config.Option) (NetworkController, error) {
return c, nil
}

var procReloadConfig = make(chan (bool), 1)

func (c *controller) ReloadConfiguration(cfgOptions ...config.Option) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we protect this function with the buffered channel to make sure only one caller is active at a given time ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

procReloadConfig <- true
defer func() { <-procReloadConfig }()

// For now we accept the configuration reload only as a mean to provide a global store config after boot.
// Refuse the configuration if it alters an existing datastore client configuration.
update := false
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can avoid adding a new flag here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it is needed to detect config change.

cfg := config.ParseConfigOptions(cfgOptions...)
for s := range c.cfg.Scopes {
if _, ok := cfg.Scopes[s]; !ok {
return types.ForbiddenErrorf("cannot accept new configuration because it removes an existing datastore client")
}
}
for s, nSCfg := range cfg.Scopes {
if eSCfg, ok := c.cfg.Scopes[s]; ok {
if eSCfg.Client.Provider != nSCfg.Client.Provider ||
Copy link
Contributor

Choose a reason for hiding this comment

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

can you replace this with

if eSCfg.Client.Provider == nSCfg.Client.Provider && eSCfg.Client.Address == nSCfg.Client.Address {
    return nil
}
return types.ForbiddenErrorf("cannot accept new configuration because it modifies an existing datastore client")

By doing this, you can remove the update flag.

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 cannot refactor that way, the config comes with up to 2 datastores configs

eSCfg.Client.Address != nSCfg.Client.Address {
return types.ForbiddenErrorf("cannot accept new configuration because it modifies an existing datastore client")
}
} else {
update = true
}
}
if !update {
return nil
}

c.Lock()
c.cfg = cfg
c.Unlock()

if err := c.initStores(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

will this result in localscoped store being initialized twice ?

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 is not an issue. It simply replaces the local ds client with an exact copy in the stores list.

return err
}

if c.discovery == nil && c.cfg.Cluster.Watcher != nil {
if err := c.initDiscovery(c.cfg.Cluster.Watcher); err != nil {
log.Errorf("Failed to Initialize Discovery after configuration update: %v", err)
}
}

var dsConfig *discoverapi.DatastoreConfigData
for scope, sCfg := range cfg.Scopes {
if scope == datastore.LocalScope || !sCfg.IsValid() {
continue
}
dsConfig = &discoverapi.DatastoreConfigData{
Scope: scope,
Provider: sCfg.Client.Provider,
Address: sCfg.Client.Address,
Config: sCfg.Client.Config,
}
break
}
if dsConfig == nil {
return nil
}

for nm, id := range c.getIpamDrivers() {
err := id.driver.DiscoverNew(discoverapi.DatastoreConfig, *dsConfig)
if err != nil {
log.Errorf("Failed to set datastore in driver %s: %v", nm, err)
}
}

for nm, id := range c.getNetDrivers() {
err := id.driver.DiscoverNew(discoverapi.DatastoreConfig, *dsConfig)
if err != nil {
log.Errorf("Failed to set datastore in driver %s: %v", nm, err)
}
}

return nil
}

func (c *controller) ID() string {
return c.id
}
Expand Down Expand Up @@ -726,6 +792,26 @@ func (c *controller) getIpamDriver(name string) (ipamapi.Ipam, error) {
return id.driver, nil
}

func (c *controller) getIpamDrivers() ipamTable {
c.Lock()
defer c.Unlock()
table := ipamTable{}
for i, d := range c.ipamDrivers {
table[i] = d
}
return table
}

func (c *controller) getNetDrivers() driverTable {
c.Lock()
defer c.Unlock()
table := driverTable{}
for i, d := range c.drivers {
table[i] = d
}
return table
}

func (c *controller) Stop() {
c.closeStores()
c.stopExternalKeyListener()
Expand Down
29 changes: 29 additions & 0 deletions datastore/datastore.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/docker/libkv/store/consul"
"github.com/docker/libkv/store/etcd"
"github.com/docker/libkv/store/zookeeper"
"github.com/docker/libnetwork/discoverapi"
"github.com/docker/libnetwork/types"
)

Expand Down Expand Up @@ -253,6 +254,34 @@ func NewDataStore(scope string, cfg *ScopeCfg) (DataStore, error) {
return newClient(scope, cfg.Client.Provider, cfg.Client.Address, cfg.Client.Config, cached)
}

// NewDataStoreFromConfig creates a new instance of LibKV data store starting from the datastore config data
func NewDataStoreFromConfig(dsc discoverapi.DatastoreConfigData) (DataStore, 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)
}

scopeCfg := &ScopeCfg{
Client: ScopeClientCfg{
Address: dsc.Address,
Provider: dsc.Provider,
Config: sCfgP,
},
}

ds, err := NewDataStore(dsc.Scope, scopeCfg)
if err != nil {
return nil, fmt.Errorf("failed to construct datastore client from datastore configuration %v: %v", dsc, err)
}

return ds, err
}

func (ds *datastore) Close() {
ds.store.Close()
}
Expand Down
9 changes: 5 additions & 4 deletions discoverapi/discoverapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ type DiscoveryType int
const (
// NodeDiscovery represents Node join/leave events provided by discovery
NodeDiscovery = iota + 1
// DatastoreUpdate represents a add/remove datastore event
DatastoreUpdate
// DatastoreConfig represents a add/remove datastore event
DatastoreConfig
)

// NodeDiscoveryData represents the structure backing the node discovery data json string
Expand All @@ -26,8 +26,9 @@ type NodeDiscoveryData struct {
Self bool
}

// DatastoreUpdateData is the data for the datastore update event message
type DatastoreUpdateData struct {
// DatastoreConfigData is the data for the datastore update event message
type DatastoreConfigData struct {
Scope string
Provider string
Address string
Config interface{}
Expand Down
11 changes: 7 additions & 4 deletions drivers.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package libnetwork
import (
"strings"

"github.com/docker/libnetwork/discoverapi"
"github.com/docker/libnetwork/driverapi"
"github.com/docker/libnetwork/ipamapi"
builtinIpam "github.com/docker/libnetwork/ipams/builtin"
Expand Down Expand Up @@ -56,10 +57,12 @@ func makeDriverConfig(c *controller, ntype string) map[string]interface{} {
if !v.IsValid() {
continue
}

config[netlabel.MakeKVProvider(k)] = v.Client.Provider
config[netlabel.MakeKVProviderURL(k)] = v.Client.Address
config[netlabel.MakeKVProviderConfig(k)] = v.Client.Config
config[netlabel.MakeKVClient(k)] = discoverapi.DatastoreConfigData{
Scope: k,
Provider: v.Client.Provider,
Address: v.Client.Address,
Config: v.Client.Config,
}
}

return config
Expand Down
28 changes: 8 additions & 20 deletions drivers/bridge/bridge_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,37 +6,25 @@ import (
"net"

"github.com/Sirupsen/logrus"
"github.com/docker/libkv/store"
"github.com/docker/libkv/store/boltdb"
"github.com/docker/libnetwork/datastore"
"github.com/docker/libnetwork/discoverapi"
"github.com/docker/libnetwork/netlabel"
"github.com/docker/libnetwork/types"
)

const bridgePrefix = "bridge"

func (d *driver) initStore(option map[string]interface{}) error {
var err error

provider, provOk := option[netlabel.LocalKVProvider]
provURL, urlOk := option[netlabel.LocalKVProviderURL]

if provOk && urlOk {
cfg := &datastore.ScopeCfg{
Client: datastore.ScopeClientCfg{
Provider: provider.(string),
Address: provURL.(string),
},
if data, ok := option[netlabel.LocalKVClient]; ok {
var err error
dsc, ok := data.(discoverapi.DatastoreConfigData)
if !ok {
return types.InternalErrorf("incorrect data in datastore configuration: %v", data)
}

provConfig, confOk := option[netlabel.LocalKVProviderConfig]
if confOk {
cfg.Client.Config = provConfig.(*store.Config)
}

d.store, err = datastore.NewDataStore(datastore.LocalScope, cfg)
d.store, err = datastore.NewDataStoreFromConfig(dsc)
if err != nil {
return fmt.Errorf("bridge driver failed to initialize data store: %v", err)
return types.InternalErrorf("bridge driver failed to initialize data store: %v", err)
}

return d.populateNetworks()
Expand Down
Loading