juju-core

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

  1. lp1247152-destroy-jenv-failed-prepare
  2. Merge into trunk
Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 2320
Proposed branch: lp:~axwalk/juju-core/lp1247152-destroy-jenv-failed-prepare
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 58 lines (+18/-10)
2 files modified
environs/open.go (+13/-9)
environs/open_test.go (+5/-1)
To merge this branch: bzr merge lp:~axwalk/juju-core/lp1247152-destroy-jenv-failed-prepare
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+205678@code.launchpad.net

Commit message

environs: destroy jenv if prepare fails

If Prepare fails mid-way, the .jenv file
can be left behind in some cases.

Fixes lp:1247152

https://codereview.appspot.com/61470046/

Description of the change

environs: destroy jenv if prepare fails

If Prepare fails mid-way, the .jenv file
can be left behind in some cases.

Fixes lp:1247152

https://codereview.appspot.com/61470046/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :

Reviewers: mp+205678_code.launchpad.net,

Message:
Please take a look.

Description:
environs: destroy jenv if prepare fails

If Prepare fails mid-way, the .jenv file
can be left behind in some cases.

Fixes lp:1247152

https://code.launchpad.net/~axwalk/juju-core/lp1247152-destroy-jenv-failed-prepare/+merge/205678

(do not edit description out of merge proposal)

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

Affected files (+20, -10 lines):
   A [revision details]
   M environs/open.go
   M environs/open_test.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: tarmac-20140210125047-3525mlq3garclj3q
+New revision: <email address hidden>

Index: environs/open.go
=== modified file 'environs/open.go'
--- environs/open.go 2014-02-03 14:31:54 +0000
+++ environs/open.go 2014-02-11 02:50:17 +0000
@@ -175,15 +175,7 @@
   if err != nil {
    return nil, fmt.Errorf("cannot create new info for environment %q: %v",
cfg.Name(), err)
   }
- cfg, err = ensureAdminSecret(cfg)
- if err != nil {
- return nil, fmt.Errorf("cannot generate admin-secret: %v", err)
- }
- cfg, err = ensureCertificate(cfg)
- if err != nil {
- return nil, fmt.Errorf("cannot ensure CA certificate: %v", err)
- }
- env, err := p.Prepare(cfg)
+ env, err := prepare(cfg, info, p)
   if err != nil {
    if err := info.Destroy(); err != nil {
     logger.Warningf("cannot destroy newly created environment info: %v",
err)
@@ -197,6 +189,18 @@
   return env, nil
  }

+func prepare(cfg *config.Config, info configstore.EnvironInfo, p
EnvironProvider) (Environ, error) {
+ cfg, err := ensureAdminSecret(cfg)
+ if err != nil {
+ return nil, fmt.Errorf("cannot generate admin-secret: %v", err)
+ }
+ cfg, err = ensureCertificate(cfg)
+ if err != nil {
+ return nil, fmt.Errorf("cannot ensure CA certificate: %v", err)
+ }
+ return p.Prepare(cfg)
+}
+
  // ensureAdminSecret returns a config with a non-empty admin-secret.
  func ensureAdminSecret(cfg *config.Config) (*config.Config, error) {
   if cfg.AdminSecret() != "" {

Index: environs/open_test.go
=== modified file 'environs/open_test.go'
--- environs/open_test.go 2013-12-20 02:38:56 +0000
+++ environs/open_test.go 2014-02-11 02:50:17 +0000
@@ -245,9 +245,13 @@
    },
   ))
   c.Assert(err, gc.IsNil)
- env, err := environs.Prepare(cfg, configstore.NewMem())
+ store := configstore.NewMem()
+ env, err := environs.Prepare(cfg, store)
   c.Assert(err, gc.ErrorMatches, "cannot ensure CA certificate: environment
configuration with a certificate but no CA private key")
   c.Assert(env, gc.IsNil)
+ // Ensure that the config storage info is cleaned up.
+ _, err = store.ReadInfo(cfg.Name())
+ c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
  }

  func (*OpenSuite) TestPrepareWithExistingKeyPair(c *gc.C) {

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

LGTM

https://codereview.appspot.com/61470046/

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (9.3 KiB)

The attempt to merge lp:~axwalk/juju-core/lp1247152-destroy-jenv-failed-prepare into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.015s
ok launchpad.net/juju-core/agent 1.228s
ok launchpad.net/juju-core/agent/tools 0.231s
ok launchpad.net/juju-core/bzr 6.865s
ok launchpad.net/juju-core/cert 3.517s
ok launchpad.net/juju-core/charm 0.568s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.040s
ok launchpad.net/juju-core/cloudinit/sshinit 1.198s
ok launchpad.net/juju-core/cmd 0.253s
ok launchpad.net/juju-core/cmd/charm-admin 0.815s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 235.166s
ok launchpad.net/juju-core/cmd/jujud 60.634s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 7.912s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.027s
ok launchpad.net/juju-core/container 0.037s
ok launchpad.net/juju-core/container/factory 0.052s
ok launchpad.net/juju-core/container/kvm 0.274s
ok launchpad.net/juju-core/container/kvm/mock 0.038s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.270s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.266s
ok launchpad.net/juju-core/environs 3.011s
ok launchpad.net/juju-core/environs/bootstrap 4.604s
ok launchpad.net/juju-core/environs/cloudinit 0.680s
ok launchpad.net/juju-core/environs/config 3.049s
ok launchpad.net/juju-core/environs/configstore 0.053s
ok launchpad.net/juju-core/environs/filestorage 0.029s
ok launchpad.net/juju-core/environs/httpstorage 0.939s
ok launchpad.net/juju-core/environs/imagemetadata 0.654s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.059s
ok launchpad.net/juju-core/environs/jujutest 0.249s
ok launchpad.net/juju-core/environs/manual 14.020s
ok launchpad.net/juju-core/environs/simplestreams 0.322s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.131s
ok launchpad.net/juju-core/environs/storage 1.113s
ok launchpad.net/juju-core/environs/sync 33.898s
ok launchpad.net/juju-core/environs/testing 0.217s
ok launchpad.net/juju-core/environs/tools 6.930s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.015s
ok launchpad.net/juju-core/instance 0.023s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 24.447s
ok launchpad.net/juju-core/juju/osenv 0.019s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.024s
ok launchpad....

Read more...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'environs/open.go'
2--- environs/open.go 2014-02-11 01:35:20 +0000
3+++ environs/open.go 2014-02-11 03:01:42 +0000
4@@ -175,15 +175,7 @@
5 if err != nil {
6 return nil, fmt.Errorf("cannot create new info for environment %q: %v", cfg.Name(), err)
7 }
8- cfg, err = ensureAdminSecret(cfg)
9- if err != nil {
10- return nil, fmt.Errorf("cannot generate admin-secret: %v", err)
11- }
12- cfg, err = ensureCertificate(cfg)
13- if err != nil {
14- return nil, fmt.Errorf("cannot ensure CA certificate: %v", err)
15- }
16- env, err := p.Prepare(cfg)
17+ env, err := prepare(cfg, info, p)
18 if err != nil {
19 if err := info.Destroy(); err != nil {
20 logger.Warningf("cannot destroy newly created environment info: %v", err)
21@@ -197,6 +189,18 @@
22 return env, nil
23 }
24
25+func prepare(cfg *config.Config, info configstore.EnvironInfo, p EnvironProvider) (Environ, error) {
26+ cfg, err := ensureAdminSecret(cfg)
27+ if err != nil {
28+ return nil, fmt.Errorf("cannot generate admin-secret: %v", err)
29+ }
30+ cfg, err = ensureCertificate(cfg)
31+ if err != nil {
32+ return nil, fmt.Errorf("cannot ensure CA certificate: %v", err)
33+ }
34+ return p.Prepare(cfg)
35+}
36+
37 // ensureAdminSecret returns a config with a non-empty admin-secret.
38 func ensureAdminSecret(cfg *config.Config) (*config.Config, error) {
39 if cfg.AdminSecret() != "" {
40
41=== modified file 'environs/open_test.go'
42--- environs/open_test.go 2013-12-20 02:38:56 +0000
43+++ environs/open_test.go 2014-02-11 03:01:42 +0000
44@@ -245,9 +245,13 @@
45 },
46 ))
47 c.Assert(err, gc.IsNil)
48- env, err := environs.Prepare(cfg, configstore.NewMem())
49+ store := configstore.NewMem()
50+ env, err := environs.Prepare(cfg, store)
51 c.Assert(err, gc.ErrorMatches, "cannot ensure CA certificate: environment configuration with a certificate but no CA private key")
52 c.Assert(env, gc.IsNil)
53+ // Ensure that the config storage info is cleaned up.
54+ _, err = store.ReadInfo(cfg.Name())
55+ c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
56 }
57
58 func (*OpenSuite) TestPrepareWithExistingKeyPair(c *gc.C) {

Subscribers

People subscribed via source and target branches

to status/vote changes: