launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/juju-core/mpv-pass-userdata into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

Commit message:
Pass user-data to MAAS when starting an instance.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/juju-core/mpv-pass-userdata/+merge/153334

Unfortunately this is hard to test without changes to the test service in gomaasapi, for which we do not have time.  And so once again, all we have is coarse integration tests.


Jeroen
-- 
https://code.launchpad.net/~jtv/juju-core/mpv-pass-userdata/+merge/153334
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/juju-core/mpv-pass-userdata into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

=== modified file 'environs/maas/environ.go'
--- environs/maas/environ.go	2013-03-14 09:05:36 +0000
+++ environs/maas/environ.go	2013-03-14 11:42:22 +0000
@@ -1,10 +1,12 @@
 package maas
 
 import (
+	"encoding/base64"
 	"errors"
 	"fmt"
 	"launchpad.net/gomaasapi"
 	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/environs/cloudinit"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/log"
 	"launchpad.net/juju-core/state"
@@ -122,12 +124,33 @@
 	return environs.MongoURL(env, v)
 }
 
-// Suppress compiler errors for unused variables.
-// TODO: Eliminate all usage of this function.  It's just for development.
-func unused(...interface{}) {}
+// makeMachineConfig sets up a basic machine configuration for use with
+// userData().  You may still need to supply more information, but this takes
+// care of the fixed entries and the ones that are always needed.
+func (env *maasEnviron) makeMachineConfig(machineID string, stateInfo *state.Info, apiInfo *api.Info, tools *state.Tools) *cloudinit.MachineConfig {
+	return &cloudinit.MachineConfig{
+		// Fixed entries.
+		MongoPort: mgoPort,
+		APIPort:   apiPort,
+		DataDir:   jujuDataDir,
+
+		// Entries based purely on what's in the environment.
+		AuthorizedKeys: env.ecfg().AuthorizedKeys(),
+
+		// Parameter entries.
+		MachineId: machineID,
+		StateInfo: stateInfo,
+		APIInfo:   apiInfo,
+		Tools:     tools,
+	}
+}
 
 // startBootstrapNode starts the juju bootstrap node for this environment.
 func (env *maasEnviron) startBootstrapNode(tools *state.Tools, cert, key []byte, password string) (environs.Instance, error) {
+	// The bootstrap instance gets machine id "0".  This is not related to
+	// instance ids or MAAS system ids.  Juju assigns the machine ID.
+	const machineID = "0"
+
 	config, err := environs.BootstrapConfig(env.Provider(), env.Config(), tools)
 	if err != nil {
 		return nil, fmt.Errorf("unable to determine initial configuration: %v", err)
@@ -145,10 +168,19 @@
 		Password: trivial.PasswordHash(password),
 		CACert:   caCert,
 	}
-	// TODO: Obtain userdata based on mongoURL cert/key, and config
-	unused(mongoURL, cert, key, config)
-	var userdata []byte
-	inst, err := env.obtainNode("0", &stateInfo, &apiInfo, tools, userdata)
+	mcfg := env.makeMachineConfig(machineID, &stateInfo, &apiInfo, tools)
+	mcfg.StateServer = true
+	mcfg.StateServerCert = cert
+	mcfg.StateServerKey = key
+	mcfg.MongoURL = mongoURL
+	mcfg.Config = config
+
+	userdata, err := userData(mcfg)
+	if err != nil {
+		msg := fmt.Errorf("could not compose userdata for bootstrap node: %v", err)
+		return nil, msg
+	}
+	inst, err := env.obtainNode(machineID, &stateInfo, &apiInfo, tools, userdata)
 	if err != nil {
 		return nil, fmt.Errorf("cannot start bootstrap instance: %v", err)
 	}
@@ -311,14 +343,17 @@
 		Total: 5 * time.Second,
 		Delay: 200 * time.Millisecond,
 	}
+	userDataParam := base64.StdEncoding.EncodeToString(userdata)
 	params := url.Values{
 		"distro_series": {tools.Series},
-// TODO: How do we pass user_data!?
-		//"user_data": {userdata},
+		// userdata is actually binary data, but so are strings in Go.
+		// Casting it will not produce text (probably not valid UTF-8)
+		// but reading it as binary data should work.
+		"user_data": {userDataParam},
 	}
 	// Initialize err to a non-nil value as a sentinel for the following
 	// loop.
-	var err error = fmt.Errorf("(no error)")
+	err := fmt.Errorf("(no error)")
 	for a := retry.Start(); a.Next() && err != nil; {
 		_, err = node.CallPost("start", params)
 	}
@@ -347,7 +382,7 @@
 }
 
 // StartInstance is specified in the Environ interface.
-func (environ *maasEnviron) StartInstance(machineId string, info *state.Info, apiInfo *api.Info, tools *state.Tools) (environs.Instance, error) {
+func (environ *maasEnviron) StartInstance(machineID string, stateInfo *state.Info, apiInfo *api.Info, tools *state.Tools) (environs.Instance, error) {
 	if tools == nil {
 		flags := environs.HighestVersion | environs.CompatVersion
 		var err error
@@ -356,9 +391,14 @@
 			return nil, err
 		}
 	}
-	// TODO: Obtain userdata.
-	var userdata []byte
-	return environ.obtainNode(machineId, info, apiInfo, tools, userdata)
+
+	mcfg := environ.makeMachineConfig(machineID, stateInfo, apiInfo, tools)
+	userdata, err := userData(mcfg)
+	if err != nil {
+		msg := fmt.Errorf("could not compose user data: %v", err)
+		return nil, msg
+	}
+	return environ.obtainNode(machineID, stateInfo, apiInfo, tools, userdata)
 }
 
 // StopInstances is specified in the Environ interface.

=== modified file 'environs/maas/environ_test.go'
--- environs/maas/environ_test.go	2013-03-14 09:05:36 +0000
+++ environs/maas/environ_test.go	2013-03-14 11:42:22 +0000
@@ -2,6 +2,7 @@
 
 import (
 	"bytes"
+	"fmt"
 	. "launchpad.net/gocheck"
 	"launchpad.net/gomaasapi"
 	"launchpad.net/juju-core/environs"
@@ -169,22 +170,31 @@
 	c.Check(storage, Equals, environs.EmptyStorage)
 }
 
+// fakeWriteCertAndKey is a stub for the writeCertAndKey to Bootstrap() that
+// always returns an error.  It should never be called.
+func fakeWriteCertAndKey(name string, cert, key []byte) error {
+	return fmt.Errorf("unexpected call to writeCertAndKey")
+}
+
 func (suite *EnvironSuite) TestStartInstanceStartsInstance(c *C) {
-	const input = `{"system_id": "test"}`
-	const machineID = "99"
-	node := suite.testMAASObject.TestServer.NewNode(input)
-	resourceURI, err := node.GetField("resource_uri")
-	c.Assert(err, IsNil)
+	suite.testMAASObject.TestServer.NewNode(`{"system_id": "node1", "hostname": "host1"}`)
+	suite.testMAASObject.TestServer.NewNode(`{"system_id": "node2", "hostname": "host2"}`)
 	env := suite.makeEnviron()
-	tools, err := environs.PutTools(env.Storage(), nil)
-	c.Assert(err, IsNil)
-
-	instance, err := env.StartInstance(machineID, nil, nil, tools)
-	c.Assert(err, IsNil)
-
-	c.Check(string(instance.Id()), Equals, resourceURI)
+	err := environs.Bootstrap(env, true, fakeWriteCertAndKey)
+	stateInfo, apiInfo, err := env.StateInfo()
+	c.Assert(err, IsNil)
+	stateInfo.EntityName = "machine-1"
+	apiInfo.EntityName = "machine-1"
+
+	instance, err := env.StartInstance("1", stateInfo, apiInfo, nil)
+	c.Assert(err, IsNil)
+	c.Check(instance, NotNil)
+
 	operations := suite.testMAASObject.TestServer.NodeOperations()
-	actions, found := operations["test"]
+	actions, found := operations["node1"]
+	c.Check(found, Equals, true)
+	c.Check(actions, DeepEquals, []string{"start"})
+	actions, found = operations["node2"]
 	c.Check(found, Equals, true)
 	c.Check(actions, DeepEquals, []string{"start"})
 }
@@ -272,15 +282,28 @@
 func (suite *EnvironSuite) TestBootstrapSucceeds(c *C) {
 	env := suite.makeEnviron()
 	suite.testMAASObject.TestServer.NewNode(`{"system_id": "thenode"}`)
+	cert := []byte{1, 2, 3}
+	key := []byte{4, 5, 6}
 
-	err := env.Bootstrap(true, []byte{}, []byte{})
+	err := env.Bootstrap(true, cert, key)
 	c.Assert(err, IsNil)
 }
 
 func (suite *EnvironSuite) TestBootstrapFailsIfNoNodes(c *C) {
 	env := suite.makeEnviron()
-	err := env.Bootstrap(true, []byte{}, []byte{})
+	cert := []byte{1, 2, 3}
+	key := []byte{4, 5, 6}
+	err := env.Bootstrap(true, cert, key)
 	// Since there are no nodes, the attempt to allocate one returns a
 	// 409: Conflict.
 	c.Check(err, ErrorMatches, ".*409.*")
 }
+
+func (suite *EnvironSuite) TestBootstrapIntegratesWithEnvirons(c *C) {
+	env := suite.makeEnviron()
+	suite.testMAASObject.TestServer.NewNode(`{"system_id": "bootstrapnode"}`)
+
+	// environs.Bootstrap calls Environ.Bootstrap.  This works.
+	err := environs.Bootstrap(env, true, fakeWriteCertAndKey)
+	c.Assert(err, IsNil)
+}