Merge lp:~allenap/juju-core/maas-environment-uuid into lp:~go-bot/juju-core/trunk

Proposed by Gavin Panella
Status: Merged
Approved by: Martin Packman
Approved revision: no longer in the source branch.
Merged at revision: 1984
Proposed branch: lp:~allenap/juju-core/maas-environment-uuid
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 66 lines (+37/-1)
2 files modified
provider/maas/environprovider.go (+12/-1)
provider/maas/environprovider_test.go (+25/-0)
To merge this branch: bzr merge lp:~allenap/juju-core/maas-environment-uuid
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+191146@code.launchpad.net

Commit message

provider/maas: Add environment UUID

We need a way to uniquely identify those machines in MAAS belonging to
a Juju environment. Until now Juju's MAAS provider has assumed that
all the machines that are allocated, are allocated to the environment
that Juju is working with. This is obviously wrong, and can be
disastrous.

This is the first part of work on the MAAS provider to use a unique
identifier for an environment. MAAS will record this identifier
(called 'agent_name' in MAAS) and allow its use for filtering.
This branch merely adds the UUID to the environment's config.

https://codereview.appspot.com/14696043/

R=rogpeppe, rvb

Description of the change

Add environment UUID to MAAS provider.

We need a way to uniquely identify those machines in MAAS belonging to
a Juju environment. Until now Juju's MAAS provider has assumed that
all the machines that are allocated, are allocated to the environment
that Juju is working with. This is obviously wrong, and can be
disastrous.

This is the first part of work on the MAAS provider to use a unique
identifier for an environment. MAAS will record this identifier
(called 'agent_name' in MAAS) and allow its use for filtering.
This branch merely adds the UUID to the environment's config.

https://codereview.appspot.com/14696043/

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :
Download full text (3.3 KiB)

Reviewers: mp+191146_code.launchpad.net,

Message:
Please take a look.

Description:
Add environment UUID to MAAS provider.

We need a way to uniquely identify those machines in MAAS belonging to
a Juju environment. Until now Juju's MAAS provider has assumed that
all the machines that are allocated, are allocated to the environment
that Juju is working with. This is obviously wrong, and can be
disastrous.

This is the first part of work on the MAAS provider to use a unique
identifier for an environment. MAAS will record this identifier
(called 'agent_name' in MAAS) and allow its use for filtering.
This branch merely adds the UUID to the environment's config.

https://code.launchpad.net/~allenap/juju-core/maas-environment-uuid/+merge/191146

(do not edit description out of merge proposal)

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

Affected files (+39, -1 lines):
   A [revision details]
   M provider/maas/environprovider.go
   M provider/maas/environprovider_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-20131014040553-f18jp0xnxw45qkt2
+New revision: <email address hidden>

Index: provider/maas/environprovider.go
=== modified file 'provider/maas/environprovider.go'
--- provider/maas/environprovider.go 2013-10-11 14:05:04 +0000
+++ provider/maas/environprovider.go 2013-10-15 10:30:29 +0000
@@ -38,7 +38,18 @@
  }

  func (p maasEnvironProvider) Prepare(cfg *config.Config)
(environs.Environ, error) {
- // TODO any attributes to prepare?
+ attrs := cfg.UnknownAttrs()
+ if _, ok := attrs["environment-uuid"]; !ok {
+ uuid, err := utils.NewUUID()
+ if err != nil {
+ return nil, err
+ }
+ attrs["environment-uuid"] = uuid.String()
+ }
+ cfg, err := cfg.Apply(attrs)
+ if err != nil {
+ return nil, err
+ }
   return p.Open(cfg)
  }

Index: provider/maas/environprovider_test.go
=== modified file 'provider/maas/environprovider_test.go'
--- provider/maas/environprovider_test.go 2013-09-25 17:04:52 +0000
+++ provider/maas/environprovider_test.go 2013-10-15 10:30:29 +0000
@@ -11,6 +11,7 @@

   "launchpad.net/juju-core/environs/config"
   "launchpad.net/juju-core/testing"
+ "launchpad.net/juju-core/utils"
  )

  type EnvironProviderSuite struct {
@@ -38,6 +39,30 @@
   c.Check(secretAttrs, gc.DeepEquals, expectedAttrs)
  }

+func (suite *EnvironProviderSuite)
TestUnknownAttrsContainEnvironmentUUID(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/",
+ })
+ config, err := config.New(config.NoDefaults, attrs)
+ c.Assert(err, gc.IsNil)
+
+ environ, err := suite.environ.Provider().Prepare(config)
+ c.Assert(err, gc.IsNil)
+
+ preparedConfig := environ.Config()
+ unknownAttrs := preparedConfig.UnknownAttrs()
+
+ uuid, ok := unknownAttrs["environment-uuid"]
+ c.Assert(ok, gc.Equals, true)
+
+ _, err = utils.UUIDFromString(uuid.(string))
+ c....

Read more...

Revision history for this message
Raphaël Badin (rvb) wrote :

On 2013/10/15 11:00:26, allenap wrote:
> Please take a look.

LGTM (pretty similar to what the openstack provider does indeed)

[0]

You might want to do that in another branch, but we need a getter to
fetch that uuid or '' if it isn't set (to use '' as the agent name for
already deployed environments).

https://codereview.appspot.com/14696043/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM - assuming environment-uuid will be added to configFields and
configDefaults
in a subsequent MP.

https://codereview.appspot.com/14696043/

Revision history for this message
Gavin Panella (allenap) wrote :

On 2013/10/15 11:41:05, rog wrote:
> LGTM - assuming environment-uuid will be added to configFields and
> configDefaults
> in a subsequent MP.

Thanks for pointing that out Roger.

The other fields in MAAS's configFields all have a "maas-" prefix. I'll
rename this field to have that prefix too, for consistency, and to mark
it out as a MAAS-specific fix, at least for now.

Thank you both for your reviews!

https://codereview.appspot.com/14696043/

Revision history for this message
Roger Peppe (rogpeppe) wrote :

On 15 October 2013 13:27, <email address hidden> wrote:
> On 2013/10/15 11:41:05, rog wrote:
>>
>> LGTM - assuming environment-uuid will be added to configFields and
>> configDefaults
>> in a subsequent MP.
>
>
> Thanks for pointing that out Roger.
>
> The other fields in MAAS's configFields all have a "maas-" prefix. I'll
> rename this field to have that prefix too, for consistency, and to mark
> it out as a MAAS-specific fix, at least for now.
>
> Thank you both for your reviews!

Might I suggest that if you do that, you change the Prepare
method to return with an error if the maas-uuid (or whatever you
decide to call it) attribute is already set? That way people
can't muck things up by setting a non-unique uuid
in their environments.yaml.

Alternatively you can leave it as environment-uuid and when
we change juju-core to generate it in Prepare for all environments,
we'll reuse that attribute and the maas code can continue to
work without further change (just a bit of code that can be
deleted at leisure).

I think I prefer the latter option, which was what I was thinking
when I LGTM'd.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'provider/maas/environprovider.go'
2--- provider/maas/environprovider.go 2013-10-11 14:05:04 +0000
3+++ provider/maas/environprovider.go 2013-10-15 11:04:44 +0000
4@@ -38,7 +38,18 @@
5 }
6
7 func (p maasEnvironProvider) Prepare(cfg *config.Config) (environs.Environ, error) {
8- // TODO any attributes to prepare?
9+ attrs := cfg.UnknownAttrs()
10+ if _, ok := attrs["environment-uuid"]; !ok {
11+ uuid, err := utils.NewUUID()
12+ if err != nil {
13+ return nil, err
14+ }
15+ attrs["environment-uuid"] = uuid.String()
16+ }
17+ cfg, err := cfg.Apply(attrs)
18+ if err != nil {
19+ return nil, err
20+ }
21 return p.Open(cfg)
22 }
23
24
25=== modified file 'provider/maas/environprovider_test.go'
26--- provider/maas/environprovider_test.go 2013-09-25 17:04:52 +0000
27+++ provider/maas/environprovider_test.go 2013-10-15 11:04:44 +0000
28@@ -11,6 +11,7 @@
29
30 "launchpad.net/juju-core/environs/config"
31 "launchpad.net/juju-core/testing"
32+ "launchpad.net/juju-core/utils"
33 )
34
35 type EnvironProviderSuite struct {
36@@ -38,6 +39,30 @@
37 c.Check(secretAttrs, gc.DeepEquals, expectedAttrs)
38 }
39
40+func (suite *EnvironProviderSuite) TestUnknownAttrsContainEnvironmentUUID(c *gc.C) {
41+ testJujuHome := c.MkDir()
42+ defer config.SetJujuHome(config.SetJujuHome(testJujuHome))
43+ attrs := testing.FakeConfig().Merge(testing.Attrs{
44+ "type": "maas",
45+ "maas-oauth": "aa:bb:cc",
46+ "maas-server": "http://maas.testing.invalid/maas/",
47+ })
48+ config, err := config.New(config.NoDefaults, attrs)
49+ c.Assert(err, gc.IsNil)
50+
51+ environ, err := suite.environ.Provider().Prepare(config)
52+ c.Assert(err, gc.IsNil)
53+
54+ preparedConfig := environ.Config()
55+ unknownAttrs := preparedConfig.UnknownAttrs()
56+
57+ uuid, ok := unknownAttrs["environment-uuid"]
58+ c.Assert(ok, gc.Equals, true)
59+
60+ _, err = utils.UUIDFromString(uuid.(string))
61+ c.Assert(err, gc.IsNil)
62+}
63+
64 // create a temporary file with the given content. The file will be cleaned
65 // up at the end of the test calling this method.
66 func createTempFile(c *gc.C, content []byte) string {

Subscribers

People subscribed via source and target branches

to status/vote changes: