Merge lp:~axwalk/juju-core/juju-failed-bootstrap-destroy-jenv into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

cmd/juju: delete .jenv if bootstrap fails

If bootstrap fails, and the .jenv file did
not previously exist, then Juju will attempt
to destroy the environment in case of partial
creation. If this succeeds, the .jenv file
is destroyed. If it fails, an error message
is logged; we do not currently attempt to
forcefully remove the environment.

NOTE: if an environment is already prepared, say
by running sync-tools, then a failed bootstrap
will not attempt to destroy the environment.

Fixes lp:1247152

https://codereview.appspot.com/59560043/

cmd/juju: delete .jenv if bootstrap fails

If bootstrap fails, and the .jenv file did
not previously exist, then Juju will attempt
to destroy the environment in case of partial
creation. If this succeeds, the .jenv file
is destroyed. If it fails, an error message
is logged; we do not currently attempt to
forcefully remove the environment.

NOTE: if an environment is already prepared, say
by running sync-tools, then a failed bootstrap
will not attempt to destroy the environment.

Fixes lp:1247152

https://codereview.appspot.com/59560043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+204421_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: delete .jenv if bootstrap fails

If bootstrap fails, and the .jenv file did
not previously exist, then Juju will attempt
to destroy the environment in case of partial
creation. If this succeeds, the .jenv file
is destroyed. If environment destruction fails,
a warning message is displayed to the user
with directions to use destroy-environment with
a new --config-only flag.

juju destroy-environment now has a --config-only
flag that tells destroy-environment to only
destroy the configstore info for the named
environment. This is equivalent to deleting the
.jenv file, but allows for additional configstore
implementations in the future.

NOTE: if an environment is already prepared, say
by running sync-tools, then a failed bootstrap
will not attempt to destroy the environment.

Fixes lp:1247152

https://code.launchpad.net/~axwalk/juju-core/juju-failed-bootstrap-destroy-jenv/+merge/204421

(do not edit description out of merge proposal)

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

Affected files (+106, -26 lines):
   [revision details]
   cmd/juju/bootstrap.go
   cmd/juju/bootstrap_test.go
   cmd/juju/destroyenvironment.go
   cmd/juju/destroyenvironment_test.go
   environs/open.go
   provider/dummy/environs.go

[Ian Booth (wallyworld)]

I'm not sure about this. I think we're getting to the point where the
mental model is getting too complicated and exposes too much of Juju's
internals. Why not just go ahead and remove the jenv file automatically
under the circumstances described ie no previous jenv file, bootstrap
fails, destroy files, so remove jenv.
What's the use case for stand alone use of the --config-only option? If
the cloud artefacts are already gone, we could just inform the user that
their cloud env has disappeared and did they want to clean up the
remaining local config, and if they say yes, just do it.

https://codereview.appspot.com/59560043/

[Andrew Wilkins (axwalk)]

On 2014/02/03 07:20:58, wallyworld wrote:
> I'm not sure about this. I think we're getting to the point where the
mental
> model is getting too complicated and exposes too much of Juju's
internals. Why
> not just go ahead and remove the jenv file automatically under the
circumstances
> described ie no previous jenv file, bootstrap fails, destroy files, so
remove
> jenv.

If we dispose of the .jenv file without attempting to destroy the
environment,
then the user cannot automatically release any partially created
environment.

> What's the use case for stand alone use of the --config-only option?
If the
> cloud artefacts are already gone, we could just inform the user that
their cloud
> env has disappeared and did they want to clean up the remaining local
config,
> and if they say yes, just do it.

The reason it exists is to clean up if bootstrap fails *and* the auto
destroy-environment fails. It would be nice if we could automatically
determine that this can be done and just do it, but I'm not convinced
it's practical.

How do you tell that the cloud artefacts are gone? By going to the
provider. What if you can't get to the provider because your config is
wrong? How can Juju tell the difference between a temporarily wrong
config and a permanently wrong one?

https://codereview.appspot.com/59560043/

[Ian Booth (wallyworld)]

On 2014/02/03 07:34:06, axw wrote:
> On 2014/02/03 07:20:58, wallyworld wrote:
> > I'm not sure about this. I think we're getting to the point where
the mental
> > model is getting too complicated and exposes too much of Juju's
internals. Why
> > not just go ahead and remove the jenv file automatically under the
> circumstances
> > described ie no previous jenv file, bootstrap fails, destroy files,
so remove
> > jenv.

> If we dispose of the .jenv file without attempting to destroy the
environment,
> then the user cannot automatically release any partially created
environment.

> > What's the use case for stand alone use of the --config-only option?
If the
> > cloud artefacts are already gone, we could just inform the user that
their
> cloud
> > env has disappeared and did they want to clean up the remaining
local config,
> > and if they say yes, just do it.

> The reason it exists is to clean up if bootstrap fails *and* the auto
> destroy-environment fails. It would be nice if we could automatically
determine
> that this can be done and just do it, but I'm not convinced it's
practical.

> How do you tell that the cloud artefacts are gone? By going to the
provider.
> What if you can't get to the provider because your config is wrong?
How can Juju
> tell the difference between a temporarily wrong config and a
permanently wrong
> one?

I think there's 2 main use cases?
1. Bootstrap of new env fails
In this case, we know the that jenv file did not previously exist so if
auto-destroy fails, we can (probably after asking) offer to the user
that any remaining juju configuration files be cleaned up and so leave
their system as it was prior to the initial bootstrap.
2. Someone deletes cloud artefacts outside of juju
So now the user may see that juju commands fail and they want to clean
up. So they juju destroy-env. It fails cause stuff has already been
deleted. So like in the first case above, we can just prompt them that
their config will be deleted. Here the wording of the prompt will be
slightly different but the end result is the same.

I guess there's a danger that if there's some transient failure reaching
the cloud we could remove the jenv and leave the user with cloud
resources still active. We could incorporate this into the above prompts
eg "Juju cannot contact the cloud to delete any running instances, this
may be a temporary failure, did you want to try again or just nuke your
env config". If they elect to clean up the config, we can let them know
that they should ensure that nothing is left running, Juju tried but
failed so now it's up to them.

I guess my main wish is that the user be guided as to what to do and the
tool does it at a high enough level that they don't have to try and
understand the internal workings of Juju, as opposed to exiting and
making them run a command with yet another option. And like
destroy-environment -y, the user could allow juju to do what it thinks
needs to be done without prompting.

https://codereview.appspot.com/59560043/

[Andrew Wilkins (axwalk)]

On 2014/02/03 08:03:18, wallyworld wrote:
> On 2014/02/03 07:34:06, axw wrote:
> > On 2014/02/03 07:20:58, wallyworld wrote:
> > > I'm not sure about this. I think we're getting to the point where
the mental
> > > model is getting too complicated and exposes too much of Juju's
internals.
> Why
> > > not just go ahead and remove the jenv file automatically under the
> > circumstances
> > > described ie no previous jenv file, bootstrap fails, destroy
files, so
> remove
> > > jenv.
> >
> > If we dispose of the .jenv file without attempting to destroy the
environment,
> > then the user cannot automatically release any partially created
environment.
> >
> > > What's the use case for stand alone use of the --config-only
option? If the
> > > cloud artefacts are already gone, we could just inform the user
that their
> > cloud
> > > env has disappeared and did they want to clean up the remaining
local
> config,
> > > and if they say yes, just do it.
> >
> > The reason it exists is to clean up if bootstrap fails *and* the
auto
> > destroy-environment fails. It would be nice if we could
automatically
> determine
> > that this can be done and just do it, but I'm not convinced it's
practical.
> >
> > How do you tell that the cloud artefacts are gone? By going to the
provider.
> > What if you can't get to the provider because your config is wrong?
How can
> Juju
> > tell the difference between a temporarily wrong config and a
permanently wrong
> > one?

> I think there's 2 main use cases?
> 1. Bootstrap of new env fails
> In this case, we know the that jenv file did not previously exist so
if
> auto-destroy fails, we can (probably after asking) offer to the user
that any
> remaining juju configuration files be cleaned up and so leave their
system as it
> was prior to the initial bootstrap.
> 2. Someone deletes cloud artefacts outside of juju
> So now the user may see that juju commands fail and they want to clean
up. So
> they juju destroy-env. It fails cause stuff has already been deleted.
So like in
> the first case above, we can just prompt them that their config will
be deleted.
> Here the wording of the prompt will be slightly different but the end
result is
> the same.

> I guess there's a danger that if there's some transient failure
reaching the
> cloud we could remove the jenv and leave the user with cloud resources
still
> active. We could incorporate this into the above prompts eg "Juju
cannot contact
> the cloud to delete any running instances, this may be a temporary
failure, did
> you want to try again or just nuke your env config". If they elect to
clean up
> the config, we can let them know that they should ensure that nothing
is left
> running, Juju tried but failed so now it's up to them.

> I guess my main wish is that the user be guided as to what to do and
the tool
> does it at a high enough level that they don't have to try and
understand the
> internal workings of Juju, as opposed to exiting and making them run a
command
> with yet another option. And like destroy-environment -y, the user
could allow
> juju to do what it thinks needs to be done without prompting.

Fair call about it being too low-level. I'm just not keen on adding
pr...

[Roger Peppe (rogpeppe)]

Thanks a lot for doing this - it will be much appreciated.

We should also destroy the environment if sync-tools fails and the env
didn't previously exist, I
think.

https://codereview.appspot.com/59560043/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/59560043/diff/1/cmd/juju/bootstrap.go#newcode118
cmd/juju/bootstrap.go:118:
I'd prefer it if result had "err" in its name here,
either by actually naming it "err", or perhaps by
naming it "resultErr".

https://codereview.appspot.com/59560043/diff/1/cmd/juju/bootstrap.go#newcode135
cmd/juju/bootstrap.go:135:
I think this case is unusual enough that we don't actually need to
special "config-only" flag. The error message will probably contain the
file name anyway. I think a better approach would be for
destroy-environment to delete the config file if it gets a definitive
"not found" error when trying to delete the environment.

https://codereview.appspot.com/59560043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/59560043/diff/1/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/59560043/diff/1/cmd/juju/bootstrap.go#newcode118
cmd/juju/bootstrap.go:118:
On 2014/02/03 08:32:22, rog wrote:
> I'd prefer it if result had "err" in its name here,
> either by actually naming it "err", or perhaps by
> naming it "resultErr".

Done.

https://codereview.appspot.com/59560043/diff/1/cmd/juju/bootstrap.go#newcode135
cmd/juju/bootstrap.go:135:
On 2014/02/03 08:32:22, rog wrote:
> I think this case is unusual enough that we don't actually need to
special
> "config-only" flag. The error message will probably contain the file
name
> anyway. I think a better approach would be for destroy-environment to
delete the
> config file if it gets a definitive "not found" error when trying to
delete the
> environment.

Okay. I'm going to take the --config-only bit out of the CL for now.
That can be addressed in a followup.

https://codereview.appspot.com/59560043/

[Andrew Wilkins (axwalk)]

On 2014/02/03 08:32:21, rog wrote:
> Thanks a lot for doing this - it will be much appreciated.

> We should also destroy the environment if sync-tools fails and the env
didn't
> previously exist, I
> think.

Done.

https://codereview.appspot.com/59560043/

[Dimiter Naydenov (dimitern)]

LGTM

https://codereview.appspot.com/59560043/

=== modified file 'cmd/juju/bootstrap.go'
--- cmd/juju/bootstrap.go	2014-01-31 00:14:11 +0000
+++ cmd/juju/bootstrap.go	2014-02-04 02:09:35 +0000
@@ -21,6 +21,7 @@
 	"launchpad.net/juju-core/environs/imagemetadata"
 	"launchpad.net/juju-core/environs/sync"
 	"launchpad.net/juju-core/environs/tools"
+	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/provider"
 	"launchpad.net/juju-core/utils/set"
 	"launchpad.net/juju-core/version"
@@ -111,18 +112,34 @@
 	return c.Context.Stderr
 }
 
+func destroyPreparedEnviron(env environs.Environ, store configstore.Storage, err *error, action string) {
+	if *err == nil {
+		return
+	}
+	if err := environs.Destroy(env, store); err != nil {
+		logger.Errorf("%s failed, and the environment could not be destroyed: %v", action, err)
+	}
+}
+
 // Run connects to the environment specified on the command line and bootstraps
 // a juju in that environment if none already exists. If there is as yet no environments.yaml file,
 // the user is informed how to create one.
-func (c *BootstrapCommand) Run(ctx *cmd.Context) error {
+func (c *BootstrapCommand) Run(ctx *cmd.Context) (resultErr error) {
 	store, err := configstore.Default()
 	if err != nil {
 		return err
 	}
+	var existing bool
+	if _, err := store.ReadInfo(c.EnvName); !errors.IsNotFoundError(err) {
+		existing = true
+	}
 	environ, err := environs.PrepareFromName(c.EnvName, store)
 	if err != nil {
 		return err
 	}
+	if !existing {
+		defer destroyPreparedEnviron(environ, store, &resultErr, "Bootstrap")
+	}
 	bootstrapContext := bootstrapContext{ctx}
 	// If the environment has a special bootstrap Storage, use it wherever
 	// we'd otherwise use environ.Storage.
 
=== modified file 'cmd/juju/bootstrap_test.go'
--- cmd/juju/bootstrap_test.go	2014-01-30 06:21:03 +0000
+++ cmd/juju/bootstrap_test.go	2014-02-04 02:09:35 +0000
@@ -476,6 +476,28 @@
 	c.Assert(errText, gc.Matches, expectedErrText)
 }
 
+func (s *BootstrapSuite) TestBootstrapDestroy(c *gc.C) {
+	_, fake := makeEmptyFakeHome(c)
+	defer fake.Restore()
+	opc, errc := runCommand(nullContext(), new(BootstrapCommand), "-e", "brokenenv")
+	err := <-errc
+	c.Assert(err, gc.ErrorMatches, "dummy.Bootstrap is broken")
+	var opDestroy *dummy.OpDestroy
+	for opDestroy == nil {
+		select {
+		case op := <-opc:
+			switch op := op.(type) {
+			case dummy.OpDestroy:
+				opDestroy = &op
+			}
+		default:
+			c.Error("expected call to env.Destroy")
+			return
+		}
+	}
+	c.Assert(opDestroy.Error, gc.ErrorMatches, "dummy.Destroy is broken")
+}
+
 // createToolsSource writes the mock tools and metadata into a temporary
 // directory and returns it.
 func createToolsSource(c *gc.C, versions []version.Binary) string {
 
=== modified file 'cmd/juju/destroyenvironment_test.go'
--- cmd/juju/destroyenvironment_test.go	2014-01-28 11:18:42 +0000
+++ cmd/juju/destroyenvironment_test.go	2014-02-04 02:09:35 +0000
@@ -94,8 +94,10 @@
 
 	// destroy with broken environment
 	opc, errc := runCommand(nullContext(), new(DestroyEnvironmentCommand), "dummyenv", "--yes")
-	c.Check(<-opc, gc.IsNil)
-	c.Check(<-errc, gc.ErrorMatches, "dummy.Destroy is broken")
+	op, ok := (<-opc).(dummy.OpDestroy)
+	c.Assert(ok, jc.IsTrue)
+	c.Assert(op.Error, gc.ErrorMatches, "dummy.Destroy is broken")
+	c.Check(<-errc, gc.Equals, op.Error)
 	c.Check(<-opc, gc.IsNil)
 }
 
 
=== modified file 'cmd/juju/synctools.go'
--- cmd/juju/synctools.go	2014-01-28 04:58:43 +0000
+++ cmd/juju/synctools.go	2014-02-04 02:09:35 +0000
@@ -12,6 +12,7 @@
 	"launchpad.net/juju-core/environs/configstore"
 	"launchpad.net/juju-core/environs/filestorage"
 	"launchpad.net/juju-core/environs/sync"
+	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/version"
 )
 
@@ -72,7 +73,7 @@
 	return cmd.CheckEmpty(args)
 }
 
-func (c *SyncToolsCommand) Run(ctx *cmd.Context) error {
+func (c *SyncToolsCommand) Run(ctx *cmd.Context) (resultErr error) {
 	// Register writer for output on screen.
 	loggo.RegisterWriter("synctools", cmd.NewCommandLogWriter("juju.environs.sync", ctx.Stdout, ctx.Stderr), loggo.INFO)
 	defer loggo.RemoveWriter("synctools")
@@ -80,10 +81,17 @@
 	if err != nil {
 		return err
 	}
+	var existing bool
+	if _, err := store.ReadInfo(c.EnvName); !errors.IsNotFoundError(err) {
+		existing = true
+	}
 	environ, err := environs.PrepareFromName(c.EnvName, store)
 	if err != nil {
 		return err
 	}
+	if !existing {
+		defer destroyPreparedEnviron(environ, store, &resultErr, "Sync-tools")
+	}
 
 	target := environ.Storage()
 	if c.localDir != "" {
 
=== modified file 'environs/open.go'
--- environs/open.go	2013-10-04 12:18:17 +0000
+++ environs/open.go	2014-02-04 02:09:35 +0000
@@ -235,7 +235,13 @@
 	if err := env.Destroy(); err != nil {
 		return err
 	}
-	info, err := store.ReadInfo(name)
+	return DestroyInfo(name, store)
+}
+
+// DestroyInfo destroys the configuration data for the named
+// environment from the given store.
+func DestroyInfo(envName string, store configstore.Storage) error {
+	info, err := store.ReadInfo(envName)
 	if err != nil {
 		if errors.IsNotFoundError(err) {
 			return nil
 
=== modified file 'provider/dummy/environs.go'
--- provider/dummy/environs.go	2014-01-28 04:58:43 +0000
+++ provider/dummy/environs.go	2014-02-04 02:09:35 +0000
@@ -97,17 +97,16 @@
 // Operation represents an action on the dummy provider.
 type Operation interface{}
 
-type GenericOperation struct {
-	Env string
-}
-
 type OpBootstrap struct {
 	Context     environs.BootstrapContext
 	Env         string
 	Constraints constraints.Value
 }
 
-type OpDestroy GenericOperation
+type OpDestroy struct {
+	Env   string
+	Error error
+}
 
 type OpStartInstance struct {
 	Env          string
@@ -637,11 +636,8 @@
 	return nil
 }
 
-func (e *environ) Destroy() error {
+func (e *environ) Destroy() (res error) {
 	defer delay()
-	if err := e.checkBroken("Destroy"); err != nil {
-		return err
-	}
 	estate, err := e.state()
 	if err != nil {
 		if err == provider.ErrDestroyed {
@@ -649,6 +645,10 @@
 		}
 		return err
 	}
+	defer func() { estate.ops <- OpDestroy{Env: estate.name, Error: res} }()
+	if err := e.checkBroken("Destroy"); err != nil {
+		return err
+	}
 	p := &providerInstance
 	p.mu.Lock()
 	delete(p.state, estate.id)
@@ -656,7 +656,6 @@
 
 	estate.mu.Lock()
 	defer estate.mu.Unlock()
-	estate.ops <- OpDestroy{Env: estate.name}
 	estate.destroy()
 	return nil
 }