Reviewers: mp+172715_code.launchpad.net,

Message:
Please take a look.

Description:
Make a common verification function for bootstrap

All the providers did the same pre-bootstrap verification
(or should have in the case of maas).  As discussed in Oakland,
I pulled the common code out into a shared function.

https://code.launchpad.net/~thumper/juju-core/consolidate-bootstrap-initial-check/+merge/172715

Requires:
https://code.launchpad.net/~thumper/juju-core/consolidate-bootstrap-state/+merge/172701

(do not edit description out of merge proposal)


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

Affected files:
   A [revision details]
   M environs/bootstrap.go
   M environs/dummy/environs.go
   M environs/ec2/ec2.go
   M environs/maas/environ.go
   M environs/openstack/provider.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: environs/bootstrap.go
=== modified file 'environs/bootstrap.go'
--- environs/bootstrap.go	2013-07-02 23:28:43 +0000
+++ environs/bootstrap.go	2013-07-03 01:16:51 +0000
@@ -7,6 +7,8 @@
  	"fmt"

  	"launchpad.net/juju-core/constraints"
+	"launchpad.net/juju-core/errors"
+	"launchpad.net/juju-core/utils"
  )

  // Bootstrap bootstraps the given environment. The supplied constraints are
@@ -33,3 +35,32 @@
  	}
  	return environ.Bootstrap(cons)
  }
+
+// VerifyBootstrapInit does the common initial check inside bootstrap to
+// confirm that the environment isn't already running, and that the storage
+// works.
+func VerifyBootstrapInit(env Environ, shortAttempt utils.AttemptStrategy)  
error {
+	var err error
+
+	// If the state file exists, it might actually have just been
+	// removed by Destroy, and eventual consistency has not caught
+	// up yet, so we retry to verify if that is happening.
+	for a := shortAttempt.Start(); a.Next(); {
+		_, err = LoadProviderState(env.Storage())
+		if err != nil {
+			break
+		}
+	}
+	if err == nil {
+		return fmt.Errorf("environment is already bootstrapped")
+	}
+	if !errors.IsNotFoundError(err) {
+		return fmt.Errorf("cannot query old bootstrap state: %v", err)
+	}
+
+	err = VerifyStorage(env.Storage())
+	if err != nil {
+		return err
+	}
+	return nil
+}


Index: environs/dummy/environs.go
=== modified file 'environs/dummy/environs.go'
--- environs/dummy/environs.go	2013-06-30 23:37:23 +0000
+++ environs/dummy/environs.go	2013-07-03 01:23:46 +0000
@@ -433,7 +433,12 @@
  	if _, ok := e.Config().CACert(); !ok {
  		return fmt.Errorf("no CA certificate in environment configuration")
  	}
-	if err := environs.VerifyStorage(e.Storage()); err != nil {
+
+	var shortAttempt = utils.AttemptStrategy{
+		Total: 200 * time.Millisecond,
+		Delay: 10 * time.Millisecond,
+	}
+	if err := environs.VerifyBootstrapInit(e, shortAttempt); err != nil {
  		return err
  	}



Index: environs/ec2/ec2.go
=== modified file 'environs/ec2/ec2.go'
--- environs/ec2/ec2.go	2013-07-02 23:15:14 +0000
+++ environs/ec2/ec2.go	2013-07-03 01:16:51 +0000
@@ -16,7 +16,6 @@
  	"launchpad.net/juju-core/environs/imagemetadata"
  	"launchpad.net/juju-core/environs/instances"
  	"launchpad.net/juju-core/environs/tools"
-	"launchpad.net/juju-core/errors"
  	"launchpad.net/juju-core/instance"
  	"launchpad.net/juju-core/log"
  	"launchpad.net/juju-core/state"
@@ -255,25 +254,9 @@
  	// If the state file exists, it might actually have just been
  	// removed by Destroy, and eventual consistency has not caught
  	// up yet, so we retry to verify if that is happening.
-	var err error
-	for a := shortAttempt.Start(); a.Next(); {
-		_, err = environs.LoadProviderState(e.Storage())
-		if err != nil {
-			break
-		}
-	}
-	if err == nil {
-		return fmt.Errorf("environment is already bootstrapped")
-	}
-	if !errors.IsNotFoundError(err) {
-		return fmt.Errorf("cannot query old bootstrap state: %v", err)
-	}
-
-	err = environs.VerifyStorage(e.Storage())
-	if err != nil {
+	if err := environs.VerifyBootstrapInit(e, shortAttempt); err != nil {
  		return err
  	}
-
  	possibleTools, err := environs.FindBootstrapTools(e, cons)
  	if err != nil {
  		return err


Index: environs/maas/environ.go
=== modified file 'environs/maas/environ.go'
--- environs/maas/environ.go	2013-07-02 23:15:14 +0000
+++ environs/maas/environ.go	2013-07-03 01:16:51 +0000
@@ -28,6 +28,16 @@
  	apiVersion = "1.0"
  )

+// A request may fail to due "eventual consistency" semantics, which
+// should resolve fairly quickly.  A request may also fail due to a slow
+// state transition (for instance an instance taking a while to release
+// a security group after termination).  The former failure mode is
+// dealt with by shortAttempt, the latter by longAttempt.
+var shortAttempt = utils.AttemptStrategy{
+	Total: 5 * time.Second,
+	Delay: 200 * time.Millisecond,
+}
+
  var longAttempt = utils.AttemptStrategy{
  	Total: 3 * time.Minute,
  	Delay: 1 * time.Second,
@@ -100,10 +110,9 @@

  // Bootstrap is specified in the Environ interface.
  func (env *maasEnviron) Bootstrap(cons constraints.Value) error {
-	// TODO(fwereade): this should check for an existing environment before
-	// starting a new one -- even given raciness, it's better than nothing.
-	if err := environs.VerifyStorage(env.Storage()); err != nil {
-		return fmt.Errorf("provider storage is not writeable")
+
+	if err := environs.VerifyBootstrapInit(env, shortAttempt); err != nil {
+		return err
  	}

  	inst, err := env.startBootstrapNode(cons)


Index: environs/openstack/provider.go
=== modified file 'environs/openstack/provider.go'
--- environs/openstack/provider.go	2013-07-02 23:15:14 +0000
+++ environs/openstack/provider.go	2013-07-03 01:16:51 +0000
@@ -22,7 +22,6 @@
  	"launchpad.net/juju-core/environs/imagemetadata"
  	"launchpad.net/juju-core/environs/instances"
  	"launchpad.net/juju-core/environs/tools"
-	coreerrors "launchpad.net/juju-core/errors"
  	"launchpad.net/juju-core/instance"
  	"launchpad.net/juju-core/log"
  	"launchpad.net/juju-core/state"
@@ -481,24 +480,8 @@

  func (e *environ) Bootstrap(cons constraints.Value) error {
  	log.Infof("environs/openstack: bootstrapping environment %q", e.name)
-	// If the state file exists, it might actually have just been
-	// removed by Destroy, and eventual consistency has not caught
-	// up yet, so we retry to verify if that is happening.
-	var err error
-	for a := shortAttempt.Start(); a.Next(); {
-		_, err = environs.LoadProviderState(e.Storage())
-		if err != nil {
-			break
-		}
-	}
-	if err == nil {
-		return fmt.Errorf("environment is already bootstrapped")
-	}
-	if !coreerrors.IsNotFoundError(err) {
-		return fmt.Errorf("cannot query old bootstrap state: %v", err)
-	}
-	err = environs.VerifyStorage(e.Storage())
-	if err != nil {
+
+	if err := environs.VerifyBootstrapInit(e, shortAttempt); err != nil {
  		return err
  	}