Merge lp:~axwalk/juju-core/azure-mode-nounitplacement into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

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://codereview.appspot.com/77950045/

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://codereview.appspot.com/77950045/

[Andrew Wilkins (axwalk)]

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, e...

[William Reade (fwereade)]

LGTM, but please don't land it until the followups are ready, so we
don't run any risk of releasing half-implemented functionality.

https://codereview.appspot.com/77950045/diff/1/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/77950045/diff/1/provider/azure/environ.go#newcode903
provider/azure/environ.go:903: return fmt.Errorf("unit placement is not
permitted with availability-sets-enabled")
s/permitted/supported/

?

https://codereview.appspot.com/77950045/diff/1/provider/azure/environprovider.go
File provider/azure/environprovider.go (right):

https://codereview.appspot.com/77950045/diff/1/provider/azure/environprovider.go#newcode47
provider/azure/environprovider.go:47: // by default, unless the user set
a value.
This behaviour definitely demands documentation -- please make sure that
(1) the release notes mention it and (2) you write something for the
html docs.

https://codereview.appspot.com/77950045/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/77950045/diff/1/provider/azure/environ.go
File provider/azure/environ.go (right):

https://codereview.appspot.com/77950045/diff/1/provider/azure/environ.go#newcode903
provider/azure/environ.go:903: return fmt.Errorf("unit placement is not
permitted with availability-sets-enabled")
On 2014/03/24 09:55:21, fwereade wrote:
> s/permitted/supported/

> ?

Done.

https://codereview.appspot.com/77950045/diff/1/provider/azure/environprovider.go
File provider/azure/environprovider.go (right):

https://codereview.appspot.com/77950045/diff/1/provider/azure/environprovider.go#newcode47
provider/azure/environprovider.go:47: // by default, unless the user set
a value.
On 2014/03/24 09:55:21, fwereade wrote:
> This behaviour definitely demands documentation -- please make sure
that (1) the
> release notes mention it and (2) you write something for the html
docs.

Filed https://bugs.launchpad.net/juju-core/+bug/1297066

https://codereview.appspot.com/77950045/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/77950045/

=== modified file 'provider/azure/config.go'
--- provider/azure/config.go	2014-03-10 20:22:44 +0000
+++ provider/azure/config.go	2014-04-03 09:09:45 +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

=== 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-04-03 09:09:45 +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")
+}

=== modified file 'provider/azure/environ.go'
--- provider/azure/environ.go	2014-04-03 07:14:57 +0000
+++ provider/azure/environ.go	2014-04-03 09:09:45 +0000
@@ -53,9 +53,13 @@
 	stateServerLabel = "juju-state-server"
 )
 
+// vars for testing purposes.
+var (
+	createInstance = (*azureEnviron).createInstance
+)
+
 type azureEnviron struct {
 	common.NopPrecheckerPolicy
-	common.SupportsUnitPlacementPolicy
 
 	// Except where indicated otherwise, all fields in this object should
 	// only be accessed using a lock or a snapshot.
@@ -436,8 +440,10 @@
 	var err error
 	var service *gwacl.HostedService
 	if serviceName != "" {
+		logger.Debugf("creating instance in existing cloud service %q", serviceName)
 		service, err = azure.GetHostedServiceProperties(serviceName, true)
 	} else {
+		logger.Debugf("creating instance in new cloud service")
 		// If we're creating a cloud service for state servers,
 		// we will want to open additional ports. We need to
 		// record this against the cloud service, so we use a
@@ -503,14 +509,10 @@
 
 // StartInstance is specified in the InstanceBroker interface.
 func (env *azureEnviron) StartInstance(args environs.StartInstanceParams) (_ instance.Instance, _ *instance.HardwareCharacteristics, err error) {
-
 	if args.MachineConfig.HasNetworks() {
 		return nil, nil, fmt.Errorf("starting instances with networks is not supported yet.")
 	}
 
-	// Declaring "err" in the function signature so that we can "defer"
-	// any cleanup that needs to run during error returns.
-
 	err = environs.FinishMachineConfig(args.MachineConfig, env.Config(), args.Constraints)
 	if err != nil {
 		return nil, nil, err
@@ -533,7 +535,8 @@
 	}
 	defer env.releaseManagementAPI(azure)
 
-	location := env.getSnapshot().ecfg.location()
+	snapshot := env.getSnapshot()
+	location := snapshot.ecfg.location()
 	instanceType, sourceImageName, err := env.selectInstanceTypeAndImage(&instances.InstanceConstraint{
 		Region:      location,
 		Series:      args.Tools.OneSeries(),
@@ -548,9 +551,7 @@
 	// the same affinity, so that machines can be be allocated to the
 	// same availability set.
 	var cloudServiceName string
-	// TODO(axw) replace "false &&" with mode check once
-	// availability-sets-enabled change is landed.
-	if false && args.DistributionGroup != nil {
+	if args.DistributionGroup != nil && snapshot.ecfg.availabilitySetsEnabled() {
 		instanceIds, err := args.DistributionGroup()
 		if err != nil {
 			return nil, nil, err
@@ -561,7 +562,6 @@
 				break
 			}
 		}
-		logger.Debugf("using existing cloud service: %q", cloudServiceName)
 	}
 
 	vhd := env.newOSDisk(sourceImageName)
@@ -575,7 +575,7 @@
 		}
 	}
 	role := env.newRole(instanceType, vhd, userData, stateServer)
-	inst, err := env.createInstance(azure.ManagementAPI, role, cloudServiceName, stateServer)
+	inst, err := createInstance(env, azure.ManagementAPI, role, cloudServiceName, stateServer)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -1165,3 +1165,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 supported with availability-sets-enabled")
+	}
+	return nil
+}

=== modified file 'provider/azure/environ_test.go'
--- provider/azure/environ_test.go	2014-04-03 07:14:57 +0000
+++ provider/azure/environ_test.go	2014-04-03 09:09:45 +0000
@@ -31,6 +31,9 @@
 	envtesting "launchpad.net/juju-core/environs/testing"
 	"launchpad.net/juju-core/environs/tools"
 	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/api"
+	apiparams "launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/testing"
 )
 
@@ -1393,3 +1396,77 @@
 	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 supported 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)
+}
+
+func (s *environSuite) TestStartInstance(c *gc.C) {
+	env := makeEnviron(c)
+	s.setDummyStorage(c, env)
+	env.ecfg.attrs["force-image-name"] = "my-image"
+	stateInfo := &state.Info{
+		Addrs:    []string{"localhost:123"},
+		CACert:   []byte(testing.CACert),
+		Password: "password",
+		Tag:      "machine-1",
+	}
+	apiInfo := &api.Info{
+		Addrs:    []string{"localhost:124"},
+		CACert:   []byte(testing.CACert),
+		Password: "admin",
+		Tag:      "machine-1",
+	}
+	var params environs.StartInstanceParams
+	params.Tools = envtesting.AssertUploadFakeToolsVersions(c, env.storage, envtesting.V120p...)
+	params.MachineConfig = environs.NewMachineConfig("1", "yanonce", nil, nil, stateInfo, apiInfo)
+
+	// Start out with availability sets disabled.
+	env.ecfg.attrs["availability-sets-enabled"] = false
+
+	var expectServiceName string
+	var expectStateServer bool
+	s.PatchValue(&createInstance, func(env *azureEnviron, azure *gwacl.ManagementAPI, role *gwacl.Role, serviceName string, stateServer bool) (instance.Instance, error) {
+		c.Assert(serviceName, gc.Equals, expectServiceName)
+		c.Assert(stateServer, gc.Equals, expectStateServer)
+		return nil, nil
+	})
+	env.StartInstance(params)
+
+	// DistributionGroup won't have an effect if availability-sets-enabled=false.
+	expectServiceName = ""
+	params.DistributionGroup = func() ([]instance.Id, error) {
+		return []instance.Id{instance.Id(env.getEnvPrefix() + "whatever-role0")}, nil
+	}
+	env.StartInstance(params)
+
+	env.ecfg.attrs["availability-sets-enabled"] = true
+	expectServiceName = "juju-testenv-whatever"
+	env.StartInstance(params)
+
+	expectServiceName = ""
+	params.DistributionGroup = nil
+	env.StartInstance(params)
+
+	// Empty distribution group is equivalent to no DistributionGroup function.
+	params.DistributionGroup = func() ([]instance.Id, error) {
+		return nil, nil
+	}
+	env.StartInstance(params)
+
+	// If the machine has the JobManagesEnviron job, we should see stateServer==true.
+	expectStateServer = true
+	params.MachineConfig.Jobs = []apiparams.MachineJob{apiparams.JobHostUnits, apiparams.JobManageEnviron}
+	env.StartInstance(params)
+}

=== modified file 'provider/azure/environprovider.go'
--- provider/azure/environprovider.go	2014-04-02 11:35:49 +0000
+++ provider/azure/environprovider.go	2014-04-03 09:09:45 +0000
@@ -39,6 +39,14 @@
 
 // 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)
 }