Merge lp:~thumper/juju-core/consolidate-bootstrap-initial-check into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Make a common verification function for bootstrap

https://codereview.appspot.com/10888044/

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

[Tim Penhey (thumper)]

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

[Jeroen T. Vermeulen (jtv)]

An historical note on why MAAS skips the verification step. When we were implementing Bootstrap() in the MAAS provider, we were told that this verification step was needed specifically to deal with lazily-consistent storage — and since MAAS storage is immediately consistent, it wasn't needed in that case.

So that's why we did not duplicate this bit of code in the MAAS provider. Replacing the duplicated code with a generic function that applies to all cases makes it a whole different ballgame, of course.

[John A Meinel (jameinel)]

If we are not 'lazily consistent' then we can probably drop shortAttemptStrategy down to a max of 1s or something like that.

This code LGTM, though it depends on your earlier work which means sorting out the syntax for that.

review: Approve

=== modified file 'environs/bootstrap.go'
--- environs/bootstrap.go	2013-07-04 23:08:28 +0000
+++ environs/bootstrap.go	2013-07-04 23:08:28 +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,27 @@
 	}
 	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(); {
+		if _, err = LoadState(env.Storage()); 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)
+	}
+
+	return VerifyStorage(env.Storage())
+}

=== modified file 'environs/dummy/environs.go'
--- environs/dummy/environs.go	2013-07-02 21:52:48 +0000
+++ environs/dummy/environs.go	2013-07-04 23:08:28 +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
 	}
 

=== modified file 'environs/ec2/ec2.go'
--- environs/ec2/ec2.go	2013-07-02 21:52:48 +0000
+++ environs/ec2/ec2.go	2013-07-04 23:08:28 +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"
@@ -240,25 +239,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.LoadState(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

=== modified file 'environs/maas/environ.go'
--- environs/maas/environ.go	2013-07-02 21:52:48 +0000
+++ environs/maas/environ.go	2013-07-04 23:08:28 +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)

=== modified file 'environs/openstack/provider.go'
--- environs/openstack/provider.go	2013-07-02 21:52:48 +0000
+++ environs/openstack/provider.go	2013-07-04 23:08:28 +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"
@@ -466,24 +465,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.LoadState(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
 	}