Merge lp:~axwalk/juju-core/lp1270043-drop-machineconfig-api into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

Eliminate MachineConfig client API

The MachineConfig API has been superseded by
ProvisioningScript for manual provisioning.
Manual provisioning has been updated to use
ProvisioningScript, and MachineConfig removed
as it was added in the 1.17.0 release, and so
is unsupported.

Accordingly, statecmd.MachineConfig has been
combined with statecmd.FinishMachineConfig,
and it deals directly with the environs/config
MachineConfig struct.

Fixes #1270043

https://codereview.appspot.com/52900045/

Eliminate MachineConfig client API

The MachineConfig API has been superseded by
ProvisioningScript for manual provisioning.
Manual provisioning has been updated to use
ProvisioningScript, and MachineConfig removed
as it was added in the 1.17.0 release, and so
is unsupported.

Accordingly, statecmd.MachineConfig has been
combined with statecmd.FinishMachineConfig,
and it deals directly with the environs/config
MachineConfig struct.

Fixes #1270043

https://codereview.appspot.com/52900045/

[Andrew Wilkins (axwalk)]

Reviewers: mp+202592_code.launchpad.net,

Message:
Please take a look.

Description:
Eliminate MachineConfig client API

The MachineConfig API has been superseded by
ProvisioningScript for manual provisioning.
Manual provisioning has been updated to use
ProvisioningScript, and MachineConfig removed
as it was added in the 1.17.0 release, and so
is unsupported.

Accordingly, statecmd.MachineConfig has been
combined with statecmd.FinishMachineConfig,
and it deals directly with the environs/config
MachineConfig struct.

Fixes #1270043

https://code.launchpad.net/~axwalk/juju-core/lp1270043-drop-machineconfig-api/+merge/202592

(do not edit description out of merge proposal)

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

Affected files (+149, -180 lines):
   A [revision details]
   M cloudinit/sshinit/configure.go
   M environs/manual/provisioner.go
   M environs/manual/provisioner_test.go
   M state/api/client.go
   M state/api/params/params.go
   M state/apiserver/client/client.go
   M state/apiserver/client/client_test.go
   M state/statecmd/machineconfig.go
   A state/statecmd/machineconfig_test.go

[Martin Packman (gz)]

LGTM, please land for 1.17.1 release. :)

https://codereview.appspot.com/52900045/diff/1/cloudinit/sshinit/configure.go
File cloudinit/sshinit/configure.go (right):

https://codereview.appspot.com/52900045/diff/1/cloudinit/sshinit/configure.go#newcode45
cloudinit/sshinit/configure.go:45: // Run connects to the specified host
over SSH,
*RunConfigureScript

https://codereview.appspot.com/52900045/diff/1/cloudinit/sshinit/configure.go#newcode49
cloudinit/sshinit/configure.go:49: logger.Infof("Provisioning machine
agent on %s", params.Host)
Why move this log line later? Having it right next to the debug one
below is just a little silly, and it seemed fine where it was (logging
even if ConfigureScript errored).

https://codereview.appspot.com/52900045/diff/1/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/52900045/diff/1/environs/manual/provisioner.go#newcode293
environs/manual/provisioner.go:293: func provisionMachineAgent(host
string, mcfg *cloudinit.MachineConfig, stderr io.Writer) error {
Pre-exisiting nit, naming the writer 'stderr' when it can be any writer
is confusing.

https://codereview.appspot.com/52900045/diff/1/state/statecmd/machineconfig_test.go
File state/statecmd/machineconfig_test.go (right):

https://codereview.appspot.com/52900045/diff/1/state/statecmd/machineconfig_test.go#newcode57
state/statecmd/machineconfig_test.go:57:
c.Assert(machineConfig.APIInfo.Addrs, gc.DeepEquals, apiInfo.Addrs)
These two DeepEquals Asserts could be Check instead?

https://codereview.appspot.com/52900045/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/52900045/diff/1/cloudinit/sshinit/configure.go
File cloudinit/sshinit/configure.go (right):

https://codereview.appspot.com/52900045/diff/1/cloudinit/sshinit/configure.go#newcode45
cloudinit/sshinit/configure.go:45: // Run connects to the specified host
over SSH,
On 2014/01/23 13:43:37, gz wrote:
> *RunConfigureScript

Done.

https://codereview.appspot.com/52900045/diff/1/cloudinit/sshinit/configure.go#newcode49
cloudinit/sshinit/configure.go:49: logger.Infof("Provisioning machine
agent on %s", params.Host)
On 2014/01/23 13:43:37, gz wrote:
> Why move this log line later? Having it right next to the debug one
below is
> just a little silly, and it seemed fine where it was (logging even if
> ConfigureScript errored).

I moved it because not everything uses Configure, and DEBUG may not be
enabled. Manual provisioning uses ConfigureScript and RunConfigureScript
separately. It doesn't really matter; I will move the log line back.

https://codereview.appspot.com/52900045/diff/1/environs/manual/provisioner.go
File environs/manual/provisioner.go (right):

https://codereview.appspot.com/52900045/diff/1/environs/manual/provisioner.go#newcode293
environs/manual/provisioner.go:293: func provisionMachineAgent(host
string, mcfg *cloudinit.MachineConfig, stderr io.Writer) error {
On 2014/01/23 13:43:37, gz wrote:
> Pre-exisiting nit, naming the writer 'stderr' when it can be any
writer is
> confusing.

Yeah, good call. I've renamed to progressWriter. Also renamed
sshinit.ConfigureParams.Stderr to ProgressWriter.

https://codereview.appspot.com/52900045/diff/1/state/statecmd/machineconfig_test.go
File state/statecmd/machineconfig_test.go (right):

https://codereview.appspot.com/52900045/diff/1/state/statecmd/machineconfig_test.go#newcode57
state/statecmd/machineconfig_test.go:57:
c.Assert(machineConfig.APIInfo.Addrs, gc.DeepEquals, apiInfo.Addrs)
On 2014/01/23 13:43:37, gz wrote:
> These two DeepEquals Asserts could be Check instead?

Done.

https://codereview.appspot.com/52900045/

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/lp1270043-drop-machineconfig-api into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 1.226s
ok launchpad.net/juju-core/agent/tools 0.231s
ok launchpad.net/juju-core/bzr 7.480s
ok launchpad.net/juju-core/cert 3.524s
ok launchpad.net/juju-core/charm 0.657s
? 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.094s
ok launchpad.net/juju-core/cmd 0.239s
ok launchpad.net/juju-core/cmd/charm-admin 0.865s
? 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 241.869s

----------------------------------------------------------------------
FAIL: machine_test.go:329: MachineSuite.TestManageEnvironRunsInstancePoller

[LOG] 64.43716 DEBUG juju.environs.configstore Making /tmp/gocheck-3724427934598140041/50/home/ubuntu/.juju/environments
[LOG] 64.53346 DEBUG juju.environs.tools reading v1.* tools
[LOG] 64.53350 INFO juju environs/testing: uploading FAKE tools 1.17.1-precise-amd64
[LOG] 64.53443 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 64.53445 DEBUG juju.environs.tools no series specified when finding tools, looking for any
[LOG] 64.53451 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:35346/dummyenv/private/tools/streams/v1/index.sjson": file "tools/streams/v1/index.sjson" not found not found
[LOG] 64.53453 DEBUG juju.environs.simplestreams cannot load index "http://127.0.0.1:35346/dummyenv/private/tools/streams/v1/index.sjson": invalid URL "http://127.0.0.1:35346/dummyenv/private/tools/streams/v1/index.sjson" not found
[LOG] 64.53457 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:35346/dummyenv/private/tools/streams/v1/index.json": file "tools/streams/v1/index.json" not found not found
[LOG] 64.53458 DEBUG juju.environs.simplestreams cannot load index "http://127.0.0.1:35346/dummyenv/private/tools/streams/v1/index.json": invalid URL "http://127.0.0.1:35346/dummyenv/private/tools/streams/v1/index.json" not found
[LOG] 64.53484 INFO juju.environs.tools Writing tools/streams/v1/index.json
[LOG] 64.53490 INFO juju.environs.tools Writing tools/streams/v1/com.ubuntu.juju:released:tools.json
[LOG] 64.53504 INFO juju.environs.bootstrap bootstrapping environment "dummyenv"
[LOG] 64.53506 DEBUG juju.environs.bootstrap looking for bootstrap tools: series="precise", arch=<nil>, version=1.17.1
[LOG] 64.53508 INFO juju.environs.tools reading tools with major.minor version 1.17
[LOG] 64.53509 INFO juju.environs.tools filtering tools by version: 1.17.1
[LOG] 64.53510 INFO juju.environs.tools filtering tools by series: precise
[LOG] 64.53512 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 64.53516 DEBUG juju.environs.simplestreams fetchData failed for "http://127.0.0.1:35346/dummyenv/private/tools/streams/v1/index.sjson": file "tools/streams/v1/index.sjson" not found...

[Go Bot (go-bot)]

The attempt to merge lp:~axwalk/juju-core/lp1270043-drop-machineconfig-api into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core/agent 0.755s
ok launchpad.net/juju-core/agent/tools 0.282s
ok launchpad.net/juju-core/bzr 7.665s
ok launchpad.net/juju-core/cert 3.393s
ok launchpad.net/juju-core/charm 0.647s
? 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.034s
ok launchpad.net/juju-core/cloudinit/sshinit 1.081s
ok launchpad.net/juju-core/cmd 0.251s
ok launchpad.net/juju-core/cmd/charm-admin 0.854s
? 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 242.473s
ok launchpad.net/juju-core/cmd/jujud 60.808s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 9.346s
? 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.051s
ok launchpad.net/juju-core/container/kvm 0.342s
ok launchpad.net/juju-core/container/kvm/mock 0.042s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.375s
? 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.260s
ok launchpad.net/juju-core/environs 3.122s
ok launchpad.net/juju-core/environs/bootstrap 4.580s
ok launchpad.net/juju-core/environs/cloudinit 0.633s
ok launchpad.net/juju-core/environs/config 4.756s
ok launchpad.net/juju-core/environs/configstore 0.043s
ok launchpad.net/juju-core/environs/filestorage 0.035s
ok launchpad.net/juju-core/environs/httpstorage 1.005s
ok launchpad.net/juju-core/environs/imagemetadata 0.525s
? 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.221s
ok launchpad.net/juju-core/environs/manual 11.195s
ok launchpad.net/juju-core/environs/simplestreams 0.416s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.233s
ok launchpad.net/juju-core/environs/storage 1.098s
ok launchpad.net/juju-core/environs/sync 33.745s
ok launchpad.net/juju-core/environs/testing 0.221s
ok launchpad.net/juju-core/environs/tools 6.737s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.016s
ok launchpad.net/juju-core/instance 0.024s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 21.152s
ok launchpad.net/juju-core/juju/osenv 0.020s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.016s
ok launchpad.net/juju-core/log/syslog 0.025s
ok launchpad.net/juju-core/names 0.027s
? launchpad...

=== modified file 'cloudinit/sshinit/configure.go'
--- cloudinit/sshinit/configure.go	2014-01-16 07:10:50 +0000
+++ cloudinit/sshinit/configure.go	2014-01-23 23:05:20 +0000
@@ -28,8 +28,9 @@
 	// Config is the cloudinit config to carry out.
 	Config *cloudinit.Config
 
-	// Stderr is required to present bootstrap progress to the user.
-	Stderr io.Writer
+	// ProgressWriter is an io.Writer to which progress will be written,
+	// for realtime feedback.
+	ProgressWriter io.Writer
 }
 
 // Configure connects to the specified host over SSH,
@@ -40,14 +41,21 @@
 	if err != nil {
 		return err
 	}
-	logger.Debugf("running script on %s: %s", params.Host, script)
+	return RunConfigureScript(script, params)
+}
+
+// RunConfigureScript connects to the specified host over
+// SSH, and executes the provided script which is expected
+// to have been returned by ConfigureScript.
+func RunConfigureScript(script string, params ConfigureParams) error {
+	logger.Debugf("Running script on %s: %s", params.Host, script)
 	client := params.Client
 	if client == nil {
 		client = ssh.DefaultClient
 	}
 	cmd := ssh.Command(params.Host, []string{"sudo", "/bin/bash"}, nil)
 	cmd.Stdin = strings.NewReader(script)
-	cmd.Stderr = params.Stderr
+	cmd.Stderr = params.ProgressWriter
 	return cmd.Run()
 }
 

=== modified file 'environs/manual/provisioner.go'
--- environs/manual/provisioner.go	2014-01-16 07:10:50 +0000
+++ environs/manual/provisioner.go	2014-01-23 23:05:20 +0000
@@ -131,23 +131,24 @@
 		return "", err
 	}
 
-	var configParameters params.MachineConfig
+	var provisioningScript string
 	if stateConn == nil {
-		configParameters, err = client.MachineConfig(machineId)
+		provisioningScript, err = client.ProvisioningScript(machineId, machineParams.Nonce)
+		if err != nil {
+			return "", err
+		}
 	} else {
-		configParameters, err = statecmd.MachineConfig(stateConn.State, machineId)
-	}
-	if err != nil {
-		return "", err
-	}
-	// Gather the information needed by the machine agent to run the provisioning script.
-	mcfg, err := statecmd.FinishMachineConfig(configParameters, machineId, machineParams.Nonce, args.DataDir)
-	if err != nil {
-		return machineId, err
+		mcfg, err := statecmd.MachineConfig(stateConn.State, machineId, machineParams.Nonce, args.DataDir)
+		if err == nil {
+			provisioningScript, err = generateProvisioningScript(mcfg)
+		}
+		if err != nil {
+			return "", err
+		}
 	}
 
 	// Finally, provision the machine agent.
-	err = provisionMachineAgent(hostname, mcfg, args.Stderr)
+	err = runProvisionScript(provisioningScript, hostname, args.Stderr)
 	if err != nil {
 		return machineId, err
 	}
@@ -289,17 +290,29 @@
 	return machineParams, nil
 }
 
-func provisionMachineAgent(host string, mcfg *cloudinit.MachineConfig, stderr io.Writer) error {
+func provisionMachineAgent(host string, mcfg *cloudinit.MachineConfig, progressWriter io.Writer) error {
+	script, err := generateProvisioningScript(mcfg)
+	if err != nil {
+		return err
+	}
+	return runProvisionScript(script, host, progressWriter)
+}
+
+func generateProvisioningScript(mcfg *cloudinit.MachineConfig) (string, error) {
 	cloudcfg := coreCloudinit.New()
 	if err := cloudinit.ConfigureJuju(mcfg, cloudcfg); err != nil {
-		return err
+		return "", err
 	}
 	// Explicitly disabling apt_upgrade so as not to trample
 	// the target machine's existing configuration.
 	cloudcfg.SetAptUpgrade(false)
-	return sshinit.Configure(sshinit.ConfigureParams{
-		Host:   "ubuntu@" + host,
-		Config: cloudcfg,
-		Stderr: stderr,
-	})
+	return sshinit.ConfigureScript(cloudcfg)
+}
+
+func runProvisionScript(script, host string, progressWriter io.Writer) error {
+	params := sshinit.ConfigureParams{
+		Host:           "ubuntu@" + host,
+		ProgressWriter: progressWriter,
+	}
+	return sshinit.RunConfigureScript(script, params)
 }

=== modified file 'environs/manual/provisioner_test.go'
--- environs/manual/provisioner_test.go	2014-01-16 07:10:50 +0000
+++ environs/manual/provisioner_test.go	2014-01-23 23:05:20 +0000
@@ -119,10 +119,7 @@
 	c.Assert(err, gc.IsNil)
 
 	// Now check what we would've configured it with.
-	client := s.APIConn.State.Client()
-	configParams, err := client.MachineConfig(machineId)
-	c.Assert(err, gc.IsNil)
-	mcfg, err := statecmd.FinishMachineConfig(configParams, machineId, state.BootstrapNonce, "/var/lib/juju")
+	mcfg, err := statecmd.MachineConfig(s.State, machineId, state.BootstrapNonce, "/var/lib/juju")
 	c.Assert(err, gc.IsNil)
 	c.Check(mcfg, gc.NotNil)
 	c.Check(mcfg.APIInfo, gc.NotNil)

=== modified file 'provider/common/bootstrap.go'
--- provider/common/bootstrap.go	2014-01-14 09:56:52 +0000
+++ provider/common/bootstrap.go	2014-01-23 23:05:20 +0000
@@ -192,10 +192,10 @@
 		return err
 	}
 	return sshinit.Configure(sshinit.ConfigureParams{
-		Host:   "ubuntu@" + addr,
-		Client: client,
-		Config: cloudcfg,
-		Stderr: ctx.Stderr(),
+		Host:           "ubuntu@" + addr,
+		Client:         client,
+		Config:         cloudcfg,
+		ProgressWriter: ctx.Stderr(),
 	})
 }
 

=== modified file 'state/api/client.go'
--- state/api/client.go	2014-01-18 01:01:42 +0000
+++ state/api/client.go	2014-01-23 23:05:20 +0000
@@ -171,16 +171,6 @@
 	return results.Machines, err
 }
 
-// MachineConfig returns information from the environment config that are
-// needed for machine cloud-init.
-func (c *Client) MachineConfig(machineId string) (result params.MachineConfig, err error) {
-	args := params.MachineConfigParams{
-		MachineId: machineId,
-	}
-	err = c.st.Call("Client", "", "MachineConfig", args, &result)
-	return result, err
-}
-
 // ProvisioningScript returns a shell script that, when run,
 // provisions a machine agent on the machine executing the script.
 func (c *Client) ProvisioningScript(machineId, nonce string) (script string, err error) {

=== modified file 'state/api/params/params.go'
--- state/api/params/params.go	2014-01-17 03:09:38 +0000
+++ state/api/params/params.go	2014-01-23 23:05:20 +0000
@@ -11,7 +11,6 @@
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/instance"
-	"launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/utils/ssh"
 	"launchpad.net/juju-core/version"
 )
@@ -516,23 +515,6 @@
 	SyslogPort              int
 }
 
-type MachineConfigParams struct {
-	MachineId string
-}
-
-// MachineConfig contains information from the environment config that is
-// needed for a machine cloud-init.
-type MachineConfig struct {
-	EnvironAttrs map[string]interface{}
-	Tools        *tools.Tools
-	// state.Info and api.Info attributes (cannot use state.Info, api.Info directly due to import loops)
-	StateAddrs []string
-	APIAddrs   []string
-	CACert     []byte
-	Tag        string
-	Password   string
-}
-
 // ProvisioningScriptParams contains the parameters for the
 // ProvisioningScript client API call.
 type ProvisioningScriptParams struct {

=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2014-01-16 07:10:50 +0000
+++ state/apiserver/client/client.go	2014-01-23 23:05:20 +0000
@@ -601,21 +601,11 @@
 	return newJobs, nil
 }
 
-// MachineConfig returns information from the environment config that is
-// needed for machine cloud-init (both state servers and host nodes).
-func (c *Client) MachineConfig(args params.MachineConfigParams) (params.MachineConfig, error) {
-	return statecmd.MachineConfig(c.api.state, args.MachineId)
-}
-
 // ProvisioningScript returns a shell script that, when run,
 // provisions a machine agent on the machine executing the script.
 func (c *Client) ProvisioningScript(args params.ProvisioningScriptParams) (params.ProvisioningScriptResult, error) {
 	var result params.ProvisioningScriptResult
-	mcfgParams, err := statecmd.MachineConfig(c.api.state, args.MachineId)
-	if err != nil {
-		return result, err
-	}
-	mcfg, err := statecmd.FinishMachineConfig(mcfgParams, args.MachineId, args.Nonce, args.DataDir)
+	mcfg, err := statecmd.MachineConfig(c.api.state, args.MachineId, args.Nonce, args.DataDir)
 	if err != nil {
 		return result, err
 	}

=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2014-01-20 23:32:00 +0000
+++ state/apiserver/client/client_test.go	2014-01-23 23:05:20 +0000
@@ -15,10 +15,8 @@
 	coreCloudinit "launchpad.net/juju-core/cloudinit"
 	"launchpad.net/juju-core/cloudinit/sshinit"
 	"launchpad.net/juju-core/constraints"
-	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/cloudinit"
 	"launchpad.net/juju-core/environs/config"
-	envtools "launchpad.net/juju-core/environs/tools"
 	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/state"
@@ -28,7 +26,6 @@
 	"launchpad.net/juju-core/state/statecmd"
 	coretesting "launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
-	coretools "launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/version"
 )
 
@@ -1654,71 +1651,6 @@
 	c.Assert(results.Machines, gc.HasLen, 1)
 }
 
-func (s *clientSuite) TestMachineConfig(c *gc.C) {
-	addrs := []instance.Address{instance.NewAddress("1.2.3.4")}
-	hc := instance.MustParseHardware("mem=4G arch=amd64")
-	apiParams := params.AddMachineParams{
-		Jobs:       []params.MachineJob{params.JobHostUnits},
-		InstanceId: instance.Id("1234"),
-		Nonce:      "foo",
-		HardwareCharacteristics: hc,
-		Addrs: addrs,
-	}
-	machines, err := s.APIState.Client().AddMachines([]params.AddMachineParams{apiParams})
-	c.Assert(err, gc.IsNil)
-	c.Assert(len(machines), gc.Equals, 1)
-
-	machineId := machines[0].Machine
-	machineConfig, err := s.APIState.Client().MachineConfig(machineId)
-	c.Assert(err, gc.IsNil)
-
-	envConfig, err := s.State.EnvironConfig()
-	c.Assert(err, gc.IsNil)
-	env, err := environs.New(envConfig)
-	c.Assert(err, gc.IsNil)
-	stateInfo, apiInfo, err := env.StateInfo()
-	c.Assert(err, gc.IsNil)
-	c.Assert(machineConfig.StateAddrs, gc.DeepEquals, stateInfo.Addrs)
-	c.Assert(machineConfig.APIAddrs, gc.DeepEquals, apiInfo.Addrs)
-	c.Assert(machineConfig.Tag, gc.Equals, "machine-0")
-	caCert, _ := envConfig.CACert()
-	c.Assert(machineConfig.CACert, gc.DeepEquals, caCert)
-	c.Assert(machineConfig.Password, gc.Not(gc.Equals), "")
-	c.Assert(machineConfig.Tools.URL, gc.Not(gc.Equals), "")
-	c.Assert(machineConfig.EnvironAttrs["name"], gc.Equals, "dummyenv")
-}
-
-func (s *clientSuite) TestMachineConfigNoArch(c *gc.C) {
-	apiParams := params.AddMachineParams{
-		Jobs:       []params.MachineJob{params.JobHostUnits},
-		InstanceId: instance.Id("1234"),
-		Nonce:      "foo",
-	}
-	machines, err := s.APIState.Client().AddMachines([]params.AddMachineParams{apiParams})
-	c.Assert(err, gc.IsNil)
-	c.Assert(len(machines), gc.Equals, 1)
-	_, err = s.APIState.Client().MachineConfig(machines[0].Machine)
-	c.Assert(err, gc.ErrorMatches, fmt.Sprintf("arch is not set for %q", "machine-"+machines[0].Machine))
-}
-
-func (s *clientSuite) TestMachineConfigNoTools(c *gc.C) {
-	s.PatchValue(&envtools.DefaultBaseURL, "")
-	addrs := []instance.Address{instance.NewAddress("1.2.3.4")}
-	hc := instance.MustParseHardware("mem=4G arch=amd64")
-	apiParams := params.AddMachineParams{
-		Series:     "quantal",
-		Jobs:       []params.MachineJob{params.JobHostUnits},
-		InstanceId: instance.Id("1234"),
-		Nonce:      "foo",
-		HardwareCharacteristics: hc,
-		Addrs: addrs,
-	}
-	machines, err := s.APIState.Client().AddMachines([]params.AddMachineParams{apiParams})
-	c.Assert(err, gc.IsNil)
-	_, err = s.APIState.Client().MachineConfig(machines[0].Machine)
-	c.Assert(err, gc.ErrorMatches, coretools.ErrNoMatches.Error())
-}
-
 func (s *clientSuite) TestProvisioningScript(c *gc.C) {
 	// Inject a machine and then call the ProvisioningScript API.
 	// The result should be the same as when calling MachineConfig,
@@ -1734,14 +1666,12 @@
 	c.Assert(err, gc.IsNil)
 	c.Assert(len(machines), gc.Equals, 1)
 	machineId := machines[0].Machine
-	machineConfig, err := s.APIState.Client().MachineConfig(machineId)
-	c.Assert(err, gc.IsNil)
 	// Call ProvisioningScript. Normally ProvisioningScript and
 	// MachineConfig are mutually exclusive; both of them will
 	// allocate a state/api password for the machine agent.
 	script, err := s.APIState.Client().ProvisioningScript(machineId, apiParams.Nonce)
 	c.Assert(err, gc.IsNil)
-	mcfg, err := statecmd.FinishMachineConfig(machineConfig, machineId, apiParams.Nonce, "")
+	mcfg, err := statecmd.MachineConfig(s.State, machineId, apiParams.Nonce, "")
 	c.Assert(err, gc.IsNil)
 	cloudcfg := coreCloudinit.New()
 	err = cloudinit.ConfigureJuju(mcfg, cloudcfg)

=== modified file 'state/statecmd/machineconfig.go'
--- state/statecmd/machineconfig.go	2014-01-17 03:09:38 +0000
+++ state/statecmd/machineconfig.go	2014-01-23 23:05:20 +0000
@@ -9,11 +9,8 @@
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/cloudinit"
-	"launchpad.net/juju-core/environs/config"
 	envtools "launchpad.net/juju-core/environs/tools"
 	"launchpad.net/juju-core/state"
-	"launchpad.net/juju-core/state/api"
-	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/tools"
 )
 
@@ -30,87 +27,58 @@
 }
 
 // MachineConfig returns information from the environment config that is
-// needed for machine cloud-init (both state servers and host nodes).
-// The code is here so that it can be shared between the API server and the CLI
-// for maintaining compatibiility with juju-1.16 (where the API MachineConfig
-// did not exist). When we drop 1.16 compatibility, this code should be moved
-// back into api.Client
-func MachineConfig(st *state.State, machineId string) (params.MachineConfig, error) {
-	result := params.MachineConfig{}
+// needed for machine cloud-init (for non-state servers only).
+//
+// The code is here so that it can be shared between the API server
+// (for ProvisioningScript) and the CLI (for manual provisioning) for maintaining
+// compatibiility with juju-1.16 (where the API MachineConfig did not exist).
+// When we drop 1.16 compatibility, this code should be moved into apiserver.
+func MachineConfig(st *state.State, machineId, nonce, dataDir string) (*cloudinit.MachineConfig, error) {
 	environConfig, err := st.EnvironConfig()
 	if err != nil {
-		return result, err
+		return nil, err
 	}
-	// The basic information.
-	result.EnvironAttrs = environConfig.AllAttrs()
 
 	// Get the machine so we can get its series and arch.
 	// If the Arch is not set in hardware-characteristics,
 	// an error is returned.
 	machine, err := st.Machine(machineId)
 	if err != nil {
-		return result, err
+		return nil, err
 	}
 	hc, err := machine.HardwareCharacteristics()
 	if err != nil {
-		return result, err
+		return nil, err
 	}
 	if hc.Arch == nil {
-		return result, fmt.Errorf("arch is not set for %q", machine.Tag())
+		return nil, fmt.Errorf("arch is not set for %q", machine.Tag())
 	}
 
 	// Find the appropriate tools information.
 	env, err := environs.New(environConfig)
 	if err != nil {
-		return result, err
+		return nil, err
 	}
 	tools, err := findInstanceTools(env, machine.Series(), *hc.Arch)
 	if err != nil {
-		return result, err
+		return nil, err
 	}
-	result.Tools = tools
 
 	// Find the secrets and API endpoints.
 	auth, err := environs.NewEnvironAuthenticator(env)
 	if err != nil {
-		return result, err
+		return nil, err
 	}
 	stateInfo, apiInfo, err := auth.SetupAuthentication(machine)
 	if err != nil {
-		return result, err
+		return nil, err
 	}
-	result.APIAddrs = apiInfo.Addrs
-	result.StateAddrs = stateInfo.Addrs
-	result.CACert = stateInfo.CACert
-	result.Password = stateInfo.Password
-	result.Tag = stateInfo.Tag
-	return result, nil
-}
 
-// FinishMachineConfig is shared by environs/manual and state/apiserver/client
-// to create a complete cloudinit.MachineConfig from a params.MachineConfig.
-func FinishMachineConfig(configParameters params.MachineConfig, machineId, nonce, dataDir string) (*cloudinit.MachineConfig, error) {
-	stateInfo := &state.Info{
-		Addrs:    configParameters.StateAddrs,
-		Password: configParameters.Password,
-		Tag:      configParameters.Tag,
-		CACert:   configParameters.CACert,
-	}
-	apiInfo := &api.Info{
-		Addrs:    configParameters.APIAddrs,
-		Password: configParameters.Password,
-		Tag:      configParameters.Tag,
-		CACert:   configParameters.CACert,
-	}
-	environConfig, err := config.New(config.NoDefaults, configParameters.EnvironAttrs)
-	if err != nil {
-		return nil, err
-	}
 	mcfg := environs.NewMachineConfig(machineId, nonce, stateInfo, apiInfo)
 	if dataDir != "" {
 		mcfg.DataDir = dataDir
 	}
-	mcfg.Tools = configParameters.Tools
+	mcfg.Tools = tools
 	err = environs.FinishMachineConfig(mcfg, environConfig, constraints.Value{})
 	if err != nil {
 		return nil, err

=== added file 'state/statecmd/machineconfig_test.go'
--- state/statecmd/machineconfig_test.go	1970-01-01 00:00:00 +0000
+++ state/statecmd/machineconfig_test.go	2014-01-23 23:05:20 +0000
@@ -0,0 +1,90 @@
+// Copyright 2012, 2013 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package statecmd_test
+
+import (
+	"fmt"
+	stdtesting "testing"
+
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/environs"
+	envtools "launchpad.net/juju-core/environs/tools"
+	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/juju/testing"
+	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/state/statecmd"
+	coretesting "launchpad.net/juju-core/testing"
+	coretools "launchpad.net/juju-core/tools"
+)
+
+func TestPackage(t *stdtesting.T) {
+	coretesting.MgoTestPackage(t)
+}
+
+type machineConfigSuite struct {
+	testing.JujuConnSuite
+}
+
+var _ = gc.Suite(&machineConfigSuite{})
+
+func (s *machineConfigSuite) TestMachineConfig(c *gc.C) {
+	addrs := []instance.Address{instance.NewAddress("1.2.3.4")}
+	hc := instance.MustParseHardware("mem=4G arch=amd64")
+	apiParams := params.AddMachineParams{
+		Jobs:       []params.MachineJob{params.JobHostUnits},
+		InstanceId: instance.Id("1234"),
+		Nonce:      "foo",
+		HardwareCharacteristics: hc,
+		Addrs: addrs,
+	}
+	machines, err := s.APIState.Client().AddMachines([]params.AddMachineParams{apiParams})
+	c.Assert(err, gc.IsNil)
+	c.Assert(len(machines), gc.Equals, 1)
+
+	machineId := machines[0].Machine
+	machineConfig, err := statecmd.MachineConfig(s.State, machineId, apiParams.Nonce, "")
+	c.Assert(err, gc.IsNil)
+
+	envConfig, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	env, err := environs.New(envConfig)
+	c.Assert(err, gc.IsNil)
+	stateInfo, apiInfo, err := env.StateInfo()
+	c.Assert(err, gc.IsNil)
+	c.Check(machineConfig.StateInfo.Addrs, gc.DeepEquals, stateInfo.Addrs)
+	c.Check(machineConfig.APIInfo.Addrs, gc.DeepEquals, apiInfo.Addrs)
+	c.Assert(machineConfig.Tools.URL, gc.Not(gc.Equals), "")
+}
+
+func (s *machineConfigSuite) TestMachineConfigNoArch(c *gc.C) {
+	apiParams := params.AddMachineParams{
+		Jobs:       []params.MachineJob{params.JobHostUnits},
+		InstanceId: instance.Id("1234"),
+		Nonce:      "foo",
+	}
+	machines, err := s.APIState.Client().AddMachines([]params.AddMachineParams{apiParams})
+	c.Assert(err, gc.IsNil)
+	c.Assert(len(machines), gc.Equals, 1)
+	_, err = statecmd.MachineConfig(s.State, machines[0].Machine, apiParams.Nonce, "")
+	c.Assert(err, gc.ErrorMatches, fmt.Sprintf("arch is not set for %q", "machine-"+machines[0].Machine))
+}
+
+func (s *machineConfigSuite) TestMachineConfigNoTools(c *gc.C) {
+	s.PatchValue(&envtools.DefaultBaseURL, "")
+	addrs := []instance.Address{instance.NewAddress("1.2.3.4")}
+	hc := instance.MustParseHardware("mem=4G arch=amd64")
+	apiParams := params.AddMachineParams{
+		Series:     "quantal",
+		Jobs:       []params.MachineJob{params.JobHostUnits},
+		InstanceId: instance.Id("1234"),
+		Nonce:      "foo",
+		HardwareCharacteristics: hc,
+		Addrs: addrs,
+	}
+	machines, err := s.APIState.Client().AddMachines([]params.AddMachineParams{apiParams})
+	c.Assert(err, gc.IsNil)
+	_, err = statecmd.MachineConfig(s.State, machines[0].Machine, apiParams.Nonce, "")
+	c.Assert(err, gc.ErrorMatches, coretools.ErrNoMatches.Error())
+}