Merge lp:~thumper/juju-core/container-hardware-characteristics into lp:~go-bot/juju-core/trunk

[Tim Penhey]

StartContainer returns hardware characteristics.

Initial change to have the hardware characteristics
of containers returned. At this stage just the arch
is filled out.

https://codereview.appspot.com/36980043/

StartContainer returns hardware characteristics.

Initial change to have the hardware characteristics
of containers returned. At this stage just the arch
is filled out.

https://codereview.appspot.com/36980043/

[Tim Penhey (thumper)]

Reviewers: mp+197644_code.launchpad.net,

Message:
Please take a look.

Description:
StartContainer returns hardware characteristics.

Initial change to have the hardware characteristics
of containers returned. At this stage just the arch
is filled out.

https://code.launchpad.net/~thumper/juju-core/container-hardware-characteristics/+merge/197644

Requires:
https://code.launchpad.net/~thumper/juju-core/kvm-provisioner/+merge/197629

(do not edit description out of merge proposal)

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

Affected files (+42, -28 lines):
   A [revision details]
   M container/interface.go
   M container/kvm/kvm.go
   M container/kvm/live_test.go
   M container/lxc/lxc.go
   M container/testing/common.go
   M provider/local/environ.go
   M worker/provisioner/kvm-broker.go
   M worker/provisioner/lxc-broker.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: container/interface.go
=== modified file 'container/interface.go'
--- container/interface.go 2013-11-20 01:07:26 +0000
+++ container/interface.go 2013-12-04 02:37:05 +0000
@@ -22,7 +22,7 @@
   StartContainer(
    machineConfig *cloudinit.MachineConfig,
    series string,
- network *NetworkConfig) (instance.Instance, error)
+ network *NetworkConfig) (instance.Instance,
*instance.HardwareCharacteristics, error)
   // StopContainer stops and destroyes the container identified by Instance.
   StopContainer(instance.Instance) error
   // ListContainers return a list of containers that have been started by

Index: container/kvm/kvm.go
=== modified file 'container/kvm/kvm.go'
--- container/kvm/kvm.go 2013-12-04 00:52:35 +0000
+++ container/kvm/kvm.go 2013-12-04 02:37:05 +0000
@@ -63,7 +63,7 @@
  func (manager *containerManager) StartContainer(
   machineConfig *cloudinit.MachineConfig,
   series string,
- network *container.NetworkConfig) (instance.Instance, error) {
+ network *container.NetworkConfig) (instance.Instance,
*instance.HardwareCharacteristics, error) {

   name := names.MachineTag(machineConfig.MachineId)
   if manager.name != "" {
@@ -77,20 +77,24 @@
   // Create the cloud-init.
   directory, err := container.NewDirectory(name)
   if err != nil {
- return nil, fmt.Errorf("failed to create container directory: %v", err)
+ return nil, nil, fmt.Errorf("failed to create container directory: %v",
err)
   }
   logger.Tracef("write cloud-init")
   userDataFilename, err := container.WriteUserData(machineConfig, directory)
   if err != nil {
- return nil, log.LoggedErrorf(logger, "failed to write user data: %v",
err)
+ return nil, nil, log.LoggedErrorf(logger, "failed to write user
data: %v", err)
   }
   // Create the container.
+ arch := version.Current.Arch
+ hardware := &instance.HardwareCharacteristics{
+ Arch: &arch,
+ }
   logger.Tracef("create the container")
- if err := kvmContainer.Start(series, version.Current.Arch,
userDataFilename, network); er...

[Ian Booth (wallyworld)]

LGTM but I'd like to see the tests more proscriptive. eg we know what
arch has to be so test that rather than no nil.

https://codereview.appspot.com/36980043/diff/1/container/kvm/live_test.go
File container/kvm/live_test.go (right):

https://codereview.appspot.com/36980043/diff/1/container/kvm/live_test.go#newcode101
container/kvm/live_test.go:101: c.Assert(hardware, gc.Not(gc.Equals),
&instance.HardwareCharacteristics{})
I do think we should be asserting an actual expected value, not just
"not nil"

https://codereview.appspot.com/36980043/

=== modified file 'container/interface.go'
--- container/interface.go	2013-11-20 01:07:26 +0000
+++ container/interface.go	2013-12-04 22:36:30 +0000
@@ -22,7 +22,7 @@
 	StartContainer(
 		machineConfig *cloudinit.MachineConfig,
 		series string,
-		network *NetworkConfig) (instance.Instance, error)
+		network *NetworkConfig) (instance.Instance, *instance.HardwareCharacteristics, error)
 	// StopContainer stops and destroyes the container identified by Instance.
 	StopContainer(instance.Instance) error
 	// ListContainers return a list of containers that have been started by

=== modified file 'container/kvm/kvm.go'
--- container/kvm/kvm.go	2013-12-04 00:52:35 +0000
+++ container/kvm/kvm.go	2013-12-04 22:36:30 +0000
@@ -63,7 +63,7 @@
 func (manager *containerManager) StartContainer(
 	machineConfig *cloudinit.MachineConfig,
 	series string,
-	network *container.NetworkConfig) (instance.Instance, error) {
+	network *container.NetworkConfig) (instance.Instance, *instance.HardwareCharacteristics, error) {
 
 	name := names.MachineTag(machineConfig.MachineId)
 	if manager.name != "" {
@@ -77,20 +77,24 @@
 	// Create the cloud-init.
 	directory, err := container.NewDirectory(name)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create container directory: %v", err)
+		return nil, nil, fmt.Errorf("failed to create container directory: %v", err)
 	}
 	logger.Tracef("write cloud-init")
 	userDataFilename, err := container.WriteUserData(machineConfig, directory)
 	if err != nil {
-		return nil, log.LoggedErrorf(logger, "failed to write user data: %v", err)
+		return nil, nil, log.LoggedErrorf(logger, "failed to write user data: %v", err)
 	}
 	// Create the container.
+	arch := version.Current.Arch
+	hardware := &instance.HardwareCharacteristics{
+		Arch: &arch,
+	}
 	logger.Tracef("create the container")
-	if err := kvmContainer.Start(series, version.Current.Arch, userDataFilename, network); err != nil {
-		return nil, log.LoggedErrorf(logger, "kvm container creation failed: %v", err)
+	if err := kvmContainer.Start(series, arch, userDataFilename, network); err != nil {
+		return nil, nil, log.LoggedErrorf(logger, "kvm container creation failed: %v", err)
 	}
 	logger.Tracef("kvm container created")
-	return &kvmInstance{kvmContainer, name}, nil
+	return &kvmInstance{kvmContainer, name}, hardware, nil
 }
 
 func (manager *containerManager) StopContainer(instance instance.Instance) error {

=== modified file 'container/kvm/live_test.go'
--- container/kvm/live_test.go	2013-11-21 09:47:12 +0000
+++ container/kvm/live_test.go	2013-12-04 22:36:30 +0000
@@ -95,9 +95,13 @@
 	err := environs.FinishMachineConfig(machineConfig, environConfig, constraints.Value{})
 	c.Assert(err, gc.IsNil)
 
-	instance, err := manager.StartContainer(machineConfig, "precise", network)
+	inst, hardware, err := manager.StartContainer(machineConfig, "precise", network)
 	c.Assert(err, gc.IsNil)
-	return instance
+	c.Assert(hardware, gc.NotNil)
+	arch := version.Current.Arch
+	expected := instance.HardwareCharacteristics{Arch: &arch}.String()
+	c.Assert(hardware.String(), gc.Equals, expected)
+	return inst
 }
 
 func (s *LiveSuite) TestShutdownMachines(c *gc.C) {

=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go	2013-12-01 23:54:35 +0000
+++ container/lxc/lxc.go	2013-12-04 22:36:30 +0000
@@ -17,6 +17,7 @@
 	"launchpad.net/juju-core/environs/cloudinit"
 	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/names"
+	"launchpad.net/juju-core/version"
 )
 
 var logger = loggo.GetLogger("juju.container.lxc")
@@ -61,7 +62,7 @@
 func (manager *containerManager) StartContainer(
 	machineConfig *cloudinit.MachineConfig,
 	series string,
-	network *container.NetworkConfig) (instance.Instance, error) {
+	network *container.NetworkConfig) (instance.Instance, *instance.HardwareCharacteristics, error) {
 
 	name := names.MachineTag(machineConfig.MachineId)
 	if manager.name != "" {
@@ -75,19 +76,19 @@
 	// Create the cloud-init.
 	directory, err := container.NewDirectory(name)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	logger.Tracef("write cloud-init")
 	userDataFilename, err := container.WriteUserData(machineConfig, directory)
 	if err != nil {
 		logger.Errorf("failed to write user data: %v", err)
-		return nil, err
+		return nil, nil, err
 	}
 	logger.Tracef("write the lxc.conf file")
 	configFile, err := writeLxcConfig(network, directory, manager.logdir)
 	if err != nil {
 		logger.Errorf("failed to write config file: %v", err)
-		return nil, err
+		return nil, nil, err
 	}
 	templateParams := []string{
 		"--debug",                      // Debug errors in the cloud image
@@ -99,19 +100,19 @@
 	logger.Tracef("create the container")
 	if err := lxcContainer.Create(configFile, defaultTemplate, templateParams...); err != nil {
 		logger.Errorf("lxc container creation failed: %v", err)
-		return nil, err
+		return nil, nil, err
 	}
 	// Make sure that the mount dir has been created.
 	logger.Tracef("make the mount dir for the shard logs")
 	if err := os.MkdirAll(internalLogDir(name), 0755); err != nil {
 		logger.Errorf("failed to create internal /var/log/juju mount dir: %v", err)
-		return nil, err
+		return nil, nil, err
 	}
 	logger.Tracef("lxc container created")
 	// Now symlink the config file into the restart directory.
 	containerConfigFile := filepath.Join(LxcContainerDir, name, "config")
 	if err := os.Symlink(containerConfigFile, restartSymlink(name)); err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	logger.Tracef("auto-restart link created")
 
@@ -126,10 +127,14 @@
 	// setting it ourselves.
 	if err = lxcContainer.Start("", consoleFile); err != nil {
 		logger.Errorf("container failed to start: %v", err)
-		return nil, err
+		return nil, nil, err
+	}
+	arch := version.Current.Arch
+	hardware := &instance.HardwareCharacteristics{
+		Arch: &arch,
 	}
 	logger.Tracef("container started")
-	return &lxcInstance{lxcContainer, name}, nil
+	return &lxcInstance{lxcContainer, name}, hardware, nil
 }
 
 func (manager *containerManager) StopContainer(instance instance.Instance) error {

=== modified file 'container/testing/common.go'
--- container/testing/common.go	2013-12-04 04:14:44 +0000
+++ container/testing/common.go	2013-12-04 22:36:30 +0000
@@ -29,8 +29,10 @@
 
 	series := "series"
 	network := container.BridgeNetworkConfig("nic42")
-	inst, err := manager.StartContainer(machineConfig, series, network)
+	inst, hardware, err := manager.StartContainer(machineConfig, series, network)
 	c.Assert(err, gc.IsNil)
+	c.Assert(hardware, gc.NotNil)
+	c.Assert(hardware.String(), gc.Not(gc.Equals), "")
 	return inst
 }
 

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2013-12-04 00:41:41 +0000
+++ provider/local/environ.go	2013-12-04 22:36:30 +0000
@@ -295,12 +295,11 @@
 		return nil, nil, err
 	}
 	machineConfig.AgentEnvironment[agent.Namespace] = env.config.namespace()
-	inst, err := env.containerManager.StartContainer(machineConfig, series, network)
+	inst, hardware, err := env.containerManager.StartContainer(machineConfig, series, network)
 	if err != nil {
 		return nil, nil, err
 	}
-	// TODO(thumper): return some hardware characteristics.
-	return inst, nil, nil
+	return inst, hardware, nil
 }
 
 // StartInstance is specified in the InstanceBroker interface.

=== modified file 'worker/provisioner/kvm-broker.go'
--- worker/provisioner/kvm-broker.go	2013-12-04 04:01:16 +0000
+++ worker/provisioner/kvm-broker.go	2013-12-04 22:36:30 +0000
@@ -90,13 +90,13 @@
 		return nil, nil, err
 	}
 
-	inst, err := broker.manager.StartContainer(machineConfig, series, network)
+	inst, hardware, err := broker.manager.StartContainer(machineConfig, series, network)
 	if err != nil {
 		kvmLogger.Errorf("failed to start container: %v", err)
 		return nil, nil, err
 	}
-	kvmLogger.Infof("started kvm container for machineId: %s, %s", machineId, inst.Id())
-	return inst, nil, nil
+	kvmLogger.Infof("started kvm container for machineId: %s, %s, %s", machineId, inst.Id(), hardware.String())
+	return inst, hardware, nil
 }
 
 // StopInstances shuts down the given instances.

=== modified file 'worker/provisioner/lxc-broker.go'
--- worker/provisioner/lxc-broker.go	2013-12-04 04:01:16 +0000
+++ worker/provisioner/lxc-broker.go	2013-12-04 22:36:30 +0000
@@ -80,13 +80,13 @@
 		return nil, nil, err
 	}
 
-	inst, err := broker.manager.StartContainer(machineConfig, series, network)
+	inst, hardware, err := broker.manager.StartContainer(machineConfig, series, network)
 	if err != nil {
 		lxcLogger.Errorf("failed to start container: %v", err)
 		return nil, nil, err
 	}
-	lxcLogger.Infof("started lxc container for machineId: %s, %s", machineId, inst.Id())
-	return inst, nil, nil
+	lxcLogger.Infof("started lxc container for machineId: %s, %s, %s", machineId, inst.Id(), hardware.String())
+	return inst, hardware, nil
 }
 
 // StopInstances shuts down the given instances.