launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/juju-core/addresses2 into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

Commit message:
Implement PrivateAddress/PublicAddress.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~rvb/juju-core/addresses2/+merge/157862

This code replicates what the Python provider does: PrivateAddress() and PublicAddress() both return the hostname of the MAAS node.

Once a node is in use in MAAS, the user cannot change it's name so I choose to pass the hostname using the same trick we used to pass the instanceId: configure cloudinit to write a file with that information on the node.

Instead of writing just the instanceId into a secret "machine file", the provider code now write a yaml structure that includes the instanceId and the hostname (I choose the YAML format because it is used in many other parts in Juju plus it will allow us to easily pass more information if we need that later).

As I say in the comment I added in environs/maas/environ_test.go, we need to improve the testservice in gomaasapi in order to properly test the change made in environs/maas/environ.go.

This was pre-imp'ed with Jeroen.
-- 
https://code.launchpad.net/~rvb/juju-core/addresses2/+merge/157862
Your team MAAS Maintainers is requested to review the proposed merge of lp:~rvb/juju-core/addresses2 into lp:~maas-maintainers/juju-core/maas-provider-skeleton.

=== modified file 'environs/maas/environ.go'
--- environs/maas/environ.go	2013-04-09 06:52:42 +0000
+++ environs/maas/environ.go	2013-04-09 13:13:21 +0000
@@ -334,8 +334,6 @@
 	return err
 }
 
-var _MAASInstanceIDFilename = jujuDataDir + "/MAASmachineID.txt"
-
 // 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, mcfg *cloudinit.MachineConfig) (*maasInstance, error) {
@@ -346,10 +344,18 @@
 	if err != nil {
 		return nil, fmt.Errorf("cannot run instances: %v", err)
 	}
+
+	hostname, err := node.GetField("hostname")
+	if err != nil {
+		return nil, err
+	}
 	instance := maasInstance{&node, environ}
-
-	script := fmt.Sprintf(`mkdir -p %s; echo -n %s > %s`, trivial.ShQuote(jujuDataDir), trivial.ShQuote(string(instance.Id())), trivial.ShQuote(_MAASInstanceIDFilename))
-	userdata, err := userData(mcfg, script)
+	info := machineInfo{string(instance.Id()), hostname}
+	runCmd, err := info.cloudinitRunCmd()
+	if err != nil {
+		return nil, err
+	}
+	userdata, err := userData(mcfg, runCmd)
 	if err != nil {
 		msg := fmt.Errorf("could not compose userdata for bootstrap node: %v", err)
 		return nil, msg

=== modified file 'environs/maas/environ_test.go'
--- environs/maas/environ_test.go	2013-04-09 06:52:42 +0000
+++ environs/maas/environ_test.go	2013-04-09 13:13:21 +0000
@@ -216,6 +216,11 @@
 	// The instance number 1 has been started.
 	actions, found = operations["node1"]
 	c.Check(found, Equals, true)
+	// TODO: check that cloudinit config include the creation of the
+	// "machine file" containing the instanceId and the hostname.
+	// In order to do that, the testservice in gomaasapi needs to be
+	// improved to get it to record the parameters sent during the "node
+	// start" operation.
 	c.Check(actions, DeepEquals, []string{"start"})
 }
 

=== modified file 'environs/maas/environprovider.go'
--- environs/maas/environprovider.go	2013-04-08 07:50:28 +0000
+++ environs/maas/environprovider.go	2013-04-09 13:13:21 +0000
@@ -1,7 +1,6 @@
 package maas
 
 import (
-	"io/ioutil"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/log"
@@ -50,21 +49,31 @@
 	return secretAttrs, nil
 }
 
+func (maasEnvironProvider) hostname() (string, error) {
+	info := machineInfo{}
+	err := info.load()
+	if err != nil {
+		return "", err
+	}
+	return info.Hostname, nil
+}
+
 // PublicAddress is specified in the EnvironProvider interface.
-func (maasEnvironProvider) PublicAddress() (string, error) {
-	panic("Not implemented.")
+func (prov maasEnvironProvider) PublicAddress() (string, error) {
+	return prov.hostname()
 }
 
 // PrivateAddress is specified in the EnvironProvider interface.
-func (maasEnvironProvider) PrivateAddress() (string, error) {
-	panic("Not implemented.")
+func (prov maasEnvironProvider) PrivateAddress() (string, error) {
+	return prov.hostname()
 }
 
 // InstanceId is specified in the EnvironProvider interface.
 func (maasEnvironProvider) InstanceId() (state.InstanceId, error) {
-	content, err := ioutil.ReadFile(_MAASInstanceIDFilename)
+	info := machineInfo{}
+	err := info.load()
 	if err != nil {
 		return "", err
 	}
-	return state.InstanceId(string(content)), nil
+	return state.InstanceId(info.InstanceId), nil
 }

=== modified file 'environs/maas/environprovider_test.go'
--- environs/maas/environprovider_test.go	2013-04-08 01:50:24 +0000
+++ environs/maas/environprovider_test.go	2013-04-09 13:13:21 +0000
@@ -35,24 +35,62 @@
 	c.Check(secretAttrs, DeepEquals, expectedAttrs)
 }
 
-func (suite *EnvironProviderSuite) TestInstanceIdReadsInstanceIdFile(c *C) {
-	instanceId := "instance-id"
-	// Create a temporary file to act as the file where the instanceID
-	// is stored.
+// create a temporary file with the given content.  The caller is responsible
+// for cleaning up the file.
+func createTempFile(c *C, content []byte) string {
 	file, err := ioutil.TempFile("", "")
 	c.Assert(err, IsNil)
 	filename := file.Name()
+	err = ioutil.WriteFile(filename, content, 0644)
+	c.Assert(err, IsNil)
+	return filename
+}
+
+// InstanceId returns the instanceId of the machine read from the file
+// _MAASInstanceFilename.
+func (suite *EnvironProviderSuite) TestInstanceIdReadsInstanceIdFromMachineFile(c *C) {
+	instanceId := "instance-id"
+	info := machineInfo{instanceId, "hostname"}
+	yaml, err := info.serializeYAML()
+	c.Assert(err, IsNil)
+	// Create a temporary file to act as the file where the instanceID
+	// is stored.
+	filename := createTempFile(c, yaml)
 	defer os.Remove(filename)
-	err = ioutil.WriteFile(filename, []byte(instanceId), 0644)
-	c.Assert(err, IsNil)
-	// "Monkey patch" the value of _MAASInstanceIDFilename with the path
+	// "Monkey patch" the value of _MAASInstanceFilename with the path
 	// to the temporary file.
-	old_MAASInstanceIDFilename := _MAASInstanceIDFilename
-	_MAASInstanceIDFilename = filename
-	defer func() { _MAASInstanceIDFilename = old_MAASInstanceIDFilename }()
+	old_MAASInstanceFilename := _MAASInstanceFilename
+	_MAASInstanceFilename = filename
+	defer func() { _MAASInstanceFilename = old_MAASInstanceFilename }()
 
 	provider := suite.environ.Provider()
 	returnedInstanceId, err := provider.InstanceId()
 	c.Assert(err, IsNil)
 	c.Check(returnedInstanceId, Equals, state.InstanceId(instanceId))
 }
+
+// PublicAddress and PrivateAddress return the hostname of the machine read
+// from the file _MAASInstanceFilename.
+func (suite *EnvironProviderSuite) TestPrivatePublicAddressReadsHostnameFromMachineFile(c *C) {
+	hostname := "myhostname"
+	info := machineInfo{"instance-id", hostname}
+	yaml, err := info.serializeYAML()
+	c.Assert(err, IsNil)
+	// Create a temporary file to act as the file where the instanceID
+	// is stored.
+	filename := createTempFile(c, yaml)
+	defer os.Remove(filename)
+	// "Monkey patch" the value of _MAASInstanceFilename with the path
+	// to the temporary file.
+	old_MAASInstanceFilename := _MAASInstanceFilename
+	_MAASInstanceFilename = filename
+	defer func() { _MAASInstanceFilename = old_MAASInstanceFilename }()
+
+	provider := suite.environ.Provider()
+	publicAddress, err := provider.PublicAddress()
+	c.Assert(err, IsNil)
+	c.Check(publicAddress, Equals, hostname)
+	privateAddress, err := provider.PrivateAddress()
+	c.Assert(err, IsNil)
+	c.Check(privateAddress, Equals, hostname)
+}

=== modified file 'environs/maas/util.go'
--- environs/maas/util.go	2013-04-05 09:16:18 +0000
+++ environs/maas/util.go	2013-04-09 13:13:21 +0000
@@ -1,6 +1,9 @@
 package maas
 
 import (
+	"fmt"
+	"io/ioutil"
+	"launchpad.net/goyaml"
 	cloudinit_core "launchpad.net/juju-core/cloudinit"
 	"launchpad.net/juju-core/environs/cloudinit"
 	"launchpad.net/juju-core/log"
@@ -47,3 +50,44 @@
 	log.Debugf("environs/maas: maas user data; %d bytes", len(cdata))
 	return cdata, nil
 }
+
+// machineInfo is the structure used to pass information between the provider
+// and the agent running on a node.
+// When a node is started, the provider code creates a machineInfo object
+// containing information about the node being started and configures
+// cloudinit to get a YAML representation of that object written on the node's
+// filesystem during its first startup.  That file is then read by the juju
+// agent running on the node and converted back into a machineInfo object.
+type machineInfo struct {
+	InstanceId string
+	Hostname   string
+}
+
+var _MAASInstanceFilename = jujuDataDir + "/MAASmachine.txt"
+
+// serializeYAML serializes the info into YAML format.
+func (info *machineInfo) serializeYAML() ([]byte, error) {
+	return goyaml.Marshal(info)
+}
+
+// cloudinitRunCmd returns the shell command that, when run, will create the
+// "machine info" file containing the instanceId and the hostname of a machine.
+// That command is destined to be used by cloudinit.
+func (info *machineInfo) cloudinitRunCmd() (string, error) {
+	yaml, err := info.serializeYAML()
+	if err != nil {
+		return "", err
+	}
+	script := fmt.Sprintf(`mkdir -p %s; echo -n %s > %s`, trivial.ShQuote(jujuDataDir), trivial.ShQuote(string(yaml)), trivial.ShQuote(_MAASInstanceFilename))
+	return script, nil
+}
+
+// load loads the "machine info" file and parse the content into the info
+// object.
+func (info *machineInfo) load() error {
+	content, err := ioutil.ReadFile(_MAASInstanceFilename)
+	if err != nil {
+		return err
+	}
+	return goyaml.Unmarshal(content, info)
+}

=== modified file 'environs/maas/util_test.go'
--- environs/maas/util_test.go	2013-04-08 09:01:27 +0000
+++ environs/maas/util_test.go	2013-04-09 13:13:21 +0000
@@ -1,6 +1,7 @@
 package maas
 
 import (
+	"fmt"
 	. "launchpad.net/gocheck"
 	"launchpad.net/goyaml"
 	"launchpad.net/juju-core/environs/cloudinit"
@@ -10,6 +11,7 @@
 	"launchpad.net/juju-core/testing"
 	"launchpad.net/juju-core/trivial"
 	"launchpad.net/juju-core/version"
+	"os"
 )
 
 type UtilSuite struct{}
@@ -90,3 +92,51 @@
 	c.Check(runCmd[0], Equals, script1)
 	c.Check(runCmd[1], Equals, script2)
 }
+
+func (s *UtilSuite) TestMachineInfoserializeYAML(c *C) {
+	instanceId := "instanceId"
+	hostname := "hostname"
+	info := machineInfo{instanceId, hostname}
+
+	yaml, err := info.serializeYAML()
+
+	c.Assert(err, IsNil)
+	expected := "instanceid: instanceId\nhostname: hostname\n"
+	c.Check(string(yaml), Equals, expected)
+}
+
+func (s *UtilSuite) TestMachineInfoCloudinitRunCmd(c *C) {
+	instanceId := "instanceId"
+	hostname := "hostname"
+	filename := "path/to/file"
+	old_MAASInstanceFilename := _MAASInstanceFilename
+	_MAASInstanceFilename = filename
+	defer func() { _MAASInstanceFilename = old_MAASInstanceFilename }()
+	info := machineInfo{instanceId, hostname}
+
+	script, err := info.cloudinitRunCmd()
+
+	c.Assert(err, IsNil)
+	yaml, err := info.serializeYAML()
+	c.Assert(err, IsNil)
+	expected := fmt.Sprintf("mkdir -p '%s'; echo -n '%s' > '%s'", jujuDataDir, yaml, filename)
+	c.Check(script, Equals, expected)
+}
+
+func (s *UtilSuite) TestMachineInfoLoad(c *C) {
+	instanceId := "instanceId"
+	hostname := "hostname"
+	yaml := fmt.Sprintf("instanceid: %s\nhostname: %s\n", instanceId, hostname)
+	filename := createTempFile(c, []byte(yaml))
+	defer os.Remove(filename)
+	old_MAASInstanceFilename := _MAASInstanceFilename
+	_MAASInstanceFilename = filename
+	defer func() { _MAASInstanceFilename = old_MAASInstanceFilename }()
+	info := machineInfo{}
+
+	err := info.load()
+
+	c.Assert(err, IsNil)
+	c.Check(info.InstanceId, Equals, instanceId)
+	c.Check(info.Hostname, Equals, hostname)
+}