Merge lp:~thedac/charms/precise/block-storage-broker/configure-repository into lp:charms/block-storage-broker

Proposed by David Ames on 2014-07-14
Status: Merged
Merged at revision: 53
Proposed branch: lp:~thedac/charms/precise/block-storage-broker/configure-repository
Merge into: lp:charms/block-storage-broker
Diff against target: 54 lines (+19/-3)
2 files modified
hooks/hooks.py (+3/-2)
hooks/util.py (+16/-1)
To merge this branch: bzr merge lp:~thedac/charms/precise/block-storage-broker/configure-repository
Reviewer Review Type Date Requested Status
Stuart Bishop 2014-07-14 Approve on 2014-07-18
Review via email: mp+226721@code.launchpad.net

Description of the change

Currently the addition of the cloud-archive:havana apt

This error out when using trusty: 'cloud-archive only supported on precise'

Allow configuration of apt repository. Default to cloud-archive:havana but allow "" when using trusty.

To post a comment you must log in.
Tom Haddon (mthaddon) wrote :

I'm not massively happy about the default being cloud-archive:havana. This seems like it's going to be a gotcha for anyone using trusty, which before too long will be everyone (I assume). If we know precise requires cloud-archive:havana but trusty doesn't, couldn't we check /etc/lsb-release and act accordingly?

54. By David Ames on 2014-07-15

Fixes for trusty. Determine series. Handle bareword in volume-show.

David Ames (thedac) wrote :

Per Tom's request I have checked for the running series and set the extra repo only when not running trusty.

Also fixed the malformed string error

volume-show had the bare word null as seen below causing the error.
[{"device": "/dev/vdc", "server_id": "6b2d686b-972e-46b8-ba32-db03b6e14ddd", "volume_id":
"a1d3c1e5-3807-4c12-a339-20125c376335", "host_name": null, "id": "a1d3c1e5-3807-4c12-a339-20125c376335"}]

Traceback (most recent call last):
  File "hooks/block-storage-relation-changed", line 144, in <module>
    hooks.execute(sys.argv)
  File
"/var/lib/juju/agents/unit-block-storage-broker-0/charm/hooks/charmhelpers/core/hookenv.py",
line 381, in execute
    self._hooks[hook_name]()
  File "hooks/block-storage-relation-changed", line 128, in
block_storage_relation_changed
    volume_label=volume_label)
  File
"/var/lib/juju/agents/unit-block-storage-broker-0/charm/hooks/util.py",
line 159, in attach_volume
    volume_id = self.get_volume_id(volume_label)
  File
"/var/lib/juju/agents/unit-block-storage-broker-0/charm/hooks/util.py",
line 93, in get_volume_id
    volumes = self.describe_volumes()
  File
"/var/lib/juju/agents/unit-block-storage-broker-0/charm/hooks/util.py",
line 83, in describe_volumes
    return method(volume_id)
  File
"/var/lib/juju/agents/unit-block-storage-broker-0/charm/hooks/util.py",
line 427, in _nova_describe_volumes
    result[volume].update(self._nova_volume_show(volume))
  File
"/var/lib/juju/agents/unit-block-storage-broker-0/charm/hooks/util.py",
line 386, in _nova_volume_show
    attachments = literal_eval(value)
  File "/usr/lib/python2.7/ast.py", line 80, in literal_eval
    return _convert(node_or_string)
  File "/usr/lib/python2.7/ast.py", line 60, in _convert
    return list(map(_convert, node.elts))
  File "/usr/lib/python2.7/ast.py", line 63, in _convert
    in zip(node.keys, node.values))
  File "/usr/lib/python2.7/ast.py", line 62, in <genexpr>
    return dict((_convert(k), _convert(v)) for k, v
  File "/usr/lib/python2.7/ast.py", line 79, in _convert
    raise ValueError('malformed string')
ValueError: malformed string

Stuart Bishop (stub) wrote :

I've also got conditionals on the series, so the helper should one day end up in charm-helpers (core/host.py probably). I'm also doing something similar in the PG charm, requiring icehouse with precise, and have the same approach. I would like to know the difference between cloud:havana and cloud-archive:havana, and when one should be chosen over the other.

This branch is good to land. I'd like a comment on the 'null' replacement, as this is unobvious, but as the entire helper is uncommented it wouldn't help much.

review: Approve
Stuart Bishop (stub) wrote :

Oh, and looking back at the original proposal, if there is a next time you can use the standard install_sources() stuff from charmhelpers.fetch rather than your own arbitrary config item.

Stuart Bishop (stub) wrote :

Jumped the gun sorry. I've identified things that need fixing.

You should be testing for 'if get_running_series() == "precise"', rather than '!= "trusty"', or this charm will break with Utopic and later.

I don't think we should be adding the repo unless hookenv.config('provider') == 'nova'.

test_install_adds_apt_source_and_installs_novaclient() now fails under trusty. This test can be duplicated, with util.get_running_series() mocked, so we test both precise and non-precise behavior.

review: Needs Fixing
55. By David Ames on 2014-07-18

More intelligence in selecting series

David Ames (thedac) wrote :

Fixed the code to be more intelligent about series.

For the life of me I cannot figure out how to mock the get_running_series() function.

Stuart Bishop (stub) wrote :

lp:~stub/charms/precise/block-storage-broker/configure-repository has tests. I didn't bother to update the test to use the modern standard 'mock' library rather than the dead mocker library. Good to land with the tests.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hooks/hooks.py'
2--- hooks/hooks.py 2014-03-21 17:38:30 +0000
3+++ hooks/hooks.py 2014-07-18 00:59:13 +0000
4@@ -7,7 +7,7 @@
5 import json
6 import os
7 import sys
8-from util import StorageServiceUtil, generate_volume_label
9+from util import StorageServiceUtil, generate_volume_label, get_running_series
10
11 hooks = hookenv.Hooks()
12
13@@ -83,7 +83,8 @@
14 provider = hookenv.config("provider")
15 if provider == "nova":
16 required_packages = ["python-novaclient"]
17- fetch.add_source("cloud-archive:havana")
18+ if int(get_running_series()['release'].split(".")[0]) < 14:
19+ fetch.add_source("cloud-archive:havana")
20 elif provider == "ec2":
21 required_packages = ["euca2ools"]
22 fetch.apt_update(fatal=True)
23
24=== modified file 'hooks/util.py'
25--- hooks/util.py 2014-03-21 17:39:48 +0000
26+++ hooks/util.py 2014-07-18 00:59:13 +0000
27@@ -373,7 +373,7 @@
28 continue
29 (_, key, value, _) = line.split("|")
30 key = key.strip()
31- value = value.strip()
32+ value = value.replace("null", "None").strip()
33 if key in ["availability_zone", "size", "id", "snapshot_id",
34 "status"]:
35 result[key] = value
36@@ -470,3 +470,18 @@
37 def generate_volume_label(remote_unit):
38 """Create a volume label for the requesting remote unit"""
39 return "%s unit volume" % remote_unit
40+
41+
42+def get_running_series():
43+ lsb_release = "/etc/lsb-release"
44+ series = {}
45+ with open(lsb_release) as f:
46+ for line in f:
47+ line = line.strip()
48+ if line.startswith('DISTRIB_CODENAME'):
49+ series['codename'] = line.split('=')[1]
50+ if line.startswith('DISTRIB_RELEASE'):
51+ series['release'] = line.split('=')[1]
52+ if line.startswith('DISTRIB_DESCRIPTION'):
53+ series['description'] = line.split('=')[1]
54+ return series

Subscribers

People subscribed via source and target branches

to all changes: