Merge lp:~axwalk/juju-core/setenvironconfig-delta into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

state: update SetEnvironConfig to take new and old

SetEnvironConfig is updated to take both new and old
configurations, and a delta computed from these. The
only functional change for now is that attributes that
exist in old but not in new are now deleted from state.

To do this right, we should also check that the
settings in state are the same as the "old" config,
so the delta computed is what the user expects. I have
not done this part yet, as it is a fairly big change
and I am not confident I understand all of the
repercussions.

https://codereview.appspot.com/38180043/

state: update SetEnvironConfig to take new and old

SetEnvironConfig is updated to take both new and old
configurations, and a delta computed from these. The
only functional change for now is that attributes that
exist in old but not in new are now deleted from state.

To do this right, we should also check that the
settings in state are the same as the "old" config,
so the delta computed is what the user expects. I have
not done this part yet, as it is a fairly big change
and I am not confident I understand all of the
repercussions.

https://codereview.appspot.com/38180043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+197989_code.launchpad.net,

Message:
Please take a look.

Description:
state: update SetEnvironConfig to take new and old

SetEnvironConfig is updated to take both new and old
configurations, and a delta computed from these. The
only functional change for now is that attributes that
exist in old but not in new are now deleted from state.

To do this right, we should also check that the
settings in state are the same as the "old" config,
so the delta computed is what the user expects. I have
not done this part yet, as it is a fairly big change
and I am not confident I understand all of the
repercussions.

https://code.launchpad.net/~axwalk/juju-core/setenvironconfig-delta/+merge/197989

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/38180043/

Affected files (+70, -52 lines):
   A [revision details]
   M cmd/juju/environment.go
   M cmd/juju/upgradejuju_test.go
   M cmd/jujud/agent_test.go
   M environs/testing/tools.go
   M juju/conn.go
   M juju/testing/repo.go
   M juju/testing/utils.go
   M state/api/provisioner/provisioner_test.go
   M state/apiserver/client/client.go
   M state/initialize_test.go
   M state/state.go
   M state/state_test.go
   M state/testing/config.go
   M worker/firewaller/firewaller_test.go
   M worker/provisioner/provisioner_test.go

[Roger Peppe (rogpeppe)]

LGTM with one thought below. Thanks!

https://codereview.appspot.com/38180043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/38180043/diff/1/state/state.go#newcode272
state/state.go:272: settings.Update(newattrs)
To do this right, shouldn't we only update
those settings that have changed between old
and new?

Otherwise if two people concurrently do:

  juju set-environment foo=a
  juju set-environment bar=b

then one of them may overwrite the other.

I suspect that the only way to do this correctly is
to have something like:

func (st *State) UpdateEnvironConfig(func (*config.Config)
(*config.Config, error)) error

which would retrieve the configuration from state, call the given
function to determine the new one and verify that it's valid, and apply
the new configuration to the state, checking that the settings have not
changed in the meantime.

I guess that's a bigger change though, and the change in this CL is at
least a significant improvement on what we had before, so I'm OK with it
for now.

https://codereview.appspot.com/38180043/

[Andrew Wilkins (axwalk)]

Thanks for the review

https://codereview.appspot.com/38180043/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/38180043/diff/1/state/state.go#newcode272
state/state.go:272: settings.Update(newattrs)
On 2013/12/06 08:46:23, rog wrote:
> To do this right, shouldn't we only update
> those settings that have changed between old
> and new?

> Otherwise if two people concurrently do:

> juju set-environment foo=a
> juju set-environment bar=b

> then one of them may overwrite the other.

I don't think it will overwrite in that example, but I may be mistaken.
Updates and deletions are translated to $set/$unset transaction
operations. Deletions are only relative to the "old" config, which is
known to the caller. In both the cases you give, neither one will cause
a deletion.

> I suspect that the only way to do this correctly is
> to have something like:

> func (st *State) UpdateEnvironConfig(func (*config.Config)
(*config.Config,
> error)) error

> which would retrieve the configuration from state, call the given
> function to determine the new one and verify that it's valid, and
apply the new
> configuration to the state, checking that the settings have not
changed in the
> meantime.

> I guess that's a bigger change though, and the change in this CL is at
least a
> significant improvement on what we had before, so I'm OK with it for
now.

https://codereview.appspot.com/38180043/

[Andrew Wilkins (axwalk)]

On 2013/12/06 09:01:26, axw wrote:
> Thanks for the review

> https://codereview.appspot.com/38180043/diff/1/state/state.go
> File state/state.go (right):

https://codereview.appspot.com/38180043/diff/1/state/state.go#newcode272
> state/state.go:272: settings.Update(newattrs)
> On 2013/12/06 08:46:23, rog wrote:
> > To do this right, shouldn't we only update
> > those settings that have changed between old
> > and new?
> >
> > Otherwise if two people concurrently do:
> >
> > juju set-environment foo=a
> > juju set-environment bar=b
> >
> > then one of them may overwrite the other.

> I don't think it will overwrite in that example, but I may be
mistaken. Updates
> and deletions are translated to $set/$unset transaction operations.
Deletions
> are only relative to the "old" config, which is known to the caller.
In both the
> cases you give, neither one will cause a deletion.

Responding to myself here: rog meant that we could overwrite foo/bar
with an old value, on account of us updating everything
(Update(AllAttrs)).

> > I suspect that the only way to do this correctly is
> > to have something like:
> >
> > func (st *State) UpdateEnvironConfig(func (*config.Config)
(*config.Config,
> > error)) error
> >
> > which would retrieve the configuration from state, call the given
> > function to determine the new one and verify that it's valid, and
apply the
> new
> > configuration to the state, checking that the settings have not
changed in the
> > meantime.
> >
> > I guess that's a bigger change though, and the change in this CL is
at least a
> > significant improvement on what we had before, so I'm OK with it for
now.

https://codereview.appspot.com/38180043/

=== modified file 'cmd/juju/environment.go'
--- cmd/juju/environment.go	2013-11-28 12:52:51 +0000
+++ cmd/juju/environment.go	2013-12-06 04:54:05 +0000
@@ -181,7 +181,7 @@
 		return err
 	}
 	// Now try to apply the new validated config.
-	return conn.State.SetEnvironConfig(newProviderConfig)
+	return conn.State.SetEnvironConfig(newProviderConfig, oldConfig)
 }
 
 func (c *SetEnvironmentCommand) Run(ctx *cmd.Context) error {

=== modified file 'cmd/juju/upgradejuju_test.go'
--- cmd/juju/upgradejuju_test.go	2013-11-18 05:41:36 +0000
+++ cmd/juju/upgradejuju_test.go	2013-12-06 04:54:05 +0000
@@ -296,15 +296,15 @@
 		}
 
 		// Set up state and environ, and run the command.
-		cfg, err := s.State.EnvironConfig()
+		oldcfg, err := s.State.EnvironConfig()
 		c.Assert(err, gc.IsNil)
 		toolsDir := c.MkDir()
-		cfg, err = cfg.Apply(map[string]interface{}{
+		cfg, err := oldcfg.Apply(map[string]interface{}{
 			"agent-version":      test.agentVersion,
 			"tools-metadata-url": "file://" + toolsDir,
 		})
 		c.Assert(err, gc.IsNil)
-		err = s.State.SetEnvironConfig(cfg)
+		err = s.State.SetEnvironConfig(cfg, oldcfg)
 		c.Assert(err, gc.IsNil)
 		versions := make([]version.Binary, len(test.tools))
 		for i, v := range test.tools {
@@ -377,14 +377,14 @@
 func (s *UpgradeJujuSuite) Reset(c *gc.C) {
 	s.JujuConnSuite.Reset(c)
 	envtesting.RemoveTools(c, s.Conn.Environ.Storage())
-	cfg, err := s.State.EnvironConfig()
+	oldcfg, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
-	cfg, err = cfg.Apply(map[string]interface{}{
+	cfg, err := oldcfg.Apply(map[string]interface{}{
 		"default-series": "raring",
 		"agent-version":  "1.2.3",
 	})
 	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(cfg)
+	err = s.State.SetEnvironConfig(cfg, oldcfg)
 	c.Assert(err, gc.IsNil)
 }
 

=== modified file 'cmd/jujud/agent_test.go'
--- cmd/jujud/agent_test.go	2013-10-24 20:58:30 +0000
+++ cmd/jujud/agent_test.go	2013-12-06 04:54:05 +0000
@@ -255,13 +255,13 @@
 }
 
 func (s *agentSuite) proposeVersion(c *gc.C, vers version.Number) {
-	cfg, err := s.State.EnvironConfig()
+	oldcfg, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
-	cfg, err = cfg.Apply(map[string]interface{}{
+	cfg, err := oldcfg.Apply(map[string]interface{}{
 		"agent-version": vers.String(),
 	})
 	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(cfg)
+	err = s.State.SetEnvironConfig(cfg, oldcfg)
 	c.Assert(err, gc.IsNil)
 }
 

=== modified file 'environs/testing/tools.go'
--- environs/testing/tools.go	2013-11-18 05:41:36 +0000
+++ environs/testing/tools.go	2013-12-06 04:54:05 +0000
@@ -453,6 +453,6 @@
 	attrs["ssl-hostname-verification"] = SSLHostnameVerification
 	newConfig, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)
-	err = st.SetEnvironConfig(newConfig)
+	err = st.SetEnvironConfig(newConfig, envConfig)
 	c.Assert(err, gc.IsNil)
 }

=== modified file 'juju/conn.go'
--- juju/conn.go	2013-11-19 17:25:24 +0000
+++ juju/conn.go	2013-12-06 04:54:05 +0000
@@ -160,11 +160,11 @@
 			attrs[k] = v
 		}
 	}
-	cfg, err = config.New(config.NoDefaults, attrs)
+	newcfg, err := config.New(config.NoDefaults, attrs)
 	if err != nil {
 		return err
 	}
-	return c.State.SetEnvironConfig(cfg)
+	return c.State.SetEnvironConfig(newcfg, cfg)
 }
 
 // PutCharm uploads the given charm to provider storage, and adds a

=== modified file 'juju/testing/repo.go'
--- juju/testing/repo.go	2013-09-13 14:48:13 +0000
+++ juju/testing/repo.go	2013-12-06 04:54:05 +0000
@@ -27,11 +27,11 @@
 	s.JujuConnSuite.SetUpTest(c)
 	// Change the environ's config to ensure we're using the one in state,
 	// not the one in the local environments.yaml
-	cfg, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	cfg, err = cfg.Apply(map[string]interface{}{"default-series": "precise"})
-	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(cfg)
+	oldcfg, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	cfg, err := oldcfg.Apply(map[string]interface{}{"default-series": "precise"})
+	c.Assert(err, gc.IsNil)
+	err = s.State.SetEnvironConfig(cfg, oldcfg)
 	c.Assert(err, gc.IsNil)
 	s.RepoPath = os.Getenv("JUJU_REPOSITORY")
 	repoPath := c.MkDir()

=== modified file 'juju/testing/utils.go'
--- juju/testing/utils.go	2013-10-17 17:45:41 +0000
+++ juju/testing/utils.go	2013-12-06 04:54:05 +0000
@@ -20,7 +20,7 @@
 	c.Assert(err, gc.IsNil)
 	newCfg, err := config.New(config.NoDefaults, change(cfg.AllAttrs()))
 	c.Assert(err, gc.IsNil)
-	err = st.SetEnvironConfig(newCfg)
+	err = st.SetEnvironConfig(newCfg, cfg)
 	c.Assert(err, gc.IsNil)
 }
 

=== modified file 'state/api/provisioner/provisioner_test.go'
--- state/api/provisioner/provisioner_test.go	2013-11-28 23:59:23 +0000
+++ state/api/provisioner/provisioner_test.go	2013-12-06 04:54:05 +0000
@@ -352,12 +352,12 @@
 	attrs["type"] = "blah"
 	newConfig, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(newConfig)
+	err = s.State.SetEnvironConfig(newConfig, envConfig)
 	c.Assert(err, gc.IsNil)
 	wc.AssertOneChange()
 
 	// Change it back to the original config.
-	err = s.State.SetEnvironConfig(envConfig)
+	err = s.State.SetEnvironConfig(envConfig, newConfig)
 	c.Assert(err, gc.IsNil)
 	wc.AssertOneChange()
 

=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2013-12-04 15:34:47 +0000
+++ state/apiserver/client/client.go	2013-12-06 04:54:05 +0000
@@ -822,7 +822,7 @@
 		return err
 	}
 	// Now try to apply the new validated config.
-	return c.api.state.SetEnvironConfig(newProviderConfig)
+	return c.api.state.SetEnvironConfig(newProviderConfig, oldConfig)
 }
 
 // SetEnvironAgentVersion sets the environment agent version.

=== modified file 'state/initialize_test.go'
--- state/initialize_test.go	2013-09-23 11:43:36 +0000
+++ state/initialize_test.go	2013-12-06 04:54:05 +0000
@@ -109,7 +109,7 @@
 	// admin-secret blocks SetEnvironConfig.
 	st := state.TestingInitialize(c, good)
 	st.Close()
-	err = s.State.SetEnvironConfig(bad)
+	err = s.State.SetEnvironConfig(bad, good)
 	c.Assert(err, gc.ErrorMatches, "admin-secret should never be written to the state")
 
 	// EnvironConfig remains inviolate.
@@ -132,7 +132,7 @@
 	// Bad agent-version blocks SetEnvironConfig.
 	st := state.TestingInitialize(c, good)
 	st.Close()
-	err = s.State.SetEnvironConfig(bad)
+	err = s.State.SetEnvironConfig(bad, good)
 	c.Assert(err, gc.ErrorMatches, "agent-version must always be set in state")
 
 	// EnvironConfig remains inviolate.

=== modified file 'state/state.go'
--- state/state.go	2013-12-04 15:34:47 +0000
+++ state/state.go	2013-12-06 04:54:05 +0000
@@ -249,18 +249,27 @@
 
 // SetEnvironConfig replaces the current configuration of the
 // environment with the provided configuration.
-func (st *State) SetEnvironConfig(cfg *config.Config) error {
+func (st *State) SetEnvironConfig(cfg, old *config.Config) error {
 	if err := checkEnvironConfig(cfg); err != nil {
 		return err
 	}
-	// TODO(niemeyer): This isn't entirely right as the change is done as a
-	// delta that the user didn't ask for. Instead, take a (old, new) config
-	// pair, and apply *known* delta.
+	// TODO(axw) 2013-12-6 #1167616
+	// Ensure that the settings on disk have not changed
+	// underneath us. The settings changes are actually
+	// applied as a delta to what's on disk; if there has
+	// been a concurrent update, the change may not be what
+	// the user asked for.
 	settings, err := readSettings(st, environGlobalKey)
 	if err != nil {
 		return err
 	}
-	settings.Update(cfg.AllAttrs())
+	newattrs := cfg.AllAttrs()
+	for k, _ := range old.AllAttrs() {
+		if _, ok := newattrs[k]; !ok {
+			settings.Delete(k)
+		}
+	}
+	settings.Update(newattrs)
 	_, err = settings.Write()
 	return err
 }

=== modified file 'state/state_test.go'
--- state/state_test.go	2013-12-04 15:34:47 +0000
+++ state/state_test.go	2013-12-06 04:54:05 +0000
@@ -805,7 +805,7 @@
 		"arbitrary-key":   "shazam!",
 	})
 	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(change)
+	err = s.State.SetEnvironConfig(change, cfg)
 	c.Assert(err, gc.IsNil)
 	cfg, err = s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
@@ -1236,9 +1236,10 @@
 		cfg, err := s.State.EnvironConfig()
 		c.Assert(err, gc.IsNil)
 		if change != nil {
+			oldcfg := cfg
 			cfg, err = cfg.Apply(change)
 			c.Assert(err, gc.IsNil)
-			err = s.State.SetEnvironConfig(cfg)
+			err = s.State.SetEnvironConfig(cfg, oldcfg)
 			c.Assert(err, gc.IsNil)
 		}
 		s.State.StartSync()
@@ -1296,6 +1297,7 @@
 func (s *StateSuite) TestWatchEnvironConfigCorruptConfig(c *gc.C) {
 	cfg, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
+	oldcfg := cfg
 
 	// Corrupt the environment configuration.
 	settings := s.Session.DB("juju").C("settings")
@@ -1331,7 +1333,7 @@
 	}
 
 	// Fix the configuration.
-	err = s.State.SetEnvironConfig(cfg)
+	err = s.State.SetEnvironConfig(cfg, oldcfg)
 	c.Assert(err, gc.IsNil)
 	fixed := cfg.AllAttrs()
 
@@ -2075,7 +2077,7 @@
 	attrs[name] = value
 	newConfig, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)
-	c.Assert(s.State.SetEnvironConfig(newConfig), gc.IsNil)
+	c.Assert(s.State.SetEnvironConfig(newConfig, envConfig), gc.IsNil)
 }
 
 func (s *StateSuite) assertAgentVersion(c *gc.C, envConfig *config.Config, vers string) {

=== modified file 'state/testing/config.go'
--- state/testing/config.go	2013-09-11 03:48:05 +0000
+++ state/testing/config.go	2013-12-06 04:54:05 +0000
@@ -14,9 +14,9 @@
 	if err != nil {
 		return err
 	}
-	cfg, err = cfg.Apply(newValues)
+	newcfg, err := cfg.Apply(newValues)
 	if err != nil {
 		return err
 	}
-	return st.SetEnvironConfig(cfg)
+	return st.SetEnvironConfig(newcfg, cfg)
 }

=== modified file 'worker/firewaller/firewaller_test.go'
--- worker/firewaller/firewaller_test.go	2013-09-30 19:40:06 +0000
+++ worker/firewaller/firewaller_test.go	2013-12-06 04:54:05 +0000
@@ -109,13 +109,14 @@
 	// TODO(rog) This should not be possible - you shouldn't
 	// be able to set the firewalling mode after an environment
 	// has bootstrapped.
-	attrs := s.Conn.Environ.Config().AllAttrs()
+	oldConfig := s.Conn.Environ.Config()
+	attrs := oldConfig.AllAttrs()
 	delete(attrs, "admin-secret")
 	delete(attrs, "ca-private-key")
 	attrs["firewall-mode"] = config.FwGlobal
 	newConfig, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(newConfig)
+	err = s.State.SetEnvironConfig(newConfig, oldConfig)
 	c.Assert(err, gc.IsNil)
 	err = s.Conn.Environ.SetConfig(newConfig)
 	c.Assert(err, gc.IsNil)

=== modified file 'worker/provisioner/provisioner_test.go'
--- worker/provisioner/provisioner_test.go	2013-11-28 09:09:30 +0000
+++ worker/provisioner/provisioner_test.go	2013-12-06 04:54:05 +0000
@@ -93,7 +93,7 @@
 	c.Assert(err, gc.IsNil)
 	cfg, err := oldCfg.Apply(map[string]interface{}{"broken": environMethod})
 	c.Assert(err, gc.IsNil)
-	err = st.SetEnvironConfig(cfg)
+	err = st.SetEnvironConfig(cfg, oldCfg)
 	c.Assert(err, gc.IsNil)
 	return fmt.Sprintf("dummy.%s is broken", environMethod)
 }
@@ -118,13 +118,17 @@
 	attrs["type"] = "unknown"
 	invalidCfg, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(invalidCfg)
+	err = s.State.SetEnvironConfig(invalidCfg, s.cfg)
 	c.Assert(err, gc.IsNil)
 }
 
 // fixEnvironment undoes the work of invalidateEnvironment.
 func (s *CommonProvisionerSuite) fixEnvironment() error {
-	return s.State.SetEnvironConfig(s.cfg)
+	cfg, err := s.State.EnvironConfig()
+	if err != nil {
+		return err
+	}
+	return s.State.SetEnvironConfig(s.cfg, cfg)
 }
 
 // stopper is stoppable.
@@ -595,13 +599,13 @@
 	cfgObserver := make(chan *config.Config, 1)
 	provisioner.SetObserver(p, cfgObserver)
 
-	cfg, err := s.State.EnvironConfig()
+	oldcfg, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
-	attrs := cfg.AllAttrs()
+	attrs := oldcfg.AllAttrs()
 	attrs["secret"] = "beef"
-	cfg, err = config.New(config.NoDefaults, attrs)
+	cfg, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(cfg)
+	err = s.State.SetEnvironConfig(cfg, oldcfg)
 
 	s.BackingState.StartSync()
 
@@ -643,13 +647,13 @@
 	c.Assert(m1.Remove(), gc.IsNil)
 
 	// turn on safe mode
-	cfg, err := s.State.EnvironConfig()
+	oldcfg, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
-	attrs := cfg.AllAttrs()
+	attrs := oldcfg.AllAttrs()
 	attrs["provisioner-safe-mode"] = true
-	cfg, err = config.New(config.NoDefaults, attrs)
+	cfg, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(cfg)
+	err = s.State.SetEnvironConfig(cfg, oldcfg)
 
 	// start a new provisioner to shut down only the machine still in state.
 	p = s.newEnvironProvisioner(c)
@@ -689,13 +693,13 @@
 	provisioner.SetObserver(p, cfgObserver)
 
 	// turn on safe mode
-	cfg, err := s.State.EnvironConfig()
+	oldcfg, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
-	attrs := cfg.AllAttrs()
+	attrs := oldcfg.AllAttrs()
 	attrs["provisioner-safe-mode"] = true
-	cfg, err = config.New(config.NoDefaults, attrs)
+	cfg, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)
-	err = s.State.SetEnvironConfig(cfg)
+	err = s.State.SetEnvironConfig(cfg, oldcfg)
 
 	s.BackingState.StartSync()