Merge lp:~james-page/charms/trusty/ceph/fix-apt-race into lp:~openstack-charmers-archive/charms/trusty/ceph/next
- Trusty Tahr (14.04)
- fix-apt-race
- Merge into next
Proposed by
James Page
Status: | Merged |
---|---|
Merged at revision: | 76 |
Proposed branch: | lp:~james-page/charms/trusty/ceph/fix-apt-race |
Merge into: | lp:~openstack-charmers-archive/charms/trusty/ceph/next |
Diff against target: |
475 lines (+189/-55) 9 files modified
hooks/ceph.py (+4/-18) hooks/charmhelpers/contrib/storage/linux/utils.py (+1/-0) hooks/charmhelpers/core/fstab.py (+116/-0) hooks/charmhelpers/core/hookenv.py (+5/-4) hooks/charmhelpers/core/host.py (+32/-12) hooks/charmhelpers/fetch/__init__.py (+24/-16) hooks/charmhelpers/fetch/bzrurl.py (+2/-1) hooks/hooks.py (+4/-3) templates/ceph.conf (+1/-1) |
To merge this branch: | bzr merge lp:~james-page/charms/trusty/ceph/fix-apt-race |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Liam Young (community) | Approve | ||
Review via email: mp+228067@code.launchpad.net |
This proposal supersedes a proposal from 2014-07-24.
Commit message
Description of the change
Rework use of package version comparison to use charm-helper and avoid apt races.
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'hooks/ceph.py' |
2 | --- hooks/ceph.py 2014-05-19 12:15:38 +0000 |
3 | +++ hooks/ceph.py 2014-07-24 09:53:39 +0000 |
4 | @@ -11,10 +11,10 @@ |
5 | import subprocess |
6 | import time |
7 | import os |
8 | -import apt_pkg as apt |
9 | from charmhelpers.core.host import ( |
10 | mkdir, |
11 | service_restart, |
12 | + cmp_pkgrevno, |
13 | ) |
14 | from charmhelpers.core.hookenv import ( |
15 | log, |
16 | @@ -126,7 +126,7 @@ |
17 | def start_osds(devices): |
18 | # Scan for ceph block devices |
19 | rescan_osd_devices() |
20 | - if get_ceph_version() >= "0.56.6": |
21 | + if cmp_pkgrevno('ceph', "0.56.6") >= 0: |
22 | # Use ceph-disk-activate for directory based OSD's |
23 | for dev_or_path in devices: |
24 | if os.path.exists(dev_or_path) and os.path.isdir(dev_or_path): |
25 | @@ -309,20 +309,6 @@ |
26 | os.unlink(keyring) |
27 | |
28 | |
29 | -def get_ceph_version(): |
30 | - apt.init() |
31 | - cache = apt.Cache() |
32 | - pkg = cache['ceph'] |
33 | - if pkg.current_ver: |
34 | - return apt.upstream_version(pkg.current_ver.ver_str) |
35 | - else: |
36 | - return None |
37 | - |
38 | - |
39 | -def version_compare(a, b): |
40 | - return apt.version_compare(a, b) |
41 | - |
42 | - |
43 | def update_monfs(): |
44 | hostname = get_unit_hostname() |
45 | monfs = '/var/lib/ceph/mon/ceph-{}'.format(hostname) |
46 | @@ -360,7 +346,7 @@ |
47 | |
48 | cmd = ['ceph-disk-prepare'] |
49 | # Later versions of ceph support more options |
50 | - if get_ceph_version() >= "0.48.3": |
51 | + if cmp_pkgrevno('ceph', "0.48.3") >= 0: |
52 | if osd_format: |
53 | cmd.append('--fs-type') |
54 | cmd.append(osd_format) |
55 | @@ -383,7 +369,7 @@ |
56 | log('Path {} is already configured as an OSD - bailing'.format(path)) |
57 | return |
58 | |
59 | - if get_ceph_version() < "0.56.6": |
60 | + if cmp_pkgrevno('ceph', "0.56.6") < 0: |
61 | log('Unable to use directories for OSDs with ceph < 0.56.6', |
62 | level=ERROR) |
63 | raise |
64 | |
65 | === modified file 'hooks/charmhelpers/contrib/storage/linux/utils.py' |
66 | --- hooks/charmhelpers/contrib/storage/linux/utils.py 2014-05-19 11:38:45 +0000 |
67 | +++ hooks/charmhelpers/contrib/storage/linux/utils.py 2014-07-24 09:53:39 +0000 |
68 | @@ -37,6 +37,7 @@ |
69 | check_call(['dd', 'if=/dev/zero', 'of=%s' % (block_device), |
70 | 'bs=512', 'count=100', 'seek=%s' % (gpt_end)]) |
71 | |
72 | + |
73 | def is_device_mounted(device): |
74 | '''Given a device path, return True if that device is mounted, and False |
75 | if it isn't. |
76 | |
77 | === added file 'hooks/charmhelpers/core/fstab.py' |
78 | --- hooks/charmhelpers/core/fstab.py 1970-01-01 00:00:00 +0000 |
79 | +++ hooks/charmhelpers/core/fstab.py 2014-07-24 09:53:39 +0000 |
80 | @@ -0,0 +1,116 @@ |
81 | +#!/usr/bin/env python |
82 | +# -*- coding: utf-8 -*- |
83 | + |
84 | +__author__ = 'Jorge Niedbalski R. <jorge.niedbalski@canonical.com>' |
85 | + |
86 | +import os |
87 | + |
88 | + |
89 | +class Fstab(file): |
90 | + """This class extends file in order to implement a file reader/writer |
91 | + for file `/etc/fstab` |
92 | + """ |
93 | + |
94 | + class Entry(object): |
95 | + """Entry class represents a non-comment line on the `/etc/fstab` file |
96 | + """ |
97 | + def __init__(self, device, mountpoint, filesystem, |
98 | + options, d=0, p=0): |
99 | + self.device = device |
100 | + self.mountpoint = mountpoint |
101 | + self.filesystem = filesystem |
102 | + |
103 | + if not options: |
104 | + options = "defaults" |
105 | + |
106 | + self.options = options |
107 | + self.d = d |
108 | + self.p = p |
109 | + |
110 | + def __eq__(self, o): |
111 | + return str(self) == str(o) |
112 | + |
113 | + def __str__(self): |
114 | + return "{} {} {} {} {} {}".format(self.device, |
115 | + self.mountpoint, |
116 | + self.filesystem, |
117 | + self.options, |
118 | + self.d, |
119 | + self.p) |
120 | + |
121 | + DEFAULT_PATH = os.path.join(os.path.sep, 'etc', 'fstab') |
122 | + |
123 | + def __init__(self, path=None): |
124 | + if path: |
125 | + self._path = path |
126 | + else: |
127 | + self._path = self.DEFAULT_PATH |
128 | + file.__init__(self, self._path, 'r+') |
129 | + |
130 | + def _hydrate_entry(self, line): |
131 | + # NOTE: use split with no arguments to split on any |
132 | + # whitespace including tabs |
133 | + return Fstab.Entry(*filter( |
134 | + lambda x: x not in ('', None), |
135 | + line.strip("\n").split())) |
136 | + |
137 | + @property |
138 | + def entries(self): |
139 | + self.seek(0) |
140 | + for line in self.readlines(): |
141 | + try: |
142 | + if not line.startswith("#"): |
143 | + yield self._hydrate_entry(line) |
144 | + except ValueError: |
145 | + pass |
146 | + |
147 | + def get_entry_by_attr(self, attr, value): |
148 | + for entry in self.entries: |
149 | + e_attr = getattr(entry, attr) |
150 | + if e_attr == value: |
151 | + return entry |
152 | + return None |
153 | + |
154 | + def add_entry(self, entry): |
155 | + if self.get_entry_by_attr('device', entry.device): |
156 | + return False |
157 | + |
158 | + self.write(str(entry) + '\n') |
159 | + self.truncate() |
160 | + return entry |
161 | + |
162 | + def remove_entry(self, entry): |
163 | + self.seek(0) |
164 | + |
165 | + lines = self.readlines() |
166 | + |
167 | + found = False |
168 | + for index, line in enumerate(lines): |
169 | + if not line.startswith("#"): |
170 | + if self._hydrate_entry(line) == entry: |
171 | + found = True |
172 | + break |
173 | + |
174 | + if not found: |
175 | + return False |
176 | + |
177 | + lines.remove(line) |
178 | + |
179 | + self.seek(0) |
180 | + self.write(''.join(lines)) |
181 | + self.truncate() |
182 | + return True |
183 | + |
184 | + @classmethod |
185 | + def remove_by_mountpoint(cls, mountpoint, path=None): |
186 | + fstab = cls(path=path) |
187 | + entry = fstab.get_entry_by_attr('mountpoint', mountpoint) |
188 | + if entry: |
189 | + return fstab.remove_entry(entry) |
190 | + return False |
191 | + |
192 | + @classmethod |
193 | + def add(cls, device, mountpoint, filesystem, options=None, path=None): |
194 | + return cls(path=path).add_entry(Fstab.Entry(device, |
195 | + mountpoint, filesystem, |
196 | + options=options)) |
197 | |
198 | === modified file 'hooks/charmhelpers/core/hookenv.py' |
199 | --- hooks/charmhelpers/core/hookenv.py 2014-05-19 11:38:45 +0000 |
200 | +++ hooks/charmhelpers/core/hookenv.py 2014-07-24 09:53:39 +0000 |
201 | @@ -25,7 +25,7 @@ |
202 | def cached(func): |
203 | """Cache return values for multiple executions of func + args |
204 | |
205 | - For example: |
206 | + For example:: |
207 | |
208 | @cached |
209 | def unit_get(attribute): |
210 | @@ -445,18 +445,19 @@ |
211 | class Hooks(object): |
212 | """A convenient handler for hook functions. |
213 | |
214 | - Example: |
215 | + Example:: |
216 | + |
217 | hooks = Hooks() |
218 | |
219 | # register a hook, taking its name from the function name |
220 | @hooks.hook() |
221 | def install(): |
222 | - ... |
223 | + pass # your code here |
224 | |
225 | # register a hook, providing a custom hook name |
226 | @hooks.hook("config-changed") |
227 | def config_changed(): |
228 | - ... |
229 | + pass # your code here |
230 | |
231 | if __name__ == "__main__": |
232 | # execute a hook based on the name the program is called by |
233 | |
234 | === modified file 'hooks/charmhelpers/core/host.py' |
235 | --- hooks/charmhelpers/core/host.py 2014-05-19 11:38:45 +0000 |
236 | +++ hooks/charmhelpers/core/host.py 2014-07-24 09:53:39 +0000 |
237 | @@ -12,11 +12,11 @@ |
238 | import string |
239 | import subprocess |
240 | import hashlib |
241 | -import apt_pkg |
242 | |
243 | from collections import OrderedDict |
244 | |
245 | from hookenv import log |
246 | +from fstab import Fstab |
247 | |
248 | |
249 | def service_start(service_name): |
250 | @@ -35,7 +35,8 @@ |
251 | |
252 | |
253 | def service_reload(service_name, restart_on_failure=False): |
254 | - """Reload a system service, optionally falling back to restart if reload fails""" |
255 | + """Reload a system service, optionally falling back to restart if |
256 | + reload fails""" |
257 | service_result = service('reload', service_name) |
258 | if not service_result and restart_on_failure: |
259 | service_result = service('restart', service_name) |
260 | @@ -144,7 +145,19 @@ |
261 | target.write(content) |
262 | |
263 | |
264 | -def mount(device, mountpoint, options=None, persist=False): |
265 | +def fstab_remove(mp): |
266 | + """Remove the given mountpoint entry from /etc/fstab |
267 | + """ |
268 | + return Fstab.remove_by_mountpoint(mp) |
269 | + |
270 | + |
271 | +def fstab_add(dev, mp, fs, options=None): |
272 | + """Adds the given device entry to the /etc/fstab file |
273 | + """ |
274 | + return Fstab.add(dev, mp, fs, options=options) |
275 | + |
276 | + |
277 | +def mount(device, mountpoint, options=None, persist=False, filesystem="ext3"): |
278 | """Mount a filesystem at a particular mountpoint""" |
279 | cmd_args = ['mount'] |
280 | if options is not None: |
281 | @@ -155,9 +168,9 @@ |
282 | except subprocess.CalledProcessError, e: |
283 | log('Error mounting {} at {}\n{}'.format(device, mountpoint, e.output)) |
284 | return False |
285 | + |
286 | if persist: |
287 | - # TODO: update fstab |
288 | - pass |
289 | + return fstab_add(device, mountpoint, filesystem, options=options) |
290 | return True |
291 | |
292 | |
293 | @@ -169,9 +182,9 @@ |
294 | except subprocess.CalledProcessError, e: |
295 | log('Error unmounting {}\n{}'.format(mountpoint, e.output)) |
296 | return False |
297 | + |
298 | if persist: |
299 | - # TODO: update fstab |
300 | - pass |
301 | + return fstab_remove(mountpoint) |
302 | return True |
303 | |
304 | |
305 | @@ -198,13 +211,13 @@ |
306 | def restart_on_change(restart_map, stopstart=False): |
307 | """Restart services based on configuration files changing |
308 | |
309 | - This function is used a decorator, for example |
310 | + This function is used a decorator, for example:: |
311 | |
312 | @restart_on_change({ |
313 | '/etc/ceph/ceph.conf': [ 'cinder-api', 'cinder-volume' ] |
314 | }) |
315 | def ceph_client_changed(): |
316 | - ... |
317 | + pass # your code here |
318 | |
319 | In this example, the cinder-api and cinder-volume services |
320 | would be restarted if /etc/ceph/ceph.conf is changed by the |
321 | @@ -300,12 +313,19 @@ |
322 | |
323 | def cmp_pkgrevno(package, revno, pkgcache=None): |
324 | '''Compare supplied revno with the revno of the installed package |
325 | - 1 => Installed revno is greater than supplied arg |
326 | - 0 => Installed revno is the same as supplied arg |
327 | - -1 => Installed revno is less than supplied arg |
328 | + |
329 | + * 1 => Installed revno is greater than supplied arg |
330 | + * 0 => Installed revno is the same as supplied arg |
331 | + * -1 => Installed revno is less than supplied arg |
332 | + |
333 | ''' |
334 | + import apt_pkg |
335 | if not pkgcache: |
336 | apt_pkg.init() |
337 | + # Force Apt to build its cache in memory. That way we avoid race |
338 | + # conditions with other applications building the cache in the same |
339 | + # place. |
340 | + apt_pkg.config.set("Dir::Cache::pkgcache", "") |
341 | pkgcache = apt_pkg.Cache() |
342 | pkg = pkgcache[package] |
343 | return apt_pkg.version_compare(pkg.current_ver.ver_str, revno) |
344 | |
345 | === modified file 'hooks/charmhelpers/fetch/__init__.py' |
346 | --- hooks/charmhelpers/fetch/__init__.py 2014-06-04 13:06:51 +0000 |
347 | +++ hooks/charmhelpers/fetch/__init__.py 2014-07-24 09:53:39 +0000 |
348 | @@ -13,7 +13,6 @@ |
349 | config, |
350 | log, |
351 | ) |
352 | -import apt_pkg |
353 | import os |
354 | |
355 | |
356 | @@ -117,6 +116,7 @@ |
357 | |
358 | def filter_installed_packages(packages): |
359 | """Returns a list of packages that require installation""" |
360 | + import apt_pkg |
361 | apt_pkg.init() |
362 | |
363 | # Tell apt to build an in-memory cache to prevent race conditions (if |
364 | @@ -235,31 +235,39 @@ |
365 | sources_var='install_sources', |
366 | keys_var='install_keys'): |
367 | """ |
368 | - Configure multiple sources from charm configuration |
369 | + Configure multiple sources from charm configuration. |
370 | + |
371 | + The lists are encoded as yaml fragments in the configuration. |
372 | + The frament needs to be included as a string. |
373 | |
374 | Example config: |
375 | - install_sources: |
376 | + install_sources: | |
377 | - "ppa:foo" |
378 | - "http://example.com/repo precise main" |
379 | - install_keys: |
380 | + install_keys: | |
381 | - null |
382 | - "a1b2c3d4" |
383 | |
384 | Note that 'null' (a.k.a. None) should not be quoted. |
385 | """ |
386 | - sources = safe_load(config(sources_var)) |
387 | - keys = config(keys_var) |
388 | - if keys is not None: |
389 | - keys = safe_load(keys) |
390 | - if isinstance(sources, basestring) and ( |
391 | - keys is None or isinstance(keys, basestring)): |
392 | - add_source(sources, keys) |
393 | + sources = safe_load((config(sources_var) or '').strip()) or [] |
394 | + keys = safe_load((config(keys_var) or '').strip()) or None |
395 | + |
396 | + if isinstance(sources, basestring): |
397 | + sources = [sources] |
398 | + |
399 | + if keys is None: |
400 | + for source in sources: |
401 | + add_source(source, None) |
402 | else: |
403 | - if not len(sources) == len(keys): |
404 | - msg = 'Install sources and keys lists are different lengths' |
405 | - raise SourceConfigError(msg) |
406 | - for src_num in range(len(sources)): |
407 | - add_source(sources[src_num], keys[src_num]) |
408 | + if isinstance(keys, basestring): |
409 | + keys = [keys] |
410 | + |
411 | + if len(sources) != len(keys): |
412 | + raise SourceConfigError( |
413 | + 'Install sources and keys lists are different lengths') |
414 | + for source, key in zip(sources, keys): |
415 | + add_source(source, key) |
416 | if update: |
417 | apt_update(fatal=True) |
418 | |
419 | |
420 | === modified file 'hooks/charmhelpers/fetch/bzrurl.py' |
421 | --- hooks/charmhelpers/fetch/bzrurl.py 2013-11-13 22:09:26 +0000 |
422 | +++ hooks/charmhelpers/fetch/bzrurl.py 2014-07-24 09:53:39 +0000 |
423 | @@ -39,7 +39,8 @@ |
424 | def install(self, source): |
425 | url_parts = self.parse_url(source) |
426 | branch_name = url_parts.path.strip("/").split("/")[-1] |
427 | - dest_dir = os.path.join(os.environ.get('CHARM_DIR'), "fetched", branch_name) |
428 | + dest_dir = os.path.join(os.environ.get('CHARM_DIR'), "fetched", |
429 | + branch_name) |
430 | if not os.path.exists(dest_dir): |
431 | mkdir(dest_dir, perms=0755) |
432 | try: |
433 | |
434 | === modified file 'hooks/hooks.py' |
435 | --- hooks/hooks.py 2014-03-25 18:44:22 +0000 |
436 | +++ hooks/hooks.py 2014-07-24 09:53:39 +0000 |
437 | @@ -29,7 +29,8 @@ |
438 | from charmhelpers.core.host import ( |
439 | service_restart, |
440 | umount, |
441 | - mkdir |
442 | + mkdir, |
443 | + cmp_pkgrevno |
444 | ) |
445 | from charmhelpers.fetch import ( |
446 | apt_install, |
447 | @@ -50,7 +51,7 @@ |
448 | |
449 | def install_upstart_scripts(): |
450 | # Only install upstart configurations for older versions |
451 | - if ceph.get_ceph_version() < "0.55.1": |
452 | + if cmp_pkgrevno('ceph', "0.55.1") < 0: |
453 | for x in glob.glob('files/upstart/*.conf'): |
454 | shutil.copy(x, '/etc/init/') |
455 | |
456 | @@ -71,7 +72,7 @@ |
457 | 'auth_supported': config('auth-supported'), |
458 | 'mon_hosts': ' '.join(get_mon_hosts()), |
459 | 'fsid': config('fsid'), |
460 | - 'version': ceph.get_ceph_version(), |
461 | + 'old_auth': cmp_pkgrevno('ceph', "0.51") < 0, |
462 | 'osd_journal_size': config('osd-journal-size'), |
463 | 'use_syslog': str(config('use-syslog')).lower() |
464 | } |
465 | |
466 | === modified file 'templates/ceph.conf' |
467 | --- templates/ceph.conf 2014-03-25 18:44:22 +0000 |
468 | +++ templates/ceph.conf 2014-07-24 09:53:39 +0000 |
469 | @@ -1,5 +1,5 @@ |
470 | [global] |
471 | -{% if version < "0.51" %} |
472 | +{% if old_auth %} |
473 | auth supported = {{ auth_supported }} |
474 | {% else %} |
475 | auth cluster required = {{ auth_supported }} |
LGTM