juju-core

Merge lp:~axwalk/juju-core/lp1271144-maas-bridge-utils into lp:~go-bot/juju-core/trunk

  1. lp1271144-maas-bridge-utils
  2. Merge into trunk
Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 2457
Proposed branch: lp:~axwalk/juju-core/lp1271144-maas-bridge-utils
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 142 lines (+65/-12)
5 files modified
provider/maas/environ.go (+21/-10)
provider/maas/environ_test.go (+15/-0)
provider/maas/export_test.go (+3/-2)
utils/apt.go (+10/-0)
utils/apt_test.go (+16/-0)
To merge this branch: bzr merge lp:~axwalk/juju-core/lp1271144-maas-bridge-utils
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+211886@code.launchpad.net

Commit message

Fix maas bridge-utils installation

Need to do an apt-get update first. I've updated
the code to use the appropriate options to disable
all interactivity.

Also, changed the code to the ifdown/ifup correctly.

Live tested with "juju deploy ubuntu --to lxc:0"

Fixes lp:1271144
Fixes lp:1248283

https://codereview.appspot.com/77890045/

Description of the change

Fix maas bridge-utils installation

Need to do an apt-get update first. I've updated
the code to use the appropriate options to disable
all interactivity.

Also, changed the code to the ifdown/ifup correctly.

Live tested with "juju deploy ubuntu --to lxc:0"

Fixes lp:1271144
Fixes lp:1248283

https://codereview.appspot.com/77890045/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (6.1 KiB)

Reviewers: mp+211886_code.launchpad.net,

Message:
Please take a look.

Description:
Fix maas bridge-utils installation

Need to do an apt-get update first. I've updated
the code to use the appropriate options to disable
all interactivity.

Also, changed the code to the ifdown/ifup correctly.

Live tested with "juju deploy ubuntu --to lxc:0"

Fixes lp:1271144
Fixes lp:1248283

https://code.launchpad.net/~axwalk/juju-core/lp1271144-maas-bridge-utils/+merge/211886

(do not edit description out of merge proposal)

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

Affected files (+67, -12 lines):
   A [revision details]
   M provider/maas/environ.go
   M provider/maas/environ_test.go
   M provider/maas/export_test.go
   M utils/apt.go
   M utils/apt_test.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: tarmac-20140320055725-noj3sbg573beucbk
+New revision: <email address hidden>

Index: utils/apt.go
=== modified file 'utils/apt.go'
--- utils/apt.go 2014-03-13 20:51:48 +0000
+++ utils/apt.go 2014-03-20 09:08:27 +0000
@@ -81,6 +81,16 @@
   }
  }

+// AptGetCommand returns a command to execute apt-get
+// with the specified arguments, and the appropriate
+// environment variables and options for a non-interactive
+// session.
+func AptGetCommand(args ...string) []string {
+ cmd := append([]string{"env"}, aptGetEnvOptions...)
+ cmd = append(cmd, aptGetCommand...)
+ return append(cmd, args...)
+}
+
  // AptGetPreparePackages returns a slice of installCommands. Each item
  // in the slice is suitable for passing directly to AptGetInstall.
  //

Index: utils/apt_test.go
=== modified file 'utils/apt_test.go'
--- utils/apt_test.go 2014-03-13 20:52:41 +0000
+++ utils/apt_test.go 2014-03-20 09:08:27 +0000
@@ -39,6 +39,22 @@
   c.Assert(packagesList[1], gc.DeepEquals, []string{"bridge-utils", "git"})
  }

+func (s *AptSuite) TestAptGetCommand(c *gc.C) {
+ s.testAptGetCommand(c)
+ s.testAptGetCommand(c, "install", "foo")
+}
+
+func (s *AptSuite) testAptGetCommand(c *gc.C, args ...string) {
+ commonArgs := []string{
+ "env", "DEBIAN_FRONTEND=noninteractive",
+ "apt-get", "--option=Dpkg::Options::=--force-confold",
+ "--option=Dpkg::options::=--force-unsafe-io", "--assume-yes", "--quiet",
+ }
+ expected := append(commonArgs, args...)
+ cmd := utils.AptGetCommand(args...)
+ c.Assert(cmd, gc.DeepEquals, expected)
+}
+
  func (s *AptSuite) TestAptGetError(c *gc.C) {
   const expected = `E: frobnicator failure detected`
   cmdError := fmt.Errorf("error")

Index: provider/maas/environ.go
=== modified file 'provider/maas/environ.go'
--- provider/maas/environ.go 2014-03-19 19:35:33 +0000
+++ provider/maas/environ.go 2014-03-20 09:08:27 +0000
@@ -276,8 +276,7 @@
   if err != nil {
    return nil, nil, err
   }
- info := machineInfo{hostname}
- runCmd, err := info.cloudinitRunCmd()
+ additionalScripts, err := additionalScripts(hostname)
   if err != nil {
    return nil, nil, err
   }
@@ -288,14 +287,7 @@
   // The machine envronment config values are being moved to the...

Read more...

Revision history for this message
Roger Peppe (rogpeppe) wrote :

LGTM with a couple of thoughts below.

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ.go#newcode316
provider/maas/environ.go:316:
utils.CommandString(utils.AptGetCommand("update")...),
It seems a pity that we can't just call SetAptUpdate on the
cloudinit config, but that would require a change to ComposeUserData
(might it make sense to pass in a possibly-nil cloudinit.Config to that
function?)

Aside: CommandString should also escape backquotes, although it won't be
a problem here

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ_test.go
File provider/maas/environ_test.go (right):

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ_test.go#newcode208
provider/maas/environ_test.go:208: const aptGetPrefix = "env
DEBIAN_FRONTEND=noninteractive apt-get
--option=Dpkg::Options::=--force-confold
--option=Dpkg::options::=--force-unsafe-io --assume-yes --quiet"
or aptGetPrefix := utils.AptGetCommand()
?

https://codereview.appspot.com/77890045/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

On 2014/03/20 09:42:15, rog wrote:
> LGTM with a couple of thoughts below.

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ.go
> File provider/maas/environ.go (right):

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ.go#newcode316
> provider/maas/environ.go:316:
> utils.CommandString(utils.AptGetCommand("update")...),
> It seems a pity that we can't just call SetAptUpdate on the
> cloudinit config, but that would require a change to ComposeUserData
> (might it make sense to pass in a possibly-nil cloudinit.Config to
 that
> function?)

> Aside: CommandString should also escape backquotes, although it won't
be a
> problem here

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ_test.go
> File provider/maas/environ_test.go (right):

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ_test.go#newcode208
> provider/maas/environ_test.go:208: const aptGetPrefix = "env
> DEBIAN_FRONTEND=noninteractive apt-get
 --option=Dpkg::Options::=--force-confold
> --option=Dpkg::options::=--force-unsafe-io --assume-yes --quiet"
> or aptGetPrefix := utils.AptGetCommand()
> ?

Except you also have to flatten, and I'd rather avoid using the code
that's used by the thing being tested.

https://codereview.appspot.com/77890045/

Revision history for this message
Wayne Witzel III (wwitzel3) wrote :

LGTM with a couple of comments, but not show stoppers.

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ.go
File provider/maas/environ.go (right):

https://codereview.appspot.com/77890045/diff/1/provider/maas/environ.go#newcode316
provider/maas/environ.go:316:
utils.CommandString(utils.AptGetCommand("update")...),
On 2014/03/20 09:42:15, rog wrote:
> It seems a pity that we can't just call SetAptUpdate on the
> cloudinit config, but that would require a change to ComposeUserData
> (might it make sense to pass in a possibly-nil cloudinit.Config to
 that
> function?)

I agree, it is a shame we can't just call SetAptUpdate ... how much
extra work would it be to make the refactor?

https://codereview.appspot.com/77890045/diff/1/utils/apt.go
File utils/apt.go (right):

https://codereview.appspot.com/77890045/diff/1/utils/apt.go#newcode88
utils/apt.go:88: func AptGetCommand(args ...string) []string {
I have no complaint about the composition of this function, just than we
are using it to perform an apt-get install, when we have a func
AptGetInstall.

At the least I feel like any of the existing Apt functions should use
this under the covers.

This also kind of ties back to if we can refactor the ComposeUserData to
call SetAptUpdate for the cloudinit. We could then use AptGetInstall for
the package install and this function could go away entirely.

https://codereview.appspot.com/77890045/

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (46.9 KiB)

The attempt to merge lp:~axwalk/juju-core/lp1271144-maas-bridge-utils into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.013s
ok launchpad.net/juju-core/agent 1.077s
ok launchpad.net/juju-core/agent/mongo 0.532s
ok launchpad.net/juju-core/agent/tools 0.259s
ok launchpad.net/juju-core/bzr 5.022s
ok launchpad.net/juju-core/cert 2.625s
ok launchpad.net/juju-core/charm 0.477s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.029s
ok launchpad.net/juju-core/cloudinit/sshinit 0.885s
ok launchpad.net/juju-core/cmd 0.186s
ok launchpad.net/juju-core/cmd/charm-admin 0.768s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 203.637s
ok launchpad.net/juju-core/cmd/jujud 63.875s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 6.760s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.204s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.021s
ok launchpad.net/juju-core/container 0.043s
ok launchpad.net/juju-core/container/factory 0.039s
ok launchpad.net/juju-core/container/kvm 0.194s
ok launchpad.net/juju-core/container/kvm/mock 0.046s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.287s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.226s
ok launchpad.net/juju-core/environs 2.361s
ok launchpad.net/juju-core/environs/bootstrap 10.541s
ok launchpad.net/juju-core/environs/cloudinit 0.491s
ok launchpad.net/juju-core/environs/config 2.939s
ok launchpad.net/juju-core/environs/configstore 0.029s
ok launchpad.net/juju-core/environs/filestorage 0.028s
ok launchpad.net/juju-core/environs/httpstorage 0.692s
ok launchpad.net/juju-core/environs/imagemetadata 0.406s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.043s
ok launchpad.net/juju-core/environs/jujutest 0.164s
ok launchpad.net/juju-core/environs/manual 10.076s
ok launchpad.net/juju-core/environs/simplestreams 0.237s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.956s
ok launchpad.net/juju-core/environs/storage 0.986s
ok launchpad.net/juju-core/environs/sync 44.513s
ok launchpad.net/juju-core/environs/testing 0.157s
ok launchpad.net/juju-core/environs/tools 4.657s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.011s
ok launchpad.net/juju-core/instance 0.017s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 19.161s
ok launchpad.net/juju-core/juju/arch 0...

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (48.8 KiB)

The attempt to merge lp:~axwalk/juju-core/lp1271144-maas-bridge-utils into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.014s
ok launchpad.net/juju-core/agent 1.049s
ok launchpad.net/juju-core/agent/mongo 0.517s
ok launchpad.net/juju-core/agent/tools 0.189s
ok launchpad.net/juju-core/bzr 5.470s
ok launchpad.net/juju-core/cert 2.330s
ok launchpad.net/juju-core/charm 0.395s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.029s
ok launchpad.net/juju-core/cloudinit/sshinit 0.777s
ok launchpad.net/juju-core/cmd 0.152s
ok launchpad.net/juju-core/cmd/charm-admin 0.723s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 203.675s

----------------------------------------------------------------------
PANIC: bootstrap_test.go:69: BootstrapSuite.SetUpTest

[LOG] 33.94440 DEBUG juju.testing tls.Dial(127.0.0.1:54846) failed with dial tcp 127.0.0.1:54846: connection refused
[LOG] 33.94453 DEBUG juju.testing tls.Dial(127.0.0.1:54846) failed with dial tcp 127.0.0.1:54846: connection refused
[LOG] 34.00495 ERROR juju mongodb has exited without being killed
[LOG] 34.00497 ERROR juju mongod: [4 lines omitted]
[LOG] 34.00499 ERROR juju mongod: Thu Mar 20 10:24:15.113 [initandlisten] build info: Linux roseapple 2.6.42-37-generic #58-Ubuntu SMP Thu Jan 24 15:28:10 UTC 2013 x86_64 BOOST_LIB_VERSION=1_46_1
[LOG] 34.00499 ERROR juju mongod: Thu Mar 20 10:24:15.113 [initandlisten] allocator: tcmalloc
[LOG] 34.00501 ERROR juju mongod: Thu Mar 20 10:24:15.113 [initandlisten] options: { auth: true, dbpath: "/tmp/test-mgo775669954", keyFile: "/tmp/test-mgo775669954/keyfile", nojournal: true, noprealloc: true, nounixsocket: true, nssize: 1, oplogSize: 10, port: 54846, smallfiles: true, sslOnNormalPorts: true, sslPEMKeyFile: "/tmp/test-mgo775669954/server.pem", sslPEMKeyPassword: "<password>" }
[LOG] 34.00502 ERROR juju mongod: Thu Mar 20 10:24:15.122 [FileAllocator] allocating new datafile /tmp/test-mgo775669954/local.ns, filling with zeroes...
[LOG] 34.00503 ERROR juju mongod: Thu Mar 20 10:24:15.122 [FileAllocator] creating directory /tmp/test-mgo775669954/_tmp
[LOG] 34.00505 ERROR juju mongod: Thu Mar 20 10:24:15.125 [FileAllocator] done allocating datafile /tmp/test-mgo775669954/local.ns, size: 1MB, took 0.001 secs
[LOG] 34.00505 ERROR juju mongod: Thu Mar 20 10:24:15.126 [FileAllocator] allocating new datafile /tmp/test-mgo775669954/local.0, filling with zeroes...
[LOG] 34.00506 ERROR juju mongod: Thu Mar 20 10:24:15.127 [FileAllocator] done allocating datafile /tmp/test-mgo775669954/local.0, size: 16MB, took 0.001 secs
[LOG] 34.00507 ERROR juju mongod: Thu Mar 20 10:24:15.128 [initandlisten] ERROR: listen(): bind() failed errno:98 Address already in use for socket: 0.0.0.0:54846
[LOG] 34.00508 ERROR juju mongod: Thu Mar 20 10:24:15.128 [initandlisten] ERROR: addr already in use
[LOG] 34.00509 ERROR juju mongod: Thu Mar 20 10:24:15.128 [initandlisten] now exiting
[LOG] 34.00510 ERROR juju mo...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'provider/maas/environ.go'
2--- provider/maas/environ.go 2014-03-19 19:35:33 +0000
3+++ provider/maas/environ.go 2014-03-20 09:13:42 +0000
4@@ -276,8 +276,7 @@
5 if err != nil {
6 return nil, nil, err
7 }
8- info := machineInfo{hostname}
9- runCmd, err := info.cloudinitRunCmd()
10+ additionalScripts, err := additionalScripts(hostname)
11 if err != nil {
12 return nil, nil, err
13 }
14@@ -288,14 +287,7 @@
15 // The machine envronment config values are being moved to the agent config.
16 // Explicitly specify that the lxc containers use the network bridge defined above.
17 args.MachineConfig.AgentEnvironment[agent.LxcBridge] = "br0"
18- userdata, err := environs.ComposeUserData(
19- args.MachineConfig,
20- runCmd,
21- "apt-get install bridge-utils",
22- createBridgeNetwork(),
23- linkBridgeInInterfaces(),
24- "service networking restart",
25- )
26+ userdata, err := environs.ComposeUserData(args.MachineConfig, additionalScripts...)
27 if err != nil {
28 msg := fmt.Errorf("could not compose userdata for bootstrap node: %v", err)
29 return nil, nil, msg
30@@ -311,6 +303,25 @@
31 return inst, nil, nil
32 }
33
34+// additionalScripts is an additional set of commands
35+// to run during cloud-init (before the synchronous phase).
36+func additionalScripts(hostname string) ([]string, error) {
37+ info := machineInfo{hostname}
38+ runCmd, err := info.cloudinitRunCmd()
39+ if err != nil {
40+ return nil, err
41+ }
42+ return []string{
43+ runCmd,
44+ utils.CommandString(utils.AptGetCommand("update")...),
45+ utils.CommandString(utils.AptGetCommand("install", "bridge-utils")...),
46+ "ifdown eth0",
47+ createBridgeNetwork(),
48+ linkBridgeInInterfaces(),
49+ "ifup br0",
50+ }, nil
51+}
52+
53 // StartInstance is specified in the InstanceBroker interface.
54 func (environ *maasEnviron) StopInstances(instances []instance.Instance) error {
55 // Shortcut to exit quickly if 'instances' is an empty slice or nil.
56
57=== modified file 'provider/maas/environ_test.go'
58--- provider/maas/environ_test.go 2014-03-19 01:20:14 +0000
59+++ provider/maas/environ_test.go 2014-03-20 09:13:42 +0000
60@@ -203,3 +203,18 @@
61 c.Assert(err, gc.IsNil)
62 c.Assert(a, gc.DeepEquals, arch.AllSupportedArches)
63 }
64+
65+func (*environSuite) TestAdditionalSCripts(c *gc.C) {
66+ const aptGetPrefix = "env DEBIAN_FRONTEND=noninteractive apt-get --option=Dpkg::Options::=--force-confold --option=Dpkg::options::=--force-unsafe-io --assume-yes --quiet"
67+ scripts, err := maas.AdditionalScripts("testing.invalid")
68+ c.Assert(err, gc.IsNil)
69+ c.Assert(scripts, gc.DeepEquals, []string{
70+ "mkdir -p '/var/lib/juju'; echo -n 'hostname: testing.invalid\n' > '/var/lib/juju/MAASmachine.txt'",
71+ aptGetPrefix + " update",
72+ aptGetPrefix + " install bridge-utils",
73+ "ifdown eth0",
74+ "cat > /etc/network/eth0.config << EOF\niface eth0 inet manual\n\nauto br0\niface br0 inet dhcp\n bridge_ports eth0\nEOF\n",
75+ `sed -i "s/iface eth0 inet dhcp/source \/etc\/network\/eth0.config/" /etc/network/interfaces`,
76+ "ifup br0",
77+ })
78+}
79
80=== modified file 'provider/maas/export_test.go'
81--- provider/maas/export_test.go 2013-12-06 11:00:58 +0000
82+++ provider/maas/export_test.go 2014-03-20 09:13:42 +0000
83@@ -10,8 +10,9 @@
84 )
85
86 var (
87- ShortAttempt = &shortAttempt
88- APIVersion = apiVersion
89+ ShortAttempt = &shortAttempt
90+ APIVersion = apiVersion
91+ AdditionalScripts = additionalScripts
92 )
93
94 func MAASAgentName(env environs.Environ) string {
95
96=== modified file 'utils/apt.go'
97--- utils/apt.go 2014-03-13 20:51:48 +0000
98+++ utils/apt.go 2014-03-20 09:13:42 +0000
99@@ -81,6 +81,16 @@
100 }
101 }
102
103+// AptGetCommand returns a command to execute apt-get
104+// with the specified arguments, and the appropriate
105+// environment variables and options for a non-interactive
106+// session.
107+func AptGetCommand(args ...string) []string {
108+ cmd := append([]string{"env"}, aptGetEnvOptions...)
109+ cmd = append(cmd, aptGetCommand...)
110+ return append(cmd, args...)
111+}
112+
113 // AptGetPreparePackages returns a slice of installCommands. Each item
114 // in the slice is suitable for passing directly to AptGetInstall.
115 //
116
117=== modified file 'utils/apt_test.go'
118--- utils/apt_test.go 2014-03-13 20:52:41 +0000
119+++ utils/apt_test.go 2014-03-20 09:13:42 +0000
120@@ -39,6 +39,22 @@
121 c.Assert(packagesList[1], gc.DeepEquals, []string{"bridge-utils", "git"})
122 }
123
124+func (s *AptSuite) TestAptGetCommand(c *gc.C) {
125+ s.testAptGetCommand(c)
126+ s.testAptGetCommand(c, "install", "foo")
127+}
128+
129+func (s *AptSuite) testAptGetCommand(c *gc.C, args ...string) {
130+ commonArgs := []string{
131+ "env", "DEBIAN_FRONTEND=noninteractive",
132+ "apt-get", "--option=Dpkg::Options::=--force-confold",
133+ "--option=Dpkg::options::=--force-unsafe-io", "--assume-yes", "--quiet",
134+ }
135+ expected := append(commonArgs, args...)
136+ cmd := utils.AptGetCommand(args...)
137+ c.Assert(cmd, gc.DeepEquals, expected)
138+}
139+
140 func (s *AptSuite) TestAptGetError(c *gc.C) {
141 const expected = `E: frobnicator failure detected`
142 cmdError := fmt.Errorf("error")

Subscribers

People subscribed via source and target branches

to status/vote changes: