Merge lp:~sidnei/juju-core/lxc-mirror into lp:~go-bot/juju-core/trunk

[Sidnei da Silva]

Fetch apt proxy config via host apt-config

Since we're passing custom userdata to the ubuntu-cloud lxc template, it ends
up not setting the apt_mirror to the MIRROR setting. Read apt proxy
configuration from the container's host instead via the apt-config command and
inject that into the container via apt_proxy cloud init setting for the http
proxy plus a runcmd that writes /etc/apt/apt.conf.d/99proxy-extra with the
same config as the host for the remaing settings not supported explicitly by
cloud-init at the moment.

https://codereview.appspot.com/12143043/

R=dimitern, rogpeppe, smoser

Fetch apt proxy config via host apt-config

Since we're passing custom userdata to the ubuntu-cloud lxc template, it ends
up not setting the apt_mirror to the MIRROR setting. Read apt proxy
configuration from the container's host instead via the apt-config command and
inject that into the container via apt_proxy cloud init setting for the http
proxy plus a runcmd that writes /etc/apt/apt.conf.d/99proxy-extra with the
same config as the host for the remaing settings not supported explicitly by
cloud-init at the moment.

https://codereview.appspot.com/12143043/

[Sidnei da Silva (sidnei)]

Reviewers: mp+177737_code.launchpad.net,

Message:
Please take a look.

Description:
Fetch MIRROR setting from /etc/default/lxc

Since we're passing custom userdata to the ubuntu-cloud lxc template, it
ends
up not setting the apt_mirror to the MIRROR setting. One could argue
that the
template should be fixed instead to always set the apt_mirror if it's
unset in
the custom userdata, but that's a separate discussion.

https://code.launchpad.net/~sidnei/juju-core/lxc-mirror/+merge/177737

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M agent/agent.go
   M cloudinit/cloudinit_test.go
   M cloudinit/options.go
   M container/lxc/lxc.go
   M container/lxc/lxc_test.go
   M environs/cloudinit/cloudinit.go
   M environs/cloudinit/cloudinit_test.go
   M utils/apt.go
   M utils/apt_test.go

[Sidnei da Silva (sidnei)]

Please take a look.

https://codereview.appspot.com/12143043/

[Dimiter Naydenov (dimitern)]

LGTM, although I think I reviewed part of that as a previous CL
(AddFile, AddScripts, etc.) did that one get merged here?

https://codereview.appspot.com/12143043/diff/7001/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/7001/container/lxc/lxc_test.go#newcode46
container/lxc/lxc_test.go:46: []byte(aptConfig), 0755)

Please put 0755 on a separate line, like the other args.

https://codereview.appspot.com/12143043/

[Sidnei da Silva (sidnei)]

Please take a look.

https://codereview.appspot.com/12143043/

[Sidnei da Silva (sidnei)]

Please take a look.

https://codereview.appspot.com/12143043/diff/7001/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/7001/container/lxc/lxc_test.go#newcode46
container/lxc/lxc_test.go:46: []byte(aptConfig), 0755)
On 2013/08/05 10:46:08, dimitern wrote:

> Please put 0755 on a separate line, like the other args.

Done.

https://codereview.appspot.com/12143043/

[Sidnei da Silva (sidnei)]

Please take a look.

https://codereview.appspot.com/12143043/

[Roger Peppe (rogpeppe)]

Looks OK to me, although I'm not qualified to
review it - I have no knowledge of apt proxy
settings, and given that we can't have tests that
actually test this for real, I'd be happier if it's
reviewed by someone that does.

Some suggestions below.

https://codereview.appspot.com/12143043/diff/15002/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/15002/container/lxc/lxc_test.go#newcode124
container/lxc/lxc_test.go:124: c.Assert(scripts[len(scripts)-4],
gc.Equals, "start jujud-machine-1-lxc-0")
c.Assert(scripts[len(scripts)-4:], gc.DeepEquals, []string{
     "start juju-machine-1-lxc-0",
     "install -m 600 /dev/null '/etc/apt.conf.d/99proxy-extra'",
     etc
})

?

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode32
utils/apt.go:32: var commandOutput = osCommandOutput

var commandOutput = exec.Cmd.Output

(and you can delete the osCommandOutput definition
and probably move most of the comment onto commandOutput)

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode60
utils/apt.go:60: var (

No need to declare these here.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode70
utils/apt.go:70: if out, err = commandOutput(cmd); err != nil {

out, err := commandOutput(cmd)
if err != nil {
     return "", err
}
return strings.TrimSpace(string(out)), nil

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode71
utils/apt.go:71: return "", err
If the command has failed for some reason other
than a missing apt-config command, this error
will be highly opaque (it will contain only
the apt-config exit code).

I suggest that you acquire the standard error output
too, and parcel it up into an error if
the command fails.

You could use the same runCommand hook mechanism that AptGetInstall
uses (and in fact, AptGetInstall suffers from the same issue, but that's
probably something to fix in another CL)

https://codereview.appspot.com/12143043/

[Sidnei da Silva (sidnei)]

Please take a look.

https://codereview.appspot.com/12143043/diff/15002/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/15002/container/lxc/lxc_test.go#newcode124
container/lxc/lxc_test.go:124: c.Assert(scripts[len(scripts)-4],
gc.Equals, "start jujud-machine-1-lxc-0")
On 2013/08/05 16:05:34, rog wrote:
> c.Assert(scripts[len(scripts)-4:], gc.DeepEquals, []string{
> "start juju-machine-1-lxc-0",
> "install -m 600 /dev/null '/etc/apt.conf.d/99proxy-extra'",
> etc
> })

> ?

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode32
utils/apt.go:32: var commandOutput = osCommandOutput
On 2013/08/05 16:05:34, rog wrote:

> var commandOutput = exec.Cmd.Output

> (and you can delete the osCommandOutput definition
> and probably move most of the comment onto commandOutput)

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode60
utils/apt.go:60: var (
On 2013/08/05 16:05:34, rog wrote:

> No need to declare these here.

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode70
utils/apt.go:70: if out, err = commandOutput(cmd); err != nil {
On 2013/08/05 16:05:34, rog wrote:

> out, err := commandOutput(cmd)
> if err != nil {
> return "", err
> }
> return strings.TrimSpace(string(out)), nil

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode70
utils/apt.go:70: if out, err = commandOutput(cmd); err != nil {
On 2013/08/05 16:05:34, rog wrote:

> out, err := commandOutput(cmd)
> if err != nil {
> return "", err
> }
> return strings.TrimSpace(string(out)), nil

Done.

https://codereview.appspot.com/12143043/diff/15002/utils/apt.go#newcode71
utils/apt.go:71: return "", err
On 2013/08/05 16:05:34, rog wrote:
> If the command has failed for some reason other
> than a missing apt-config command, this error
> will be highly opaque (it will contain only
> the apt-config exit code).

> I suggest that you acquire the standard error output
> too, and parcel it up into an error if
> the command fails.

> You could use the same runCommand hook mechanism that AptGetInstall
> uses (and in fact, AptGetInstall suffers from the same issue, but
that's
> probably something to fix in another CL)

Done, fixed in this branch.

https://codereview.appspot.com/12143043/

[Roger Peppe (rogpeppe)]

LGTM with a couple of trivials.
Thanks a lot.

https://codereview.appspot.com/12143043/diff/22001/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/22001/container/lxc/lxc_test.go#newcode128
container/lxc/lxc_test.go:128: "ifconfig"})

We'd usually put the }) on a new line here, so each string gets its own
line.

https://codereview.appspot.com/12143043/diff/22001/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/12143043/diff/22001/utils/apt.go#newcode61
utils/apt.go:61: "Acquire::ftp::Proxy"}

newline before } please

https://codereview.appspot.com/12143043/

[Scott Moser (smoser)]

so i think this is reasonable.
the only comments (that I think Sidnei is aware of) is that https_proxy set via runcmnd will be too late to affect 'apt-get installs' from cloud-config.

they should work fine for subsequent juju charm installs. though.

the principle seems sound here.

review: Approve

[Sidnei da Silva (sidnei)]

Please take a look.

https://codereview.appspot.com/12143043/diff/22001/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/12143043/diff/22001/container/lxc/lxc_test.go#newcode128
container/lxc/lxc_test.go:128: "ifconfig"})
On 2013/08/05 17:35:39, rog wrote:

> We'd usually put the }) on a new line here, so each string gets its
own line.

Done.

https://codereview.appspot.com/12143043/diff/22001/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/12143043/diff/22001/utils/apt.go#newcode61
utils/apt.go:61: "Acquire::ftp::Proxy"}
On 2013/08/05 17:35:39, rog wrote:

> newline before } please

Done.

https://codereview.appspot.com/12143043/