Merge lp:~rogpeppe/juju-core/453-maas-instance-uuid into lp:~go-bot/juju-core/trunk

[Roger Peppe]

provider/maas: do not use environ-uuid

As pointed out by fwereade, it could be
confusing to have two things in the system
that both purport to be a uuid for the environ.

Also make the MAAS EnvironProvider.Validate
check that we're not changing the maas-agent-name.

https://codereview.appspot.com/14741045/

provider/maas: do not use environ-uuid

As pointed out by fwereade, it could be
confusing to have two things in the system
that both purport to be a uuid for the environ.

Also make the MAAS EnvironProvider.Validate
check that we're not changing the maas-instance-uuid.

https://codereview.appspot.com/14741045/

[Roger Peppe (rogpeppe)]

Reviewers: mp+191367_code.launchpad.net,

Message:
Please take a look.

Description:
provider/maas: do not use environ-uuid

As pointed out by fwereade, it could be
confusing to have two things in the system
that both purport to be a uuid for the environ.

Also make the MAAS EnvironProvider.Validate
check that we're not changing the maas-instance-uuid.

https://code.launchpad.net/~rogpeppe/juju-core/453-maas-instance-uuid/+merge/191367

(do not edit description out of merge proposal)

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

Affected files (+58, -37 lines):
   A [revision details]
   M provider/maas/config.go
   M provider/maas/config_test.go
   M provider/maas/environprovider.go
   M provider/maas/environprovider_test.go
   M provider/maas/maas_test.go
   M provider/maas/storage_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20131016061150-dqoy06taxa0edywm
+New revision: <email address hidden>

Index: provider/maas/config.go
=== modified file 'provider/maas/config.go'
--- provider/maas/config.go 2013-10-15 16:18:32 +0000
+++ provider/maas/config.go 2013-10-16 10:29:29 +0000
@@ -18,14 +18,14 @@
   // maas-oauth is a colon-separated triplet of:
   // consumer-key:resource-token:resource-secret
   "maas-oauth": schema.String(),
- // environment-uuid is an optional UUID to group the machines
+ // maas-instance-uuid is an optional UUID to group the instances
   // acquired from MAAS, to support multiple environments per MAAS user.
- "environment-uuid": schema.String(),
+ "maas-instance-uuid": schema.String(),
  }
  var configDefaults = schema.Defaults{
- // For backward-compatibility, environment-uuid is the empty string
+ // For backward-compatibility, maas-instance-uuid is the empty string
   // by default. However, new environments should all use a UUID.
- "environment-uuid": "",
+ "maas-instance-uuid": "",
  }

  type maasEnvironConfig struct {
@@ -42,7 +42,7 @@
  }

  func (cfg *maasEnvironConfig) maasEnvironmentUUID() string {
- if uuid, ok := cfg.attrs["environment-uuid"].(string); ok {
+ if uuid, ok := cfg.attrs["maas-instance-uuid"].(string); ok {
    return uuid
   }
   return ""
@@ -72,6 +72,12 @@
   if err != nil {
    return nil, err
   }
+ if oldCfg != nil {
+ oldAttrs := oldCfg.UnknownAttrs()
+ if validated["maas-instance-uuid"] != oldAttrs["maas-instance-uuid"] {
+ return nil, fmt.Errorf("cannot change maas-instance-uuid")
+ }
+ }
   envCfg := new(maasEnvironConfig)
   envCfg.Config = cfg
   envCfg.attrs = validated

Index: provider/maas/config_test.go
=== modified file 'provider/maas/config_test.go'
--- provider/maas/config_test.go 2013-10-15 16:52:58 +0000
+++ provider/maas/config_test.go 2013-10-16 10:42:01 +0000
@@ -47,10 +47,10 @@
   uuid, err := utils.NewUUID()
   c.Assert(err, gc.IsNil)
   ecfg, err := newConfig(map[string]interface{}{
- "maas-server": server,
- "maas-oauth": oauth,
- "environment-uuid": uuid.String(),
- "future-key": future,
+ "maas-server": server,
+ "ma...

[Martin Packman (gz)]

LGTM. We will need to be careful when we fiddle with this mechanism in
future to make sure upgrades don't start using a new name for instances
as it needs to be stable across the lifetime of the environment.

https://codereview.appspot.com/14741045/

[Nate Finch (natefinch)]

LGTM, although I'd prefer if someone factored out all the magic strings.

https://codereview.appspot.com/14741045/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/14741045/

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/14741045/

[Raphaël Badin (rvb)]

LTGM

https://codereview.appspot.com/14741045/

=== modified file 'provider/maas/config.go'
--- provider/maas/config.go	2013-10-15 16:18:32 +0000
+++ provider/maas/config.go	2013-10-16 12:45:44 +0000
@@ -18,14 +18,14 @@
 	// maas-oauth is a colon-separated triplet of:
 	// consumer-key:resource-token:resource-secret
 	"maas-oauth": schema.String(),
-	// environment-uuid is an optional UUID to group the machines
+	// maas-agent-name is an optional UUID to group the instances
 	// acquired from MAAS, to support multiple environments per MAAS user.
-	"environment-uuid": schema.String(),
+	"maas-agent-name": schema.String(),
 }
 var configDefaults = schema.Defaults{
-	// For backward-compatibility, environment-uuid is the empty string
+	// For backward-compatibility, maas-agent-name is the empty string
 	// by default. However, new environments should all use a UUID.
-	"environment-uuid": "",
+	"maas-agent-name": "",
 }
 
 type maasEnvironConfig struct {
@@ -41,8 +41,8 @@
 	return cfg.attrs["maas-oauth"].(string)
 }
 
-func (cfg *maasEnvironConfig) maasEnvironmentUUID() string {
-	if uuid, ok := cfg.attrs["environment-uuid"].(string); ok {
+func (cfg *maasEnvironConfig) maasAgentName() string {
+	if uuid, ok := cfg.attrs["maas-agent-name"].(string); ok {
 		return uuid
 	}
 	return ""
@@ -72,6 +72,12 @@
 	if err != nil {
 		return nil, err
 	}
+	if oldCfg != nil {
+		oldAttrs := oldCfg.UnknownAttrs()
+		if validated["maas-agent-name"] != oldAttrs["maas-agent-name"] {
+			return nil, fmt.Errorf("cannot change maas-agent-name")
+		}
+	}
 	envCfg := new(maasEnvironConfig)
 	envCfg.Config = cfg
 	envCfg.attrs = validated

=== modified file 'provider/maas/config_test.go'
--- provider/maas/config_test.go	2013-10-15 16:52:58 +0000
+++ provider/maas/config_test.go	2013-10-16 12:45:44 +0000
@@ -47,25 +47,25 @@
 	uuid, err := utils.NewUUID()
 	c.Assert(err, gc.IsNil)
 	ecfg, err := newConfig(map[string]interface{}{
-		"maas-server":      server,
-		"maas-oauth":       oauth,
-		"environment-uuid": uuid.String(),
-		"future-key":       future,
+		"maas-server":     server,
+		"maas-oauth":      oauth,
+		"maas-agent-name": uuid.String(),
+		"future-key":      future,
 	})
 	c.Assert(err, gc.IsNil)
 	c.Check(ecfg.maasServer(), gc.Equals, server)
 	c.Check(ecfg.maasOAuth(), gc.DeepEquals, oauth)
-	c.Check(ecfg.maasEnvironmentUUID(), gc.Equals, uuid.String())
+	c.Check(ecfg.maasAgentName(), gc.Equals, uuid.String())
 	c.Check(ecfg.UnknownAttrs()["future-key"], gc.DeepEquals, future)
 }
 
-func (*configSuite) TestEnvironmentUUIDDefault(c *gc.C) {
+func (*configSuite) TestMaasAgentNameDefault(c *gc.C) {
 	ecfg, err := newConfig(map[string]interface{}{
 		"maas-server": "http://maas.testing.invalid/maas/",
 		"maas-oauth":  "consumer-key:resource-token:resource-secret",
 	})
 	c.Assert(err, gc.IsNil)
-	c.Check(ecfg.maasEnvironmentUUID(), gc.Equals, "")
+	c.Check(ecfg.maasAgentName(), gc.Equals, "")
 }
 
 func (*configSuite) TestChecksWellFormedMaasServer(c *gc.C) {
@@ -105,3 +105,19 @@
 	c.Assert(err, gc.NotNil)
 	c.Check(err, gc.ErrorMatches, ".*cannot change name.*")
 }
+
+func (*configSuite) TestValidateCannotChangeAgentName(c *gc.C) {
+	baseAttrs := map[string]interface{}{
+		"maas-server":     "http://maas.testing.invalid/maas/",
+		"maas-oauth":      "consumer-key:resource-token:resource-secret",
+		"maas-agent-name": "1234-5678",
+	}
+	oldCfg, err := newConfig(baseAttrs)
+	c.Assert(err, gc.IsNil)
+	newCfg, err := oldCfg.Apply(map[string]interface{}{
+		"maas-agent-name": "9876-5432",
+	})
+	c.Assert(err, gc.IsNil)
+	_, err = maasEnvironProvider{}.Validate(newCfg, oldCfg.Config)
+	c.Assert(err, gc.ErrorMatches, "cannot change maas-agent-name")
+}

=== modified file 'provider/maas/environ.go'
--- provider/maas/environ.go	2013-10-15 16:09:23 +0000
+++ provider/maas/environ.go	2013-10-16 12:45:44 +0000
@@ -171,7 +171,7 @@
 // acquireNode allocates a node from the MAAS.
 func (environ *maasEnviron) acquireNode(cons constraints.Value, possibleTools tools.List) (gomaasapi.MAASObject, *tools.Tools, error) {
 	acquireParams := convertConstraints(cons)
-	acquireParams.Add("agent_name", environ.ecfg().maasEnvironmentUUID())
+	acquireParams.Add("agent_name", environ.ecfg().maasAgentName())
 	var result gomaasapi.JSONObject
 	var err error
 	for a := shortAttempt.Start(); a.Next(); {
@@ -323,7 +323,7 @@
 func (environ *maasEnviron) instances(ids []instance.Id) ([]instance.Instance, error) {
 	nodeListing := environ.getMAASClient().GetSubObject("nodes")
 	filter := getSystemIdValues(ids)
-	filter.Add("agent_name", environ.ecfg().maasEnvironmentUUID())
+	filter.Add("agent_name", environ.ecfg().maasAgentName())
 	listNodeObjects, err := nodeListing.CallGet("list", filter)
 	if err != nil {
 		return nil, err

=== modified file 'provider/maas/environ_test.go'
--- provider/maas/environ_test.go	2013-10-16 05:21:12 +0000
+++ provider/maas/environ_test.go	2013-10-16 12:45:44 +0000
@@ -310,7 +310,7 @@
 	c.Assert(nodeRequestValues[0].Get("mem"), gc.Equals, "1024")
 }
 
-func (suite *environSuite) TestAcquireNodePassedEnvironmentUUID(c *gc.C) {
+func (suite *environSuite) TestAcquireNodePassedAgentName(c *gc.C) {
 	stor := NewStorage(suite.makeEnviron())
 	fakeTools := envtesting.MustUploadFakeToolsVersions(stor, version.Current)[0]
 	env := suite.makeEnviron()
@@ -322,7 +322,7 @@
 	requestValues := suite.testMAASObject.TestServer.NodeOperationRequestValues()
 	nodeRequestValues, found := requestValues["node0"]
 	c.Assert(found, gc.Equals, true)
-	c.Assert(nodeRequestValues[0].Get("agent_name"), gc.Equals, exampleUUID)
+	c.Assert(nodeRequestValues[0].Get("agent_name"), gc.Equals, exampleAgentName)
 }
 
 func (*environSuite) TestConvertConstraints(c *gc.C) {

=== modified file 'provider/maas/environprovider.go'
--- provider/maas/environprovider.go	2013-10-15 16:36:59 +0000
+++ provider/maas/environprovider.go	2013-10-16 12:45:44 +0000
@@ -38,24 +38,21 @@
 	return env, nil
 }
 
-var errUUIDAlreadySet = errors.New(
-	"environment-uuid is already set; this should not be set by hand")
+var errAgentNameAlreadySet = errors.New(
+	"maas-agent-name is already set; this should not be set by hand")
 
 func (p maasEnvironProvider) Prepare(cfg *config.Config) (environs.Environ, error) {
 	attrs := cfg.UnknownAttrs()
-	uuid, found := attrs["environment-uuid"]
-	if found {
-		if uuid != "" {
-			return nil, errUUIDAlreadySet
-		}
-	} else {
-		uuid, err := utils.NewUUID()
-		if err != nil {
-			return nil, err
-		}
-		attrs["environment-uuid"] = uuid.String()
-	}
-	cfg, err := cfg.Apply(attrs)
+	oldName, found := attrs["maas-agent-name"]
+	if found && oldName != "" {
+		return nil, errAgentNameAlreadySet
+	}
+	uuid, err := utils.NewUUID()
+	if err != nil {
+		return nil, err
+	}
+	attrs["maas-agent-name"] = uuid.String()
+	cfg, err = cfg.Apply(attrs)
 	if err != nil {
 		return nil, err
 	}

=== modified file 'provider/maas/environprovider_test.go'
--- provider/maas/environprovider_test.go	2013-10-15 16:36:59 +0000
+++ provider/maas/environprovider_test.go	2013-10-16 12:45:44 +0000
@@ -11,6 +11,7 @@
 
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/testing"
+	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/utils"
 )
 
@@ -39,7 +40,7 @@
 	c.Check(secretAttrs, gc.DeepEquals, expectedAttrs)
 }
 
-func (suite *EnvironProviderSuite) TestUnknownAttrsContainEnvironmentUUID(c *gc.C) {
+func (suite *EnvironProviderSuite) TestUnknownAttrsContainAgentName(c *gc.C) {
 	testJujuHome := c.MkDir()
 	defer config.SetJujuHome(config.SetJujuHome(testJujuHome))
 	attrs := testing.FakeConfig().Merge(testing.Attrs{
@@ -56,27 +57,26 @@
 	preparedConfig := environ.Config()
 	unknownAttrs := preparedConfig.UnknownAttrs()
 
-	uuid, ok := unknownAttrs["environment-uuid"]
-	c.Assert(ok, gc.Equals, true)
+	uuid, ok := unknownAttrs["maas-agent-name"]
 
-	_, err = utils.UUIDFromString(uuid.(string))
-	c.Assert(err, gc.IsNil)
+	c.Assert(ok, jc.IsTrue)
+	c.Assert(uuid, jc.Satisfies, utils.IsValidUUIDString)
 }
 
-func (suite *EnvironProviderSuite) TestEnvironmentUUIDShouldNotBeSetByHand(c *gc.C) {
+func (suite *EnvironProviderSuite) TestAgentNameShouldNotBeSetByHand(c *gc.C) {
 	testJujuHome := c.MkDir()
 	defer config.SetJujuHome(config.SetJujuHome(testJujuHome))
 	attrs := testing.FakeConfig().Merge(testing.Attrs{
-		"type":             "maas",
-		"maas-oauth":       "aa:bb:cc",
-		"maas-server":      "http://maas.testing.invalid/maas/",
-		"environment-uuid": "foobar",
+		"type":            "maas",
+		"maas-oauth":      "aa:bb:cc",
+		"maas-server":     "http://maas.testing.invalid/maas/",
+		"maas-agent-name": "foobar",
 	})
 	config, err := config.New(config.NoDefaults, attrs)
 	c.Assert(err, gc.IsNil)
 
 	_, err = suite.makeEnviron().Provider().Prepare(config)
-	c.Assert(err, gc.Equals, errUUIDAlreadySet)
+	c.Assert(err, gc.Equals, errAgentNameAlreadySet)
 }
 
 // create a temporary file with the given content.  The file will be cleaned

=== modified file 'provider/maas/maas_test.go'
--- provider/maas/maas_test.go	2013-10-15 16:52:58 +0000
+++ provider/maas/maas_test.go	2013-10-16 12:45:44 +0000
@@ -52,16 +52,16 @@
 	s.LoggingSuite.TearDownSuite(c)
 }
 
-const exampleUUID = "dfb69555-0bc4-4d1f-85f2-4ee390974984"
+const exampleAgentName = "dfb69555-0bc4-4d1f-85f2-4ee390974984"
 
 // makeEnviron creates a functional maasEnviron for a test.
 func (suite *providerSuite) makeEnviron() *maasEnviron {
 	attrs := coretesting.FakeConfig().Merge(coretesting.Attrs{
-		"name":             "test env",
-		"type":             "maas",
-		"maas-oauth":       "a:b:c",
-		"maas-server":      suite.testMAASObject.TestServer.URL,
-		"environment-uuid": exampleUUID,
+		"name":            "test env",
+		"type":            "maas",
+		"maas-oauth":      "a:b:c",
+		"maas-server":     suite.testMAASObject.TestServer.URL,
+		"maas-agent-name": exampleAgentName,
 	})
 	cfg, err := config.New(config.NoDefaults, attrs)
 	if err != nil {

=== modified file 'provider/maas/storage.go'
--- provider/maas/storage.go	2013-10-16 05:21:12 +0000
+++ provider/maas/storage.go	2013-10-16 12:45:44 +0000
@@ -91,10 +91,10 @@
 
 // All filenames need to be namespaced so they are private to this environment.
 // This prevents different environments from interfering with each other.
-// We're using the environment's UUID here.
+// We're using the agent name UUID here.
 func (stor *maasStorage) prefixWithPrivateNamespace(name string) string {
 	env := stor.getSnapshot().environUnlocked
-	prefix := env.ecfg().maasEnvironmentUUID()
+	prefix := env.ecfg().maasAgentName()
 	if prefix != "" {
 		return prefix + "-" + name
 	}

=== modified file 'provider/maas/storage_test.go'
--- provider/maas/storage_test.go	2013-10-16 05:21:12 +0000
+++ provider/maas/storage_test.go	2013-10-16 12:45:44 +0000
@@ -398,22 +398,22 @@
 	c.Assert(listing, gc.DeepEquals, []string{})
 }
 
-func (s *storageSuite) TestprefixWithPrivateNamespacePrefixesWithUUID(c *gc.C) {
+func (s *storageSuite) TestprefixWithPrivateNamespacePrefixesWithAgentName(c *gc.C) {
 	sstor := NewStorage(s.makeEnviron())
 	stor := sstor.(*maasStorage)
-	uuid := stor.environUnlocked.ecfg().maasEnvironmentUUID()
-	c.Assert(uuid, gc.Not(gc.Equals), "")
-	expectedPrefix := uuid + "-"
+	agentName := stor.environUnlocked.ecfg().maasAgentName()
+	c.Assert(agentName, gc.Not(gc.Equals), "")
+	expectedPrefix := agentName + "-"
 	const name = "myname"
 	expectedResult := expectedPrefix + name
 	c.Assert(stor.prefixWithPrivateNamespace(name), gc.Equals, expectedResult)
 }
 
-func (s *storageSuite) TesttprefixWithPrivateNamespaceIgnoresEmptyUUID(c *gc.C) {
+func (s *storageSuite) TesttprefixWithPrivateNamespaceIgnoresAgentName(c *gc.C) {
 	sstor := NewStorage(s.makeEnviron())
 	stor := sstor.(*maasStorage)
 	ecfg := stor.environUnlocked.ecfg()
-	ecfg.attrs["environment-uuid"] = ""
+	ecfg.attrs["maas-agent-name"] = ""
 
 	const name = "myname"
 	c.Assert(stor.prefixWithPrivateNamespace(name), gc.Equals, name)