juju-core

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

  1. azure-mode-nounitplacement
  2. Merge into trunk
Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 2555
Proposed branch: lp:~axwalk/juju-core/azure-mode-nounitplacement
Merge into: lp:~go-bot/juju-core/trunk
Prerequisite: lp:~axwalk/juju-core/state-environcapability
Diff against target: 301 lines (+154/-12)
5 files modified
provider/azure/config.go (+16/-0)
provider/azure/config_test.go (+33/-0)
provider/azure/environ.go (+19/-11)
provider/azure/environ_test.go (+77/-0)
provider/azure/environprovider.go (+9/-1)
To merge this branch: bzr merge lp:~axwalk/juju-core/azure-mode-nounitplacement
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+211863@code.launchpad.net

Commit message

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/

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (7.3 KiB)

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

Read more...

Revision history for this message
William Reade (fwereade) wrote :

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/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

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/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

Please take a look.

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
18 "management-certificate": schema.String(),18 "management-certificate": schema.String(),
19 "storage-account-name": schema.String(),19 "storage-account-name": schema.String(),
20 "force-image-name": schema.String(),20 "force-image-name": schema.String(),
21 "availability-sets-enabled": schema.Bool(),
21}22}
22var configDefaults = schema.Defaults{23var configDefaults = schema.Defaults{
23 "location": "",24 "location": "",
24 "management-certificate": "",25 "management-certificate": "",
25 "management-certificate-path": "",26 "management-certificate-path": "",
26 "force-image-name": "",27 "force-image-name": "",
28 // availability-sets-enabled is set to Omit (equivalent
29 // to false) for backwards compatibility.
30 "availability-sets-enabled": schema.Omit,
27}31}
2832
29type azureEnvironConfig struct {33type azureEnvironConfig struct {
@@ -51,6 +55,11 @@
51 return cfg.attrs["force-image-name"].(string)55 return cfg.attrs["force-image-name"].(string)
52}56}
5357
58func (cfg *azureEnvironConfig) availabilitySetsEnabled() bool {
59 enabled, _ := cfg.attrs["availability-sets-enabled"].(bool)
60 return enabled
61}
62
54func (prov azureEnvironProvider) newConfig(cfg *config.Config) (*azureEnvironConfig, error) {63func (prov azureEnvironProvider) newConfig(cfg *config.Config) (*azureEnvironConfig, error) {
55 validCfg, err := prov.Validate(cfg, nil)64 validCfg, err := prov.Validate(cfg, nil)
56 if err != nil {65 if err != nil {
@@ -71,6 +80,13 @@
71 return nil, err80 return nil, err
72 }81 }
7382
83 // User cannot change availability-sets-enabled after environment is prepared.
84 if oldCfg != nil {
85 if oldCfg.AllAttrs()["availability-sets-enabled"] != cfg.AllAttrs()["availability-sets-enabled"] {
86 return nil, fmt.Errorf("cannot change availability-sets-enabled")
87 }
88 }
89
74 validated, err := cfg.ValidateUnknownAttrs(configFields, configDefaults)90 validated, err := cfg.ValidateUnknownAttrs(configFields, configDefaults)
75 if err != nil {91 if err != nil {
76 return nil, err92 return nil, err
7793
=== 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 @@
7 "io/ioutil"7 "io/ioutil"
8 "strings"8 "strings"
99
10 jc "github.com/juju/testing/checkers"
10 gc "launchpad.net/gocheck"11 gc "launchpad.net/gocheck"
1112
12 "launchpad.net/juju-core/environs/config"13 "launchpad.net/juju-core/environs/config"
@@ -202,3 +203,35 @@
202 _, err = provider.Validate(cfg, nil)203 _, err = provider.Validate(cfg, nil)
203 c.Assert(err, gc.IsNil)204 c.Assert(err, gc.IsNil)
204}205}
206
207func (*configSuite) TestAvailabilitySetsEnabledDefault(c *gc.C) {
208 userValues := []interface{}{nil, false, true}
209 for _, userValue := range userValues {
210 attrs := makeAzureConfigMap(c)
211 // If availability-sets-enabled isn't specified, it's set to true.
212 checker := jc.IsTrue
213 if userValue, ok := userValue.(bool); ok {
214 attrs["availability-sets-enabled"] = userValue
215 if !userValue {
216 checker = jc.IsFalse
217 }
218 }
219 cfg, err := config.New(config.UseDefaults, attrs)
220 c.Assert(err, gc.IsNil)
221 env, err := azureEnvironProvider{}.Prepare(testing.Context(c), cfg)
222 c.Assert(err, gc.IsNil)
223 azureEnv := env.(*azureEnviron)
224 c.Assert(azureEnv.ecfg.availabilitySetsEnabled(), checker)
225 }
226}
227
228func (*configSuite) TestAvailabilitySetsEnabledImmutable(c *gc.C) {
229 cfg, err := config.New(config.UseDefaults, makeAzureConfigMap(c))
230 c.Assert(err, gc.IsNil)
231 env, err := azureEnvironProvider{}.Prepare(testing.Context(c), cfg)
232 c.Assert(err, gc.IsNil)
233 cfg, err = env.Config().Apply(map[string]interface{}{"availability-sets-enabled": false})
234 c.Assert(err, gc.IsNil)
235 err = env.SetConfig(cfg)
236 c.Assert(err, gc.ErrorMatches, "cannot change availability-sets-enabled")
237}
205238
=== 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 @@
53 stateServerLabel = "juju-state-server"53 stateServerLabel = "juju-state-server"
54)54)
5555
56// vars for testing purposes.
57var (
58 createInstance = (*azureEnviron).createInstance
59)
60
56type azureEnviron struct {61type azureEnviron struct {
57 common.NopPrecheckerPolicy62 common.NopPrecheckerPolicy
58 common.SupportsUnitPlacementPolicy
5963
60 // Except where indicated otherwise, all fields in this object should64 // Except where indicated otherwise, all fields in this object should
61 // only be accessed using a lock or a snapshot.65 // only be accessed using a lock or a snapshot.
@@ -436,8 +440,10 @@
436 var err error440 var err error
437 var service *gwacl.HostedService441 var service *gwacl.HostedService
438 if serviceName != "" {442 if serviceName != "" {
443 logger.Debugf("creating instance in existing cloud service %q", serviceName)
439 service, err = azure.GetHostedServiceProperties(serviceName, true)444 service, err = azure.GetHostedServiceProperties(serviceName, true)
440 } else {445 } else {
446 logger.Debugf("creating instance in new cloud service")
441 // If we're creating a cloud service for state servers,447 // If we're creating a cloud service for state servers,
442 // we will want to open additional ports. We need to448 // we will want to open additional ports. We need to
443 // record this against the cloud service, so we use a449 // record this against the cloud service, so we use a
@@ -503,14 +509,10 @@
503509
504// StartInstance is specified in the InstanceBroker interface.510// StartInstance is specified in the InstanceBroker interface.
505func (env *azureEnviron) StartInstance(args environs.StartInstanceParams) (_ instance.Instance, _ *instance.HardwareCharacteristics, err error) {511func (env *azureEnviron) StartInstance(args environs.StartInstanceParams) (_ instance.Instance, _ *instance.HardwareCharacteristics, err error) {
506
507 if args.MachineConfig.HasNetworks() {512 if args.MachineConfig.HasNetworks() {
508 return nil, nil, fmt.Errorf("starting instances with networks is not supported yet.")513 return nil, nil, fmt.Errorf("starting instances with networks is not supported yet.")
509 }514 }
510515
511 // Declaring "err" in the function signature so that we can "defer"
512 // any cleanup that needs to run during error returns.
513
514 err = environs.FinishMachineConfig(args.MachineConfig, env.Config(), args.Constraints)516 err = environs.FinishMachineConfig(args.MachineConfig, env.Config(), args.Constraints)
515 if err != nil {517 if err != nil {
516 return nil, nil, err518 return nil, nil, err
@@ -533,7 +535,8 @@
533 }535 }
534 defer env.releaseManagementAPI(azure)536 defer env.releaseManagementAPI(azure)
535537
536 location := env.getSnapshot().ecfg.location()538 snapshot := env.getSnapshot()
539 location := snapshot.ecfg.location()
537 instanceType, sourceImageName, err := env.selectInstanceTypeAndImage(&instances.InstanceConstraint{540 instanceType, sourceImageName, err := env.selectInstanceTypeAndImage(&instances.InstanceConstraint{
538 Region: location,541 Region: location,
539 Series: args.Tools.OneSeries(),542 Series: args.Tools.OneSeries(),
@@ -548,9 +551,7 @@
548 // the same affinity, so that machines can be be allocated to the551 // the same affinity, so that machines can be be allocated to the
549 // same availability set.552 // same availability set.
550 var cloudServiceName string553 var cloudServiceName string
551 // TODO(axw) replace "false &&" with mode check once554 if args.DistributionGroup != nil && snapshot.ecfg.availabilitySetsEnabled() {
552 // availability-sets-enabled change is landed.
553 if false && args.DistributionGroup != nil {
554 instanceIds, err := args.DistributionGroup()555 instanceIds, err := args.DistributionGroup()
555 if err != nil {556 if err != nil {
556 return nil, nil, err557 return nil, nil, err
@@ -561,7 +562,6 @@
561 break562 break
562 }563 }
563 }564 }
564 logger.Debugf("using existing cloud service: %q", cloudServiceName)
565 }565 }
566566
567 vhd := env.newOSDisk(sourceImageName)567 vhd := env.newOSDisk(sourceImageName)
@@ -575,7 +575,7 @@
575 }575 }
576 }576 }
577 role := env.newRole(instanceType, vhd, userData, stateServer)577 role := env.newRole(instanceType, vhd, userData, stateServer)
578 inst, err := env.createInstance(azure.ManagementAPI, role, cloudServiceName, stateServer)578 inst, err := createInstance(env, azure.ManagementAPI, role, cloudServiceName, stateServer)
579 if err != nil {579 if err != nil {
580 return nil, nil, err580 return nil, nil, err
581 }581 }
@@ -1165,3 +1165,11 @@
1165 Endpoint: string(gwacl.GetEndpoint(ecfg.location())),1165 Endpoint: string(gwacl.GetEndpoint(ecfg.location())),
1166 }, nil1166 }, nil
1167}1167}
1168
1169// SupportsUnitPlacement is specified in the state.EnvironCapability interface.
1170func (env *azureEnviron) SupportsUnitPlacement() error {
1171 if env.getSnapshot().ecfg.availabilitySetsEnabled() {
1172 return fmt.Errorf("unit placement is not supported with availability-sets-enabled")
1173 }
1174 return nil
1175}
11681176
=== 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 @@
31 envtesting "launchpad.net/juju-core/environs/testing"31 envtesting "launchpad.net/juju-core/environs/testing"
32 "launchpad.net/juju-core/environs/tools"32 "launchpad.net/juju-core/environs/tools"
33 "launchpad.net/juju-core/instance"33 "launchpad.net/juju-core/instance"
34 "launchpad.net/juju-core/state"
35 "launchpad.net/juju-core/state/api"
36 apiparams "launchpad.net/juju-core/state/api/params"
34 "launchpad.net/juju-core/testing"37 "launchpad.net/juju-core/testing"
35)38)
3639
@@ -1393,3 +1396,77 @@
1393 c.Assert(len(sources), gc.Equals, 1)1396 c.Assert(len(sources), gc.Equals, 1)
1394 assertSourceContents(c, sources[0], "filename", data)1397 assertSourceContents(c, sources[0], "filename", data)
1395}1398}
1399
1400func (s *environSuite) TestCheckUnitAssignment(c *gc.C) {
1401 // If availability-sets-enabled is true, then placement is disabled.
1402 attrs := makeAzureConfigMap(c)
1403 attrs["availability-sets-enabled"] = true
1404 env := environs.Environ(makeEnvironWithConfig(c, attrs))
1405 err := env.SupportsUnitPlacement()
1406 c.Assert(err, gc.ErrorMatches, "unit placement is not supported with availability-sets-enabled")
1407
1408 // If the user disables availability sets, they can do what they want.
1409 attrs["availability-sets-enabled"] = false
1410 env = environs.Environ(makeEnvironWithConfig(c, attrs))
1411 err = env.SupportsUnitPlacement()
1412 c.Assert(err, gc.IsNil)
1413}
1414
1415func (s *environSuite) TestStartInstance(c *gc.C) {
1416 env := makeEnviron(c)
1417 s.setDummyStorage(c, env)
1418 env.ecfg.attrs["force-image-name"] = "my-image"
1419 stateInfo := &state.Info{
1420 Addrs: []string{"localhost:123"},
1421 CACert: []byte(testing.CACert),
1422 Password: "password",
1423 Tag: "machine-1",
1424 }
1425 apiInfo := &api.Info{
1426 Addrs: []string{"localhost:124"},
1427 CACert: []byte(testing.CACert),
1428 Password: "admin",
1429 Tag: "machine-1",
1430 }
1431 var params environs.StartInstanceParams
1432 params.Tools = envtesting.AssertUploadFakeToolsVersions(c, env.storage, envtesting.V120p...)
1433 params.MachineConfig = environs.NewMachineConfig("1", "yanonce", nil, nil, stateInfo, apiInfo)
1434
1435 // Start out with availability sets disabled.
1436 env.ecfg.attrs["availability-sets-enabled"] = false
1437
1438 var expectServiceName string
1439 var expectStateServer bool
1440 s.PatchValue(&createInstance, func(env *azureEnviron, azure *gwacl.ManagementAPI, role *gwacl.Role, serviceName string, stateServer bool) (instance.Instance, error) {
1441 c.Assert(serviceName, gc.Equals, expectServiceName)
1442 c.Assert(stateServer, gc.Equals, expectStateServer)
1443 return nil, nil
1444 })
1445 env.StartInstance(params)
1446
1447 // DistributionGroup won't have an effect if availability-sets-enabled=false.
1448 expectServiceName = ""
1449 params.DistributionGroup = func() ([]instance.Id, error) {
1450 return []instance.Id{instance.Id(env.getEnvPrefix() + "whatever-role0")}, nil
1451 }
1452 env.StartInstance(params)
1453
1454 env.ecfg.attrs["availability-sets-enabled"] = true
1455 expectServiceName = "juju-testenv-whatever"
1456 env.StartInstance(params)
1457
1458 expectServiceName = ""
1459 params.DistributionGroup = nil
1460 env.StartInstance(params)
1461
1462 // Empty distribution group is equivalent to no DistributionGroup function.
1463 params.DistributionGroup = func() ([]instance.Id, error) {
1464 return nil, nil
1465 }
1466 env.StartInstance(params)
1467
1468 // If the machine has the JobManagesEnviron job, we should see stateServer==true.
1469 expectStateServer = true
1470 params.MachineConfig.Jobs = []apiparams.MachineJob{apiparams.JobHostUnits, apiparams.JobManageEnviron}
1471 env.StartInstance(params)
1472}
13961473
=== 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 @@
3939
40// Prepare is specified in the EnvironProvider interface.40// Prepare is specified in the EnvironProvider interface.
41func (prov azureEnvironProvider) Prepare(ctx environs.BootstrapContext, cfg *config.Config) (environs.Environ, error) {41func (prov azureEnvironProvider) Prepare(ctx environs.BootstrapContext, cfg *config.Config) (environs.Environ, error) {
42 // TODO prepare environment as necessary42 // Set availability-sets-enabled to true
43 // by default, unless the user set a value.
44 if _, ok := cfg.AllAttrs()["availability-sets-enabled"]; !ok {
45 var err error
46 cfg, err = cfg.Apply(map[string]interface{}{"availability-sets-enabled": true})
47 if err != nil {
48 return nil, err
49 }
50 }
43 return prov.Open(cfg)51 return prov.Open(cfg)
44}52}

Subscribers

People subscribed via source and target branches

to status/vote changes: