Code review comment for lp:~thumper/juju-core/consolidate-bootstrap-initial-check

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
   }