Merge lp:~wallyworld/juju-core/upgrade-remove-old-config into lp:~go-bot/juju-core/trunk

[Ian Booth]

Deprecated attributes upgrader

Previous versions of Juju will have left deprecated
attributes hanging around in the environment. This
upgrader will remove such attributes from env state.
eg default-image-id which is ignored will be put
into env state. This upgrader removes it. Also,
the deprecated attributes are no longer propagated
when a new env config is made.

https://codereview.appspot.com/70230049/

[Ian Booth (wallyworld)]

Reviewers: mp+208986_code.launchpad.net,

Message:
Please take a look.

Description:
Deprecated attributes upgrader

Previous versions of Juju will have left deprecated
attributes hanging around in the environment. This
upgrader will remove such attributes from env state.
eg default-image-id which is ignored will be put
into env state. This upgrader removes it. Also,
the deprecated attributes are no longer propagated
when a new env config is made.

https://code.launchpad.net/~wallyworld/juju-core/upgrade-remove-old-config/+merge/208986

Requires:
https://code.launchpad.net/~axwalk/juju-core/upgrade-syslog-port/+merge/208724

(do not edit description out of merge proposal)

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

Affected files (+165, -16 lines):
   A [revision details]
   M cmd/jujud/machine.go
   M cmd/jujud/unit.go
   M cmd/jujud/upgrade_test.go
   M cmd/logging.go
   M cmd/logging_test.go
   M provider/openstack/config.go
   M provider/openstack/config_test.go
   A upgrades/deprecatedattributes.go
   A upgrades/deprecatedattributes_test.go
   M upgrades/export_test.go
   M upgrades/steps118.go
   M upgrades/steps118_test.go
   M upgrades/systemsshkey_test.go

[Tim Penhey (thumper)]

LGTM - although not a fan of all the value asserts in the setup.

https://codereview.appspot.com/70230049/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/70230049/diff/1/cmd/jujud/machine.go#newcode121
cmd/jujud/machine.go:121: logger.Infof("machine agent %v start (%s)",
a.Tag(), version.Current)
I'm sure I saw this somewhere else, missing prereq?

https://codereview.appspot.com/70230049/diff/1/upgrades/deprecatedattributes_test.go
File upgrades/deprecatedattributes_test.go (right):

https://codereview.appspot.com/70230049/diff/1/upgrades/deprecatedattributes_test.go#newcode46
upgrades/deprecatedattributes_test.go:46:
c.Assert(allAttrs["public-bucket"], gc.Equals, "foo")
Seems a lot to me to have the asserts in setup for this

https://codereview.appspot.com/70230049/

[Ian Booth (wallyworld)]

https://codereview.appspot.com/70230049/diff/1/upgrades/deprecatedattributes_test.go
File upgrades/deprecatedattributes_test.go (right):

https://codereview.appspot.com/70230049/diff/1/upgrades/deprecatedattributes_test.go#newcode46
upgrades/deprecatedattributes_test.go:46:
c.Assert(allAttrs["public-bucket"], gc.Equals, "foo")
On 2014/03/04 03:04:57, thumper wrote:
> Seems a lot to me to have the asserts in setup for this

I can remove them but these serve to double check that the attributes
really are set so that we can be sure the test is testing what we think
it is.

https://codereview.appspot.com/70230049/

[Ian Booth (wallyworld)]

https://codereview.appspot.com/70230049/diff/1/upgrades/deprecatedattributes_test.go
File upgrades/deprecatedattributes_test.go (right):

https://codereview.appspot.com/70230049/diff/1/upgrades/deprecatedattributes_test.go#newcode46
upgrades/deprecatedattributes_test.go:46:
c.Assert(allAttrs["public-bucket"], gc.Equals, "foo")
On 2014/03/04 03:04:57, thumper wrote:
> Seems a lot to me to have the asserts in setup for this

I added an explicit test

https://codereview.appspot.com/70230049/

=== modified file 'cmd/jujud/upgrade_test.go'
--- cmd/jujud/upgrade_test.go	2014-03-04 04:39:34 +0000
+++ cmd/jujud/upgrade_test.go	2014-03-04 04:39:35 +0000
@@ -114,6 +114,12 @@
 	cfg, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
 	c.Assert(cfg.SyslogPort(), gc.Equals, config.DefaultSyslogPort)
+	// Deprecated attributes should have been deleted - just test a couple.
+	allAttrs := cfg.AllAttrs()
+	_, ok := allAttrs["public-bucket"]
+	c.Assert(ok, jc.IsFalse)
+	_, ok = allAttrs["public-bucket-region"]
+	c.Assert(ok, jc.IsFalse)
 }
 
 func (s *UpgradeSuite) assertHostUpgrades(c *gc.C) {

=== modified file 'provider/openstack/config.go'
--- provider/openstack/config.go	2014-01-24 16:32:15 +0000
+++ provider/openstack/config.go	2014-03-04 04:39:35 +0000
@@ -198,7 +198,8 @@
 
 	// Check for deprecated fields and log a warning. We also print to stderr to ensure the user sees the message
 	// even if they are not running with --debug.
-	if defaultImageId := cfg.AllAttrs()["default-image-id"]; defaultImageId != nil && defaultImageId.(string) != "" {
+	cfgAttrs := cfg.AllAttrs()
+	if defaultImageId := cfgAttrs["default-image-id"]; defaultImageId != nil && defaultImageId.(string) != "" {
 		msg := fmt.Sprintf(
 			"Config attribute %q (%v) is deprecated and ignored.\n"+
 				"Your cloud provider should have set up image metadata to provide the correct image id\n"+
@@ -208,7 +209,7 @@
 			"default-image-id", defaultImageId)
 		logger.Warningf(msg)
 	}
-	if defaultInstanceType := cfg.AllAttrs()["default-instance-type"]; defaultInstanceType != nil && defaultInstanceType.(string) != "" {
+	if defaultInstanceType := cfgAttrs["default-instance-type"]; defaultInstanceType != nil && defaultInstanceType.(string) != "" {
 		msg := fmt.Sprintf(
 			"Config attribute %q (%v) is deprecated and ignored.\n"+
 				"The correct instance flavor is determined using constraints, globally specified\n"+
@@ -217,7 +218,13 @@
 			"default-instance-type", defaultInstanceType)
 		logger.Warningf(msg)
 	}
-
-	// Apply the coerced unknown values back into the config.
-	return cfg.Apply(ecfg.attrs)
+	// Construct a new config with the deprecated attributes removed.
+	for _, attr := range []string{"default-image-id", "default-instance-type"} {
+		delete(cfgAttrs, attr)
+		delete(ecfg.attrs, attr)
+	}
+	for k, v := range ecfg.attrs {
+		cfgAttrs[k] = v
+	}
+	return config.New(config.NoDefaults, cfgAttrs)
 }

=== modified file 'provider/openstack/config_test.go'
--- provider/openstack/config_test.go	2014-02-13 02:46:58 +0000
+++ provider/openstack/config_test.go	2014-03-04 04:39:35 +0000
@@ -55,7 +55,6 @@
 	envVars                 map[string]string
 	region                  string
 	controlBucket           string
-	toolsURL                string
 	useFloatingIP           bool
 	useDefaultSecurityGroup bool
 	network                 string
@@ -152,11 +151,6 @@
 		c.Assert(err, gc.IsNil)
 		c.Assert(expected, gc.DeepEquals, actual)
 	}
-	if t.toolsURL != "" {
-		toolsURL, ok := ecfg.ToolsURL()
-		c.Assert(ok, jc.IsTrue)
-		c.Assert(toolsURL, gc.Equals, t.toolsURL)
-	}
 	if t.firewallMode != "" {
 		c.Assert(ecfg.FirewallMode(), gc.Equals, t.firewallMode)
 	}
@@ -452,6 +446,28 @@
 	}
 }
 
+func (s *ConfigSuite) TestDeprecatedAttributesRemoved(c *gc.C) {
+	s.setupEnvCredentials()
+	attrs := testing.FakeConfig().Merge(testing.Attrs{
+		"type":                  "openstack",
+		"control-bucket":        "x",
+		"default-image-id":      "id-1234",
+		"default-instance-type": "big",
+	})
+
+	cfg, err := config.New(config.NoDefaults, attrs)
+	c.Assert(err, gc.IsNil)
+	// Keep err for validation below.
+	valid, err := providerInstance.Validate(cfg, nil)
+	c.Assert(err, gc.IsNil)
+	// Check deprecated attributes removed.
+	allAttrs := valid.AllAttrs()
+	for _, attr := range []string{"default-image-id", "default-instance-type"} {
+		_, ok := allAttrs[attr]
+		c.Assert(ok, jc.IsFalse)
+	}
+}
+
 func (s *ConfigSuite) TestPrepareInsertsUniqueControlBucket(c *gc.C) {
 	s.setupEnvCredentials()
 	attrs := testing.FakeConfig().Merge(testing.Attrs{

=== added file 'upgrades/deprecatedattributes.go'
--- upgrades/deprecatedattributes.go	1970-01-01 00:00:00 +0000
+++ upgrades/deprecatedattributes.go	2014-03-04 04:39:35 +0000
@@ -0,0 +1,30 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package upgrades
+
+import (
+	"fmt"
+
+	"launchpad.net/juju-core/environs/config"
+)
+
+func processDeprecatedAttributes(context Context) error {
+	st := context.State()
+	cfg, err := st.EnvironConfig()
+	if err != nil {
+		return fmt.Errorf("failed to read current config: %v", err)
+	}
+	newAttrs := cfg.AllAttrs()
+	delete(newAttrs, "public-bucket")
+	delete(newAttrs, "public-bucket-region")
+	delete(newAttrs, "public-bucket-url")
+	delete(newAttrs, "default-image-id")
+	delete(newAttrs, "default-instance-type")
+	// TODO (wallyworld) - delete tools-url in 1.20
+	newCfg, err := config.New(config.NoDefaults, newAttrs)
+	if err != nil {
+		return fmt.Errorf("failed to create new config: %v", err)
+	}
+	return st.SetEnvironConfig(newCfg, cfg)
+}

=== added file 'upgrades/deprecatedattributes_test.go'
--- upgrades/deprecatedattributes_test.go	1970-01-01 00:00:00 +0000
+++ upgrades/deprecatedattributes_test.go	2014-03-04 04:39:35 +0000
@@ -0,0 +1,83 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package upgrades_test
+
+import (
+	gc "launchpad.net/gocheck"
+
+	jujutesting "launchpad.net/juju-core/juju/testing"
+	"launchpad.net/juju-core/state"
+	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/upgrades"
+)
+
+type processDeprecatedAttributesSuite struct {
+	jujutesting.JujuConnSuite
+	ctx upgrades.Context
+}
+
+var _ = gc.Suite(&processDeprecatedAttributesSuite{})
+
+func (s *processDeprecatedAttributesSuite) SetUpTest(c *gc.C) {
+	s.JujuConnSuite.SetUpTest(c)
+	apiState, _ := s.OpenAPIAsNewMachine(c, state.JobManageEnviron)
+	s.ctx = &mockContext{
+		agentConfig: &mockAgentConfig{dataDir: s.DataDir()},
+		apiState:    apiState,
+		state:       s.State,
+	}
+	cfg, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	// Add in old public bucket config.
+	newCfg, err := cfg.Apply(map[string]interface{}{
+		"public-bucket":         "foo",
+		"public-bucket-region":  "bar",
+		"public-bucket-url":     "shazbot",
+		"default-instance-type": "vulch",
+		"default-image-id":      "1234",
+	})
+	c.Assert(err, gc.IsNil)
+	err = s.State.SetEnvironConfig(newCfg, cfg)
+	c.Assert(err, gc.IsNil)
+}
+
+func (s *processDeprecatedAttributesSuite) TestAttributesSet(c *gc.C) {
+	cfg, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	allAttrs := cfg.AllAttrs()
+	c.Assert(allAttrs["public-bucket"], gc.Equals, "foo")
+	c.Assert(allAttrs["public-bucket-region"], gc.Equals, "bar")
+	c.Assert(allAttrs["public-bucket-url"], gc.Equals, "shazbot")
+	c.Assert(allAttrs["default-instance-type"], gc.Equals, "vulch")
+	c.Assert(allAttrs["default-image-id"], gc.Equals, "1234")
+}
+
+func (s *processDeprecatedAttributesSuite) assertConfigProcessed(c *gc.C) {
+	cfg, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	allAttrs := cfg.AllAttrs()
+	for _, deprecated := range []string{
+		"public-bucket", "public-bucket-region", "public-bucket-url",
+		"default-image-id", "default-instance-type",
+	} {
+		_, ok := allAttrs[deprecated]
+		c.Assert(ok, jc.IsFalse)
+	}
+}
+
+func (s *processDeprecatedAttributesSuite) TestOldConfigRemoved(c *gc.C) {
+	err := upgrades.ProcessDeprecatedAttributes(s.ctx)
+	c.Assert(err, gc.IsNil)
+	s.assertConfigProcessed(c)
+}
+
+func (s *processDeprecatedAttributesSuite) TestIdempotent(c *gc.C) {
+	err := upgrades.ProcessDeprecatedAttributes(s.ctx)
+	c.Assert(err, gc.IsNil)
+	s.assertConfigProcessed(c)
+
+	err = upgrades.ProcessDeprecatedAttributes(s.ctx)
+	c.Assert(err, gc.IsNil)
+	s.assertConfigProcessed(c)
+}

=== modified file 'upgrades/export_test.go'
--- upgrades/export_test.go	2014-03-04 04:39:34 +0000
+++ upgrades/export_test.go	2014-03-04 04:39:35 +0000
@@ -12,4 +12,5 @@
 	EnsureLockDirExistsAndUbuntuWritable = ensureLockDirExistsAndUbuntuWritable
 	EnsureSystemSSHKey                   = ensureSystemSSHKey
 	UpdateRsyslogPort                    = updateRsyslogPort
+	ProcessDeprecatedAttributes          = processDeprecatedAttributes
 )

=== modified file 'upgrades/steps118.go'
--- upgrades/steps118.go	2014-03-04 04:39:34 +0000
+++ upgrades/steps118.go	2014-03-04 04:39:35 +0000
@@ -26,5 +26,10 @@
 			targets:     []Target{AllMachines},
 			run:         installRsyslogGnutls,
 		},
+		&upgradeStep{
+			description: "remove deprecated attribute values",
+			targets:     []Target{StateServer},
+			run:         processDeprecatedAttributes,
+		},
 	}
 }

=== modified file 'upgrades/steps118_test.go'
--- upgrades/steps118_test.go	2014-03-04 04:39:34 +0000
+++ upgrades/steps118_test.go	2014-03-04 04:39:35 +0000
@@ -21,6 +21,7 @@
 	"generate system ssh key",
 	"update rsyslog port",
 	"install rsyslog-gnutls",
+	"remove deprecated attribute values",
 }
 
 func (s *steps118Suite) TestUpgradeOperationsContent(c *gc.C) {

=== modified file 'upgrades/systemsshkey_test.go'
--- upgrades/systemsshkey_test.go	2014-02-27 03:17:45 +0000
+++ upgrades/systemsshkey_test.go	2014-03-04 04:39:35 +0000
@@ -34,7 +34,7 @@
 	_, err := os.Stat(s.keyFile())
 	c.Assert(err, jc.Satisfies, os.IsNotExist)
 	// There's initially one authorised key for the test user.
-	cfg, err := s.JujuConnSuite.State.EnvironConfig()
+	cfg, err := s.State.EnvironConfig()
 	c.Assert(err, gc.IsNil)
 	authKeys := ssh.SplitAuthorisedKeys(cfg.AuthorizedKeys())
 	c.Assert(authKeys, gc.HasLen, 1)