Issue 11330043: Conditionally install lxc

Can't Edit
Can't Publish+Mail
Start Review

Created:
10 years, 9 months ago by thumper

Modified:
10 years, 9 months ago

Reviewers:
mue, mp+174928, gz

Visibility:
Public.

Description

Conditionally install lxc There are two primary situations where we don't want the machine agent to install lxc: 1. when the machine agent is inside an lxc container as known by state 2. when the machine has been provisioned by the local provider (because it is an lxc container) When lxc is installed inside a container, it adds a bridge network device with the same defaults as the host. This kills any routing in or out of othe container. Upstart provides support for setting environment variables as part of the config, and we use this to pass the provider type through. https://code.launchpad.net/~thumper/juju-core/selectively-install-lxc/+merge/174928 Requires: https://code.launchpad.net/~thumper/juju-core/lxc-identity/+merge/174922 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 10

Created: 10 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+60 lines, -15 lines)			Patch
A	[revision details]	View	1 chunk	+2 lines, -0 lines	0 comments	Download
M	cmd/jujud/machine.go	View	1 chunk	+1 line, -1 line	0 comments	Download
M	cmd/jujud/upgradevalidation.go	View	2 chunks	+18 lines, -1 line	4 comments	Download
M	environs/azure/customdata_test.go	View	1 chunk	+1 line, -0 lines	0 comments	Download
M	environs/cloudinit.go	View	1 chunk	+1 line, -0 lines	0 comments	Download
M	environs/cloudinit/cloudinit.go	View	3 chunks	+7 lines, -1 line	0 comments	Download
M	environs/cloudinit/cloudinit_test.go	View	8 chunks	+9 lines, -4 lines	3 comments	Download
M	environs/cloudinit_test.go	View	1 chunk	+1 line, -0 lines	0 comments	Download
M	environs/local/environ.go	View	1 chunk	+3 lines, -1 line	2 comments	Download
M	environs/local/environprovider.go	View	2 chunks	+3 lines, -1 line	1 comment	Download
M	environs/maas/util_test.go	View	1 chunk	+6 lines, -5 lines	0 comments	Download
M	upstart/service.go	View	2 chunks	+4 lines, -1 line	0 comments	Download
M	worker/deployer/simple.go	View	1 chunk	+4 lines, -0 lines	0 comments	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

LGTM https://codereview.appspot.com/11330043/diff/1/cmd/jujud/upgradevalidation.go File cmd/jujud/upgradevalidation.go (right): https://codereview.appspot.com/11330043/diff/1/cmd/jujud/upgradevalidation.go#newcode62 cmd/jujud/upgradevalidation.go:62: if providerType == "local" || containerType == instance.LXC ...

10 years, 9 months ago (2013-07-16 08:45:23 UTC) #2

mue

LGTM together with Martins comments. https://codereview.appspot.com/11330043/diff/1/cmd/jujud/upgradevalidation.go File cmd/jujud/upgradevalidation.go (right): https://codereview.appspot.com/11330043/diff/1/cmd/jujud/upgradevalidation.go#newcode62 cmd/jujud/upgradevalidation.go:62: if providerType == "local" ...

10 years, 9 months ago (2013-07-16 10:52:37 UTC) #3

thumper

10 years, 9 months ago (2013-07-17 02:11:08 UTC) #4

https://codereview.appspot.com/11330043/diff/1/cmd/jujud/upgradevalidation.go
File cmd/jujud/upgradevalidation.go (right):

https://codereview.appspot.com/11330043/diff/1/cmd/jujud/upgradevalidation.go...
cmd/jujud/upgradevalidation.go:62: if providerType == "local" || containerType
== instance.LXC {
On 2013/07/16 08:45:23, gz wrote:
> It's a little surprising that these are two different condiditions. Should the
> local provider 'machines' not be aware that they're lxc containers anyway?

Ideally yes, and hopefully relatively soon.

When we get multi-provider environments, the machines should know which provider
had created them.  The more interesting bit here is that we don't have access to
the machine doc here, so can't check anyway.

https://codereview.appspot.com/11330043/diff/1/cmd/jujud/upgradevalidation.go...
cmd/jujud/upgradevalidation.go:62: if providerType == "local" || containerType
== instance.LXC {
On 2013/07/16 10:52:37, mue wrote:
> Please change "local" to a constant usable in the whole system.

This adds a bucket load of dependencies that it shouldn't have.

I propose that RSN, we change the way we do some constants.

I'll think about the best solution, but may well be independent packages that
have no other dependencies.

https://codereview.appspot.com/11330043/diff/1/environs/cloudinit/cloudinit_t...
File environs/cloudinit/cloudinit_test.go (right):

https://codereview.appspot.com/11330043/diff/1/environs/cloudinit/cloudinit_t...
environs/cloudinit/cloudinit_test.go:108: cat >> /etc/init/jujud-machine-0\.conf
<< 'EOF'\\ndescription "juju machine-0 agent"\\nauthor "Juju Team
<juju@lists\.ubuntu\.com>"\\nstart on runlevel \[2345\]\\nstop on runlevel
\[!2345\]\\nrespawn\\nnormal exit 0\\nenv JUJU_PROVIDER_TYPE="dummy"\\n\\nlimit
nofile 20000 20000\\n\\nexec /var/lib/juju/tools/machine-0/jujud machine
--log-file '/var/log/juju/machine-0\.log' --data-dir '/var/lib/juju'
--machine-id 0  --debug >> /var/log/juju/machine-0\.log 2>&1\\nEOF\\n
On 2013/07/16 10:52:37, mue wrote:
> On 2013/07/16 08:45:23, gz wrote:
> > These test assertions are so bad...
> 
> +1
> 
> As it grows we should take time to find a more elegant way checking the script
> content, e.g. different contains() with regex or so.

I agree, it is just getting crazy.

Certainly worth spending some time on later.

https://codereview.appspot.com/11330043/diff/1/environs/local/environ.go
File environs/local/environ.go (right):

https://codereview.appspot.com/11330043/diff/1/environs/local/environ.go#newc...
environs/local/environ.go:382: agent.Env["HOME"] = os.Getenv("HOME")
On 2013/07/16 08:45:23, gz wrote:
> Not strictly releated to this proposal?

Not entirely, no.

Expand All Messages | Collapse All Messages