Merge lp:~axwalk/juju-core/lp1239550-check-jenv-consistency into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

environs: warn if .jenv/env.yaml are inconsistent

Changes in environments.yaml do not take effect
for an environment that has already been prepared.
In some cases, it might not be obvious to a user
that the environment is prepared, and that changes
will not take effect. e.g.:
 - User called sync-tools, then modified env.yaml,
   then bootstrapped.
 - User bootstrapped, bootstrap failed, then modified
   env.yaml and tried again. We could remove the
   .jenv file on failure, but we can't guarantee
   juju won't die before doing so.
 - User is bootstrapping the null provider; since
   destroy-environment is not implemented, the user
   must remove the .jenv file manually. It's easy to
   forget to do that.

We now log a warning if any values have been
changed in environments.yaml after preparing,
and provide some direction on what to do.

Also: don't call Prepare for config retrieved from
the config store (i.e. already prepared config).

Fixes #1239550

https://codereview.appspot.com/14880043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+191748_code.launchpad.net,

Message:
Please take a look.

Description:
environs: warn if .jenv/env.yaml are inconsistent

Changes in environments.yaml do not take effect
for an environment that has already been prepared.
In some cases, it might not be obvious to a user
that the environment is prepared, and that changes
will not take effect. e.g.:
  - User called sync-tools, then modified env.yaml,
    then bootstrapped.
  - User bootstrapped, bootstrap failed, then modified
    env.yaml and tried again. We could remove the
    .jenv file on failure, but we can't guarantee
    juju won't die before doing so.
  - User is bootstrapping the null provider; since
    destroy-environment is not implemented, the user
    must remove the .jenv file manually. It's easy to
    forget to do that.

We now log a warning if any values have been
changed in environments.yaml after preparing,
and provide some direction on what to do.

Also: don't call Prepare for config retrieved from
the config store (i.e. already prepared config).

Fixes #1239550

https://code.launchpad.net/~axwalk/juju-core/lp1239550-check-jenv-consistency/+merge/191748

(do not edit description out of merge proposal)

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

Affected files (+38, -4 lines):
   [revision details]
   environs/open.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-20131017053700-k2l0flmzzs7feuv2
+New revision: <email address hidden>

Index: environs/open.go
=== modified file 'environs/open.go'
--- environs/open.go 2013-10-04 12:18:17 +0000
+++ environs/open.go 2013-10-18 03:39:49 +0000
@@ -6,6 +6,7 @@
  import (
   "fmt"
   "io/ioutil"
+ "reflect"
   "strings"
   "time"

@@ -44,6 +45,28 @@
   ConfigFromEnvirons
  )

+// checkBootstrapConfigConsistency checks that the bootstrap configuration
+// in the store (.jenv) is consistent with the configuration in
+// environments.yaml, logging warnings for each changed value.
+func checkBootstrapConfigConsistency(infoConfig, environsConfig
*config.Config) {
+ if infoConfig == nil || environsConfig == nil {
+ return
+ }
+ environsConfigAttrs := environsConfig.AllAttrs()
+ var changed bool
+ for k, v := range infoConfig.AllAttrs() {
+ if v2, ok := environsConfigAttrs[k]; ok {
+ if !reflect.DeepEqual(v, v2) {
+ logger.Warningf("config %q was changed in environments.yaml from %q
to %q", k, v, v2)
+ changed = true
+ }
+ }
+ }
+ if changed {
+ logger.Warningf(`changes in environments.yaml will not take effect after
bootstrapping. Update %s.jenv, revert environments.yaml, or
destroy-environment and re-bootstrap`, infoConfig.Name())
+ }
+}
+
  // ConfigForName returns the configuration for the environment with the
  // given name from the default environments file. If the name is blank,
  // the default environment will be used. If the configuration is not
@@ -62,6 +85,7 @@
   if name == "" {
    name = envs.Default
   }
+ environsCfg, err := envs.Config(name)
   // TODO(rog) 2013-10-04 https://bu...

[Andrew Wilkins (axwalk)]

There's some issues with this, for example logging config is changed after Prepare. Will need to revisit.

Unmerged revisions

1995. By Andrew Wilkins on 2013-10-18

environs: warn if .jenv/env.yaml are inconsistent

Changes in environments.yaml do not take effect
for an environment that has already been prepared.
In some cases, it might not be obvious to a user
that the environment is prepared, and that changes
will not take effect. e.g.:
 - User called sync-tools, then modified env.yaml,
   then bootstrapped.
 - User bootstrapped, bootstrap failed, then modified
   env.yaml and tried again. We could remove the
   .jenv file on failure, but we can't guarantee
   juju won't die before doing so.
 - User is bootstrapping the null provider; since
   destroy-environment is not implemented, the user
   must remove the .jenv file manually. It's easy to
   forget to do that.

We now log a warning if any values have been
changed in environments.yaml after preparing,
and provide some direction on what to do.

Also: don't call Prepare for config retrieved from
the config store (i.e. already prepared config).

Fixes #1239550

=== modified file 'environs/open.go'
--- environs/open.go	2013-10-04 12:18:17 +0000
+++ environs/open.go	2013-10-18 04:03:27 +0000
@@ -6,6 +6,7 @@
 import (
 	"fmt"
 	"io/ioutil"
+	"reflect"
 	"strings"
 	"time"
 
@@ -44,6 +45,28 @@
 	ConfigFromEnvirons
 )
 
+// checkBootstrapConfigConsistency checks that the bootstrap configuration
+// in the store (.jenv) is consistent with the configuration in
+// environments.yaml, logging warnings for each changed value.
+func checkBootstrapConfigConsistency(infoConfig, environsConfig *config.Config) {
+	if infoConfig == nil || environsConfig == nil {
+		return
+	}
+	environsConfigAttrs := environsConfig.AllAttrs()
+	var changed bool
+	for k, v := range infoConfig.AllAttrs() {
+		if v2, ok := environsConfigAttrs[k]; ok {
+			if !reflect.DeepEqual(v, v2) {
+				logger.Warningf("config %q was changed in environments.yaml from %q to %q", k, v, v2)
+				changed = true
+			}
+		}
+	}
+	if changed {
+		logger.Warningf(`changes in environments.yaml will not take effect after bootstrapping. Update %s.jenv, revert environments.yaml, or destroy-environment and re-bootstrap`, infoConfig.Name())
+	}
+}
+
 // ConfigForName returns the configuration for the environment with the
 // given name from the default environments file. If the name is blank,
 // the default environment will be used. If the configuration is not
@@ -62,6 +85,7 @@
 	if name == "" {
 		name = envs.Default
 	}
+	environsCfg, err := envs.Config(name)
 	// TODO(rog) 2013-10-04 https://bugs.launchpad.net/juju-core/+bug/1235217
 	// Don't fall back to reading from environments.yaml
 	// when we can be sure that everyone has a
@@ -74,14 +98,17 @@
 			}
 			logger.Debugf("ConfigForName found bootstrap config %#v", info.BootstrapConfig())
 			cfg, err := config.New(config.NoDefaults, info.BootstrapConfig())
-			return cfg, ConfigFromInfo, err
+			if err != nil {
+				return nil, ConfigFromInfo, err
+			}
+			checkBootstrapConfigConsistency(cfg, environsCfg)
+			return cfg, ConfigFromInfo, nil
 		}
 		if err != nil && !errors.IsNotFoundError(err) {
 			return nil, ConfigFromInfo, fmt.Errorf("cannot read environment info for %q: %v", name, err)
 		}
 	}
-	cfg, err := envs.Config(name)
-	return cfg, ConfigFromEnvirons, err
+	return environsCfg, ConfigFromEnvirons, err
 }
 
 // NewFromName opens the environment with the given
@@ -117,10 +144,13 @@
 // given store. If the environment is already prepared,
 // it behaves like NewFromName.
 func PrepareFromName(name string, store configstore.Storage) (Environ, error) {
-	cfg, _, err := ConfigForName(name, store)
+	cfg, source, err := ConfigForName(name, store)
 	if err != nil {
 		return nil, err
 	}
+	if source == ConfigFromInfo {
+		return New(cfg)
+	}
 	return Prepare(cfg, store)
 }
 
@@ -164,10 +194,12 @@
 		if len(info.BootstrapConfig()) == 0 {
 			return nil, fmt.Errorf("found environment info but no bootstrap config")
 		}
+		environsCfg := cfg
 		cfg, err = config.New(config.NoDefaults, info.BootstrapConfig())
 		if err != nil {
 			return nil, fmt.Errorf("cannot parse bootstrap config: %v", err)
 		}
+		checkBootstrapConfigConsistency(cfg, environsCfg)
 		return New(cfg)
 	}
 	if err != nil {