Merge lp:~thumper/juju-core/maas-lxc-trunk into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Make LXC containers work on MAAS again.

Cherry pick the r1980 from lp:juju-core/1.16 that fixes
the issue on the release branch.

In addition to those changes, I changed the client api
to return the params struct rather than uppacking the
values.

https://codereview.appspot.com/20480043/

Make LXC containers work on MAAS again.

Cherry pick the r1980 from lp:juju-core/1.16 that fixes
the issue on the release branch.

In addition to those changes, I changed the client api
to return the params struct rather than uppacking the
values.

https://codereview.appspot.com/20480043/

[Tim Penhey (thumper)]

Reviewers: mp+193502_code.launchpad.net,

Message:
Please take a look.

Description:
Make LXC containers work on MAAS again.

Cherry pick the r1980 from lp:juju-core/1.16 that fixes
the issue on the release branch.

In addition to those changes, I changed the client api
to return the params struct rather than uppacking the
values.

https://code.launchpad.net/~thumper/juju-core/maas-lxc-trunk/+merge/193502

(do not edit description out of merge proposal)

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

Affected files (+144, -107 lines):
   A [revision details]
   M container/lxc/lxc.go
   M container/lxc/lxc_test.go
   M environs/cloudinit.go
   M provider/local/environ.go
   M state/api/params/params.go
   M state/api/provisioner/provisioner.go
   M state/api/provisioner/provisioner_test.go
   M state/apiserver/provisioner/provisioner.go
   M state/apiserver/provisioner/provisioner_test.go
   M worker/provisioner/lxc-broker.go
   M worker/provisioner/lxc-broker_test.go
   M worker/provisioner/provisioner.go

[Ian Booth (wallyworld)]

LGTM

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

https://codereview.appspot.com/20480043/diff/1/state/api/provisioner/provisioner.go#newcode160
state/api/provisioner/provisioner.go:160: func (st *State)
ContainerConfig() (result params.ContainerConfig, err error) {
ContainerConfig is very generic.

Perhaps ContainerCloudInitConfig ?

https://codereview.appspot.com/20480043/

[Tim Penhey (thumper)]

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

https://codereview.appspot.com/20480043/diff/1/state/api/provisioner/provisioner.go#newcode160
state/api/provisioner/provisioner.go:160: func (st *State)
ContainerConfig() (result params.ContainerConfig, err error) {
On 2013/11/04 01:40:24, wallyworld wrote:
> ContainerConfig is very generic.

> Perhaps ContainerCloudInitConfig ?

Yes. Perhaps in the future we'll want to return more than just some
cloud init stuff. Given that we don't yet know, I feel it is better to
leave this a little more generic here for now.

https://codereview.appspot.com/20480043/

=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go	2013-10-01 02:33:14 +0000
+++ container/lxc/lxc.go	2013-10-31 21:39:38 +0000
@@ -14,15 +14,9 @@
 	"launchpad.net/golxc"
 	"launchpad.net/loggo"
 
-	"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"
 	"launchpad.net/juju-core/names"
-	"launchpad.net/juju-core/state"
-	"launchpad.net/juju-core/state/api"
-	"launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/utils"
 )
 
@@ -83,12 +77,9 @@
 type ContainerManager interface {
 	// StartContainer creates and starts a new lxc container for the specified machine.
 	StartContainer(
-		machineId, series, nonce string,
-		network *NetworkConfig,
-		tools *tools.Tools,
-		environConfig *config.Config,
-		stateInfo *state.Info,
-		apiInfo *api.Info) (instance.Instance, error)
+		machineConfig *cloudinit.MachineConfig,
+		series string,
+		network *NetworkConfig) (instance.Instance, error)
 	// StopContainer stops and destroyes the lxc container identified by Instance.
 	StopContainer(instance.Instance) error
 	// ListContainers return a list of containers that have been started by
@@ -113,14 +104,11 @@
 }
 
 func (manager *containerManager) StartContainer(
-	machineId, series, nonce string,
-	network *NetworkConfig,
-	tools *tools.Tools,
-	environConfig *config.Config,
-	stateInfo *state.Info,
-	apiInfo *api.Info) (instance.Instance, error) {
+	machineConfig *cloudinit.MachineConfig,
+	series string,
+	network *NetworkConfig) (instance.Instance, error) {
 
-	name := names.MachineTag(machineId)
+	name := names.MachineTag(machineConfig.MachineId)
 	if manager.name != "" {
 		name = fmt.Sprintf("%s-%s", manager.name, name)
 	}
@@ -137,7 +125,7 @@
 		return nil, err
 	}
 	logger.Tracef("write cloud-init")
-	userDataFilename, err := writeUserData(directory, machineId, nonce, tools, environConfig, stateInfo, apiInfo)
+	userDataFilename, err := writeUserData(machineConfig, directory)
 	if err != nil {
 		logger.Errorf("failed to write user data: %v", err)
 		return nil, err
@@ -300,14 +288,8 @@
 	return configFilename, nil
 }
 
-func writeUserData(
-	directory, machineId, nonce string,
-	tools *tools.Tools,
-	environConfig *config.Config,
-	stateInfo *state.Info,
-	apiInfo *api.Info,
-) (string, error) {
-	userData, err := cloudInitUserData(machineId, nonce, tools, environConfig, stateInfo, apiInfo)
+func writeUserData(machineConfig *cloudinit.MachineConfig, directory string) (string, error) {
+	userData, err := cloudInitUserData(machineConfig)
 	if err != nil {
 		logger.Errorf("failed to create user data: %v", err)
 		return "", err
@@ -320,25 +302,8 @@
 	return userDataFilename, nil
 }
 
-func cloudInitUserData(
-	machineId, nonce string,
-	tools *tools.Tools,
-	environConfig *config.Config,
-	stateInfo *state.Info,
-	apiInfo *api.Info,
-) ([]byte, error) {
-	machineConfig := &cloudinit.MachineConfig{
-		MachineId:            machineId,
-		MachineNonce:         nonce,
-		MachineContainerType: instance.LXC,
-		StateInfo:            stateInfo,
-		APIInfo:              apiInfo,
-		DataDir:              "/var/lib/juju",
-		Tools:                tools,
-	}
-	if err := environs.FinishMachineConfig(machineConfig, environConfig, constraints.Value{}); err != nil {
-		return nil, err
-	}
+func cloudInitUserData(machineConfig *cloudinit.MachineConfig) ([]byte, error) {
+	machineConfig.DataDir = "/var/lib/juju"
 	cloudConfig, err := cloudinit.New(machineConfig)
 	if err != nil {
 		return nil, err

=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go	2013-10-01 21:21:01 +0000
+++ container/lxc/lxc_test.go	2013-10-31 21:39:38 +0000
@@ -16,10 +16,10 @@
 	"launchpad.net/loggo"
 
 	"launchpad.net/juju-core/container/lxc"
+	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/instance"
 	instancetest "launchpad.net/juju-core/instance/testing"
 	jujutesting "launchpad.net/juju-core/juju/testing"
-	"launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
 	"launchpad.net/juju-core/tools"
@@ -65,19 +65,17 @@
 )
 
 func StartContainer(c *gc.C, manager lxc.ContainerManager, machineId string) instance.Instance {
-	config := testing.EnvironConfig(c)
 	stateInfo := jujutesting.FakeStateInfo(machineId)
 	apiInfo := jujutesting.FakeAPIInfo(machineId)
-	network := lxc.BridgeNetworkConfig("nic42")
-
-	series := "series"
-	nonce := "fake-nonce"
-	tools := &tools.Tools{
+	machineConfig := environs.NewMachineConfig(machineId, "fake-nonce", stateInfo, apiInfo)
+	machineConfig.Tools = &tools.Tools{
 		Version: version.MustParseBinary("2.3.4-foo-bar"),
 		URL:     "http://tools.testing.invalid/2.3.4-foo-bar.tgz",
 	}
 
-	inst, err := manager.StartContainer(machineId, series, nonce, network, tools, config, stateInfo, apiInfo)
+	series := "series"
+	network := lxc.BridgeNetworkConfig("nic42")
+	inst, err := manager.StartContainer(machineConfig, series, network)
 	c.Assert(err, gc.IsNil)
 	return inst
 }

=== modified file 'environs/cloudinit.go'
--- environs/cloudinit.go	2013-10-02 19:30:07 +0000
+++ environs/cloudinit.go	2013-10-31 21:39:38 +0000
@@ -51,6 +51,23 @@
 	return mcfg
 }
 
+func PopulateMachineConfig(mcfg *cloudinit.MachineConfig,
+	providerType, authorizedKeys string,
+	sslHostnameVerification bool) error {
+
+	if authorizedKeys == "" {
+		return fmt.Errorf("environment configuration has no authorized-keys")
+	}
+	mcfg.AuthorizedKeys = authorizedKeys
+	if mcfg.AgentEnvironment == nil {
+		mcfg.AgentEnvironment = make(map[string]string)
+	}
+	mcfg.AgentEnvironment[agent.ProviderType] = providerType
+	mcfg.AgentEnvironment[agent.ContainerType] = string(mcfg.MachineContainerType)
+	mcfg.DisableSSLHostnameVerification = !sslHostnameVerification
+	return nil
+}
+
 // FinishMachineConfig sets fields on a MachineConfig that can be determined by
 // inspecting a plain config.Config and the machine constraints at the last
 // moment before bootstrapping. It assumes that the supplied Config comes from
@@ -64,18 +81,9 @@
 func FinishMachineConfig(mcfg *cloudinit.MachineConfig, cfg *config.Config, cons constraints.Value) (err error) {
 	defer utils.ErrorContextf(&err, "cannot complete machine configuration")
 
-	// Everything needs the environment's authorized keys.
-	authKeys := cfg.AuthorizedKeys()
-	if authKeys == "" {
-		return fmt.Errorf("environment configuration has no authorized-keys")
-	}
-	mcfg.AuthorizedKeys = authKeys
-	if mcfg.AgentEnvironment == nil {
-		mcfg.AgentEnvironment = make(map[string]string)
-	}
-	mcfg.AgentEnvironment[agent.ProviderType] = cfg.Type()
-	mcfg.AgentEnvironment[agent.ContainerType] = string(mcfg.MachineContainerType)
-	mcfg.DisableSSLHostnameVerification = !cfg.SSLHostnameVerification()
+	if err := PopulateMachineConfig(mcfg, cfg.Type(), cfg.AuthorizedKeys(), cfg.SSLHostnameVerification()); err != nil {
+		return err
+	}
 
 	// The following settings are only appropriate at bootstrap time. At the
 	// moment, the only state server is the bootstrap node, but this

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2013-10-31 10:28:40 +0000
+++ provider/local/environ.go	2013-10-31 21:39:38 +0000
@@ -261,17 +261,15 @@
 func (env *localEnviron) StartInstance(cons constraints.Value, possibleTools tools.List,
 	machineConfig *cloudinit.MachineConfig) (instance.Instance, *instance.HardwareCharacteristics, error) {
 
-	machineId := machineConfig.MachineId
 	series := possibleTools.OneSeries()
-	logger.Debugf("StartInstance: %q, %s", machineId, series)
-	agenttools := possibleTools[0]
-	logger.Debugf("tools: %#v", agenttools)
-
+	logger.Debugf("StartInstance: %q, %s", machineConfig.MachineId, series)
+	machineConfig.Tools = possibleTools[0]
+	logger.Debugf("tools: %#v", machineConfig.Tools)
 	network := lxc.BridgeNetworkConfig(env.config.networkBridge())
-	inst, err := env.containerManager.StartContainer(
-		machineId, series, machineConfig.MachineNonce, network,
-		agenttools, env.config.Config,
-		machineConfig.StateInfo, machineConfig.APIInfo)
+	if err := environs.FinishMachineConfig(machineConfig, env.config.Config, cons); err != nil {
+		return nil, nil, err
+	}
+	inst, err := env.containerManager.StartContainer(machineConfig, series, network)
 	if err != nil {
 		return nil, nil, err
 	}

=== modified file 'state/api/params/params.go'
--- state/api/params/params.go	2013-09-18 15:57:30 +0000
+++ state/api/params/params.go	2013-10-31 21:39:38 +0000
@@ -405,3 +405,11 @@
 		Id:   i.Tag,
 	}
 }
+
+// ContainerConfig contains information from the environment config that are
+// needed for container cloud-init.
+type ContainerConfig struct {
+	ProviderType            string
+	AuthorizedKeys          string
+	SSLHostnameVerification bool
+}

=== modified file 'state/api/provisioner/provisioner.go'
--- state/api/provisioner/provisioner.go	2013-10-02 14:26:55 +0000
+++ state/api/provisioner/provisioner.go	2013-10-31 21:39:38 +0000
@@ -154,3 +154,10 @@
 	}
 	return result.Tools, nil
 }
+
+// ContainerConfig returns information from the environment config that are
+// needed for container cloud-init.
+func (st *State) ContainerConfig() (result params.ContainerConfig, err error) {
+	err = st.caller.Call("Provisioner", "", "ContainerConfig", nil, &result)
+	return result, err
+}

=== modified file 'state/api/provisioner/provisioner_test.go'
--- state/api/provisioner/provisioner_test.go	2013-10-17 17:45:41 +0000
+++ state/api/provisioner/provisioner_test.go	2013-10-31 21:39:38 +0000
@@ -368,6 +368,14 @@
 	c.Assert(addresses, gc.DeepEquals, apiAddresses)
 }
 
+func (s *provisionerSuite) TestContainerConfig(c *gc.C) {
+	result, err := s.provisioner.ContainerConfig()
+	c.Assert(err, gc.IsNil)
+	c.Assert(result.ProviderType, gc.Equals, "dummy")
+	c.Assert(result.AuthorizedKeys, gc.Equals, "my-keys")
+	c.Assert(result.SSLHostnameVerification, jc.IsTrue)
+}
+
 func (s *provisionerSuite) TestCACert(c *gc.C) {
 	caCert, err := s.provisioner.CACert()
 	c.Assert(err, gc.IsNil)

=== modified file 'state/apiserver/provisioner/provisioner.go'
--- state/apiserver/provisioner/provisioner.go	2013-10-02 10:03:25 +0000
+++ state/apiserver/provisioner/provisioner.go	2013-10-31 21:39:38 +0000
@@ -182,6 +182,20 @@
 	return result, nil
 }
 
+// ContainerConfig returns information from the environment config that are
+// needed for container cloud-init.
+func (p *ProvisionerAPI) ContainerConfig() (params.ContainerConfig, error) {
+	result := params.ContainerConfig{}
+	config, err := p.st.EnvironConfig()
+	if err != nil {
+		return result, err
+	}
+	result.ProviderType = config.Type()
+	result.AuthorizedKeys = config.AuthorizedKeys()
+	result.SSLHostnameVerification = config.SSLHostnameVerification()
+	return result, nil
+}
+
 // Status returns the status of each given machine entity.
 func (p *ProvisionerAPI) Status(args params.Entities) (params.StatusResults, error) {
 	result := params.StatusResults{

=== modified file 'state/apiserver/provisioner/provisioner_test.go'
--- state/apiserver/provisioner/provisioner_test.go	2013-10-18 14:07:34 +0000
+++ state/apiserver/provisioner/provisioner_test.go	2013-10-31 21:39:38 +0000
@@ -679,6 +679,14 @@
 	c.Check(results.Results, gc.HasLen, 0)
 }
 
+func (s *provisionerSuite) TestContainerConfig(c *gc.C) {
+	results, err := s.provisioner.ContainerConfig()
+	c.Check(err, gc.IsNil)
+	c.Check(results.ProviderType, gc.Equals, "dummy")
+	c.Check(results.AuthorizedKeys, gc.Equals, "my-keys")
+	c.Check(results.SSLHostnameVerification, jc.IsTrue)
+}
+
 func (s *provisionerSuite) TestToolsRefusesWrongAgent(c *gc.C) {
 	anAuthorizer := s.authorizer
 	anAuthorizer.Tag = "machine-12354"

=== modified file 'worker/provisioner/lxc-broker.go'
--- worker/provisioner/lxc-broker.go	2013-08-30 04:50:15 +0000
+++ worker/provisioner/lxc-broker.go	2013-10-31 21:39:38 +0000
@@ -11,8 +11,8 @@
 	"launchpad.net/juju-core/container/lxc"
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/environs/cloudinit"
-	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/state/api/params"
 	"launchpad.net/juju-core/tools"
 )
 
@@ -21,10 +21,14 @@
 var _ environs.InstanceBroker = (*lxcBroker)(nil)
 var _ tools.HasTools = (*lxcBroker)(nil)
 
-func NewLxcBroker(config *config.Config, tools *tools.Tools, agentConfig agent.Config) environs.InstanceBroker {
+type APICalls interface {
+	ContainerConfig() (params.ContainerConfig, error)
+}
+
+func NewLxcBroker(api APICalls, tools *tools.Tools, agentConfig agent.Config) environs.InstanceBroker {
 	return &lxcBroker{
 		manager:     lxc.NewContainerManager(lxc.ManagerConfig{Name: "juju"}),
-		config:      config,
+		api:         api,
 		tools:       tools,
 		agentConfig: agentConfig,
 	}
@@ -32,7 +36,7 @@
 
 type lxcBroker struct {
 	manager     lxc.ContainerManager
-	config      *config.Config
+	api         APICalls
 	tools       *tools.Tools
 	agentConfig agent.Config
 }
@@ -56,9 +60,25 @@
 	network := lxc.BridgeNetworkConfig(bridgeDevice)
 
 	series := possibleTools.OneSeries()
-	inst, err := broker.manager.StartContainer(
-		machineId, series, machineConfig.MachineNonce, network, possibleTools[0], broker.config,
-		machineConfig.StateInfo, machineConfig.APIInfo)
+	machineConfig.MachineContainerType = instance.LXC
+	machineConfig.Tools = possibleTools[0]
+
+	config, err := broker.api.ContainerConfig()
+	if err != nil {
+		lxcLogger.Errorf("failed to get container config: %v", err)
+		return nil, nil, err
+	}
+	if err := environs.PopulateMachineConfig(
+		machineConfig,
+		config.ProviderType,
+		config.AuthorizedKeys,
+		config.SSLHostnameVerification,
+	); err != nil {
+		lxcLogger.Errorf("failed to populate machine config: %v", err)
+		return nil, nil, err
+	}
+
+	inst, err := broker.manager.StartContainer(machineConfig, series, network)
 	if err != nil {
 		lxcLogger.Errorf("failed to start container: %v", err)
 		return nil, nil, err

=== modified file 'worker/provisioner/lxc-broker_test.go'
--- worker/provisioner/lxc-broker_test.go	2013-10-31 11:37:11 +0000
+++ worker/provisioner/lxc-broker_test.go	2013-10-31 21:39:38 +0000
@@ -22,6 +22,7 @@
 	jujutesting "launchpad.net/juju-core/juju/testing"
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/state/api/params"
 	coretesting "launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
 	coretools "launchpad.net/juju-core/tools"
@@ -64,7 +65,6 @@
 		Version: version.MustParseBinary("2.3.4-foo-bar"),
 		URL:     "http://tools.testing.invalid/2.3.4-foo-bar.tgz",
 	}
-	config := coretesting.EnvironConfig(c)
 	var err error
 	s.agentConfig, err = agent.NewAgentConfig(
 		agent.AgentConfigParams{
@@ -76,7 +76,7 @@
 			CACert:       []byte(coretesting.CACert),
 		})
 	c.Assert(err, gc.IsNil)
-	s.broker = provisioner.NewLxcBroker(config, tools, s.agentConfig)
+	s.broker = provisioner.NewLxcBroker(&fakeAPI{}, tools, s.agentConfig)
 }
 
 func (s *lxcBrokerSuite) startInstance(c *gc.C, machineId string) instance.Instance {
@@ -277,3 +277,9 @@
 	s.expectStopped(c, instId)
 	s.waitRemoved(c, container)
 }
+
+type fakeAPI struct{}
+
+func (*fakeAPI) ContainerConfig() (params.ContainerConfig, error) {
+	return params.ContainerConfig{"fake", "my-keys", true}, nil
+}

=== modified file 'worker/provisioner/provisioner.go'
--- worker/provisioner/provisioner.go	2013-10-02 10:03:25 +0000
+++ worker/provisioner/provisioner.go	2013-10-31 21:39:38 +0000
@@ -15,6 +15,7 @@
 	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/instance"
 	apiprovisioner "launchpad.net/juju-core/state/api/provisioner"
+	apiwatcher "launchpad.net/juju-core/state/api/watcher"
 	"launchpad.net/juju-core/state/watcher"
 	coretools "launchpad.net/juju-core/tools"
 	"launchpad.net/juju-core/worker"
@@ -75,26 +76,21 @@
 }
 
 func (p *Provisioner) loop() error {
-	environWatcher, err := p.st.WatchForEnvironConfigChanges()
-	if err != nil {
-		return err
-	}
-	environConfigChanges := environWatcher.Changes()
-	defer watcher.Stop(environWatcher, &p.tomb)
-
-	p.environ, err = worker.WaitForEnviron(environWatcher, p.st, p.tomb.Dying())
-	if err != nil {
-		return err
-	}
-
-	if p.pt != ENVIRON {
-		// Only the environment provisioner cares about
-		// changes to the environment configuration.
-		if err := environWatcher.Stop(); err != nil {
-			return err
-		}
-		// Don't wait for changes on the channel below.
-		environConfigChanges = nil
+	// Only wait for the environment if we are an environmental provisioner.
+	var environConfigChanges <-chan struct{}
+	var environWatcher apiwatcher.NotifyWatcher
+	if p.pt == ENVIRON {
+		environWatcher, err := p.st.WatchForEnvironConfigChanges()
+		if err != nil {
+			return err
+		}
+		environConfigChanges = environWatcher.Changes()
+		defer watcher.Stop(environWatcher, &p.tomb)
+
+		p.environ, err = worker.WaitForEnviron(environWatcher, p.st, p.tomb.Dying())
+		if err != nil {
+			return err
+		}
 	}
 
 	auth, err := NewAPIAuthenticator(p.st)
@@ -177,13 +173,12 @@
 	case ENVIRON:
 		return p.environ, nil
 	case LXC:
-		config := p.environ.Config()
 		tools, err := p.getAgentTools()
 		if err != nil {
 			logger.Errorf("cannot get tools from machine for lxc broker")
 			return nil, err
 		}
-		return NewLxcBroker(config, tools, p.agentConfig), nil
+		return NewLxcBroker(p.st, tools, p.agentConfig), nil
 	}
 	return nil, fmt.Errorf("unknown provisioner type")
 }