Merge lp:~hazmat/pyjuju/local-respect-series into lp:pyjuju

Proposed by Kapil Thangavelu
Status: Merged
Approved by: Jim Baker
Approved revision: 450
Merged at revision: 454
Proposed branch: lp:~hazmat/pyjuju/local-respect-series
Merge into: lp:pyjuju
Diff against target: 348 lines (+56/-27)
9 files modified
juju/lib/lxc/__init__.py (+10/-2)
juju/lib/lxc/data/juju-create (+5/-2)
juju/lib/lxc/tests/test_lxc.py (+18/-10)
juju/machine/tests/test_unit_deployment.py (+6/-3)
juju/machine/unit.py (+5/-2)
juju/providers/local/__init__.py (+2/-1)
juju/providers/local/agent.py (+5/-2)
juju/providers/local/tests/test_agent.py (+4/-4)
juju/providers/local/tests/test_provider.py (+1/-1)
To merge this branch: bzr merge lp:~hazmat/pyjuju/local-respect-series
Reviewer Review Type Date Requested Status
Jim Baker (community) Approve
Benjamin Saller (community) Approve
Review via email: mp+90182@code.launchpad.net

Description of the change

Modify local provider to respect the environment's default-series

Fixes issues with creating precise containers.

To post a comment you must log in.
Revision history for this message
Benjamin Saller (bcsaller) wrote :

[1] There are now a few places that precise and oneiric are included in the code. It might be worth it to make these imported constants from a single file CURRENT_UBUNTU_LTS_SERIES="", CURRENT_UBUNTU_SERIES="" and use those imported symbols through the codebase.

[2] You assert juju series in machine unit.
    [2.1] No validation that the series name is valid is preformed, though this might be YAGNI
    [2.2] It seems series is required, we might want to update the docs to reflect that a value must be provided

review: Approve
Revision history for this message
Clint Byrum (clint-fewbar) wrote :

Excerpts from Kapil Thangavelu's message of Wed Jan 25 18:31:29 UTC 2012:
> Kapil Thangavelu has proposed merging lp:~hazmat/juju/local-respect-series into lp:juju.
>
> Requested reviews:
> juju hackers (juju)
> Related bugs:
> Bug #914392 in juju: "LXC local provider does not respect 'series' (only installs oneiric)"
> https://bugs.launchpad.net/juju/+bug/914392
>
> For more details, see:
> https://code.launchpad.net/~hazmat/juju/local-respect-series/+merge/90182
>
> Modify local provider to respect the environment's default-series
>
> Fixes issues with creating precise containers.

I'm a little nervous about the fact that some functions have "oneiric"
and some "precise" as defaults.

It seems like it would be safer to *always* require the user to specify,
or at the very least, take this from the environment default-series.

Revision history for this message
Kapil Thangavelu (hazmat) wrote :

Excerpts from Clint Byrum's message of Thu Jan 26 01:58:23 UTC 2012:
> Excerpts from Kapil Thangavelu's message of Wed Jan 25 18:31:29 UTC 2012:
> > Kapil Thangavelu has proposed merging lp:~hazmat/juju/local-respect-series into lp:juju.
> >
> > Requested reviews:
> > juju hackers (juju)
> > Related bugs:
> > Bug #914392 in juju: "LXC local provider does not respect 'series' (only installs oneiric)"
> > https://bugs.launchpad.net/juju/+bug/914392
> >
> > For more details, see:
> > https://code.launchpad.net/~hazmat/juju/local-respect-series/+merge/90182
> >
> > Modify local provider to respect the environment's default-series
> >
> > Fixes issues with creating precise containers.
>
> I'm a little nervous about the fact that some functions have "oneiric"
> and some "precise" as defaults.
>
> It seems like it would be safer to *always* require the user to specify,
> or at the very least, take this from the environment default-series.
>

The value is required and always passed, but there are some defaults in place
which where just to ease impl. I'll remove those and make it required
throughout. Most of the precise/oneiric values are in place for testing which
would remain regardless.

-k

Revision history for this message
Jim Baker (jimbaker) wrote :

+1, looks good to me. The only thing I found problematic was this signature:

=== modified file 'juju/lib/lxc/__init__.py'
--- juju/lib/lxc/__init__.py 2011-10-04 23:38:52 +0000
+++ juju/lib/lxc/__init__.py 2012-02-03 00:27:50 +0000
@@ -154,7 +154,8 @@
                  customize_script=None,
                  debug_log=None,
                  console_log=None,
- customize_log=None):
+ customize_log=None,
+ series="oneiric"):

But according to the earlier comments, you are planning to make this arg required for non-tests anyway.

review: Approve
451. By Kapil Thangavelu

remove series default values inline, series is required

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'juju/lib/lxc/__init__.py'
2--- juju/lib/lxc/__init__.py 2011-10-04 23:38:52 +0000
3+++ juju/lib/lxc/__init__.py 2012-02-11 15:52:20 +0000
4@@ -148,6 +148,7 @@
5 def __init__(self,
6 container_name,
7 public_key,
8+ series,
9 origin,
10 origin_source=None,
11 network_name="virbr0",
12@@ -161,6 +162,8 @@
13
14 :param public_key: SSH public key
15
16+ :param series: distro release series (oneiric, precise, etc)
17+
18 :param origin: distro|ppa|branch
19
20 :param origin_source: when origin is branch supply a valid bzr branch
21@@ -186,6 +189,7 @@
22 self.public_key = public_key
23 self.origin = origin
24 self.source = origin_source
25+ self.series = series
26 self.network_name = network_name
27
28 @property
29@@ -260,7 +264,10 @@
30 """Create the container synchronously."""
31 if not self.is_constructed():
32 lxc_config = self._make_lxc_config(self.network_name)
33- _lxc_create(self.container_name, config_file=lxc_config)
34+ _lxc_create(
35+ self.container_name,
36+ config_file=lxc_config,
37+ release=self.series)
38 os.unlink(lxc_config)
39 self._customize_container()
40
41@@ -286,7 +293,8 @@
42 debug_log=self.debug_log,
43 console_log=self.console_log,
44 customize_script=self.customize_script,
45- network_name=self.network_name)
46+ network_name=self.network_name,
47+ series=self.series)
48
49 if not container.is_constructed():
50 _lxc_clone(self.container_name, container_name)
51
52=== modified file 'juju/lib/lxc/data/juju-create'
53--- juju/lib/lxc/data/juju-create 2011-10-12 18:08:30 +0000
54+++ juju/lib/lxc/data/juju-create 2012-02-11 15:52:20 +0000
55@@ -1,6 +1,8 @@
56 #!/bin/bash
57 set -x
58
59+APT_OPTIONS="-o Dpkg::Options::= --force-confnew --force-yes -fuy"
60+
61 setup_apt_cache()
62 {
63 # We ask the host to run apt-cache-server by default speeding the deployment of n units
64@@ -17,6 +19,9 @@
65
66 setup_networking()
67 {
68+ # Ensure we have the resolvconf installed before configuring it.
69+ apt-get install $APT_OPTIONS resolvconf
70+
71 # Use dnsmasq to resolve names from the bridge
72 # This script executes from a chroot, so directly modify the output file.
73 cat <<EOF > /etc/resolvconf/run/resolv.conf
74@@ -89,8 +94,6 @@
75 fi
76
77 export DEBIAN_FRONTEND=noninteractive
78- APT_OPTIONS="-o Dpkg::Options::= --force-confnew --force-yes -fuy"
79-
80 echo "Setting up juju in container"
81 apt-get install $APT_OPTIONS bzr tmux sudo python-software-properties python-yaml
82
83
84=== modified file 'juju/lib/lxc/tests/test_lxc.py'
85--- juju/lib/lxc/tests/test_lxc.py 2011-10-01 00:04:14 +0000
86+++ juju/lib/lxc/tests/test_lxc.py 2012-02-11 15:52:20 +0000
87@@ -69,7 +69,6 @@
88 self.addCleanup(self.clean_container, DEFAULT_CONTAINER)
89
90 _lxc_create(DEFAULT_CONTAINER, config_file=self.config)
91-
92 _lxc_start(DEFAULT_CONTAINER)
93 _lxc_stop(DEFAULT_CONTAINER)
94
95@@ -84,12 +83,15 @@
96 def test_lxc_container(self):
97 self.addCleanup(self.clean_container, DEFAULT_CONTAINER)
98 customize_log = self.makeFile()
99- c = LXCContainer(
100- DEFAULT_CONTAINER, "dsa...", "ppa", customize_log=customize_log)
101-
102+ c = LXCContainer(DEFAULT_CONTAINER,
103+ "dsa...",
104+ "precise",
105+ "ppa",
106+ customize_log=customize_log)
107 running = yield c.is_running()
108 self.assertFalse(running)
109 self.assertFalse(c.is_constructed())
110+
111 # verify we can't run a non-constructed container
112 failure = c.run()
113 yield self.assertFailure(failure, LXCError)
114@@ -107,13 +109,13 @@
115 output = _lxc_ls()
116 self.assertIn(DEFAULT_CONTAINER, output)
117
118- # verify we have a path into the container
119+ # Verify we have a path into the container
120 self.assertTrue(os.path.exists(c.rootfs))
121 self.assertTrue(c.is_constructed())
122
123- self.verify_container(c, "dsa...", "ppa")
124+ self.verify_container(c, "dsa...", "precise", "ppa")
125
126- # verify that we are in containers
127+ # Verify that we are in containers
128 containers = yield get_containers(None)
129 self.assertEqual(containers[DEFAULT_CONTAINER], True)
130
131@@ -158,7 +160,8 @@
132
133 master_container = LXCContainer(DEFAULT_CONTAINER,
134 origin="ppa",
135- public_key="dsa...")
136+ public_key="dsa...",
137+ series="oneiric")
138
139 # verify that we cannot clone an unconstructed container
140 failure = master_container.clone("test_lxc_fail")
141@@ -182,7 +185,7 @@
142 output = _lxc_ls()
143 self.assertIn(DEFAULT_CONTAINER, output)
144
145- self.verify_container(c, "dsa...", "ppa")
146+ self.verify_container(c, "dsa...", "oneiric", "ppa")
147
148 # verify that we are in containers
149 containers = yield get_containers(None)
150@@ -202,7 +205,7 @@
151
152 yield master_container.destroy()
153
154- def verify_container(self, c, public_key, origin):
155+ def verify_container(self, c, public_key, series, origin):
156 """Verify properties of an LXCContainer"""
157
158 def p(path):
159@@ -259,6 +262,11 @@
160 self.assertIn('Acquire::http { Proxy "http://192.168.122.1:3142"; };',
161 apt_proxy)
162
163+ # Verify the container release series.
164+ with open(os.path.join(c.rootfs, "etc", "lsb-release")) as fh:
165+ lsb_info = fh.read()
166+ self.assertIn(series, lsb_info)
167+
168 # check basic juju installation
169 # these could be more through
170 if origin == "ppa":
171
172=== modified file 'juju/machine/tests/test_unit_deployment.py'
173--- juju/machine/tests/test_unit_deployment.py 2011-10-05 13:59:44 +0000
174+++ juju/machine/tests/test_unit_deployment.py 2012-02-11 15:52:20 +0000
175@@ -348,6 +348,8 @@
176 # Setup unit namespace
177 environ = dict(os.environ)
178 environ["JUJU_UNIT_NS"] = "ns1"
179+ environ["JUJU_SERIES"] = "precise"
180+
181 self.change_environment(**environ)
182
183 self.unit_deploy = UnitContainerDeployment(
184@@ -411,7 +413,8 @@
185
186 @inlineCallbacks
187 def test_start(self):
188- container = LXCContainer(self.unit_name, None, None, None)
189+ container = LXCContainer(
190+ self.unit_name, None, None, None, series="precise")
191 rootfs = self.makeDir()
192 env = dict(os.environ)
193 env["JUJU_PUBLIC_KEY"] = "dsa ..."
194@@ -442,7 +445,6 @@
195 self.assertIn('JUJU_ZOOKEEPER="127.0.1.1:2181"', job)
196 self.assertIn('JUJU_MACHINE_ID="0"', job)
197 self.assertIn('JUJU_UNIT_NAME="riak/0"', job)
198-
199 # Verify the symlink exists
200 self.assertTrue(os.path.lexists(os.path.join(
201 self.unit_deploy.juju_home, "units",
202@@ -468,7 +470,8 @@
203 @inlineCallbacks
204 def test_get_container(self):
205 rootfs = self.makeDir()
206- container = LXCContainer(self.unit_name, None, None, None)
207+ container = LXCContainer(
208+ self.unit_name, None, None, None, series="precise")
209
210 mock_deploy = self.mocker.patch(self.unit_deploy)
211 mock_deploy._get_master_template(ANY, ANY, ANY)
212
213=== modified file 'juju/machine/unit.py'
214--- juju/machine/unit.py 2011-10-01 00:04:14 +0000
215+++ juju/machine/unit.py 2012-02-11 15:52:20 +0000
216@@ -189,10 +189,13 @@
217
218 self._unit_namespace = os.environ.get("JUJU_UNIT_NS")
219 self._juju_origin = os.environ.get("JUJU_ORIGIN")
220+ self._juju_series = os.environ.get("JUJU_SERIES")
221 assert self._unit_namespace is not None, "Required unit ns not found"
222+ assert self._juju_series is not None, "Required juju series not found"
223
224 self.pid_file = None
225- self.container = LXCContainer(self.container_name, None, None, None)
226+ self.container = LXCContainer(
227+ self.container_name, None, None, None, series=self._juju_series)
228
229 @property
230 def container_name(self):
231@@ -250,7 +253,7 @@
232
233 master_template = LXCContainer(
234 container_template_name, origin=self._juju_origin,
235- public_key=public_key)
236+ public_key=public_key, series=self._juju_series)
237
238 # Debug log for the customize script, customize is only run on master.
239 customize_log_path = os.path.join(
240
241=== modified file 'juju/providers/local/__init__.py'
242--- juju/providers/local/__init__.py 2011-11-16 13:56:03 +0000
243+++ juju/providers/local/__init__.py 2012-02-11 15:52:20 +0000
244@@ -141,7 +141,8 @@
245 log_file=log_file,
246 juju_origin=juju_origin,
247 juju_unit_namespace=self._qualified_name,
248- public_key=public_key)
249+ public_key=public_key,
250+ juju_series=self.config["default-series"])
251 log.info(
252 "Starting machine agent (origin: %s)... ", agent.juju_origin)
253 yield agent.start()
254
255=== modified file 'juju/providers/local/agent.py'
256--- juju/providers/local/agent.py 2011-10-05 12:14:41 +0000
257+++ juju/providers/local/agent.py 2012-02-11 15:52:20 +0000
258@@ -15,11 +15,12 @@
259 agent_module = "juju.agents.machine"
260
261 def __init__(
262- self, pid_file, zookeeper_hosts=None, machine_id="0",
263+ self, pid_file, juju_series=None, zookeeper_hosts=None, machine_id="0",
264 log_file=None, juju_directory="/var/lib/juju",
265 juju_unit_namespace="", public_key=None, juju_origin="ppa"):
266 """
267 :param pid_file: Path to file used to store process id.
268+ :param juju_series: The release series to use (maverick, natty, etc).
269 :param machine_id: machine id for the local machine.
270 :param zookeeper_hosts: Zookeeper hosts to connect.
271 :param log_file: A file to use for the agent logs.
272@@ -39,6 +40,7 @@
273 self._log_file = log_file
274 self._public_key = public_key
275 self._juju_origin = juju_origin
276+ self._juju_series = juju_series
277
278 if self._juju_origin is None:
279 origin, source = get_default_origin()
280@@ -54,7 +56,7 @@
281 def start(self):
282 """Start the machine agent.
283 """
284- assert self._zookeeper_hosts and self._log_file
285+ assert self._zookeeper_hosts and self._log_file and self._juju_series
286
287 if (yield self.is_running()):
288 return
289@@ -67,6 +69,7 @@
290 "JUJU_MACHINE_ID=%s" % self._machine_id,
291 "JUJU_HOME=%s" % self._juju_directory,
292 "JUJU_UNIT_NS=%s" % self._juju_unit_namespace,
293+ "JUJU_SERIES=%s" % self._juju_series,
294 "PYTHONPATH=%s" % ":".join(sys.path),
295 sys.executable, "-m", self.agent_module,
296 "-n", "--pidfile", self._pid_file,
297
298=== modified file 'juju/providers/local/tests/test_agent.py'
299--- juju/providers/local/tests/test_agent.py 2011-10-05 12:14:41 +0000
300+++ juju/providers/local/tests/test_agent.py 2012-02-11 15:52:20 +0000
301@@ -27,7 +27,7 @@
302 log_file = self.makeFile()
303
304 agent = ManagedMachineAgent(
305- pid_file, get_test_zookeeper_address(),
306+ pid_file, "precise", get_test_zookeeper_address(),
307 juju_directory=juju_directory,
308 log_file=log_file, juju_unit_namespace="ns1",
309 juju_origin="lp:juju/trunk")
310@@ -50,7 +50,8 @@
311 JUJU_MACHINE_ID="0",
312 JUJU_HOME=juju_directory,
313 JUJU_ORIGIN="lp:juju/trunk",
314- JUJU_UNIT_NS="ns1"))
315+ JUJU_UNIT_NS="ns1",
316+ JUJU_SERIES="precise"))
317
318 @inlineCallbacks
319 def test_managed_agent_root(self):
320@@ -66,14 +67,13 @@
321 self.addCleanup(cleanup_root_file, log_file)
322
323 agent = ManagedMachineAgent(
324- pid_file, machine_id="0", log_file=log_file,
325+ pid_file, "precise", machine_id="0", log_file=log_file,
326 zookeeper_hosts=get_test_zookeeper_address(),
327 juju_directory=juju_directory)
328
329 agent.agent_module = "juju.agents.dummy"
330 self.assertFalse((yield agent.is_running()))
331 yield agent.start()
332-
333 # Give a moment for the process to start and write its config
334 yield self.sleep(0.1)
335 self.assertTrue((yield agent.is_running()))
336
337=== modified file 'juju/providers/local/tests/test_provider.py'
338--- juju/providers/local/tests/test_provider.py 2011-10-07 18:40:48 +0000
339+++ juju/providers/local/tests/test_provider.py 2012-02-11 15:52:20 +0000
340@@ -29,7 +29,7 @@
341 self.provider = MachineProvider(
342 "local-test", {
343 "admin-secret": "admin:abc", "data-dir": self.makeDir(),
344- "authorized-keys": "fooabc123"})
345+ "authorized-keys": "fooabc123", "default-series": "oneiric"})
346 self.output = self.capture_logging(
347 "juju.local-dev", level=logging.DEBUG)
348 zookeeper.set_debug_level(0)

Subscribers

People subscribed via source and target branches

to status/vote changes: