launchpad-reviewers team mailing list archive

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

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

Commit message:
Basic StartInstance() implementation (and Bootstrap() gets its version), minus userdata.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/juju-core/mpv-sketch-startinstance/+merge/152725

We have a test that calls Bootstrap() and watches it complete successfully, running against the gomaasapi test service.  We have found that the problems and successes in that test matched our experiences in a live Q/A environment fairly well.

There is no real unit testing.  It's not easy to get good unit testing when requirements are determined to this extent by cargo-culting.  But we did end up with a much simpler, cleaner factoring of the code than the examples in the ec2/openstack providers.  The negative side of this is that our provider will need some creative changes as the provider API changes, as opposed to cargo-culting the changes from the other providers.  The positive side is that it will probably be a bit easier and a bit less cargo-culty to do so.

This change required an update in gomaasapi so that its test service would support the “acquire” API call.  Make sure the version in your Go environment is up to date.

Raphaël meanwhile has implemented generation of user data, so a later branch can hook that into what you see here.  There is still the question of what the right way is to provide userdata to the http request: url.Values ultimately contains strings, not byte arrays.


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

=== modified file 'environs/maas/environ.go'
--- environs/maas/environ.go	2013-03-12 16:06:05 +0000
+++ environs/maas/environ.go	2013-03-12 17:01:21 +0000
@@ -11,6 +11,7 @@
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/trivial"
 	"launchpad.net/juju-core/version"
+	"net/url"
 	"sync"
 	"time"
 )
@@ -136,9 +137,10 @@
 		Password: trivial.PasswordHash(password),
 		CACert:   caCert,
 	}
-	// TODO: mongoURL, cert/key, and config need to go into the userdata somehow.
+	// TODO: Obtain userdata based on mongoURL cert/key, and config
 	unused(mongoURL, cert, key, config)
-	inst, err := env.StartInstance("0", &stateInfo, &apiInfo, tools)
+	var userdata []byte
+	inst, err := env.obtainNode("0", &stateInfo, &apiInfo, tools, userdata)
 	if err != nil {
 		return nil, fmt.Errorf("cannot start bootstrap instance: %v", err)
 	}
@@ -223,18 +225,85 @@
 	return nil
 }
 
+// nonNilError is a placeholder for an error.  It is not nil, but it's not an
+// actual error either.  This should never be returned from anywhere.
+var nonNilError = fmt.Errorf("(Not a real error)")
+
+// acquireNode allocates a node from the MAAS.
+func (environ *maasEnviron) acquireNode() (gomaasapi.MAASObject, error) {
+	retry := trivial.AttemptStrategy{
+		Total: 5 * time.Second,
+		Delay: 200 * time.Millisecond,
+	}
+	var result gomaasapi.JSONObject
+	var err error = nonNilError
+	for a := retry.Start(); a.Next() && err != nil; {
+		client := environ.maasClientUnlocked.GetSubObject("nodes/")
+		result, err = client.CallPost("acquire", nil)
+	}
+	if err != nil {
+		return gomaasapi.MAASObject{}, err
+	}
+	node, err := result.GetMAASObject()
+	if err != nil {
+		msg := fmt.Errorf("unexpected result from 'acquire' on MAAS API: %v", err)
+		return gomaasapi.MAASObject{}, msg
+	}
+	return node, nil
+}
+
+// startNode installs and boots a node.
+func (environ *maasEnviron) startNode(node gomaasapi.MAASObject, tools *state.Tools, userdata []byte) error {
+	retry := trivial.AttemptStrategy{
+		Total: 5 * time.Second,
+		Delay: 200 * time.Millisecond,
+	}
+	params := url.Values{
+		"distro_series": {tools.Series},
+// TODO: How do we pass user_data!?
+		//"user_data": {userdata},
+	}
+	var err error = nonNilError
+	for a := retry.Start(); a.Next() && err != nil; {
+		_, err = node.CallPost("start", params)
+	}
+	return err
+}
+
+// obtainNode allocates and starts a MAAS node.  It is used both for the
+// implementation of StartInstance, and to initialize the bootstrap node.
+func (environ *maasEnviron) obtainNode(machineId string, stateInfo *state.Info, apiInfo *api.Info, tools *state.Tools, userdata []byte) (*maasInstance, error) {
+
+	log.Printf("environs/maas: starting machine %s in $q running tools version %q from %q", machineId, environ.name, tools.Binary, tools.URL)
+
+	node, err := environ.acquireNode()
+	if err != nil {
+		return nil, fmt.Errorf("cannot run instances: %v", err)
+	}
+	instance := maasInstance{&node, environ}
+
+	err = environ.startNode(node, tools, userdata)
+	if err != nil {
+		environ.StopInstances([]environs.Instance{&instance})
+		return nil, fmt.Errorf("cannot start instance: %v", err)
+	}
+	log.Printf("environs/maas: started instance %q", instance.Id())
+	return &instance, nil
+}
+
 // 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) {
-	node, err := environ.maasClientUnlocked.GetSubObject(machineId).Get()
-	if err != nil {
-		return nil, err
-	}
-	_, err = node.CallPost("start", nil)
-	if err != nil {
-		return nil, err
-	}
-	instance := &maasInstance{maasObject: &node, environ: environ}
-	return instance, nil
+	if tools == nil {
+		flags := environs.HighestVersion | environs.CompatVersion
+		var err error
+		tools, err = environs.FindTools(environ, version.Current, flags)
+		if err != nil {
+			return nil, err
+		}
+	}
+	// TODO: Obtain userdata.
+	var userdata []byte
+	return environ.obtainNode(machineId, info, apiInfo, tools, userdata)
 }
 
 // StopInstances is specified in the Environ interface.

=== modified file 'environs/maas/environ_test.go'
--- environs/maas/environ_test.go	2013-03-12 16:06:05 +0000
+++ environs/maas/environ_test.go	2013-03-12 17:01:21 +0000
@@ -170,13 +170,18 @@
 }
 
 func (suite *EnvironSuite) TestStartInstanceStartsInstance(c *C) {
-	input := `{"system_id": "test"}`
+	const input = `{"system_id": "test"}`
+	const machineID = "99"
 	node := suite.testMAASObject.TestServer.NewNode(input)
-	resourceURI, _ := node.GetField("resource_uri")
-
-	instance, err := suite.environ.StartInstance(resourceURI, nil, nil, nil)
-
-	c.Check(err, IsNil)
+	resourceURI, err := node.GetField("resource_uri")
+	c.Assert(err, IsNil)
+	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)
 	operations := suite.testMAASObject.TestServer.NodeOperations()
 	actions, found := operations["test"]
@@ -242,11 +247,10 @@
 
 func (suite *EnvironSuite) TestBootstrap(c *C) {
 	env := suite.makeEnviron()
+	suite.testMAASObject.TestServer.NewNode(`{"system_id": "thenode"}`)
 
 	err := env.Bootstrap(true, []byte{}, []byte{})
-	// TODO: Get this to succeed.
-	unused(err)
-	// c.Assert(err, IsNil)
+	c.Assert(err, IsNil)
 
 	// TODO: Verify a simile of success.
 }