Merge lp:~axwalk/juju-core/provisioningscript-client-api into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

ProvisioningScript client API

This is a new client API that, given the ID and nonce
of a machine injected into state, returns a script
that can be executed on that machine to install and
configure a Juju machine agent.

The existing MachineConfig client API call has been
modified: it was taking a series and arch. Series is
guaranteed to be in state.Machine; the arch is not,
but we make it a requirement for calling MachineConfig.
In the only place where MachineConfig is already called,
this is true. ProvisioningScript is expected to be
used similarly.

I have tested "live" by temporarily modifying
environs/manual/provisioner.go to call ProvisioningScript
and then running it via utils/ssh.Command.

https://codereview.appspot.com/53040043/

ProvisioningScript client API

This is a new client API that, given the ID and nonce
of a machine injected into state, returns a script
that can be executed on that machine to install and
configure a Juju machine agent.

The existing MachineConfig client API call has been
modified: it was taking a series and arch. Series is
guaranteed to be in state.Machine; the arch is not,
but we make it a requirement for calling MachineConfig.
In the only place where MachineConfig is already called,
this is true. ProvisioningScript is expected to be
used similarly.

I have tested "live" by temporarily modifying
environs/manual/provisioner.go to call ProvisioningScript
and then running it via utils/ssh.Command.

https://codereview.appspot.com/53040043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+201893_code.launchpad.net,

Message:
Please take a look.

Description:
ProvisioningScript client API

This is a new client API that, given the ID and nonce
of a machine injected into state, returns a script
that can be executed on that machine to install and
configure a Juju machine agent.

The existing MachineConfig client API call has been
modified: it was taking a series and arch. Series is
guaranteed to be in state.Machine; the arch is not,
but we make it a requirement for calling MachineConfig.
In the only place where MachineConfig is already called,
this is true. ProvisioningScript is expected to be
used similarly.

I have tested "live" by temporarily modifying
environs/manual/provisioner.go to call ProvisioningScript
and then running it via utils/ssh.Command.

https://code.launchpad.net/~axwalk/juju-core/provisioningscript-client-api/+merge/201893

(do not edit description out of merge proposal)

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

Affected files (+197, -69 lines):
   A [revision details]
   M cloudinit/sshinit/configure.go
   M cloudinit/sshinit/configure_test.go
   A cloudinit/sshinit/export_test.go
   M cloudinit/sshinit/suite_test.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

[Roger Peppe (rogpeppe)]

LGTM

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

https://codereview.appspot.com/53040043/diff/1/state/statecmd/machineconfig.go#newcode92
state/statecmd/machineconfig.go:92: func
FinishMachineConfig(configParameters params.MachineConfig, machineId,
nonce, dataDir string) (*cloudinit.MachineConfig, error) {
This seems fine, but I don't entirely understand its rationale. Perhaps
you could explain why this needs to be separate from MachineConfig?

https://codereview.appspot.com/53040043/

[Dimiter Naydenov (dimitern)]

Looking good, apart from some grumbles below.

https://codereview.appspot.com/53040043/diff/1/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/53040043/diff/1/state/api/client.go#newcode191
state/api/client.go:191: return result.Script, err
I think it's a bit better like this:
err = ...
if err != nil {
   return "", err
}
return result.Script, nil

(like other similar methods).

https://codereview.appspot.com/53040043/diff/1/state/api/params/params.go
File state/api/params/params.go (left):

https://codereview.appspot.com/53040043/diff/1/state/api/params/params.go#oldcode521
state/api/params/params.go:521: Series string
I'm not sure we can remove these fields just yet (1.16 compatibility).

https://codereview.appspot.com/53040043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/53040043/diff/1/state/api/params/params.go#newcode545
state/api/params/params.go:545: // ProvisioningSCript client API call.
s/ProvisioningSCript/ProvisioningScript/ (and in the comment above as
well)

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client.go#newcode18
state/apiserver/client/client.go:18: coreCloudinit
"launchpad.net/juju-core/cloudinit"
I'd prefer corecloudinit for consistency sake.

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client_test.go#newcode15
state/apiserver/client/client_test.go:15: coreCloudinit
"launchpad.net/juju-core/cloudinit"
corecloudinit please

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client_test.go#newcode1764
state/apiserver/client/client_test.go:1764:
I'd like to see a test about using ProvisioningScript against 1.16 API
server, to make sure we're handling it gracefully when it's not
implemented.

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

https://codereview.appspot.com/53040043/diff/1/state/statecmd/machineconfig.go#newcode59
state/statecmd/machineconfig.go:59: return result, fmt.Errorf("arch is
not set for %s", machine.Tag())
s/for %s/for %q/ ?

https://codereview.appspot.com/53040043/diff/1/state/statecmd/machineconfig.go#newcode92
state/statecmd/machineconfig.go:92: func
FinishMachineConfig(configParameters params.MachineConfig, machineId,
nonce, dataDir string) (*cloudinit.MachineConfig, error) {
On 2014/01/16 12:02:47, rog wrote:
> This seems fine, but I don't entirely understand its rationale.
Perhaps you
> could explain why this needs to be separate from MachineConfig?

I think it's mostly because of the provisioner, which also needs to call
this separately from MachineConfig.

https://codereview.appspot.com/53040043/

[Kapil Thangavelu (hazmat)]

On 2014/01/16 12:02:47, rog wrote:
> LGTM

> This seems fine, but I don't entirely understand its rationale.
Perhaps you
> could explain why this needs to be separate from MachineConfig?

Machine config exposes data (mostly internal imo) via the api that a
client has to then format into an install script. This is a fragile
error prone process as it is effectively reimplementing internal juju
details and the various cloudinit modules and options within juju core
are subject to change without notice. By exposing a provisioning script
to the client, the client basically gets a blob that it can use for
initialization which is a much cleaner api then exposing internal
details and leaving the burden on the client.

https://codereview.appspot.com/53040043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/53040043/diff/1/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/53040043/diff/1/state/api/client.go#newcode191
state/api/client.go:191: return result.Script, err
On 2014/01/16 13:08:18, dimitern wrote:
> I think it's a bit better like this:
> err = ...
> if err != nil {
> return "", err
> }
> return result.Script, nil

> (like other similar methods).

Sure, though there's quite a lot that do it the way I did it (I just
copied MachineConfig).

https://codereview.appspot.com/53040043/diff/1/state/api/params/params.go
File state/api/params/params.go (left):

https://codereview.appspot.com/53040043/diff/1/state/api/params/params.go#oldcode521
state/api/params/params.go:521: Series string
On 2014/01/16 13:08:18, dimitern wrote:
> I'm not sure we can remove these fields just yet (1.16 compatibility).

MachineConfig was added in 1.17.0. I may just remove MachineConfig
altogether. It's only used by manual provisioning, which could just use
the new API instead.

https://codereview.appspot.com/53040043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/53040043/diff/1/state/api/params/params.go#newcode545
state/api/params/params.go:545: // ProvisioningSCript client API call.
On 2014/01/16 13:08:18, dimitern wrote:
> s/ProvisioningSCript/ProvisioningScript/ (and in the comment above as
well)

Done.

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client.go#newcode18
state/apiserver/client/client.go:18: coreCloudinit
"launchpad.net/juju-core/cloudinit"
On 2014/01/16 13:08:18, dimitern wrote:
> I'd prefer corecloudinit for consistency sake.

coreCloudinit is consistent with the rest of the code base (there are
zero instances of corecloudinit, a bunch of coreCloudinit).

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client_test.go
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/53040043/diff/1/state/apiserver/client/client_test.go#newcode1764
state/apiserver/client/client_test.go:1764:
On 2014/01/16 13:08:18, dimitern wrote:
> I'd like to see a test about using ProvisioningScript against 1.16 API
server,
> to make sure we're handling it gracefully when it's not implemented.

I was going to do this, but it doesn't make sense to yet*. The only
place it's used is by an external client, where the version is
guaranteed to include the method.

* When I change environs/manual to use ProvisioningScript instead of
MachineConfig, it may make sense. However, I don't see *any* tests for
compatibility, nor do I know how this would even be accomplished. If
there really are tests for the 1dot16 code paths, please point me at
one.

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

https://codereview.appspot.com/53040043/diff/1/state/statecmd/machineconfig.go#newcode59
state/statecmd/machineconfig.go:59: return result, fmt.Errorf("arch is
not...

=== modified file 'cloudinit/sshinit/configure.go'
--- cloudinit/sshinit/configure.go	2014-01-13 06:58:19 +0000
+++ cloudinit/sshinit/configure.go	2014-01-17 03:18:53 +0000
@@ -36,7 +36,7 @@
 // and executes a script that carries out cloud-config.
 func Configure(params ConfigureParams) error {
 	logger.Infof("Provisioning machine agent on %s", params.Host)
-	script, err := generateScript(params.Config)
+	script, err := ConfigureScript(params.Config)
 	if err != nil {
 		return err
 	}
@@ -51,9 +51,9 @@
 	return cmd.Run()
 }
 
-// generateScript generates the script that applies
+// ConfigureScript generates the bash script that applies
 // the specified cloud-config.
-func generateScript(cloudcfg *cloudinit.Config) (string, error) {
+func ConfigureScript(cloudcfg *cloudinit.Config) (string, error) {
 	// TODO(axw): 2013-08-23 bug 1215777
 	// Carry out configuration for ssh-keys-per-user,
 	// machine-updates-authkeys, using cloud-init config.

=== modified file 'cloudinit/sshinit/configure_test.go'
--- cloudinit/sshinit/configure_test.go	2013-12-18 21:47:48 +0000
+++ cloudinit/sshinit/configure_test.go	2014-01-17 03:18:53 +0000
@@ -1,7 +1,7 @@
 // Copyright 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-package sshinit
+package sshinit_test
 
 import (
 	"regexp"
@@ -9,6 +9,7 @@
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/cloudinit"
+	"launchpad.net/juju-core/cloudinit/sshinit"
 	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/environs"
 	envcloudinit "launchpad.net/juju-core/environs/cloudinit"
@@ -79,12 +80,12 @@
 	return gc.Not(checker)
 }
 
-var aptgetRegexp = "(.|\n)*" + regexp.QuoteMeta(aptget)
+var aptgetRegexp = "(.|\n)*" + regexp.QuoteMeta(sshinit.Aptget)
 
 func (s *configureSuite) TestAptSources(c *gc.C) {
 	for _, series := range allSeries {
 		vers := version.MustParseBinary("1.16.0-" + series + "-amd64")
-		script, err := generateScript(s.getCloudConfig(c, true, vers))
+		script, err := sshinit.ConfigureScript(s.getCloudConfig(c, true, vers))
 		c.Assert(err, gc.IsNil)
 
 		// Only Precise requires the cloud-tools pocket.
@@ -122,7 +123,7 @@
 }
 
 func assertScriptMatches(c *gc.C, cfg *cloudinit.Config, pattern string, match bool) {
-	script, err := generateScript(cfg)
+	script, err := sshinit.ConfigureScript(cfg)
 	c.Assert(err, gc.IsNil)
 	checker := gc.Matches
 	if !match {

=== added file 'cloudinit/sshinit/export_test.go'
--- cloudinit/sshinit/export_test.go	1970-01-01 00:00:00 +0000
+++ cloudinit/sshinit/export_test.go	2014-01-17 03:18:53 +0000
@@ -0,0 +1,6 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package sshinit
+
+const Aptget = aptget

=== modified file 'cloudinit/sshinit/suite_test.go'
--- cloudinit/sshinit/suite_test.go	2013-11-15 08:47:05 +0000
+++ cloudinit/sshinit/suite_test.go	2014-01-17 03:18:53 +0000
@@ -1,7 +1,7 @@
 // Copyright 2013 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-package sshinit
+package sshinit_test
 
 import (
 	stdtesting "testing"

=== modified file 'environs/manual/provisioner.go'
--- environs/manual/provisioner.go	2014-01-09 09:39:56 +0000
+++ environs/manual/provisioner.go	2014-01-17 03:18:53 +0000
@@ -14,8 +14,6 @@
 
 	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"
 	"launchpad.net/juju-core/instance"
@@ -118,10 +116,6 @@
 	if err != nil {
 		return "", err
 	}
-	arch := ""
-	if machineParams.HardwareCharacteristics.Arch != nil {
-		arch = *machineParams.HardwareCharacteristics.Arch
-	}
 
 	// Inform Juju that the machine exists.
 	machineId, err = recordMachineInState(client, *machineParams)
@@ -139,20 +133,15 @@
 
 	var configParameters params.MachineConfig
 	if stateConn == nil {
-		configParameters, err = client.MachineConfig(machineId, machineParams.Series, arch)
+		configParameters, err = client.MachineConfig(machineId)
 	} else {
-		request := params.MachineConfigParams{
-			MachineId: machineId,
-			Series:    machineParams.Series,
-			Arch:      arch,
-		}
-		configParameters, err = statecmd.MachineConfig(stateConn.State, request)
+		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 := finishMachineConfig(configParameters, machineId, machineParams.Nonce, args.DataDir)
+	mcfg, err := statecmd.FinishMachineConfig(configParameters, machineId, machineParams.Nonce, args.DataDir)
 	if err != nil {
 		return machineId, err
 	}
@@ -300,35 +289,6 @@
 	return machineParams, nil
 }
 
-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
-	err = environs.FinishMachineConfig(mcfg, environConfig, constraints.Value{})
-	if err != nil {
-		return nil, err
-	}
-	return mcfg, nil
-}
-
 func provisionMachineAgent(host string, mcfg *cloudinit.MachineConfig, stderr io.Writer) error {
 	cloudcfg := coreCloudinit.New()
 	if err := cloudinit.ConfigureJuju(mcfg, cloudcfg); err != nil {

=== modified file 'environs/manual/provisioner_test.go'
--- environs/manual/provisioner_test.go	2014-01-09 09:39:56 +0000
+++ environs/manual/provisioner_test.go	2014-01-17 03:18:53 +0000
@@ -14,6 +14,7 @@
 	"launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api/params"
+	"launchpad.net/juju-core/state/statecmd"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/version"
 )
@@ -119,9 +120,9 @@
 
 	// Now check what we would've configured it with.
 	client := s.APIConn.State.Client()
-	configParams, err := client.MachineConfig(machineId, series, arch)
+	configParams, err := client.MachineConfig(machineId)
 	c.Assert(err, gc.IsNil)
-	mcfg, err := finishMachineConfig(configParams, machineId, state.BootstrapNonce, "/var/lib/juju")
+	mcfg, err := statecmd.FinishMachineConfig(configParams, machineId, state.BootstrapNonce, "/var/lib/juju")
 	c.Assert(err, gc.IsNil)
 	c.Check(mcfg, gc.NotNil)
 	c.Check(mcfg.APIInfo, gc.NotNil)

=== modified file 'state/api/client.go'
--- state/api/client.go	2014-01-12 20:28:43 +0000
+++ state/api/client.go	2014-01-17 03:18:53 +0000
@@ -171,16 +171,28 @@
 
 // MachineConfig returns information from the environment config that are
 // needed for machine cloud-init.
-func (c *Client) MachineConfig(machineId, series, arch string) (result params.MachineConfig, err error) {
+func (c *Client) MachineConfig(machineId string) (result params.MachineConfig, err error) {
 	args := params.MachineConfigParams{
 		MachineId: machineId,
-		Series:    series,
-		Arch:      arch,
 	}
 	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) {
+	args := params.ProvisioningScriptParams{
+		MachineId: machineId,
+		Nonce:     nonce,
+	}
+	var result params.ProvisioningScriptResult
+	if err = c.st.Call("Client", "", "ProvisioningScript", args, &result); err != nil {
+		return "", err
+	}
+	return result.Script, nil
+}
+
 // DestroyMachines removes a given set of machines.
 func (c *Client) DestroyMachines(machines ...string) error {
 	params := params.DestroyMachines{MachineNames: machines}

=== modified file 'state/api/params/params.go'
--- state/api/params/params.go	2014-01-07 07:35:12 +0000
+++ state/api/params/params.go	2014-01-17 03:18:53 +0000
@@ -518,8 +518,6 @@
 
 type MachineConfigParams struct {
 	MachineId string
-	Series    string
-	Arch      string
 }
 
 // MachineConfig contains information from the environment config that is
@@ -535,6 +533,20 @@
 	Password   string
 }
 
+// ProvisioningScriptParams contains the parameters for the
+// ProvisioningScript client API call.
+type ProvisioningScriptParams struct {
+	MachineId string
+	Nonce     string
+	DataDir   string
+}
+
+// ProvisioningScriptResult contains the result of the
+// ProvisioningScript client API call.
+type ProvisioningScriptResult struct {
+	Script string
+}
+
 // EnvironmentGetResults contains the result of EnvironmentGet client
 // API call.
 type EnvironmentGetResults struct {

=== modified file 'state/apiserver/client/client.go'
--- state/apiserver/client/client.go	2014-01-10 04:24:50 +0000
+++ state/apiserver/client/client.go	2014-01-17 03:18:53 +0000
@@ -15,7 +15,10 @@
 	"launchpad.net/loggo"
 
 	"launchpad.net/juju-core/charm"
+	coreCloudinit "launchpad.net/juju-core/cloudinit"
+	"launchpad.net/juju-core/cloudinit/sshinit"
 	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/cloudinit"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
@@ -601,7 +604,31 @@
 // 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)
+	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)
+	if err != nil {
+		return result, err
+	}
+	cloudcfg := coreCloudinit.New()
+	if err := cloudinit.ConfigureJuju(mcfg, cloudcfg); err != nil {
+		return result, err
+	}
+	// ProvisioningScript is run on an existing machine;
+	// we explicitly disable apt_upgrade so as not to
+	// trample the machine's existing configuration.
+	cloudcfg.SetAptUpgrade(false)
+	result.Script, err = sshinit.ConfigureScript(cloudcfg)
+	return result, err
 }
 
 // DestroyMachines removes a given set of machines.

=== modified file 'state/apiserver/client/client_test.go'
--- state/apiserver/client/client_test.go	2014-01-07 07:35:12 +0000
+++ state/apiserver/client/client_test.go	2014-01-17 03:18:53 +0000
@@ -7,12 +7,16 @@
 	"fmt"
 	"net/url"
 	"strconv"
+	"strings"
 
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/charm"
+	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"
 	"launchpad.net/juju-core/errors"
 	"launchpad.net/juju-core/instance"
@@ -20,6 +24,7 @@
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/state/apiserver/client"
+	"launchpad.net/juju-core/state/statecmd"
 	coretesting "launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/tools"
@@ -1650,7 +1655,7 @@
 
 func (s *clientSuite) TestMachineConfig(c *gc.C) {
 	addrs := []instance.Address{instance.NewAddress("1.2.3.4")}
-	hc := instance.MustParseHardware("mem=4G")
+	hc := instance.MustParseHardware("mem=4G arch=amd64")
 	apiParams := params.AddMachineParams{
 		Jobs:       []params.MachineJob{params.JobHostUnits},
 		InstanceId: instance.Id("1234"),
@@ -1663,7 +1668,7 @@
 	c.Assert(len(machines), gc.Equals, 1)
 
 	machineId := machines[0].Machine
-	machineConfig, err := s.APIState.Client().MachineConfig(machineId, config.DefaultSeries, "amd64")
+	machineConfig, err := s.APIState.Client().MachineConfig(machineId)
 	c.Assert(err, gc.IsNil)
 
 	envConfig, err := s.State.EnvironConfig()
@@ -1682,9 +1687,22 @@
 	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) {
 	addrs := []instance.Address{instance.NewAddress("1.2.3.4")}
-	hc := instance.MustParseHardware("mem=4G")
+	hc := instance.MustParseHardware("mem=4G arch=amd64")
 	apiParams := params.AddMachineParams{
 		Series:     "quantal",
 		Jobs:       []params.MachineJob{params.JobHostUnits},
@@ -1695,10 +1713,55 @@
 	}
 	machines, err := s.APIState.Client().AddMachines([]params.AddMachineParams{apiParams})
 	c.Assert(err, gc.IsNil)
-	_, err = s.APIState.Client().MachineConfig(machines[0].Machine, "quantal", "amd64")
+	_, err = s.APIState.Client().MachineConfig(machines[0].Machine)
 	c.Assert(err, gc.ErrorMatches, tools.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,
+	// converting it to a cloudinit.MachineConfig, and disabling
+	// apt_upgrade.
+	apiParams := params.AddMachineParams{
+		Jobs:       []params.MachineJob{params.JobHostUnits},
+		InstanceId: instance.Id("1234"),
+		Nonce:      "foo",
+		HardwareCharacteristics: instance.MustParseHardware("arch=amd64"),
+	}
+	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)
+	// 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, "")
+	c.Assert(err, gc.IsNil)
+	cloudcfg := coreCloudinit.New()
+	err = cloudinit.ConfigureJuju(mcfg, cloudcfg)
+	c.Assert(err, gc.IsNil)
+	cloudcfg.SetAptUpgrade(false)
+	sshinitScript, err := sshinit.ConfigureScript(cloudcfg)
+	c.Assert(err, gc.IsNil)
+	// ProvisioningScript internally calls MachineConfig,
+	// which allocates a new, random password. Everything
+	// about the scripts should be the same other than
+	// the line containing "oldpassword" from agent.conf.
+	scriptLines := strings.Split(script, "\n")
+	sshinitScriptLines := strings.Split(sshinitScript, "\n")
+	c.Assert(scriptLines, gc.HasLen, len(sshinitScriptLines))
+	for i, line := range scriptLines {
+		if strings.Contains(line, "oldpassword") {
+			continue
+		}
+		c.Assert(line, gc.Equals, sshinitScriptLines[i])
+	}
+}
+
 func (s *clientSuite) TestClientAuthorizeStoreOnDeployServiceSetCharmAndAddCharm(c *gc.C) {
 	store, restore := makeMockCharmStore()
 	defer restore()

=== modified file 'state/statecmd/machineconfig.go'
--- state/statecmd/machineconfig.go	2013-12-19 09:42:05 +0000
+++ state/statecmd/machineconfig.go	2014-01-17 03:18:53 +0000
@@ -6,9 +6,13 @@
 import (
 	"fmt"
 
+	"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"
 )
@@ -31,7 +35,7 @@
 // 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, args params.MachineConfigParams) (params.MachineConfig, error) {
+func MachineConfig(st *state.State, machineId string) (params.MachineConfig, error) {
 	result := params.MachineConfig{}
 	environConfig, err := st.EnvironConfig()
 	if err != nil {
@@ -40,12 +44,27 @@
 	// 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
+	}
+	hc, err := machine.HardwareCharacteristics()
+	if err != nil {
+		return result, err
+	}
+	if hc.Arch == nil {
+		return result, 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
 	}
-	tools, err := findInstanceTools(env, args.Series, args.Arch)
+	tools, err := findInstanceTools(env, machine.Series(), *hc.Arch)
 	if err != nil {
 		return result, err
 	}
@@ -56,10 +75,6 @@
 	if err != nil {
 		return result, err
 	}
-	machine, err := st.Machine(args.MachineId)
-	if err != nil {
-		return result, err
-	}
 	stateInfo, apiInfo, err := auth.SetupAuthentication(machine)
 	if err != nil {
 		return result, err
@@ -71,3 +86,34 @@
 	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
+	err = environs.FinishMachineConfig(mcfg, environConfig, constraints.Value{})
+	if err != nil {
+		return nil, err
+	}
+	return mcfg, nil
+}