Merge lp:~axwalk/juju-core/lp1238934-manual-provision-aptrepos into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

environs/manual: process apt sources, bootcmds

This change takes care of processing apt repository
sources and bootcmds from the cloud-init config
and adding appropriate commands into the manual
provisioning script.

In the future, we should work towards using
cloud-init directly on the machine, as discussed
in lp:1238934. This task should be handled in lp:1215777

Fixes #1238934

https://codereview.appspot.com/14775044/

environs/manual: process apt sources, bootcmds

This change takes care of processing apt repository
sources and bootcmds from the cloud-init config
and adding appropriate commands into the manual
provisioning script.

In the future, we should work towards using
cloud-init directly on the machine, as discussed
in lp:1238934. This task should be handled in lp:1215777

Fixes #1238934

https://codereview.appspot.com/14775044/

[Andrew Wilkins (axwalk)]

Reviewers: mp+191561_code.launchpad.net,

Message:
Please take a look.

Description:
environs/manual: process apt sources, bootcmds

This change takes care of processing apt repository
sources and bootcmds from the cloud-init config
and adding appropriate commands into the manual
provisioning script.

The only slightly complicated part to this is
handling of keyids; if a keyid is specified, then
we must fetch the key from keyserver.ubuntu.com
(or some other keyserver, if specified), and then
add it to the apt keyring.

In the future, we should work towards using
cloud-init directly on the machine, as discussed
in lp:1238934. This task should be handled in lp:1215777

Fixes #1238934

https://code.launchpad.net/~axwalk/juju-core/lp1238934-manual-provision-aptrepos/+merge/191561

(do not edit description out of merge proposal)

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

Affected files (+262, -33 lines):
   A [revision details]
   M cloudinit/cloudinit.go
   M cloudinit/options.go
   M environs/cloudinit/cloudinit.go
   M environs/manual/agent.go
   A environs/manual/agent_test.go

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/14775044/

[Tim Penhey (thumper)]

LGTM

https://codereview.appspot.com/14775044/

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/lp1238934-manual-provision-aptrepos into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.710s
ok launchpad.net/juju-core/agent/tools 0.293s
ok launchpad.net/juju-core/bzr 6.901s
ok launchpad.net/juju-core/cert 3.245s
ok launchpad.net/juju-core/charm 0.537s
? launchpad.net/juju-core/charm/hooks [no test files]
ok launchpad.net/juju-core/cloudinit 0.029s
ok launchpad.net/juju-core/cmd 0.236s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]

----------------------------------------------------------------------
PANIC: addmachine.go:0: AddMachineSuite.SetUpTest

[LOG] 2.12880 DEBUG juju.environs.configstore Making /tmp/gocheck-7504504064263669287/5/home/ubuntu/.juju/environments
[LOG] 2.21823 DEBUG juju.environs.tools reading v1.* tools
[LOG] 2.21827 INFO juju environs/testing: uploading FAKE tools 1.17.0-precise-amd64
[LOG] 2.21955 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 2.21957 DEBUG juju.environs.tools no series specified when finding tools, looking for any
[LOG] 2.21963 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:58320/dummyenv/private/tools/streams/v1/index.sjson": file "tools/streams/v1/index.sjson" not found not found
[LOG] 2.21965 DEBUG juju.environs.simplestreams cannot load index "http://127.0.0.1:58320/dummyenv/private/tools/streams/v1/index.sjson": invalid URL "http://127.0.0.1:58320/dummyenv/private/tools/streams/v1/index.sjson" not found
[LOG] 2.21970 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:58320/dummyenv/private/tools/streams/v1/index.json": file "tools/streams/v1/index.json" not found not found
[LOG] 2.21972 DEBUG juju.environs.simplestreams cannot load index "http://127.0.0.1:58320/dummyenv/private/tools/streams/v1/index.json": invalid URL "http://127.0.0.1:58320/dummyenv/private/tools/streams/v1/index.json" not found
[LOG] 2.22000 INFO juju.environs.tools Writing tools/streams/v1/index.json
[LOG] 2.22015 INFO juju.environs.tools Writing tools/streams/v1/com.ubuntu.juju:released:tools.json
[LOG] 2.22020 INFO juju.environs.boostrap bootstrapping environment "dummyenv"
[LOG] 2.22023 INFO juju.environs.tools reading tools with major.minor version 1.17
[LOG] 2.22025 INFO juju.environs.tools filtering tools by version: 1.17.0
[LOG] 2.22026 INFO juju.environs.tools filtering tools by series: precise
[LOG] 2.22029 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 2.22034 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:58320/dummyenv/private/tools/streams/v1/index.sjson": file "tools/streams/v1/index.sjson" not found not found
[LOG] 2.22036 DEBUG juju.environs.simplestreams cannot load index "http://127.0.0.1:58320/dummyenv/private/tools/streams/v1/index.sjson": invalid URL "http://127.0.0.1:58320/dummyenv/private/tools/streams/v1/index.sjson" not found
[LOG] 2.22046 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:58320/dummyenv/private/tools/streams/v1/mirr...

=== modified file 'cloudinit/cloudinit.go'
--- cloudinit/cloudinit.go	2013-10-23 23:51:56 +0000
+++ cloudinit/cloudinit.go	2013-11-01 04:21:16 +0000
@@ -37,8 +37,9 @@
 	}
 }
 
-// source is a Source and a Key
-type source struct {
+// AptSource is an apt(8) source, comprising a source location,
+// with an optional Key.
+type AptSource struct {
 	Source string `yaml:"source"`
 	Key    string `yaml:"key,omitempty"`
 }

=== modified file 'cloudinit/options.go'
--- cloudinit/options.go	2013-10-23 22:17:14 +0000
+++ cloudinit/options.go	2013-11-01 04:21:16 +0000
@@ -37,6 +37,13 @@
 	cfg.set("apt_update", yes, yes)
 }
 
+// AptUpdate returns the value set by SetAptUpdate, or
+// false if no call to SetAptUpdate has been made.
+func (cfg *Config) AptUpdate() bool {
+	update, _ := cfg.attrs["apt_update"].(bool)
+	return update
+}
+
 // SetAptProxy sets the URL to be used as the apt
 // proxy.
 func (cfg *Config) SetAptProxy(url string) {
@@ -60,14 +67,20 @@
 // AddAptSource adds an apt source. The key holds the
 // public key of the source, in the form expected by apt-key(8).
 func (cfg *Config) AddAptSource(name, key string) {
-	src, _ := cfg.attrs["apt_sources"].([]*source)
+	src, _ := cfg.attrs["apt_sources"].([]*AptSource)
 	cfg.attrs["apt_sources"] = append(src,
-		&source{
+		&AptSource{
 			Source: name,
 			Key:    key,
 		})
 }
 
+// AptSources returns the apt sources added with AddAptSource.
+func (cfg *Config) AptSources() []*AptSource {
+	srcs, _ := cfg.attrs["apt_sources"].([]*AptSource)
+	return srcs
+}
+
 // SetDebconfSelections provides preseeded debconf answers
 // for the boot process. The given answers will be used as input
 // to debconf-set-selections(1).
@@ -98,14 +111,11 @@
 	return cmds
 }
 
-// RunCmds returns a list of commands that will be
-// run at first boot.
-//
-// Each element in the resultant slice is either a
-// string or []string, corresponding to how the command
-// was added.
-func (cfg *Config) RunCmds() []interface{} {
-	cmds := cfg.getCmds("runcmd")
+// getCmdStrings returns a slice of interface{}, where
+// each interface's dynamic value is either a string
+// or slice of strings.
+func (cfg *Config) getCmdStrings(kind string) []interface{} {
+	cmds := cfg.getCmds(kind)
 	result := make([]interface{}, len(cmds))
 	for i, cmd := range cmds {
 		if cmd.args != nil {
@@ -117,6 +127,26 @@
 	return result
 }
 
+// BootCmds returns a list of commands added with
+// AddBootCmd*.
+//
+// Each element in the resultant slice is either a
+// string or []string, corresponding to how the command
+// was added.
+func (cfg *Config) BootCmds() []interface{} {
+	return cfg.getCmdStrings("bootcmd")
+}
+
+// RunCmds returns a list of commands that will be
+// run at first boot.
+//
+// Each element in the resultant slice is either a
+// string or []string, corresponding to how the command
+// was added.
+func (cfg *Config) RunCmds() []interface{} {
+	return cfg.getCmdStrings("runcmd")
+}
+
 // AddRunCmd adds a command to be executed
 // at first boot. The command will be run
 // by the shell with any metacharacters retaining

=== modified file 'environs/cloudinit/cloudinit.go'
--- environs/cloudinit/cloudinit.go	2013-10-23 22:17:14 +0000
+++ environs/cloudinit/cloudinit.go	2013-11-01 04:21:16 +0000
@@ -197,8 +197,12 @@
 	cfg.MaybeAddCloudArchiveCloudTools(c)
 
 	if cfg.StateServer {
-		// disable the default mongodb installed by the mongodb-server package.
-		c.AddBootCmd(`echo ENABLE_MONGODB="no" > /etc/default/mongodb`)
+		// Disable the default mongodb installed by the mongodb-server package.
+		// Only do this if the file doesn't exist already, so users can run
+		// their own mongodb server if they wish to.
+		c.AddBootCmd(
+			`[ -f /etc/default/mongodb ] ||
+             (echo ENABLE_MONGODB="no" > /etc/default/mongodb)`)
 
 		if cfg.NeedMongoPPA() {
 			const key = "" // key is loaded from PPA

=== modified file 'environs/manual/agent.go'
--- environs/manual/agent.go	2013-10-02 19:30:07 +0000
+++ environs/manual/agent.go	2013-11-01 04:21:16 +0000
@@ -93,31 +93,88 @@
 	// TODO(axw): 2013-08-23 bug 1215777
 	// Carry out configuration for ssh-keys-per-user,
 	// machine-updates-authkeys, using cloud-init config.
-
-	// Convert runcmds to a series of shell commands.
-	script := []string{"#!/bin/sh"}
-	for _, cmd := range cloudcfg.RunCmds() {
+	//
+	// We should work with smoser to get a supported
+	// command in (or next to) cloud-init for manually
+	// invoking cloud-config. This would address the
+	// above comment by removing the need to generate a
+	// script "by hand".
+
+	// Bootcmds must be run before anything else,
+	// as they may affect package installation.
+	bootcmds, err := cmdlist(cloudcfg.BootCmds())
+	if err != nil {
+		return "", err
+	}
+
+	// Add package sources and packages.
+	pkgcmds, err := addPackageCommands(cloudcfg)
+	if err != nil {
+		return "", err
+	}
+
+	// Runcmds come last.
+	runcmds, err := cmdlist(cloudcfg.RunCmds())
+	if err != nil {
+		return "", err
+	}
+
+	// We prepend "set -xe". This is already in runcmds,
+	// but added here to avoid relying on that to be
+	// invariant.
+	script := []string{"#!/bin/bash", "set -xe"}
+	script = append(script, bootcmds...)
+	script = append(script, pkgcmds...)
+	script = append(script, runcmds...)
+	return strings.Join(script, "\n"), nil
+}
+
+// addPackageCommands returns a slice of commands that, when run,
+// will add the required apt repositories and packages.
+func addPackageCommands(cfg *corecloudinit.Config) ([]string, error) {
+	var cmds []string
+	if len(cfg.AptSources()) > 0 {
+		// Ensure apt-add-repository is available.
+		cmds = append(cmds, "apt-get -y install python-software-properties")
+	}
+	for _, src := range cfg.AptSources() {
+		// PPA keys are obtained by apt-add-repository, from launchpad.
+		if !strings.HasPrefix(src.Source, "ppa:") {
+			if src.Key != "" {
+				key := utils.ShQuote(src.Key)
+				cmd := fmt.Sprintf("printf '%%s\\n' %s | apt-key add -", key)
+				cmds = append(cmds, cmd)
+			}
+		}
+		cmds = append(cmds, "apt-add-repository -y "+utils.ShQuote(src.Source))
+	}
+	if cfg.AptUpdate() {
+		cmds = append(cmds, "apt-get -y update")
+	}
+	// Note: explicitly ignoring apt_upgrade, so as not to trample the target
+	// machine's existing configuration.
+	for _, pkg := range cfg.Packages() {
+		cmd := fmt.Sprintf("apt-get -y install %s", utils.ShQuote(pkg))
+		cmds = append(cmds, cmd)
+	}
+	return cmds, nil
+}
+
+func cmdlist(cmds []interface{}) ([]string, error) {
+	result := make([]string, 0, len(cmds))
+	for _, cmd := range cmds {
 		switch cmd := cmd.(type) {
 		case []string:
 			// Quote args, so shell meta-characters are not interpreted.
 			for i, arg := range cmd[1:] {
 				cmd[i] = utils.ShQuote(arg)
 			}
-			script = append(script, strings.Join(cmd, " "))
+			result = append(result, strings.Join(cmd, " "))
 		case string:
-			script = append(script, cmd)
+			result = append(result, cmd)
 		default:
-			return "", fmt.Errorf("unexpected runcmd type: %T", cmd)
+			return nil, fmt.Errorf("unexpected command type: %T", cmd)
 		}
 	}
-
-	// The first command is "set -xe", which we want to leave in place.
-	head := []string{script[0]}
-	tail := script[1:]
-	for _, pkg := range cloudcfg.Packages() {
-		cmd := fmt.Sprintf("apt-get -y install %s", utils.ShQuote(pkg))
-		head = append(head, cmd)
-	}
-	script = append(head, tail...)
-	return strings.Join(script, "\n"), nil
+	return result, nil
 }

=== added file 'environs/manual/agent_test.go'
--- environs/manual/agent_test.go	1970-01-01 00:00:00 +0000
+++ environs/manual/agent_test.go	2013-11-01 04:21:16 +0000
@@ -0,0 +1,100 @@
+// Copyright 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package manual
+
+import (
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/environs/config"
+	envtools "launchpad.net/juju-core/environs/tools"
+	_ "launchpad.net/juju-core/provider/dummy"
+	coretesting "launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/testing/testbase"
+	"launchpad.net/juju-core/tools"
+	"launchpad.net/juju-core/version"
+)
+
+type agentSuite struct {
+	testbase.LoggingSuite
+}
+
+var _ = gc.Suite(&agentSuite{})
+
+func dummyConfig(c *gc.C, stateServer bool, vers version.Binary) *config.Config {
+	testConfig, err := config.New(config.UseDefaults, coretesting.FakeConfig())
+	c.Assert(err, gc.IsNil)
+	testConfig, err = testConfig.Apply(map[string]interface{}{
+		"type":           "dummy",
+		"state-server":   stateServer,
+		"default-series": vers.Series,
+		"agent-version":  vers.Number.String(),
+	})
+	c.Assert(err, gc.IsNil)
+	return testConfig
+}
+
+func (s *agentSuite) getArgs(c *gc.C, stateServer bool, vers version.Binary) provisionMachineAgentArgs {
+	tools := &tools.Tools{Version: vers}
+	tools.URL = "file:///var/lib/juju/storage/" + envtools.StorageName(vers)
+	return provisionMachineAgentArgs{
+		bootstrap:     stateServer,
+		environConfig: dummyConfig(c, stateServer, vers),
+		machineId:     "0",
+		nonce:         "ya",
+		stateFileURL:  "http://whatever/dotcom",
+		tools:         tools,
+		// stateInfo *state.Info
+		// apiInfo *api.Info
+		agentEnv: make(map[string]string),
+	}
+}
+
+var allSeries = [...]string{"precise", "quantal", "raring", "saucy"}
+
+func checkIff(checker gc.Checker, condition bool) gc.Checker {
+	if condition {
+		return checker
+	}
+	return gc.Not(checker)
+}
+
+func (s *agentSuite) TestAptSources(c *gc.C) {
+	for _, series := range allSeries {
+		vers := version.MustParseBinary("1.16.0-" + series + "-amd64")
+		script, err := provisionMachineAgentScript(s.getArgs(c, true, vers))
+		c.Assert(err, gc.IsNil)
+
+		// Only Precise requires the cloud-tools pocket.
+		//
+		// The only source we add that requires an explicitly
+		// specified key is cloud-tools.
+		needsCloudTools := series == "precise"
+		c.Assert(
+			script,
+			checkIff(gc.Matches, needsCloudTools),
+			"(.|\n)*apt-key add.*(.|\n)*",
+		)
+		c.Assert(
+			script,
+			checkIff(gc.Matches, needsCloudTools),
+			"(.|\n)*apt-add-repository.*cloud-tools(.|\n)*",
+		)
+
+		// Only Quantal requires the PPA (for mongo).
+		needsJujuPPA := series == "quantal"
+		c.Assert(
+			script,
+			checkIff(gc.Matches, needsJujuPPA),
+			"(.|\n)*apt-add-repository.*ppa:juju/stable(.|\n)*",
+		)
+
+		// Only install python-software-properties (apt-add-repository)
+		// if we need to.
+		c.Assert(
+			script,
+			checkIff(gc.Matches, needsCloudTools || needsJujuPPA),
+			"(.|\n)*apt-get -y install.*python-software-properties(.|\n)*",
+		)
+	}
+}