Merge lp:~dimitern/juju-core/270-lp-1257649-ssh-timeout-bootstrap into lp:~go-bot/juju-core/trunk

[Dimiter Naydenov]

bootstrap: Added configurable timeout and delays

Fixed bug #1257649: ssh timeout for bootstrap
could be configurable, by adding 3 new config
settings:
- bootstrap-timeout (default: 600s = 10m)
- bootstrap-retry-delay (default: 5s)
- bootstrap-addresses-delay (default: 10s)

All of them are integers, measured in seconds,
and also all of them are optional and have the
specified defaults.

Updated the bootstrap command's help text to
include the new config settings.

https://codereview.appspot.com/58170045/

R=natefinch

bootstrap: Added configurable timeout and delays

Fixed bug #1257649: ssh timeout for bootstrap
could be configurable, by adding 3 new config
settings:
- bootstrap-timeout (default: 600s = 10m)
- bootstrap-retry-delay (default: 5s)
- bootstrap-addresses-delay (default: 10s)

All of them are integers, measured in seconds,
and also all of them are optional and have the
specified defaults.

Updated the bootstrap command's help text to
include the new config settings.

https://codereview.appspot.com/58170045/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+203832_code.launchpad.net,

Message:
Please take a look.

Description:
bootstrap: Added configurable timeout and delays

Fixed bug #1257649: ssh timeout for bootstrap
could be configurable, by adding 3 new config
settings:
- bootstrap-ssh-timeout (default: 600s = 10m)
- bootstrap-ssh-retry-delay (default: 5s)
- bootstrap-ssh-addresses-delay (default: 10s)

All of them are integers, measured in seconds,
and also all of them are optional and have the
specified defaults.

Updated the bootstrap command's help text to
include the new config settings.

https://code.launchpad.net/~dimitern/juju-core/270-lp-1257649-ssh-timeout-bootstrap/+merge/203832

Requires:
https://code.launchpad.net/~dimitern/juju-core/260-lp-1067979-duplicate-charm/+merge/203291

(do not edit description out of merge proposal)

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

Affected files (+236, -101 lines):
   A [revision details]
   M cmd/juju/bootstrap.go
   M environs/config/config.go
   M environs/config/config_test.go
   M provider/common/bootstrap.go

[Nate Finch (natefinch)]

Please remove all user-facing mentions of SSH, since that's an internal
detail and something that would just confuse users. This is just a
bootstrap timeout to an end user, even if it does use SSH behind the
scenes.

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

https://codereview.appspot.com/58170045/diff/1/cmd/juju/bootstrap.go#newcode48
cmd/juju/bootstrap.go:48: bootstrap-ssh-timeout: 600 # default: 10
minutes
ssh is an internal detail that will just be confusing to users. This is
just the "bootstrap-timeout" to an end user.
  Same goes for retry delay and addresses delay.

https://codereview.appspot.com/58170045/

[Nate Finch (natefinch)]

Oh yeah, otherwise LGTM :)

https://codereview.appspot.com/58170045/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/58170045/

[Go Bot (go-bot)]

The prerequisite https://code.launchpad.net/~dimitern/juju-core/260-lp-1067979-duplicate-charm has not yet been merged into https://code.launchpad.net/~go-bot/juju-core/trunk.

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/58170045/

[Dimiter Naydenov (dimitern)]

Self-approving after fixing merge conflicts.

review: Approve

[Go Bot (go-bot)]

The attempt to merge lp:~dimitern/juju-core/270-lp-1257649-ssh-timeout-bootstrap into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.242s
ok launchpad.net/juju-core/agent/tools 0.247s
ok launchpad.net/juju-core/bzr 7.292s
ok launchpad.net/juju-core/cert 3.276s
ok launchpad.net/juju-core/charm 0.675s
? 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.033s
ok launchpad.net/juju-core/cloudinit/sshinit 1.206s
ok launchpad.net/juju-core/cmd 0.228s
ok launchpad.net/juju-core/cmd/charm-admin 0.839s
? 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 253.798s
ok launchpad.net/juju-core/cmd/jujud 67.489s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 13.106s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container 0.047s
ok launchpad.net/juju-core/container/factory 0.054s
ok launchpad.net/juju-core/container/kvm 0.299s
ok launchpad.net/juju-core/container/kvm/mock 0.064s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.368s
? 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.289s
ok launchpad.net/juju-core/environs 3.146s
ok launchpad.net/juju-core/environs/bootstrap 4.543s
ok launchpad.net/juju-core/environs/cloudinit 0.614s
ok launchpad.net/juju-core/environs/config 2.040s
ok launchpad.net/juju-core/environs/configstore 0.041s
ok launchpad.net/juju-core/environs/filestorage 0.033s
ok launchpad.net/juju-core/environs/httpstorage 0.976s
ok launchpad.net/juju-core/environs/imagemetadata 0.658s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.062s
ok launchpad.net/juju-core/environs/jujutest 0.219s
ok launchpad.net/juju-core/environs/manual 9.762s
ok launchpad.net/juju-core/environs/simplestreams 0.331s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.488s
ok launchpad.net/juju-core/environs/storage 1.212s
ok launchpad.net/juju-core/environs/sync 37.095s
ok launchpad.net/juju-core/environs/testing 0.212s
ok launchpad.net/juju-core/environs/tools 6.858s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.017s
ok launchpad.net/juju-core/instance 0.027s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 25.189s
ok launchpad.net/juju-core/juju/osenv 0.021s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.027s
ok launchpad.net/juju-core/log/syslog 0.023s
ok launchpad.net/juju-core/names 0.030s
? lau...

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/58170045/

[Dimiter Naydenov (dimitern)]

Self-approving after fixing a test.

review: Approve

=== modified file 'cmd/juju/bootstrap.go'
--- cmd/juju/bootstrap.go	2014-01-30 06:21:03 +0000
+++ cmd/juju/bootstrap.go	2014-01-30 17:28:09 +0000
@@ -42,7 +42,16 @@
 Bootstrap initializes the cloud environment synchronously and displays information
 about the current installation steps.  The time for bootstrap to complete varies 
 across cloud providers from a few seconds to several minutes.  Once bootstrap has 
-completed, you can run other juju commands against your environment.
+completed, you can run other juju commands against your environment. You can change
+the default timeout and retry delays used during the bootstrap by changing the
+following settings in your environments.yaml (all values represent number of seconds):
+
+    # How long to wait for a connection to the state server.
+    bootstrap-timeout: 600 # default: 10 minutes
+    # How long to wait between connection attempts to a state server address.
+    bootstrap-retry-delay: 5 # default: 5 seconds
+    # How often to refresh state server addresses from the API server.
+    bootstrap-addresses-delay: 10 # default: 10 seconds
 
 Private clouds may need to specify their own custom image metadata, and possibly upload
 Juju tools to cloud storage if no outgoing Internet access is available. In this case,

=== modified file 'environs/config/config.go'
--- environs/config/config.go	2014-01-29 09:58:08 +0000
+++ environs/config/config.go	2014-01-30 17:28:09 +0000
@@ -45,6 +45,19 @@
 	// DefaultSyslogPort is the default port that the syslog UDP listener is
 	// listening on.
 	DefaultSyslogPort int = 514
+
+	// DefaultBootstrapSSHTimeout is the amount of time to wait
+	// contacting a state server, in seconds.
+	DefaultBootstrapSSHTimeout int = 600
+
+	// DefaultBootstrapSSHRetryDelay is the amount of time between
+	// attempts to connect to an address, in seconds.
+	DefaultBootstrapSSHRetryDelay int = 5
+
+	// DefaultBootstrapSSHAddressesDelay is the amount of time between
+	// refreshing the addresses, in seconds. Not too frequent, as we
+	// refresh addresses from the provider each time.
+	DefaultBootstrapSSHAddressesDelay int = 10
 )
 
 // Config holds an immutable environment configuration.
@@ -473,6 +486,26 @@
 	return c.getWithFallback("apt-ftp-proxy", "ftp-proxy")
 }
 
+// BootstrapSSHOpts returns the SSH timeout and retry delays used
+// during bootstrap.
+func (c *Config) BootstrapSSHOpts() SSHTimeoutOpts {
+	opts := SSHTimeoutOpts{
+		Timeout:        time.Duration(DefaultBootstrapSSHTimeout) * time.Second,
+		RetryDelay:     time.Duration(DefaultBootstrapSSHRetryDelay) * time.Second,
+		AddressesDelay: time.Duration(DefaultBootstrapSSHAddressesDelay) * time.Second,
+	}
+	if v, ok := c.m["bootstrap-timeout"].(int); ok && v != 0 {
+		opts.Timeout = time.Duration(v) * time.Second
+	}
+	if v, ok := c.m["bootstrap-retry-delay"].(int); ok && v != 0 {
+		opts.RetryDelay = time.Duration(v) * time.Second
+	}
+	if v, ok := c.m["bootstrap-addresses-delay"].(int); ok && v != 0 {
+		opts.AddressesDelay = time.Duration(v) * time.Second
+	}
+	return opts
+}
+
 // CACert returns the certificate of the CA that signed the state server
 // certificate, in PEM format, and whether the setting is available.
 func (c *Config) CACert() ([]byte, bool) {
@@ -639,6 +672,9 @@
 	"apt-http-proxy":            schema.String(),
 	"apt-https-proxy":           schema.String(),
 	"apt-ftp-proxy":             schema.String(),
+	"bootstrap-timeout":         schema.ForceInt(),
+	"bootstrap-retry-delay":     schema.ForceInt(),
+	"bootstrap-addresses-delay": schema.ForceInt(),
 
 	// Deprecated fields, retain for backwards compatibility.
 	"tools-url": schema.String(),
@@ -653,21 +689,24 @@
 // but some fields listed as optional here are actually mandatory
 // with NoDefaults and are checked at the later Validate stage.
 var alwaysOptional = schema.Defaults{
-	"agent-version":         schema.Omit,
-	"ca-cert":               schema.Omit,
-	"authorized-keys":       schema.Omit,
-	"authorized-keys-path":  schema.Omit,
-	"ca-cert-path":          schema.Omit,
-	"ca-private-key-path":   schema.Omit,
-	"logging-config":        schema.Omit,
-	"provisioner-safe-mode": schema.Omit,
-	"http-proxy":            schema.Omit,
-	"https-proxy":           schema.Omit,
-	"ftp-proxy":             schema.Omit,
-	"apt-http-proxy":        schema.Omit,
-	"apt-https-proxy":       schema.Omit,
-	"apt-ftp-proxy":         schema.Omit,
-	"image-stream":          schema.Omit,
+	"agent-version":             schema.Omit,
+	"ca-cert":                   schema.Omit,
+	"authorized-keys":           schema.Omit,
+	"authorized-keys-path":      schema.Omit,
+	"ca-cert-path":              schema.Omit,
+	"ca-private-key-path":       schema.Omit,
+	"logging-config":            schema.Omit,
+	"provisioner-safe-mode":     schema.Omit,
+	"http-proxy":                schema.Omit,
+	"https-proxy":               schema.Omit,
+	"ftp-proxy":                 schema.Omit,
+	"apt-http-proxy":            schema.Omit,
+	"apt-https-proxy":           schema.Omit,
+	"apt-ftp-proxy":             schema.Omit,
+	"image-stream":              schema.Omit,
+	"bootstrap-timeout":         schema.Omit,
+	"bootstrap-retry-delay":     schema.Omit,
+	"bootstrap-addresses-delay": schema.Omit,
 
 	// Deprecated fields, retain for backwards compatibility.
 	"tools-url": "",
@@ -706,6 +745,9 @@
 		"state-port":                DefaultStatePort,
 		"api-port":                  DefaultAPIPort,
 		"syslog-port":               DefaultSyslogPort,
+		"bootstrap-timeout":         DefaultBootstrapSSHTimeout,
+		"bootstrap-retry-delay":     DefaultBootstrapSSHRetryDelay,
+		"bootstrap-addresses-delay": DefaultBootstrapSSHAddressesDelay,
 	}
 	for attr, val := range alwaysOptional {
 		d[attr] = val
@@ -739,6 +781,9 @@
 	"state-port",
 	"api-port",
 	"syslog-port",
+	"bootstrap-timeout",
+	"bootstrap-retry-delay",
+	"bootstrap-addresses-delay",
 }
 
 var (
@@ -802,6 +847,24 @@
 	return repo
 }
 
+// SSHTimeoutOpts lists the amount of time we will wait for various
+// parts of the SSH connection to complete. This is similar to
+// DialOpts, see http://pad.lv/1258889 about possibly deduplicating
+// them.
+type SSHTimeoutOpts struct {
+	// Timeout is the amount of time to wait contacting a state
+	// server.
+	Timeout time.Duration
+
+	// RetryDelay is the amount of time between attempts to connect to
+	// an address.
+	RetryDelay time.Duration
+
+	// AddressesDelay is the amount of time between refreshing the
+	// addresses.
+	AddressesDelay time.Duration
+}
+
 // ProxyConfigMap returns a map suitable to be applied to a Config to update
 // proxy settings.
 func ProxyConfigMap(proxy osenv.ProxySettings) map[string]interface{} {

=== modified file 'environs/config/config_test.go'
--- environs/config/config_test.go	2014-01-29 09:58:08 +0000
+++ environs/config/config_test.go	2014-01-30 17:28:09 +0000
@@ -513,6 +513,57 @@
 		},
 		err: `syslog-port: expected number, got string\("illegal"\)`,
 	}, {
+		about:       "Explicit bootstrap timeout",
+		useDefaults: config.UseDefaults,
+		attrs: testing.Attrs{
+			"type":              "my-type",
+			"name":              "my-name",
+			"bootstrap-timeout": 300,
+		},
+	}, {
+		about:       "Invalid bootstrap timeout",
+		useDefaults: config.UseDefaults,
+		attrs: testing.Attrs{
+			"type":              "my-type",
+			"name":              "my-name",
+			"bootstrap-timeout": "illegal",
+		},
+		err: `bootstrap-timeout: expected number, got string\("illegal"\)`,
+	}, {
+		about:       "Explicit bootstrap retry delay",
+		useDefaults: config.UseDefaults,
+		attrs: testing.Attrs{
+			"type":                  "my-type",
+			"name":                  "my-name",
+			"bootstrap-retry-delay": 5,
+		},
+	}, {
+		about:       "Invalid bootstrap retry delay",
+		useDefaults: config.UseDefaults,
+		attrs: testing.Attrs{
+			"type":                  "my-type",
+			"name":                  "my-name",
+			"bootstrap-retry-delay": "illegal",
+		},
+		err: `bootstrap-retry-delay: expected number, got string\("illegal"\)`,
+	}, {
+		about:       "Explicit bootstrap addresses delay",
+		useDefaults: config.UseDefaults,
+		attrs: testing.Attrs{
+			"type": "my-type",
+			"name": "my-name",
+			"bootstrap-addresses-delay": 15,
+		},
+	}, {
+		about:       "Invalid bootstrap addresses delay",
+		useDefaults: config.UseDefaults,
+		attrs: testing.Attrs{
+			"type": "my-type",
+			"name": "my-name",
+			"bootstrap-addresses-delay": "illegal",
+		},
+		err: `bootstrap-addresses-delay: expected number, got string\("illegal"\)`,
+	}, {
 		about:       "Invalid logging configuration",
 		useDefaults: config.UseDefaults,
 		attrs: testing.Attrs{
@@ -854,6 +905,25 @@
 	} else {
 		c.Assert(cfg.ProvisionerSafeMode(), gc.Equals, false)
 	}
+	sshOpts := cfg.BootstrapSSHOpts()
+	test.assertDuration(
+		c,
+		"bootstrap-timeout",
+		sshOpts.Timeout,
+		config.DefaultBootstrapSSHTimeout,
+	)
+	test.assertDuration(
+		c,
+		"bootstrap-retry-delay",
+		sshOpts.RetryDelay,
+		config.DefaultBootstrapSSHRetryDelay,
+	)
+	test.assertDuration(
+		c,
+		"bootstrap-addresses-delay",
+		sshOpts.AddressesDelay,
+		config.DefaultBootstrapSSHAddressesDelay,
+	)
 
 	if v, ok := test.attrs["image-stream"]; ok {
 		c.Assert(cfg.ImageStream(), gc.Equals, v)
@@ -888,6 +958,15 @@
 	}
 }
 
+func (test configTest) assertDuration(c *gc.C, name string, actual time.Duration, defaultInSeconds int) {
+	value, ok := test.attrs[name].(int)
+	if !ok || value == 0 {
+		c.Assert(actual, gc.Equals, time.Duration(defaultInSeconds)*time.Second)
+	} else {
+		c.Assert(actual, gc.Equals, time.Duration(value)*time.Second)
+	}
+}
+
 func (s *ConfigSuite) TestConfigAttrs(c *gc.C) {
 	// Normally this is handled by testing.FakeHome
 	s.PatchEnvironment(osenv.JujuLoggingConfigEnvKey, "")
@@ -905,6 +984,9 @@
 		"state-port":                1234,
 		"api-port":                  4321,
 		"syslog-port":               2345,
+		"bootstrap-timeout":         3600,
+		"bootstrap-retry-delay":     30,
+		"bootstrap-addresses-delay": 10,
 		"default-series":            "precise",
 		"charm-store-auth":          "token=auth",
 	}
@@ -921,17 +1003,18 @@
 	attrs["tools-url"] = ""
 	// Default firewall mode is instance
 	attrs["firewall-mode"] = string(config.FwInstance)
-	c.Assert(cfg.AllAttrs(), gc.DeepEquals, attrs)
-	c.Assert(cfg.UnknownAttrs(), gc.DeepEquals, map[string]interface{}{"unknown": "my-unknown"})
+	c.Assert(cfg.AllAttrs(), jc.DeepEquals, attrs)
+	c.Assert(cfg.UnknownAttrs(), jc.DeepEquals, map[string]interface{}{"unknown": "my-unknown"})
 
 	newcfg, err := cfg.Apply(map[string]interface{}{
 		"name":        "new-name",
 		"new-unknown": "my-new-unknown",
 	})
+	c.Assert(err, gc.IsNil)
 
 	attrs["name"] = "new-name"
 	attrs["new-unknown"] = "my-new-unknown"
-	c.Assert(newcfg.AllAttrs(), gc.DeepEquals, attrs)
+	c.Assert(newcfg.AllAttrs(), jc.DeepEquals, attrs)
 }
 
 type validationTest struct {

=== modified file 'provider/common/bootstrap.go'
--- provider/common/bootstrap.go	2014-01-28 04:58:43 +0000
+++ provider/common/bootstrap.go	2014-01-30 17:28:09 +0000
@@ -19,6 +19,7 @@
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/bootstrap"
 	"launchpad.net/juju-core/environs/cloudinit"
+	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/instance"
 	coretools "launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/utils"
@@ -191,10 +192,14 @@
 		exit 1
 	fi
 	`, nonceFile, utils.ShQuote(machineConfig.MachineNonce))
-	// TODO: jam 2013-12-04 bug #1257649
-	// It would be nice if users had some controll over their bootstrap
-	// timeout, since it is unlikely to be a perfect match for all clouds.
-	addr, err := waitSSH(ctx, interrupted, client, checkNonceCommand, inst, DefaultBootstrapSSHTimeout())
+	addr, err := waitSSH(
+		ctx,
+		interrupted,
+		client,
+		checkNonceCommand,
+		inst,
+		machineConfig.Config.BootstrapSSHOpts(),
+	)
 	if err != nil {
 		return err
 	}
@@ -216,33 +221,6 @@
 	})
 }
 
-// SSHTimeoutOpts lists the amount of time we will wait for various parts of
-// the SSH connection to complete. This is similar to DialOpts, see
-// http://pad.lv/1258889 about possibly deduplicating them.
-type SSHTimeoutOpts struct {
-	// Timeout is the amount of time to wait contacting
-	// a state server.
-	Timeout time.Duration
-
-	// ConnectDelay is the amount of time between attempts to connect to an address.
-	ConnectDelay time.Duration
-
-	// AddressesDelay is the amount of time between refreshing the addresses.
-	AddressesDelay time.Duration
-}
-
-// DefaultBootstrapSSHTimeout is the time we'll wait for SSH to come up on the bootstrap node
-func DefaultBootstrapSSHTimeout() SSHTimeoutOpts {
-	return SSHTimeoutOpts{
-		Timeout: 10 * time.Minute,
-
-		ConnectDelay: 5 * time.Second,
-
-		// Not too frequent, as we refresh addresses from the provider each time.
-		AddressesDelay: 10 * time.Second,
-	}
-}
-
 type addresser interface {
 	// Refresh refreshes the addresses for the instance.
 	Refresh() error
@@ -377,7 +355,7 @@
 // the presence of a file on the machine that contains the
 // machine's nonce. The "checkHostScript" is a bash script
 // that performs this file check.
-func waitSSH(ctx environs.BootstrapContext, interrupted <-chan os.Signal, client ssh.Client, checkHostScript string, inst addresser, timeout SSHTimeoutOpts) (addr string, err error) {
+func waitSSH(ctx environs.BootstrapContext, interrupted <-chan os.Signal, client ssh.Client, checkHostScript string, inst addresser, timeout config.SSHTimeoutOpts) (addr string, err error) {
 	globalTimeout := time.After(timeout.Timeout)
 	pollAddresses := time.NewTimer(0)
 
@@ -389,7 +367,7 @@
 		client:          client,
 		stderr:          ctx.Stderr(),
 		active:          make(map[instance.Address]chan struct{}),
-		checkDelay:      timeout.ConnectDelay,
+		checkDelay:      timeout.RetryDelay,
 		checkHostScript: checkHostScript,
 	}
 	defer checker.Kill()

=== modified file 'provider/common/bootstrap_test.go'
--- provider/common/bootstrap_test.go	2014-01-28 04:58:43 +0000
+++ provider/common/bootstrap_test.go	2014-01-30 17:28:09 +0000
@@ -261,9 +261,9 @@
 	return nil, nil
 }
 
-var testSSHTimeout = common.SSHTimeoutOpts{
+var testSSHTimeout = config.SSHTimeoutOpts{
 	Timeout:        coretesting.ShortWait,
-	ConnectDelay:   1 * time.Millisecond,
+	RetryDelay:     1 * time.Millisecond,
 	AddressesDelay: 1 * time.Millisecond,
 }