launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/juju-core/mpv-fwreade-4 into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

Commit message:
fwreade's review comment #4 on the MAAS provider feature branch: quiesceStateFile is not needed for MAAS.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/juju-core/mpv-fwreade-4/+merge/158283

This is for merging into the MAAS provider feature branch.  It addresses William's comment:

«4) quiesceStateFile [FIX if not necessary]

«Only necessary if maas storage is not consistent.»

The MAAS storage implementation is consistent, so quiesceStateFile (which was cargo-culted from a portion of the EC2 provider's implementation of Bootstrap()) does not appear to be needed.  There was a comment mentioning consistency, but it wasn't clear about what kind of eventual consistency was meant.  If Juju will not issue create a new environment before a previous Destroy() has completed, then there is no problem.

(This does show the value in splitting this kind of functionality out into separate functions — it'd have been harder to identify and excise as part of a large amorphous Bootstrap() method!)


Jeroen
-- 
https://code.launchpad.net/~jtv/juju-core/mpv-fwreade-4/+merge/158283
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/juju-core/mpv-fwreade-4 into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

=== modified file 'environs/maas/environ.go'
--- environs/maas/environ.go	2013-04-11 04:42:00 +0000
+++ environs/maas/environ.go	2013-04-11 07:01:20 +0000
@@ -68,35 +68,6 @@
 	return env.name
 }
 
-// quiesceStateFile waits (up to a few seconds) for any existing state file to
-// disappear.
-//
-// This is used when bootstrapping, to deal with any previous state file that
-// may have been removed by a Destroy that hasn't reached its eventual
-// consistent state yet.
-func (env *maasEnviron) quiesceStateFile() error {
-	// This was all cargo-culted off the EC2 provider.
-	var err error
-	retry := trivial.AttemptStrategy{
-		Total: 5 * time.Second,
-		Delay: 200 * time.Millisecond,
-	}
-	for a := retry.Start(); err == nil && a.Next(); {
-		_, err = env.loadState()
-	}
-	if err == nil {
-		// The state file outlived the timeout.  Looks like it wasn't
-		// being destroyed after all.
-		return fmt.Errorf("environment is already bootstrapped")
-	}
-	if _, notFound := err.(*environs.NotFoundError); !notFound {
-		return fmt.Errorf("cannot query old bootstrap state: %v", err)
-	}
-	// Got to this point?  Then the error was "not found," which is the
-	// state we're looking for.
-	return nil
-}
-
 // TODO: this code is cargo-culted from the openstack/ec2 providers.
 func (env *maasEnviron) findTools() (*state.Tools, error) {
 	flags := environs.HighestVersion | environs.CompatVersion
@@ -178,12 +149,7 @@
 		return fmt.Errorf("admin-secret is required for bootstrap")
 	}
 	log.Debugf("environs/maas: bootstrapping environment %q.", env.Name())
-	err := env.quiesceStateFile()
-	if err != nil {
-		return err
-	}
-	var tools *state.Tools
-	tools, err = env.findTools()
+	tools, err := env.findTools()
 	if err != nil {
 		return err
 	}

=== modified file 'environs/maas/environ_test.go'
--- environs/maas/environ_test.go	2013-04-11 05:45:26 +0000
+++ environs/maas/environ_test.go	2013-04-11 07:01:20 +0000
@@ -1,7 +1,6 @@
 package maas
 
 import (
-	"bytes"
 	"fmt"
 	. "launchpad.net/gocheck"
 	"launchpad.net/gomaasapi"
@@ -256,21 +255,6 @@
 	c.Check(operations, DeepEquals, expectedOperations)
 }
 
-func (suite *EnvironSuite) TestQuiesceStateFileIsHappyWithoutStateFile(c *C) {
-	err := suite.makeEnviron().quiesceStateFile()
-	c.Check(err, IsNil)
-}
-
-func (suite *EnvironSuite) TestQuiesceStateFileFailsWithStateFile(c *C) {
-	env := suite.makeEnviron()
-	err := env.saveState(&bootstrapState{})
-	c.Assert(err, IsNil)
-
-	err = env.quiesceStateFile()
-
-	c.Check(err, Not(IsNil))
-}
-
 func (suite *EnvironSuite) TestStateInfo(c *C) {
 	env := suite.makeEnviron()
 	hostname := "test"
@@ -295,18 +279,6 @@
 	c.Check(err, FitsTypeOf, &environs.NotFoundError{})
 }
 
-func (suite *EnvironSuite) TestQuiesceStateFileFailsOnBrokenStateFile(c *C) {
-	const content = "@#$(*&Y%!"
-	reader := bytes.NewReader([]byte(content))
-	env := suite.makeEnviron()
-	err := env.Storage().Put(stateFile, reader, int64(len(content)))
-	c.Assert(err, IsNil)
-
-	err = env.quiesceStateFile()
-
-	c.Check(err, Not(IsNil))
-}
-
 func (suite *EnvironSuite) TestDestroy(c *C) {
 	env := suite.makeEnviron()
 	suite.getInstance("test1")