Fix configuration reloading

There are five options 'debug' 'labels' 'cluster-store' 'cluster-store-opts'
and 'cluster-advertise' that can be reconfigured, configure any of these
options should not affect other options which may have configured in flags.
But this is not true, for example, I start a daemon with -D to enable the
debugging, and after a while, I want reconfigure the 'label', so I add a file
'/etc/docker/daemon.json' with content '"labels":["test"]' and send SIGHUP to daemon
to reconfigure the daemon, it work, but the debugging of the daemon is also diabled.
I don't think this is a expeted behaviour.
This patch also have some minor refactor of reconfiguration of cluster-advertiser.
Enable user to reconfigure cluster-advertiser without cluster-store in config file
since cluster-store could also be already set in flag, and we only want to reconfigure
the cluster-advertiser.

Signed-off-by: Lei Jitang <leijitang@huawei.com>

daemon/daemon.go

@@ -1602,24 +1602,37 @@ func (daemon *Daemon) initDiscovery(config *Config) error {
 func (daemon *Daemon) Reload(config *Config) error {
 	daemon.configStore.reloadLock.Lock()
 	defer daemon.configStore.reloadLock.Unlock()
-	daemon.configStore.Labels = config.Labels
+	if config.IsValueSet("label") {
+		daemon.configStore.Labels = config.Labels
+	}
+	if config.IsValueSet("debug") {
+		daemon.configStore.Debug = config.Debug
+	}
 	return daemon.reloadClusterDiscovery(config)
 }
 
 func (daemon *Daemon) reloadClusterDiscovery(config *Config) error {
-	newAdvertise, err := parseClusterAdvertiseSettings(config.ClusterStore, config.ClusterAdvertise)
-	if err != nil && err != errDiscoveryDisabled {
-		return err
+	var err error
+	newAdvertise := daemon.configStore.ClusterAdvertise
+	newClusterStore := daemon.configStore.ClusterStore
+	if config.IsValueSet("cluster-advertise") {
+		if config.IsValueSet("cluster-store") {
+			newClusterStore = config.ClusterStore
+		}
+		newAdvertise, err = parseClusterAdvertiseSettings(newClusterStore, config.ClusterAdvertise)
+		if err != nil && err != errDiscoveryDisabled {
+			return err
+		}
 	}
 
 	// check discovery modifications
-	if !modifiedDiscoverySettings(daemon.configStore, newAdvertise, config.ClusterStore, config.ClusterOpts) {
+	if !modifiedDiscoverySettings(daemon.configStore, newAdvertise, newClusterStore, config.ClusterOpts) {
 		return nil
 	}
 
 	// enable discovery for the first time if it was not previously enabled
 	if daemon.discoveryWatcher == nil {
-		discoveryWatcher, err := initDiscovery(config.ClusterStore, newAdvertise, config.ClusterOpts)
+		discoveryWatcher, err := initDiscovery(newClusterStore, newAdvertise, config.ClusterOpts)
 		if err != nil {
 			return fmt.Errorf("discovery initialization failed (%v)", err)
 		}
@@ -1636,7 +1649,7 @@ func (daemon *Daemon) reloadClusterDiscovery(config *Config) error {
 		}
 	}
 
-	daemon.configStore.ClusterStore = config.ClusterStore
+	daemon.configStore.ClusterStore = newClusterStore
 	daemon.configStore.ClusterOpts = config.ClusterOpts
 	daemon.configStore.ClusterAdvertise = newAdvertise
 

daemon/daemon_test.go

@@ -315,9 +315,12 @@ func TestDaemonReloadLabels(t *testing.T) {
 		},
 	}
 
+	valuesSets := make(map[string]interface{})
+	valuesSets["label"] = "foo:baz"
 	newConfig := &Config{
 		CommonConfig: CommonConfig{
-			Labels: []string{"foo:baz"},
+			Labels:    []string{"foo:baz"},
+			valuesSet: valuesSets,
 		},
 	}
 
@@ -328,6 +331,35 @@ func TestDaemonReloadLabels(t *testing.T) {
 	}
 }
 
+func TestDaemonReloadNotAffectOthers(t *testing.T) {
+	daemon := &Daemon{}
+	daemon.configStore = &Config{
+		CommonConfig: CommonConfig{
+			Labels: []string{"foo:bar"},
+			Debug:  true,
+		},
+	}
+
+	valuesSets := make(map[string]interface{})
+	valuesSets["label"] = "foo:baz"
+	newConfig := &Config{
+		CommonConfig: CommonConfig{
+			Labels:    []string{"foo:baz"},
+			valuesSet: valuesSets,
+		},
+	}
+
+	daemon.Reload(newConfig)
+	label := daemon.configStore.Labels[0]
+	if label != "foo:baz" {
+		t.Fatalf("Expected daemon label `foo:baz`, got %s", label)
+	}
+	debug := daemon.configStore.Debug
+	if !debug {
+		t.Fatalf("Expected debug 'enabled', got 'disabled'")
+	}
+}
+
 func TestDaemonDiscoveryReload(t *testing.T) {
 	daemon := &Daemon{}
 	daemon.configStore = &Config{
@@ -360,10 +392,14 @@ func TestDaemonDiscoveryReload(t *testing.T) {
 		t.Fatal(e)
 	}
 
+	valuesSets := make(map[string]interface{})
+	valuesSets["cluster-store"] = "memory://127.0.0.1:2222"
+	valuesSets["cluster-advertise"] = "127.0.0.1:5555"
 	newConfig := &Config{
 		CommonConfig: CommonConfig{
 			ClusterStore:     "memory://127.0.0.1:2222",
 			ClusterAdvertise: "127.0.0.1:5555",
+			valuesSet:        valuesSets,
 		},
 	}
 
@@ -392,10 +428,14 @@ func TestDaemonDiscoveryReloadFromEmptyDiscovery(t *testing.T) {
 	daemon := &Daemon{}
 	daemon.configStore = &Config{}
 
+	valuesSet := make(map[string]interface{})
+	valuesSet["cluster-store"] = "memory://127.0.0.1:2222"
+	valuesSet["cluster-advertise"] = "127.0.0.1:5555"
 	newConfig := &Config{
 		CommonConfig: CommonConfig{
 			ClusterStore:     "memory://127.0.0.1:2222",
 			ClusterAdvertise: "127.0.0.1:5555",
+			valuesSet:        valuesSet,
 		},
 	}
 
@@ -421,3 +461,42 @@ func TestDaemonDiscoveryReloadFromEmptyDiscovery(t *testing.T) {
 		t.Fatal(e)
 	}
 }
+
+func TestDaemonDiscoveryReloadOnlyClusterAdvertise(t *testing.T) {
+	daemon := &Daemon{}
+	daemon.configStore = &Config{
+		CommonConfig: CommonConfig{
+			ClusterStore: "memory://127.0.0.1",
+		},
+	}
+	valuesSets := make(map[string]interface{})
+	valuesSets["cluster-advertise"] = "127.0.0.1:5555"
+	newConfig := &Config{
+		CommonConfig: CommonConfig{
+			ClusterAdvertise: "127.0.0.1:5555",
+			valuesSet:        valuesSets,
+		},
+	}
+	expected := discovery.Entries{
+		&discovery.Entry{Host: "127.0.0.1", Port: "5555"},
+	}
+
+	if err := daemon.Reload(newConfig); err != nil {
+		t.Fatal(err)
+	}
+	stopCh := make(chan struct{})
+	defer close(stopCh)
+	ch, errCh := daemon.discoveryWatcher.Watch(stopCh)
+
+	select {
+	case <-time.After(1 * time.Second):
+		t.Fatal("failed to get discovery advertisements in time")
+	case e := <-ch:
+		if !reflect.DeepEqual(e, expected) {
+			t.Fatalf("expected %v, got %v\n", expected, e)
+		}
+	case e := <-errCh:
+		t.Fatal(e)
+	}
+
+}

docker/daemon.go

@@ -289,15 +289,17 @@ func (cli *DaemonCli) CmdDaemon(args ...string) error {
 			logrus.Errorf("Error reconfiguring the daemon: %v", err)
 			return
 		}
+		if config.IsValueSet("debug") {
+			debugEnabled := utils.IsDebugEnabled()
+			switch {
+			case debugEnabled && !config.Debug: // disable debug
+				utils.DisableDebug()
+				api.DisableProfiler()
+			case config.Debug && !debugEnabled: // enable debug
+				utils.EnableDebug()
+				api.EnableProfiler()
+			}
 
-		debugEnabled := utils.IsDebugEnabled()
-		switch {
-		case debugEnabled && !config.Debug: // disable debug
-			utils.DisableDebug()
-			api.DisableProfiler()
-		case config.Debug && !debugEnabled: // enable debug
-			utils.EnableDebug()
-			api.EnableProfiler()
 		}
 	}
 

docs/reference/commandline/daemon.md

@@ -896,6 +896,8 @@ The list of currently supported options that can be reconfigured is this:
 
 Updating and reloading the cluster configurations such as `--cluster-store`,
 `--cluster-advertise` and `--cluster-store-opts` will take effect only if
-these configurations were not previously configured. Configuration reload will
-log a warning message if it detects a change in previously configured cluster
-configurations.
+these configurations were not previously configured. If `--cluster-store`
+has been provided in flags and `cluster-advertise` not, `cluster-advertise`
+can be added in the configuration file without accompanied by `--cluster-store`
+Configuration reload will log a warning message if it detects a change in
+previously configured cluster configurations.