Merge lp:~thumper/juju-core/verify-storage-move into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Fail earlier when bootstrapping if up already.

I broke apart the VerifyBootstrap method into two:
  EnsureNotBootstrapped
  VerifyStorage
Well I didn't create the second, just didn't call it
from the EnsureNotBootstrapped call.

The ensure method is now called in the command bootstrap
prior to any upload tools checks.

However the implication of this was much futher reaching
than I expected. Many tests assumed that bootstrap.Bootstrap
was enough to bootstrap and environment, and that it would
complain if already bootstrapped. Since the complaining was
moved into the command function, some changes had to be made.
Since some of the tests were refactored to check the storage,
this required making the dummy provider actually write the
provider-state into storage like a normal provider.

I moved the VerifyStorage out of emptystorage.go and into open.go
as it seemed better fitting there (and the comment actually
makes sense now).

Also used the environs constant for the VerificationFilename.

https://codereview.appspot.com/14279044/

Fail earlier when bootstrapping if up already.

I broke apart the VerifyBootstrap method into two:
  EnsureNotBootstrapped
  VerifyStorage
Well I didn't create the second, just didn't call it
from the EnsureNotBootstrapped call.

The ensure method is now called in the command bootstrap
prior to any upload tools checks.

However the implication of this was much futher reaching
than I expected. Many tests assumed that bootstrap.Bootstrap
was enough to bootstrap and environment, and that it would
complain if already bootstrapped. Since the complaining was
moved into the command function, some changes had to be made.
Since some of the tests were refactored to check the storage,
this required making the dummy provider actually write the
provider-state into storage like a normal provider.

I moved the VerifyStorage out of emptystorage.go and into open.go
as it seemed better fitting there (and the comment actually
makes sense now).

Also used the environs constant for the VerificationFilename.

https://codereview.appspot.com/14279044/

[Tim Penhey (thumper)]

Reviewers: mp+188974_code.launchpad.net,

Message:
Please take a look.

Description:
Fail earlier when bootstrapping if up already.

I broke apart the VerifyBootstrap method into two:
   EnsureNotBootstrapped
   VerifyStorage
Well I didn't create the second, just didn't call it
from the EnsureNotBootstrapped call.

The ensure method is now called in the command bootstrap
prior to any upload tools checks.

However the implication of this was much futher reaching
than I expected. Many tests assumed that bootstrap.Bootstrap
was enough to bootstrap and environment, and that it would
complain if already bootstrapped. Since the complaining was
moved into the command function, some changes had to be made.
Since some of the tests were refactored to check the storage,
this required making the dummy provider actually write the
provider-state into storage like a normal provider.

I moved the VerifyStorage out of emptystorage.go and into open.go
as it seemed better fitting there (and the comment actually
makes sense now).

Also used the environs constant for the VerificationFilename.

https://code.launchpad.net/~thumper/juju-core/verify-storage-move/+merge/188974

(do not edit description out of merge proposal)

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

Affected files (+119, -74 lines):
   A [revision details]
   M cmd/juju/bootstrap.go
   M cmd/juju/bootstrap_test.go
   M environs/bootstrap/bootstrap.go
   M environs/emptystorage.go
   M environs/emptystorage_test.go
   M environs/jujutest/livetests.go
   M environs/jujutest/tests.go
   M environs/open.go
   M environs/open_test.go
   M provider/dummy/environs.go

[Andrew Wilkins (axwalk)]

On 2013/10/03 03:19:01, dfc wrote:
> LGTM. Some nice boy scout work in there as well.

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go
> File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go#newcode92
> environs/bootstrap/bootstrap.go:92: _, err :=
common.LoadState(env.Storage())
> this method hurt my head, but it makes sense in a double negative sort
of way.

LGTM

https://codereview.appspot.com/14279044/

[Andrew Wilkins (axwalk)]

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (left):

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go#oldcode96
environs/bootstrap/bootstrap.go:96: // TODO(rog) this feels like a
layering violation - providers
Is this TODO no longer applicable? Seems to be...

https://codereview.appspot.com/14279044/

[Tim Penhey (thumper)]

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (left):

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go#oldcode96
environs/bootstrap/bootstrap.go:96: // TODO(rog) this feels like a
layering violation - providers
On 2013/10/03 03:21:00, axw1 wrote:
> Is this TODO no longer applicable? Seems to be...

I took it out because everything expects it to be there.
It is how bootstrap checks to see if the environment
is already bootstrapped. I think for consistency across
providers this comment no longer makes sense.

https://codereview.appspot.com/14279044/

[Roger Peppe (rogpeppe)]

I have reservations about this.
I don't think we should be further entrenching the arbitrary way in
which the ec2 provider happened to mark its server instances.
Some thoughts below.

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (left):

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go#oldcode96
environs/bootstrap/bootstrap.go:96: // TODO(rog) this feels like a
layering violation - providers
On 2013/10/03 03:38:01, thumper wrote:
> On 2013/10/03 03:21:00, axw1 wrote:
> > Is this TODO no longer applicable? Seems to be...

> I took it out because everything expects it to be there.
> It is how bootstrap checks to see if the environment
> is already bootstrapped. I think for consistency across
> providers this comment no longer makes sense.

The comment *is* still relevant IMHO. It has long been
the plan to move away from using file storage on ec2 to
indicate state servers (it could be done nicely with
instance tags) and therefore allow the possibility of
having an environment with no local storage at all.

It is *currently* how bootstrap checks to see if the
environment is already bootstrapped, but there's no
particular necessity for it to be this way.
It would be better to have an Environ.IsBootstrapped
call instead.

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go
File environs/bootstrap/bootstrap.go (right):

https://codereview.appspot.com/14279044/diff/1/environs/bootstrap/bootstrap.go#newcode91
environs/bootstrap/bootstrap.go:91: func EnsureNotBootstrapped(env
environs.Environ) error {
We usually use the word "ensure" to mean "ensure that something is so".
That's not the case here.

I'd prefer:

// IsBootstrapped returns whether the environment is
// bootstrapped.
func IsBootstrapped(env environs.Environ) (bool, error)

https://codereview.appspot.com/14279044/diff/1/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/14279044/diff/1/environs/open.go#newcode29
environs/open.go:29: VerifyStorageError error = fmt.Errorf(
s/ error//

https://codereview.appspot.com/14279044/

=== modified file 'cmd/juju/bootstrap.go'
--- cmd/juju/bootstrap.go	2013-10-02 05:18:02 +0000
+++ cmd/juju/bootstrap.go	2013-10-03 03:45:40 +0000
@@ -78,6 +78,12 @@
 			return fmt.Errorf("failed to enable bootstrap storage: %v", err)
 		}
 	}
+	// Check to see if the environment is already bootstrapped
+	// before potentially uploading any tools.
+	if err := bootstrap.EnsureNotBootstrapped(environ); err != nil {
+		return err
+	}
+
 	// TODO (wallyworld): 2013-09-20 bug 1227931
 	// We can set a custom tools data source instead of doing an
 	// unecessary upload.

=== modified file 'cmd/juju/bootstrap_test.go'
--- cmd/juju/bootstrap_test.go	2013-10-02 18:34:59 +0000
+++ cmd/juju/bootstrap_test.go	2013-10-03 03:45:40 +0000
@@ -53,6 +53,13 @@
 	s.LoggingSuite.TearDownSuite(c)
 }
 
+func (s *BootstrapSuite) TearDownTest(c *gc.C) {
+	s.ToolsFixture.TearDownTest(c)
+	s.MgoSuite.TearDownTest(c)
+	s.LoggingSuite.TearDownTest(c)
+	dummy.Reset()
+}
+
 type bootstrapRetryTest struct {
 	info               string
 	args               []string
@@ -101,13 +108,6 @@
 	c.Assert(err, gc.ErrorMatches, test.err)
 }
 
-func (s *BootstrapSuite) TearDownTest(c *gc.C) {
-	s.ToolsFixture.TearDownTest(c)
-	s.MgoSuite.TearDownTest(c)
-	s.LoggingSuite.TearDownTest(c)
-	dummy.Reset()
-}
-
 func (s *BootstrapSuite) TestTest(c *gc.C) {
 	uploadTools = mockUploadTools
 	defer func() { uploadTools = sync.Upload }()
@@ -190,7 +190,11 @@
 	}
 	opPutBootstrapVerifyFile := (<-opc).(dummy.OpPutFile)
 	c.Check(opPutBootstrapVerifyFile.Env, gc.Equals, "peckham")
-	c.Check(opPutBootstrapVerifyFile.FileName, gc.Equals, "bootstrap-verify")
+	c.Check(opPutBootstrapVerifyFile.FileName, gc.Equals, environs.VerificationFilename)
+
+	opPutBootstrapInitFile := (<-opc).(dummy.OpPutFile)
+	c.Check(opPutBootstrapInitFile.Env, gc.Equals, "peckham")
+	c.Check(opPutBootstrapInitFile.FileName, gc.Equals, "provider-state")
 
 	opBootstrap := (<-opc).(dummy.OpBootstrap)
 	c.Check(opBootstrap.Env, gc.Equals, "peckham")
@@ -265,6 +269,23 @@
 	},
 }}
 
+func (s *BootstrapSuite) TestBootstrapTwice(c *gc.C) {
+	restore := createToolsStore(c)
+	defer restore()
+	_, fake := makeEmptyFakeHome(c)
+	defer fake.Restore()
+
+	ctx := coretesting.Context(c)
+	code := cmd.Main(&BootstrapCommand{}, ctx, nil)
+	c.Check(code, gc.Equals, 0)
+
+	ctx2 := coretesting.Context(c)
+	code2 := cmd.Main(&BootstrapCommand{}, ctx2, nil)
+	c.Check(code2, gc.Equals, 1)
+	c.Check(coretesting.Stderr(ctx2), gc.Equals, "error: environment is already bootstrapped\n")
+	c.Check(coretesting.Stdout(ctx2), gc.Equals, "")
+}
+
 func (s *BootstrapSuite) TestAutoSync(c *gc.C) {
 	// Prepare a mock storage for testing and store the
 	// dummy tools in there.

=== modified file 'environs/bootstrap/bootstrap.go'
--- environs/bootstrap/bootstrap.go	2013-10-02 10:39:12 +0000
+++ environs/bootstrap/bootstrap.go	2013-10-03 03:45:40 +0000
@@ -42,11 +42,8 @@
 	if _, hasCAKey := cfg.CAPrivateKey(); !hasCAKey {
 		return fmt.Errorf("environment configuration has no ca-private-key")
 	}
-	// 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.
-	err := verifyBootstrapInit(environ)
-	if err != nil {
+	// Write out the bootstrap-init file, and confirm storage is writeable.
+	if err := environs.VerifyStorage(environ.Storage()); err != nil {
 		return err
 	}
 
@@ -89,24 +86,18 @@
 	return environ.Bootstrap(cons, newestTools)
 }
 
-// verifyBootstrapInit does the common initial check before bootstrapping, to
-// confirm that the environment isn't already running, and that the storage
-// works.
-func verifyBootstrapInit(env environs.Environ) error {
-	// TODO(rog) this feels like a layering violation - providers
-	// should not necessarily be required to store their bootstrap
-	// state in a file. This verification should probably
-	// be moved into provider and called by the providers themselves.
-	stor := env.Storage()
-	_, err := common.LoadState(stor)
+// EnsureNotBootstrapped returns null if the environment is not bootstrapped,
+// and an error if it is or if the function was not able to tell.
+func EnsureNotBootstrapped(env environs.Environ) error {
+	_, err := common.LoadState(env.Storage())
+	// If there is no error loading the bootstrap state, then we are bootstrapped.
 	if err == nil {
 		return fmt.Errorf("environment is already bootstrapped")
 	}
-	if err != environs.ErrNotBootstrapped {
-		return fmt.Errorf("cannot query old bootstrap state: %v", err)
+	if err == environs.ErrNotBootstrapped {
+		return nil
 	}
-
-	return environs.VerifyStorage(stor)
+	return err
 }
 
 // ConfigureBootstrapMachine adds the initial machine into state.  As a part

=== modified file 'environs/emptystorage.go'
--- environs/emptystorage.go	2013-09-18 07:13:09 +0000
+++ environs/emptystorage.go	2013-10-03 03:45:40 +0000
@@ -6,11 +6,9 @@
 import (
 	"fmt"
 	"io"
-	"strings"
 
 	"launchpad.net/juju-core/environs/storage"
 	"launchpad.net/juju-core/errors"
-	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/utils"
 )
 
@@ -20,16 +18,6 @@
 
 type emptyStorage struct{}
 
-// File named `verificationFilename` in the storage will contain
-// `verificationContent`.  This is also used to differentiate between
-// Python Juju and juju-core environments, so change the content with
-// care (and update CheckEnvironment below when you do that).
-const verificationFilename string = "bootstrap-verify"
-const verificationContent = "juju-core storage writing verified: ok\n"
-
-var VerifyStorageError error = fmt.Errorf(
-	"provider storage is not writable")
-
 func (s emptyStorage) Get(name string) (io.ReadCloser, error) {
 	return nil, errors.NotFoundf("file %q", name)
 }
@@ -51,16 +39,3 @@
 func (s emptyStorage) ShouldRetry(err error) bool {
 	return false
 }
-
-func VerifyStorage(stor storage.Storage) error {
-	reader := strings.NewReader(verificationContent)
-	err := stor.Put(verificationFilename, reader,
-		int64(len(verificationContent)))
-	if err != nil {
-		log.Debugf(
-			"environs: failed to write bootstrap-verify file: %v",
-			err)
-		return VerifyStorageError
-	}
-	return nil
-}

=== modified file 'environs/emptystorage_test.go'
--- environs/emptystorage_test.go	2013-10-01 16:53:02 +0000
+++ environs/emptystorage_test.go	2013-10-03 03:45:40 +0000
@@ -68,7 +68,7 @@
 	stor := environ.Storage()
 	err = environs.VerifyStorage(stor)
 	c.Assert(err, gc.IsNil)
-	reader, err := storage.Get(stor, "bootstrap-verify")
+	reader, err := storage.Get(stor, environs.VerificationFilename)
 	c.Assert(err, gc.IsNil)
 	defer reader.Close()
 	contents, err := ioutil.ReadAll(reader)
@@ -84,7 +84,7 @@
 	c.Assert(err, gc.IsNil)
 	stor := environ.Storage()
 	someError := errors.Unauthorizedf("you shall not pass")
-	dummy.Poison(stor, "bootstrap-verify", someError)
+	dummy.Poison(stor, environs.VerificationFilename, someError)
 	err = environs.VerifyStorage(stor)
 	c.Assert(err, gc.Equals, environs.VerifyStorageError)
 }

=== modified file 'environs/jujutest/livetests.go'
--- environs/jujutest/livetests.go	2013-10-02 10:39:12 +0000
+++ environs/jujutest/livetests.go	2013-10-03 03:45:40 +0000
@@ -137,7 +137,9 @@
 		c.Assert(err, gc.IsNil)
 	}
 	envtesting.UploadFakeTools(c, t.Env.Storage())
-	err := bootstrap.Bootstrap(t.Env, cons)
+	err := bootstrap.EnsureNotBootstrapped(t.Env)
+	c.Assert(err, gc.IsNil)
+	err = bootstrap.Bootstrap(t.Env, cons)
 	c.Assert(err, gc.IsNil)
 	t.bootstrapped = true
 }
@@ -385,9 +387,11 @@
 }
 
 func (t *LiveTests) TestBootstrapMultiple(c *gc.C) {
+	// bootstrap.Bootstrap no longer raises errors if the environment is
+	// already up, this has been moved into the bootstrap command.
 	t.BootstrapOnce(c)
 
-	err := bootstrap.Bootstrap(t.Env, constraints.Value{})
+	err := bootstrap.EnsureNotBootstrapped(t.Env)
 	c.Assert(err, gc.ErrorMatches, "environment is already bootstrapped")
 
 	c.Logf("destroy env")

=== modified file 'environs/jujutest/tests.go'
--- environs/jujutest/tests.go	2013-10-01 12:03:33 +0000
+++ environs/jujutest/tests.go	2013-10-03 03:45:40 +0000
@@ -131,19 +131,21 @@
 func (t *Tests) TestBootstrap(c *gc.C) {
 	e := t.Prepare(c)
 	envtesting.UploadFakeTools(c, e.Storage())
-	err := bootstrap.Bootstrap(e, constraints.Value{})
+	err := bootstrap.EnsureNotBootstrapped(e)
+	c.Assert(err, gc.IsNil)
+	err = bootstrap.Bootstrap(e, constraints.Value{})
 	c.Assert(err, gc.IsNil)
 
 	info, apiInfo, err := e.StateInfo()
 	c.Check(info.Addrs, gc.Not(gc.HasLen), 0)
 	c.Check(apiInfo.Addrs, gc.Not(gc.HasLen), 0)
 
-	err = bootstrap.Bootstrap(e, constraints.Value{})
+	err = bootstrap.EnsureNotBootstrapped(e)
 	c.Assert(err, gc.ErrorMatches, "environment is already bootstrapped")
 
 	e2 := t.Open(c)
 	envtesting.UploadFakeTools(c, e2.Storage())
-	err = bootstrap.Bootstrap(e2, constraints.Value{})
+	err = bootstrap.EnsureNotBootstrapped(e2)
 	c.Assert(err, gc.ErrorMatches, "environment is already bootstrapped")
 
 	info2, apiInfo2, err := e2.StateInfo()
@@ -157,11 +159,13 @@
 	e3 := t.Prepare(c)
 	envtesting.UploadFakeTools(c, e3.Storage())
 
+	err = bootstrap.EnsureNotBootstrapped(e3)
+	c.Assert(err, gc.IsNil)
 	err = bootstrap.Bootstrap(e3, constraints.Value{})
 	c.Assert(err, gc.IsNil)
 
-	err = bootstrap.Bootstrap(e3, constraints.Value{})
-	c.Assert(err, gc.NotNil)
+	err = bootstrap.EnsureNotBootstrapped(e3)
+	c.Assert(err, gc.ErrorMatches, "environment is already bootstrapped")
 }
 
 var noRetry = utils.AttemptStrategy{}

=== modified file 'environs/open.go'
--- environs/open.go	2013-10-02 18:41:54 +0000
+++ environs/open.go	2013-10-03 03:45:40 +0000
@@ -6,6 +6,7 @@
 import (
 	"fmt"
 	"io/ioutil"
+	"strings"
 	"time"
 
 	"launchpad.net/juju-core/cert"
@@ -15,7 +16,21 @@
 	"launchpad.net/juju-core/errors"
 )
 
-var InvalidEnvironmentError = fmt.Errorf("environment is not a juju-core environment")
+// File named `VerificationFilename` in the storage will contain
+// `verificationContent`.  This is also used to differentiate between
+// Python Juju and juju-core environments, so change the content with
+// care (and update CheckEnvironment below when you do that).
+const (
+	VerificationFilename = "bootstrap-verify"
+	verificationContent  = "juju-core storage writing verified: ok\n"
+)
+
+var (
+	VerifyStorageError error = fmt.Errorf(
+		"provider storage is not writable")
+	InvalidEnvironmentError = fmt.Errorf(
+		"environment is not a juju-core environment")
+)
 
 // ConfigForName returns the configuration for the environment with the
 // given name from the default environments file. If the name is blank,
@@ -199,6 +214,19 @@
 	return nil
 }
 
+// VerifyStorage writes the bootstrap init file to the storage to indicate
+// that the storage is writable.
+func VerifyStorage(stor storage.Storage) error {
+	reader := strings.NewReader(verificationContent)
+	err := stor.Put(VerificationFilename, reader,
+		int64(len(verificationContent)))
+	if err != nil {
+		logger.Warningf("failed to write bootstrap-verify file: %v", err)
+		return VerifyStorageError
+	}
+	return nil
+}
+
 // CheckEnvironment checks if an environment has a bootstrap-verify
 // that is written by juju-core commands (as compared to one being
 // written by Python juju).
@@ -210,7 +238,7 @@
 // Returns InvalidEnvironmentError on failure, nil otherwise.
 func CheckEnvironment(environ Environ) error {
 	stor := environ.Storage()
-	reader, err := storage.Get(stor, verificationFilename)
+	reader, err := storage.Get(stor, VerificationFilename)
 	if errors.IsNotFoundError(err) {
 		// When verification file does not exist, this is a juju-core
 		// environment.

=== modified file 'environs/open_test.go'
--- environs/open_test.go	2013-10-02 18:41:54 +0000
+++ environs/open_test.go	2013-10-03 03:45:40 +0000
@@ -297,7 +297,7 @@
 	// When the bootstrap-verify file does not exist, it still believes
 	// the environment is a juju-core one because earlier versions
 	// did not create that file.
-	err = stor.Remove("bootstrap-verify")
+	err = stor.Remove(environs.VerificationFilename)
 	c.Assert(err, gc.IsNil)
 	err = environs.CheckEnvironment(environ)
 	c.Assert(err, gc.IsNil)
@@ -318,7 +318,7 @@
 	// When fetching the verification file from storage fails,
 	// we get an InvalidEnvironmentError.
 	someError := errors.Unauthorizedf("you shall not pass")
-	dummy.Poison(stor, "bootstrap-verify", someError)
+	dummy.Poison(stor, environs.VerificationFilename, someError)
 	err = environs.CheckEnvironment(environ)
 	c.Assert(err, gc.Equals, someError)
 }
@@ -333,7 +333,7 @@
 	stor := environ.Storage()
 	content := "bad verification content"
 	reader := strings.NewReader(content)
-	err = stor.Put("bootstrap-verify", reader, int64(len(content)))
+	err = stor.Put(environs.VerificationFilename, reader, int64(len(content)))
 	c.Assert(err, gc.IsNil)
 
 	// When the bootstrap-verify file contains unexpected content,

=== modified file 'provider/dummy/environs.go'
--- provider/dummy/environs.go	2013-10-03 00:40:14 +0000
+++ provider/dummy/environs.go	2013-10-03 03:45:40 +0000
@@ -33,6 +33,8 @@
 	"sync"
 	"time"
 
+	"launchpad.net/loggo"
+
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/cloudinit"
@@ -42,7 +44,6 @@
 	"launchpad.net/juju-core/environs/storage"
 	"launchpad.net/juju-core/environs/tools"
 	"launchpad.net/juju-core/instance"
-	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/provider"
 	"launchpad.net/juju-core/provider/common"
@@ -55,6 +56,8 @@
 	"launchpad.net/juju-core/utils"
 )
 
+var logger = loggo.GetLogger("juju.provider.dummy")
+
 // SampleConfig() returns an environment configuration with all required
 // attributes set.
 func SampleConfig() testing.Attrs {
@@ -208,7 +211,7 @@
 // operation listener.  All opened environments after Reset will share
 // the same underlying state.
 func Reset() {
-	log.Infof("environs/dummy: reset environment")
+	logger.Infof("reset environment")
 	p := &providerInstance
 	p.mu.Lock()
 	defer p.mu.Unlock()
@@ -538,7 +541,7 @@
 		return fmt.Errorf("no CA certificate in environment configuration")
 	}
 
-	log.Infof("environs/dummy: would pick tools from %s", possibleTools)
+	logger.Infof("would pick tools from %s", possibleTools)
 	cfg, err := environs.BootstrapConfig(e.Config())
 	if err != nil {
 		return fmt.Errorf("cannot make bootstrap config: %v", err)
@@ -553,6 +556,17 @@
 	if estate.bootstrapped {
 		return fmt.Errorf("environment is already bootstrapped")
 	}
+	// Write the bootstrap file just like a normal provider. However
+	// we need to release the mutex for the save state to work, so regain
+	// it after the call.
+	estate.mu.Unlock()
+	if err := common.SaveState(e.Storage(), &common.BootstrapState{StateInstances: []instance.Id{"localhost"}}); err != nil {
+		logger.Errorf("failed to save state instances: %v", err)
+		estate.mu.Lock() // otherwise defered unlock will fail
+		return err
+	}
+	estate.mu.Lock() // back at it
+
 	if e.ecfg().stateServer() {
 		// TODO(rog) factor out relevant code from cmd/jujud/bootstrap.go
 		// so that we can call it here.
@@ -653,7 +667,7 @@
 
 	defer delay()
 	machineId := machineConfig.MachineId
-	log.Infof("environs/dummy: dummy startinstance, machine %s", machineId)
+	logger.Infof("dummy startinstance, machine %s", machineId)
 	if err := e.checkBroken("StartInstance"); err != nil {
 		return nil, nil, err
 	}
@@ -675,7 +689,7 @@
 	if machineConfig.APIInfo.Tag != names.MachineTag(machineId) {
 		return nil, nil, fmt.Errorf("entity tag must match started machine")
 	}
-	log.Infof("environs/dummy: would pick tools from %s", possibleTools)
+	logger.Infof("would pick tools from %s", possibleTools)
 	series := possibleTools.OneSeries()
 	i := &dummyInstance{
 		id:           instance.Id(fmt.Sprintf("%s-%d", e.name, estate.maxId)),
@@ -899,7 +913,7 @@
 
 func (inst *dummyInstance) OpenPorts(machineId string, ports []instance.Port) error {
 	defer delay()
-	log.Infof("environs/dummy: openPorts %s, %#v", machineId, ports)
+	logger.Infof("openPorts %s, %#v", machineId, ports)
 	if inst.firewallMode != config.FwInstance {
 		return fmt.Errorf("invalid firewall mode %q for opening ports on instance",
 			inst.firewallMode)
@@ -970,7 +984,7 @@
 // pause execution to simulate the latency of a real provider
 func delay() {
 	if providerDelay > 0 {
-		log.Infof("environs/dummy: pausing for %v", providerDelay)
+		logger.Infof("pausing for %v", providerDelay)
 		<-time.After(providerDelay)
 	}
 }