Code review comment for lp:~axwalk/juju-core/azure-mode-nounitplacement

Reviewers: mp+211863_code.launchpad.net,

Message:
Please take a look.

Description:
provider/azure: availability-sets-enabled config

A new config attribute availability-sets-enabled
is defined, which controls the mode in which the
Azure environment operates. If true (the default),
availability set support is enabled and instances
will be grouped by service. The attribute cannot
be changed after the environment is prepared.

If the availability-sets-enabled is true, unit
placement is prohibited. This is necessary to
avoid various issues such as incorrectly load-
balancing ports across heterogenous services, and
makes it possible to unambiguously determine
which Cloud Service to assign units to for high
availability.

https://code.launchpad.net/~axwalk/juju-core/azure-mode-nounitplacement/+merge/211863

Requires:
https://code.launchpad.net/~axwalk/juju-core/state-environcapability/+merge/211749

(do not edit description out of merge proposal)

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

Affected files (+83, -2 lines):
   A [revision details]
   M provider/azure/config.go
   M provider/azure/config_test.go
   M provider/azure/environ.go
   M provider/azure/environ_test.go
   M provider/azure/environprovider.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: <email address hidden>
+New revision: <email address hidden>

Index: provider/azure/config.go
=== modified file 'provider/azure/config.go'
--- provider/azure/config.go 2014-03-10 20:22:44 +0000
+++ provider/azure/config.go 2014-03-20 04:44:57 +0000
@@ -18,12 +18,16 @@
   "management-certificate": schema.String(),
   "storage-account-name": schema.String(),
   "force-image-name": schema.String(),
+ "availability-sets-enabled": schema.Bool(),
  }
  var configDefaults = schema.Defaults{
   "location": "",
   "management-certificate": "",
   "management-certificate-path": "",
   "force-image-name": "",
+ // availability-sets-enabled is set to Omit (equivalent
+ // to false) for backwards compatibility.
+ "availability-sets-enabled": schema.Omit,
  }

  type azureEnvironConfig struct {
@@ -51,6 +55,11 @@
   return cfg.attrs["force-image-name"].(string)
  }

+func (cfg *azureEnvironConfig) availabilitySetsEnabled() bool {
+ enabled, _ := cfg.attrs["availability-sets-enabled"].(bool)
+ return enabled
+}
+
  func (prov azureEnvironProvider) newConfig(cfg *config.Config)
(*azureEnvironConfig, error) {
   validCfg, err := prov.Validate(cfg, nil)
   if err != nil {
@@ -71,6 +80,13 @@
    return nil, err
   }

+ // User cannot change availability-sets-enabled after environment is
prepared.
+ if oldCfg != nil {
+ if oldCfg.AllAttrs()["availability-sets-enabled"] !=
cfg.AllAttrs()["availability-sets-enabled"] {
+ return nil, fmt.Errorf("cannot change availability-sets-enabled")
+ }
+ }
+
   validated, err := cfg.ValidateUnknownAttrs(configFields, configDefaults)
   if err != nil {
    return nil, err

Index: provider/azure/config_test.go
=== modified file 'provider/azure/config_test.go'
--- provider/azure/config_test.go 2014-02-12 04:54:19 +0000
+++ provider/azure/config_test.go 2014-03-20 04:44:57 +0000
@@ -7,6 +7,7 @@
   "io/ioutil"
   "strings"

+ jc "github.com/juju/testing/checkers"
   gc "launchpad.net/gocheck"

   "launchpad.net/juju-core/environs/config"
@@ -202,3 +203,35 @@
   _, err = provider.Validate(cfg, nil)
   c.Assert(err, gc.IsNil)
  }
+
+func (*configSuite) TestAvailabilitySetsEnabledDefault(c *gc.C) {
+ userValues := []interface{}{nil, false, true}
+ for _, userValue := range userValues {
+ attrs := makeAzureConfigMap(c)
+ // If availability-sets-enabled isn't specified, it's set to true.
+ checker := jc.IsTrue
+ if userValue, ok := userValue.(bool); ok {
+ attrs["availability-sets-enabled"] = userValue
+ if !userValue {
+ checker = jc.IsFalse
+ }
+ }
+ cfg, err := config.New(config.UseDefaults, attrs)
+ c.Assert(err, gc.IsNil)
+ env, err := azureEnvironProvider{}.Prepare(testing.Context(c), cfg)
+ c.Assert(err, gc.IsNil)
+ azureEnv := env.(*azureEnviron)
+ c.Assert(azureEnv.ecfg.availabilitySetsEnabled(), checker)
+ }
+}
+
+func (*configSuite) TestAvailabilitySetsEnabledImmutable(c *gc.C) {
+ cfg, err := config.New(config.UseDefaults, makeAzureConfigMap(c))
+ c.Assert(err, gc.IsNil)
+ env, err := azureEnvironProvider{}.Prepare(testing.Context(c), cfg)
+ c.Assert(err, gc.IsNil)
+ cfg, err =
env.Config().Apply(map[string]interface{}{"availability-sets-enabled":
false})
+ c.Assert(err, gc.IsNil)
+ err = env.SetConfig(cfg)
+ c.Assert(err, gc.ErrorMatches, "cannot change availability-sets-enabled")
+}

Index: provider/azure/environ.go
=== modified file 'provider/azure/environ.go'
--- provider/azure/environ.go 2014-03-20 03:27:26 +0000
+++ provider/azure/environ.go 2014-03-20 04:44:57 +0000
@@ -52,7 +52,6 @@

  type azureEnviron struct {
   common.NopPrechecker
- common.DoesSupportUnitPlacement

   // Except where indicated otherwise, all fields in this object should
   // only be accessed using a lock or a snapshot.
@@ -897,3 +896,11 @@
    Endpoint: string(gwacl.GetEndpoint(ecfg.location())),
   }, nil
  }
+
+// SupportsUnitPlacement is specified in the state.EnvironCapability
interface.
+func (env *azureEnviron) SupportsUnitPlacement() error {
+ if env.getSnapshot().ecfg.availabilitySetsEnabled() {
+ return fmt.Errorf("unit placement is not permitted with
availability-sets-enabled")
+ }
+ return nil
+}

Index: provider/azure/environ_test.go
=== modified file 'provider/azure/environ_test.go'
--- provider/azure/environ_test.go 2014-03-18 01:25:52 +0000
+++ provider/azure/environ_test.go 2014-03-20 04:44:57 +0000
@@ -1285,3 +1285,18 @@
   c.Assert(len(sources), gc.Equals, 1)
   assertSourceContents(c, sources[0], "filename", data)
  }
+
+func (s *environSuite) TestCheckUnitAssignment(c *gc.C) {
+ // If availability-sets-enabled is true, then placement is disabled.
+ attrs := makeAzureConfigMap(c)
+ attrs["availability-sets-enabled"] = true
+ env := environs.Environ(makeEnvironWithConfig(c, attrs))
+ err := env.SupportsUnitPlacement()
+ c.Assert(err, gc.ErrorMatches, "unit placement is not permitted with
availability-sets-enabled")
+
+ // If the user disables availability sets, they can do what they want.
+ attrs["availability-sets-enabled"] = false
+ env = environs.Environ(makeEnvironWithConfig(c, attrs))
+ err = env.SupportsUnitPlacement()
+ c.Assert(err, gc.IsNil)
+}

Index: provider/azure/environprovider.go
=== modified file 'provider/azure/environprovider.go'
--- provider/azure/environprovider.go 2014-03-05 19:41:34 +0000
+++ provider/azure/environprovider.go 2014-03-20 04:44:57 +0000
@@ -43,7 +43,15 @@

  // Prepare is specified in the EnvironProvider interface.
  func (prov azureEnvironProvider) Prepare(ctx environs.BootstrapContext,
cfg *config.Config) (environs.Environ, error) {
- // TODO prepare environment as necessary
+ // Set availability-sets-enabled to true
+ // by default, unless the user set a value.
+ if _, ok := cfg.AllAttrs()["availability-sets-enabled"]; !ok {
+ var err error
+ cfg, err = cfg.Apply(map[string]interface{}{"availability-sets-enabled":
true})
+ if err != nil {
+ return nil, err
+ }
+ }
   return prov.Open(cfg)
  }