Merge lp:~hazmat/pyjuju/local-respect-series into lp:pyjuju
- local-respect-series
- Merge into trunk
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 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jim Baker (community) | Approve | ||
Benjamin Saller (community) | Approve | ||
Review via email: mp+90182@code.launchpad.net |
Commit message
Description of the change
Modify local provider to respect the environment's default-series
Fixes issues with creating precise containers.
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:/
>
> For more details, see:
> https:/
>
> 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.
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:/
> >
> > For more details, see:
> > https:/
> >
> > 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
Jim Baker (jimbaker) wrote : | # |
+1, looks good to me. The only thing I found problematic was this signature:
=== modified file 'juju/lib/
--- juju/lib/
+++ juju/lib/
@@ -154,7 +154,8 @@
- customize_
+ customize_log=None,
+ series="oneiric"):
But according to the earlier comments, you are planning to make this arg required for non-tests anyway.
- 451. By Kapil Thangavelu
-
remove series default values inline, series is required
Preview Diff
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) |
[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