Merge pull request juju#8352 from manadart/constraints-from-space-config

ObviouslyGreen · Feb 15, 2018 · 37fbc9d · 37fbc9d
2 parents ab7f5b7 + b4e971e
commit 37fbc9d
Show file tree

Hide file tree

Showing 6 changed files with 205 additions and 48 deletions.
diff --git a/apiserver/facades/client/highavailability/highavailability.go b/apiserver/facades/client/highavailability/highavailability.go
@@ -150,40 +150,24 @@ func (api *HighAvailabilityAPI) enableHASingle(st *state.State, spec params.Cont
 		}
 		series = templateMachine.Series()
 	}
+
+	// If there were no supplied constraints, use the original bootstrap
+	// constraints.
 	if constraints.IsEmpty(&spec.Constraints) {
-		// No constraints specified, so we'll use the constraints off
-		// a running controller.
-		controllerInfo, err := st.ControllerInfo()
+		var err error
+		spec.Constraints, err = getBootstrapConstraints(st)
 		if err != nil {
-			return params.ControllersChanges{}, err
-		}
-		// We'll sort the controller ids to find the smallest.
-		// This will typically give the initial bootstrap machine.
-		var controllerIds []int
-		for _, id := range controllerInfo.MachineIds {
-			idNum, err := strconv.Atoi(id)
-			if err != nil {
-				logger.Warningf("ignoring non numeric controller id %v", id)
-				continue
-			}
-			controllerIds = append(controllerIds, idNum)
+			return params.ControllersChanges{}, errors.Trace(err)
 		}
-		if len(controllerIds) == 0 {
-			errors.Errorf("internal error, failed to find any controllers")
-		}
-		sort.Ints(controllerIds)
+	}
 
-		// Load the controller machine and get its constraints.
-		controllerId := controllerIds[0]
-		controller, err := st.Machine(strconv.Itoa(controllerId))
-		if err != nil {
-			return params.ControllersChanges{}, errors.Annotatef(err, "reading controller id %v", controllerId)
-		}
-		spec.Constraints, err = controller.Constraints()
-		if err != nil {
-			return params.ControllersChanges{}, errors.Annotatef(err, "reading constraints for controller id %v", controllerId)
-		}
+	// Retrieve the controller configuration and merge any implied space
+	// constraints into the spec constraints.
+	cfg, err := st.ControllerConfig()
+	if err != nil {
+		return params.ControllersChanges{}, errors.Annotate(err, "retrieving controller config")
 	}
+	spec.Constraints.Spaces = cfg.AsSpaceConstraints(spec.Constraints.Spaces)
 
 	changes, err := st.EnableHA(spec.NumControllers, spec.Constraints, series, spec.Placement, api.machineID)
 	if err != nil {
@@ -192,6 +176,41 @@ func (api *HighAvailabilityAPI) enableHASingle(st *state.State, spec params.Cont
 	return controllersChanges(changes), nil
 }
 
+// getBootstrapConstraints attempts to return the constraints for the initial
+// bootstrapped controller.
+func getBootstrapConstraints(st *state.State) (constraints.Value, error) {
+	controllerInfo, err := st.ControllerInfo()
+	if err != nil {
+		return constraints.Value{}, err
+	}
+
+	// Sort the controller IDs from low to high and take the first.
+	// This will typically give the initial bootstrap machine.
+	var controllerIds []int
+	for _, id := range controllerInfo.MachineIds {
+		idNum, err := strconv.Atoi(id)
+		if err != nil {
+			logger.Warningf("ignoring non numeric controller id %v", id)
+			continue
+		}
+		controllerIds = append(controllerIds, idNum)
+	}
+	if len(controllerIds) == 0 {
+		errors.Errorf("internal error, failed to find any controllers")
+	}
+	sort.Ints(controllerIds)
+	controllerId := controllerIds[0]
+
+	// Load the controller machine and get its constraints.
+	controller, err := st.Machine(strconv.Itoa(controllerId))
+	if err != nil {
+		return constraints.Value{}, errors.Annotatef(err, "reading controller id %v", controllerId)
+	}
+
+	cons, err := controller.Constraints()
+	return cons, errors.Annotatef(err, "reading constraints for controller id %v", controllerId)
+}
+
 // StopHAReplicationForUpgrade will prompt the HA cluster to enter upgrade
 // mongo mode.
 func (api *HighAvailabilityAPI) StopHAReplicationForUpgrade(args params.UpgradeMongoParams) (params.MongoUpgradeResults, error) {

diff --git a/apiserver/facades/client/highavailability/highavailability_test.go b/apiserver/facades/client/highavailability/highavailability_test.go
@@ -9,14 +9,15 @@ import (
 	"github.com/juju/errors"
 	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
-	worker "gopkg.in/juju/worker.v1"
+	"gopkg.in/juju/worker.v1"
 
 	"github.com/juju/juju/apiserver/common"
 	commontesting "github.com/juju/juju/apiserver/common/testing"
 	"github.com/juju/juju/apiserver/facades/client/highavailability"
 	"github.com/juju/juju/apiserver/params"
 	apiservertesting "github.com/juju/juju/apiserver/testing"
 	"github.com/juju/juju/constraints"
+	"github.com/juju/juju/controller"
 	"github.com/juju/juju/juju/testing"
 	"github.com/juju/juju/state"
 	"github.com/juju/juju/state/presence"
@@ -39,10 +40,6 @@ type clientSuite struct {
 	commontesting.BlockHelper
 }
 
-type KillerForTesting interface {
-	KillForTesting() error
-}
-
 var _ = gc.Suite(&clientSuite{})
 
 var (
@@ -102,7 +99,12 @@ func (s *clientSuite) enableHA(
 }
 
 func enableHA(
-	c *gc.C, haServer *highavailability.HighAvailabilityAPI, numControllers int, cons constraints.Value, series string, placement []string,
+	c *gc.C,
+	haServer *highavailability.HighAvailabilityAPI,
+	numControllers int,
+	cons constraints.Value,
+	series string,
+	placement []string,
 ) (params.ControllersChanges, error) {
 	arg := params.ControllersSpecs{
 		Specs: []params.ControllersSpec{{
@@ -206,6 +208,33 @@ func (s *clientSuite) TestEnableHAEmptyConstraints(c *gc.C) {
 	}
 }
 
+func (s *clientSuite) TestEnableHAControllerConfigConstraints(c *gc.C) {
+	controllerSettings, _ := s.State.ReadSettings("controllers", "controllerSettings")
+	controllerSettings.Set(controller.JujuHASpace, "ha-space")
+	controllerSettings.Write()
+
+	enableHAResult, err := s.enableHA(c, 3, constraints.MustParse("spaces=random-space"), defaultSeries, nil)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(enableHAResult.Maintained, gc.DeepEquals, []string{"machine-0"})
+	c.Assert(enableHAResult.Added, gc.DeepEquals, []string{"machine-1", "machine-2"})
+	c.Assert(enableHAResult.Removed, gc.HasLen, 0)
+	c.Assert(enableHAResult.Converted, gc.HasLen, 0)
+
+	machines, err := s.State.AllMachines()
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(machines, gc.HasLen, 3)
+	expectedCons := []constraints.Value{
+		controllerCons,
+		constraints.MustParse("spaces=random-space,ha-space"),
+		constraints.MustParse("spaces=random-space,ha-space"),
+	}
+	for i, m := range machines {
+		cons, err := m.Constraints()
+		c.Assert(err, jc.ErrorIsNil)
+		c.Check(cons, gc.DeepEquals, expectedCons[i])
+	}
+}
+
 func (s *clientSuite) TestBlockMakeHA(c *gc.C) {
 	// Block all changes.
 	s.BlockAllChanges(c, "TestBlockEnableHA")

diff --git a/cmd/juju/commands/bootstrap.go b/cmd/juju/commands/bootstrap.go
@@ -485,9 +485,11 @@ func (c *bootstrapCommand) Run(ctx *cmd.Context) (resultErr error) {
 	}
 
 	// Set the current model to the initial hosted model.
-	if err := store.UpdateModel(c.controllerName, c.hostedModelName, jujuclient.ModelDetails{
-		hostedModelUUID.String(),
-	}); err != nil {
+	if err := store.UpdateModel(
+		c.controllerName,
+		c.hostedModelName,
+		jujuclient.ModelDetails{hostedModelUUID.String()},
+	); err != nil {
 		return errors.Trace(err)
 	}
 
@@ -545,7 +547,7 @@ See `[1:] + "`juju kill-controller`" + `.`)
 	ctx.InterruptNotify(interrupted)
 	defer ctx.StopInterruptNotify(interrupted)
 	go func() {
-		for _ = range interrupted {
+		for range interrupted {
 			ctx.Infof("Interrupt signalled: waiting for bootstrap to exit")
 		}
 	}()
@@ -557,14 +559,20 @@ See `[1:] + "`juju kill-controller`" + `.`)
 		metadataDir = ctx.AbsPath(c.MetadataSource)
 	}
 
-	// Merge environ and bootstrap-specific constraints.
 	constraintsValidator, err := environ.ConstraintsValidator()
 	if err != nil {
 		return errors.Trace(err)
 	}
-	bootstrapConstraints, err := constraintsValidator.Merge(
-		c.Constraints, c.BootstrapConstraints,
-	)
+
+	// Merge in any space constraints that should be implied from controller
+	// space config.
+	// Do it before calling merge, because the constraints will be validated
+	// there.
+	constraints := c.Constraints
+	constraints.Spaces = config.controller.AsSpaceConstraints(constraints.Spaces)
+
+	// Merge environ and bootstrap-specific constraints.
+	bootstrapConstraints, err := constraintsValidator.Merge(constraints, c.BootstrapConstraints)
 	if err != nil {
 		return errors.Trace(err)
 	}
@@ -1118,7 +1126,7 @@ func handleBootstrapError(ctx *cmd.Context, cleanup func() error) {
 	defer ctx.StopInterruptNotify(ch)
 	defer close(ch)
 	go func() {
-		for _ = range ch {
+		for range ch {
 			fmt.Fprintln(ctx.GetStderr(), "Cleaning up failed bootstrap")
 		}
 	}()

diff --git a/cmd/juju/commands/bootstrap_test.go b/cmd/juju/commands/bootstrap_test.go
@@ -572,6 +572,25 @@ func (s *BootstrapSuite) TestBootstrapTimeout(c *gc.C) {
 	c.Assert(bootstrap.args.DialOpts.Timeout, gc.Equals, 99*time.Second)
 }
 
+func (s *BootstrapSuite) TestBootstrapAllSpacesAsConstraintsMerged(c *gc.C) {
+	s.patchVersionAndSeries(c, "raring")
+
+	var bootstrap fakeBootstrapFuncs
+	s.PatchValue(&getBootstrapFuncs, func() BootstrapInterface {
+		return &bootstrap
+	})
+	cmdtesting.RunCommand(
+		c, s.newBootstrapCommand(), "dummy", "devcontroller", "--auto-upgrade",
+		"--config", "juju-ha-space=ha-space", "--config", "juju-mgmt-space=management-space",
+		"--constraints", "spaces=ha-space,random-space",
+	)
+
+	// Order is unimportant
+	got := *(bootstrap.args.BootstrapConstraints.Spaces)
+	sort.Strings(got)
+	c.Check(got, gc.DeepEquals, []string{"ha-space", "management-space", "random-space"})
+}
+
 func (s *BootstrapSuite) TestBootstrapDefaultConfigStripsProcessedAttributes(c *gc.C) {
 	s.patchVersionAndSeries(c, "raring")
 
@@ -2031,7 +2050,7 @@ func (noCloudRegionsProvider) DetectRegions() ([]cloud.Region, error) {
 }
 
 func (noCloudRegionsProvider) CredentialSchemas() map[cloud.AuthType]cloud.CredentialSchema {
-	return map[cloud.AuthType]cloud.CredentialSchema{cloud.EmptyAuthType: cloud.CredentialSchema{}}
+	return map[cloud.AuthType]cloud.CredentialSchema{cloud.EmptyAuthType: {}}
 }
 
 type noCredentialsProvider struct {

diff --git a/controller/config.go b/controller/config.go
@@ -514,11 +514,11 @@ func Validate(c Config) error {
 		}
 	}
 
-	if err := validateSpaceConfig(c, JujuHASpace, "juju HA"); err != nil {
+	if err := c.validateSpaceConfig(JujuHASpace, "juju HA"); err != nil {
 		return errors.Trace(err)
 	}
 
-	if err := validateSpaceConfig(c, JujuManagementSpace, "juju mgmt"); err != nil {
+	if err := c.validateSpaceConfig(JujuManagementSpace, "juju mgmt"); err != nil {
 		return errors.Trace(err)
 	}
 
@@ -559,7 +559,7 @@ func Validate(c Config) error {
 	return nil
 }
 
-func validateSpaceConfig(c Config, key, topic string) error {
+func (c Config) validateSpaceConfig(key, topic string) error {
 	val := c[key]
 	if val == nil {
 		return nil
@@ -575,6 +575,35 @@ func validateSpaceConfig(c Config, key, topic string) error {
 	return nil
 }
 
+// AsSpaceConstraints checks to see whether config has spaces names populated
+// for management and/or HA (Mongo).
+// Non-empty values are merged with any input spaces and returned as a new
+// slice reference.
+// A slice pointer is used for congruence with the Spaces member in
+// constraints.Value.
+func (c Config) AsSpaceConstraints(spaces *[]string) *[]string {
+	newSpaces := set.NewStrings()
+	if spaces != nil {
+		for _, s := range *spaces {
+			newSpaces.Add(s)
+		}
+	}
+
+	for _, c := range []string{c.JujuManagementSpace(), c.JujuHASpace()} {
+		if c != "" {
+			newSpaces.Add(c)
+		}
+	}
+
+	// Preserve a nil pointer if there is no change. This conveys information
+	// in constraints.Value (not set vs. deliberately set as empty).
+	if spaces == nil && len(newSpaces) == 0 {
+		return nil
+	}
+	ns := newSpaces.Values()
+	return &ns
+}
+
 // GenerateControllerCertAndKey makes sure that the config has a CACert and
 // CAPrivateKey, generates and returns new certificate and key.
 func GenerateControllerCertAndKey(caCert, caKey string, hostAddresses []string) (string, string, error) {

diff --git a/controller/config_test.go b/controller/config_test.go
@@ -4,6 +4,7 @@
 package controller_test
 
 import (
+	"sort"
 	stdtesting "testing"
 	"time"
 
@@ -320,3 +321,55 @@ func (s *ConfigSuite) TestAuditLogFloatBackupsLoadedDirectly(c *gc.C) {
 	}
 	c.Assert(cfg.AuditLogMaxBackups(), gc.Equals, 10)
 }
+
+func (s *ConfigSuite) TestConfigManagementSpaceAsConstraint(c *gc.C) {
+	managementSpace := "management-space"
+	cfg, err := controller.NewConfig(
+		testing.ControllerTag.Id(),
+		testing.CACert,
+		map[string]interface{}{controller.JujuHASpace: managementSpace},
+	)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Check(*cfg.AsSpaceConstraints(nil), gc.DeepEquals, []string{managementSpace})
+}
+
+func (s *ConfigSuite) TestConfigHASpaceAsConstraint(c *gc.C) {
+	haSpace := "ha-space"
+	cfg, err := controller.NewConfig(
+		testing.ControllerTag.Id(),
+		testing.CACert,
+		map[string]interface{}{controller.JujuHASpace: haSpace},
+	)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Check(*cfg.AsSpaceConstraints(nil), gc.DeepEquals, []string{haSpace})
+}
+
+func (s *ConfigSuite) TestConfigAllSpacesAsMergedConstraints(c *gc.C) {
+	haSpace := "ha-space"
+	managementSpace := "management-space"
+	constraintSpace := "constraint-space"
+
+	cfg, err := controller.NewConfig(
+		testing.ControllerTag.Id(),
+		testing.CACert,
+		map[string]interface{}{
+			controller.JujuHASpace:         haSpace,
+			controller.JujuManagementSpace: managementSpace,
+		},
+	)
+	c.Assert(err, jc.ErrorIsNil)
+
+	got := *cfg.AsSpaceConstraints(&[]string{constraintSpace})
+	sort.Strings(got)
+	c.Check(got, gc.DeepEquals, []string{constraintSpace, haSpace, managementSpace})
+}
+
+func (s *ConfigSuite) TestConfigNoSpacesNilSpaceConfigPreserved(c *gc.C) {
+	cfg, err := controller.NewConfig(
+		testing.ControllerTag.Id(),
+		testing.CACert,
+		map[string]interface{}{},
+	)
+	c.Assert(err, jc.ErrorIsNil)
+	c.Check(cfg.AsSpaceConstraints(nil), gc.IsNil)
+}