Move env checks to apiserver

apiserver/client/client.go

@@ -17,6 +17,7 @@ import (
 	"github.com/juju/juju/apiserver/highavailability"
 	"github.com/juju/juju/apiserver/params"
 	"github.com/juju/juju/apiserver/service"
+	"github.com/juju/juju/environs"
 	"github.com/juju/juju/environs/config"
 	"github.com/juju/juju/environs/manual"
 	"github.com/juju/juju/instance"
@@ -841,9 +842,30 @@ func (c *Client) SetEnvironAgentVersion(args params.SetEnvironAgentVersion) erro
 	if err := c.check.ChangeAllowed(); err != nil {
 		return errors.Trace(err)
 	}
+	// Before changing the agent version to trigger an upgrade or downgrade,
+	// we'll do a very basic check to ensure the
+	cfg, err := c.api.stateAccessor.EnvironConfig()
+	if err != nil {
+		return errors.Trace(err)
+	}
+	env, err := getEnvironment(cfg)
+	if err != nil {
+		return errors.Trace(err)
+	}
+	if err := environs.CheckProviderAPI(env); err != nil {
+		return err
+	}
 	return c.api.stateAccessor.SetEnvironAgentVersion(args.Version)
 }
 
+var getEnvironment = func(cfg *config.Config) (environs.Environ, error) {
+	env, err := environs.New(cfg)
+	if err != nil {
+		return nil, err
+	}
+	return env, nil
+}
+
 // AbortCurrentUpgrade aborts and archives the current upgrade
 // synchronisation record, if any.
 func (c *Client) AbortCurrentUpgrade() error {

apiserver/client/client_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/juju/juju/apiserver/testing"
 	apiservertesting "github.com/juju/juju/apiserver/testing"
 	"github.com/juju/juju/constraints"
+	"github.com/juju/juju/environs"
 	"github.com/juju/juju/environs/config"
 	"github.com/juju/juju/environs/manual"
 	toolstesting "github.com/juju/juju/environs/tools/testing"
@@ -446,6 +447,44 @@ func (s *serverSuite) TestSetEnvironAgentVersion(c *gc.C) {
 	c.Assert(agentVersion, gc.Equals, "9.8.7")
 }
 
+type mockEnviron struct {
+	environs.Environ
+	allInstancesCalled bool
+	err                error
+}
+
+func (m *mockEnviron) AllInstances() ([]instance.Instance, error) {
+	m.allInstancesCalled = true
+	return nil, m.err
+}
+
+func (s *serverSuite) assertCheckProviderAPI(c *gc.C, envError error, expectErr string) {
+	env := &mockEnviron{err: envError}
+	s.PatchValue(client.GetEnvironment, func(cfg *config.Config) (environs.Environ, error) {
+		return env, nil
+	})
+	args := params.SetEnvironAgentVersion{
+		Version: version.MustParse("9.8.7"),
+	}
+	err := s.client.SetEnvironAgentVersion(args)
+	c.Assert(env.allInstancesCalled, jc.IsTrue)
+	if expectErr != "" {
+		c.Assert(err, gc.ErrorMatches, expectErr)
+	} else {
+		c.Assert(err, jc.ErrorIsNil)
+	}
+}
+
+func (s *serverSuite) TestCheckProviderAPISuccess(c *gc.C) {
+	s.assertCheckProviderAPI(c, nil, "")
+	s.assertCheckProviderAPI(c, environs.ErrPartialInstances, "")
+	s.assertCheckProviderAPI(c, environs.ErrNoInstances, "")
+}
+
+func (s *serverSuite) TestCheckProviderAPIFail(c *gc.C) {
+	s.assertCheckProviderAPI(c, fmt.Errorf("instances error"), "cannot make API call to provider: instances error")
+}
+
 func (s *serverSuite) assertSetEnvironAgentVersion(c *gc.C) {
 	args := params.SetEnvironAgentVersion{
 		Version: version.MustParse("9.8.7"),

apiserver/client/export_test.go

@@ -24,7 +24,10 @@ var (
 
 type MachineAndContainers machineAndContainers
 
-var StartSerialWaitParallel = startSerialWaitParallel
+var (
+	StartSerialWaitParallel = startSerialWaitParallel
+	GetEnvironment          = &getEnvironment
+)
 
 type StateInterface stateInterface
 

environs/utils.go

@@ -113,3 +113,16 @@ func APIInfo(env Environ) (*api.Info, error) {
 	apiInfo := &api.Info{Addrs: apiAddrs, CACert: cert, EnvironTag: envTag}
 	return apiInfo, nil
 }
+
+// CheckProviderAPI returns an error if a simple API call
+// to check a basic response from the specified environ fails.
+func CheckProviderAPI(env Environ) error {
+	// We will make a simple API call to the provider
+	// to ensure the underlying substrate is ok.
+	_, err := env.AllInstances()
+	switch err {
+	case nil, ErrPartialInstances, ErrNoInstances:
+		return nil
+	}
+	return errors.Annotate(err, "cannot make API call to provider")
+}

upgrades/export_test.go

@@ -18,7 +18,6 @@ var (
 	StateToolsStorage         = &stateToolsStorage
 	AddAZToInstData           = &addAZToInstData
 	MinDiskSpaceGib           = &minDiskSpaceGib
-	GetEnvironment            = &getEnvironment
 
 	ChownPath       = &chownPath
 	IsLocalEnviron  = &isLocalEnviron

upgrades/preupgradesteps.go

@@ -9,7 +9,6 @@ import (
 	"github.com/juju/utils/du"
 
 	"github.com/juju/juju/agent"
-	"github.com/juju/juju/environs"
 	"github.com/juju/juju/state"
 )
 
@@ -19,11 +18,6 @@ func PreUpgradeSteps(st *state.State, agentConf agent.Config, isMaster bool) err
 	if err := checkDiskSpace(agentConf.DataDir()); err != nil {
 		return err
 	}
-	if isMaster {
-		if err := checkProviderAPI(st); err != nil {
-			return err
-		}
-	}
 	return nil
 }
 
@@ -34,34 +28,8 @@ func checkDiskSpace(dir string) error {
 	usage := du.NewDiskUsage(dir)
 	free := usage.Free()
 	if free < uint64(minDiskSpaceGib*humanize.GiByte) {
-		return errors.Errorf("not enough free disk space for upgrade %dGiB available, require %dGiB",
-			free/humanize.GiByte, minDiskSpaceGib)
-	}
-	return nil
-}
-
-func checkProviderAPI(st *state.State) error {
-	// We will make a simple API call to the provider
-	// to ensure the underlying substrate is ok.
-	env, err := getEnvironment(st)
-	if err != nil {
-		return errors.Trace(err)
-	}
-	_, err = env.AllInstances()
-	if err != nil {
-		return errors.Annotate(err, "cannot make API call to provider")
+		return errors.Errorf("not enough free disk space for upgrade: %s available, require %dGiB",
+			humanize.IBytes(free), minDiskSpaceGib)
 	}
 	return nil
 }
-
-var getEnvironment = func(st *state.State) (environs.Environ, error) {
-	cfg, err := st.EnvironConfig()
-	if err != nil {
-		return nil, err
-	}
-	env, err := environs.New(cfg)
-	if err != nil {
-		return nil, err
-	}
-	return env, nil
-}

upgrades/preupgradesteps_test.go

@@ -5,13 +5,8 @@ package upgrades_test
 
 import (
 	"github.com/dustin/go-humanize"
-	"github.com/juju/errors"
-	jc "github.com/juju/testing/checkers"
 	gc "gopkg.in/check.v1"
 
-	"github.com/juju/juju/environs"
-	"github.com/juju/juju/instance"
-	"github.com/juju/juju/state"
 	"github.com/juju/juju/testing"
 	"github.com/juju/juju/upgrades"
 )
@@ -28,43 +23,3 @@ func (s *preupgradechecksSuite) TestCheckFreeDiskSpace(c *gc.C) {
 	err := upgrades.PreUpgradeSteps(nil, &mockAgentConfig{dataDir: "/"}, false)
 	c.Assert(err, gc.ErrorMatches, "not enough free disk space for upgrade .*")
 }
-
-type mockEnviron struct {
-	environs.Environ
-	allInstancesCalled bool
-}
-
-func (m *mockEnviron) AllInstances() ([]instance.Instance, error) {
-	m.allInstancesCalled = true
-	return nil, errors.New("instances error")
-}
-
-func (s *preupgradechecksSuite) TestCheckProviderAPI(c *gc.C) {
-	// We don't want to fail on disk space in this test.
-	s.PatchValue(upgrades.MinDiskSpaceGib, 0)
-
-	env := &mockEnviron{}
-	var callSt *state.State
-	s.PatchValue(upgrades.GetEnvironment, func(st *state.State) (environs.Environ, error) {
-		c.Assert(st, gc.Equals, callSt)
-		return env, nil
-	})
-	err := upgrades.PreUpgradeSteps(callSt, &mockAgentConfig{dataDir: "/"}, true)
-	c.Assert(err, gc.ErrorMatches, "cannot make API call to provider: instances error")
-	c.Assert(env.allInstancesCalled, jc.IsTrue)
-}
-
-func (s *preupgradechecksSuite) TestCheckProviderAPINotMaster(c *gc.C) {
-	// We don't want to fail on disk space in this test.
-	s.PatchValue(upgrades.MinDiskSpaceGib, 0)
-
-	env := &mockEnviron{}
-	var callSt *state.State
-	s.PatchValue(upgrades.GetEnvironment, func(st *state.State) (environs.Environ, error) {
-		c.Assert(st, gc.Equals, callSt)
-		return env, nil
-	})
-	err := upgrades.PreUpgradeSteps(callSt, &mockAgentConfig{dataDir: "/"}, false)
-	c.Assert(err, jc.ErrorIsNil)
-	c.Assert(env.allInstancesCalled, jc.IsFalse)
-}

worker/upgradesteps/worker.go

@@ -255,7 +255,7 @@ func (w *upgradesteps) runUpgrades() error {
 func (w *upgradesteps) prepareForUpgrade() (*state.UpgradeInfo, error) {
 	logger.Infof("checking that upgrade can proceed")
 	if err := w.preUpgradeSteps(w.st, w.agent.CurrentConfig(), w.isMaster); err != nil {
-		return nil, errors.Annotatef(err, "machine %q cannot be upgraded", w.tag)
+		return nil, errors.Annotatef(err, "%s cannot be upgraded", names.ReadableString(w.tag))
 	}
 
 	if !w.isStateServer {

worker/upgradesteps/worker_test.go

@@ -375,16 +375,15 @@ func (s *UpgradeSuite) TestJobsToTargets(c *gc.C) {
 
 func (s *UpgradeSuite) TestPreUpgradeFail(c *gc.C) {
 	s.preUpgradeError = true
-	defer s.captureLogs(c)
+	s.captureLogs(c)
 
 	workerErr, config, statusCalls, doneCh := s.runUpgradeWorker(c, multiwatcher.JobHostUnits)
 
 	c.Check(workerErr, jc.ErrorIsNil)
 	c.Check(config.Version, gc.Equals, s.oldVersion.Number) // Upgrade didn't finish
 	assertUpgradeNotComplete(c, doneCh)
-	assertUpgradeNotComplete(c, doneCh)
 
-	causeMessage := `machine "machine-0" cannot be upgraded: preupgrade error`
+	causeMessage := `machine 0 cannot be upgraded: preupgrade error`
 	failMessage := fmt.Sprintf(
 		`upgrade from %s to %s for "machine-0" failed \(giving up\): %s`,
 		s.oldVersion.Number, version.Current, causeMessage)
@@ -394,7 +393,7 @@ func (s *UpgradeSuite) TestPreUpgradeFail(c *gc.C) {
 	})
 
 	statusMessage := fmt.Sprintf(
-		`upgrade to %s failed \(giving up\): %s`, version.Current, causeMessage)
+		`upgrade to %s failed (giving up): %s`, version.Current, causeMessage)
 	c.Assert(statusCalls, jc.DeepEquals, []StatusCall{{
 		params.StatusError, statusMessage,
 	}})