Repository: juju/python-libjuju

Issue:
Cannot deploy bundle with storage
Everytime I deploy an application with storage with libjuju, I'm getting this error:

```
Traceback (most recent call last):
  File "examples/deploy_k8s.py", line 42, in <module>
    loop.run(main())
  File "/home/davigar15/.local/lib/python3.6/site-packages/juju/loop.py", line 38, in run
    raise task.exception()
  File "examples/deploy_k8s.py", line 23, in main
    application = await model.deploy( 'cs:osm' )
  File "/home/davigar15/.local/lib/python3.6/site-packages/juju/model.py", line 1286, in deploy
    await handler.execute_plan()
  File "/home/davigar15/.local/lib/python3.6/site-packages/juju/model.py", line 1969, in execute_plan
    result = await method(*step.args)
  File "/home/davigar15/.local/lib/python3.6/site-packages/juju/model.py", line 2140, in deploy
    num_units=num_units,
  File "/home/davigar15/.local/lib/python3.6/site-packages/juju/model.py", line 1415, in _deploy
    result = await app_facade.Deploy([app])
  File "/home/davigar15/.local/lib/python3.6/site-packages/juju/client/facade.py", line 420, in wrapper
    reply = await f(*args, **kwargs)
  File "/home/davigar15/.local/lib/python3.6/site-packages/juju/client/_client8.py", line 745, in Deploy
    reply = await self.rpc(msg)
  File "/home/davigar15/.local/lib/python3.6/site-packages/juju/client/facade.py", line 545, in rpc
    result = await self.connection.rpc(msg, encoder=TypeEncoder)
  File "/home/davigar15/.local/lib/python3.6/site-packages/juju/client/connection.py", line 326, in rpc
    raise errors.JujuAPIError(result)
```

Discussion:

Patch:
diff --git a/juju/bundle.py b/juju/bundle.py
index 8e96c95..2c28811 100644
--- a/juju/bundle.py
+++ b/juju/bundle.py
@@ -8,7 +8,7 @@ import yaml
 from toposort import toposort_flatten
 
 from .client import client
-from .constraints import parse as parse_constraints
+from .constraints import parse as parse_constraints, parse_storage_constraint, parse_device_constraint
 from .errors import JujuError
 
 log = logging.getLogger(__name__)
@@ -277,17 +277,18 @@ class AddApplicationChange(ChangeInfo):
             self.application = params[2]
             self.options = params[3]
             self.constraints = params[4]
-            self.storage = params[5]
-            self.endpoint_bindings = params[6]
+            self.storage = {k: parse_storage_constraint(v) for k, v in params[5].items()}
             if len(params) == 8:
-                # Juju 2.4 and below only sends the resources
+                # Juju 2.4 and below only sends the endpoint bindings and resources
+                self.endpoint_bindings = params[6]
                 self.resources = params[7]
                 self.devices = None
                 self.num_units = None
             else:
-                # Juju 2.5+ sends devices before resources, as well as num_units
+                # Juju 2.5+ sends devices before endpoint bindings, as well as num_units
                 # There might be placement but we need to ignore that.
-                self.devices = params[7]
+                self.devices = {k: parse_device_constraint(v) for k, v in params[6].items()}
+                self.endpoint_bindings = params[7]
                 self.resources = params[8]
                 self.num_units = params[9]
 
diff --git a/juju/client/_definitions.py b/juju/client/_definitions.py
index 4576c40..fb95f40 100644
--- a/juju/client/_definitions.py
+++ b/juju/client/_definitions.py
@@ -1529,32 +1529,38 @@ class ApplicationConstraint(Type):
 
 
 class ApplicationDeploy(Type):
-    _toSchema = {'application': 'application', 'channel': 'channel', 'charm_url': 'charm-url', 'config': 'config', 'config_yaml': 'config-yaml', 'constraints': 'constraints', 'endpoint_bindings': 'endpoint-bindings', 'num_units': 'num-units', 'placement': 'placement', 'resources': 'resources', 'series': 'series', 'storage': 'storage'}
-    _toPy = {'application': 'application', 'channel': 'channel', 'charm-url': 'charm_url', 'config': 'config', 'config-yaml': 'config_yaml', 'constraints': 'constraints', 'endpoint-bindings': 'endpoint_bindings', 'num-units': 'num_units', 'placement': 'placement', 'resources': 'resources', 'series': 'series', 'storage': 'storage'}
-    def __init__(self, application=None, channel=None, charm_url=None, config=None, config_yaml=None, constraints=None, endpoint_bindings=None, num_units=None, placement=None, resources=None, series=None, storage=None, **unknown_fields):
+    _toSchema = {'application': 'application', 'attach_storage': 'attach-storage', 'channel': 'channel', 'charm_url': 'charm-url', 'config': 'config', 'config_yaml': 'config-yaml', 'constraints': 'constraints', 'devices': 'devices', 'endpoint_bindings': 'endpoint-bindings', 'num_units': 'num-units', 'placement': 'placement', 'policy': 'policy', 'resources': 'resources', 'series': 'series', 'storage': 'storage'}
+    _toPy = {'application': 'application', 'attach-storage': 'attach_storage', 'channel': 'channel', 'charm-url': 'charm_url', 'config': 'config', 'config-yaml': 'config_yaml', 'constraints': 'constraints', 'devices': 'devices', 'endpoint-bindings': 'endpoint_bindings', 'num-units': 'num_units', 'placement': 'placement', 'policy': 'policy', 'resources': 'resources', 'series': 'series', 'storage': 'storage'}
+    def __init__(self, application=None, attach_storage=None, channel=None, charm_url=None, config=None, config_yaml=None, constraints=None, devices=None, endpoint_bindings=None, num_units=None, placement=None, policy=None, resources=None, series=None, storage=None, **unknown_fields):
         '''
         application : str
+        attach_storage : typing.Sequence[str]
         channel : str
         charm_url : str
         config : typing.Mapping[str, str]
         config_yaml : str
         constraints : Value
+        devices : typing.Mapping[str, ~Constraints]
         endpoint_bindings : typing.Mapping[str, str]
         num_units : int
         placement : typing.Sequence[~Placement]
+        policy : str
         resources : typing.Mapping[str, str]
         series : str
         storage : typing.Mapping[str, ~Constraints]
         '''
         application_ = application
+        attach_storage_ = attach_storage
         channel_ = channel
         charm_url_ = charm_url
         config_ = config
         config_yaml_ = config_yaml
         constraints_ = Value.from_json(constraints) if constraints else None
+        devices_ = devices
         endpoint_bindings_ = endpoint_bindings
         num_units_ = num_units
         placement_ = [Placement.from_json(o) for o in placement or []]
+        policy_ = policy
         resources_ = resources
         series_ = series
         storage_ = storage
@@ -1563,6 +1569,9 @@ class ApplicationDeploy(Type):
         if application_ is not None and not isinstance(application_, (bytes, str)):
             raise Exception("Expected application_ to be a str, received: {}".format(type(application_)))
 
+        if attach_storage_ is not None and not isinstance(attach_storage_, (bytes, str, list)):
+            raise Exception("Expected attach_storage_ to be a Sequence, received: {}".format(type(attach_storage_)))
+
         if channel_ is not None and not isinstance(channel_, (bytes, str)):
             raise Exception("Expected channel_ to be a str, received: {}".format(type(channel_)))
 
@@ -1578,6 +1587,9 @@ class ApplicationDeploy(Type):
         if constraints_ is not None and not isinstance(constraints_, (dict, Value)):
             raise Exception("Expected constraints_ to be a Value, received: {}".format(type(constraints_)))
 
+        if devices_ is not None and not isinstance(devices_, dict):
+            raise Exception("Expected devices_ to be a Mapping, received: {}".format(type(devices_)))
+
         if endpoint_bindings_ is not None and not isinstance(endpoint_bindings_, dict):
             raise Exception("Expected endpoint_bindings_ to be a Mapping, received: {}".format(type(endpoint_bindings_)))
 
@@ -1587,6 +1599,9 @@ class ApplicationDeploy(Type):
         if placement_ is not None and not isinstance(placement_, (bytes, str, list)):
             raise Exception("Expected placement_ to be a Sequence, received: {}".format(type(placement_)))
 
+        if policy_ is not None and not isinstance(policy_, (bytes, str)):
+            raise Exception("Expected policy_ to be a str, received: {}".format(type(policy_)))
+
         if resources_ is not None and not isinstance(resources_, dict):
             raise Exception("Expected resources_ to be a Mapping, received: {}".format(type(resources_)))
 
@@ -1597,14 +1612,17 @@ class ApplicationDeploy(Type):
             raise Exception("Expected storage_ to be a Mapping, received: {}".format(type(storage_)))
 
         self.application = application_
+        self.attach_storage = attach_storage_
         self.channel = channel_
         self.charm_url = charm_url_
         self.config = config_
         self.config_yaml = config_yaml_
         self.constraints = constraints_
+        self.devices = devices_
         self.endpoint_bindings = endpoint_bindings_
         self.num_units = num_units_
         self.placement = placement_
+        self.policy = policy_
         self.resources = resources_
         self.series = series_
         self.storage = storage_
diff --git a/juju/client/facade.py b/juju/client/facade.py
index bc67398..53e0cf7 100644
--- a/juju/client/facade.py
+++ b/juju/client/facade.py
@@ -18,8 +18,7 @@ _marker = object()
 
 JUJU_VERSION = re.compile(r'[0-9]+\.[0-9-]+[\.\-][0-9a-z]+(\.[0-9]+)?')
 # Workaround for https://bugs.launchpad.net/juju/+bug/1683906
-NAUGHTY_CLASSES = ['ClientFacade', 'Client', 'ModelStatusInfo',
-                   'ApplicationDeploy']
+NAUGHTY_CLASSES = ['ClientFacade', 'Client', 'ModelStatusInfo']
 
 
 # Map basic types to Python's typing with a callable
diff --git a/juju/constraints.py b/juju/constraints.py
index 0050673..43cbd00 100644
--- a/juju/constraints.py
+++ b/juju/constraints.py
@@ -23,10 +23,13 @@ MEM = re.compile('^[1-9][0-9]*[MGTP]$')
 # Multiplication factors to get Megabytes
 # https://github.com/juju/juju/blob/master/constraints/constraints.go#L666
 FACTORS = {
-    "M": 1,
-    "G": 1024,
-    "T": 1024 * 1024,
-    "P": 1024 * 1024 * 1024
+    "M": 1024 ** 0,
+    "G": 1024 ** 1,
+    "T": 1024 ** 2,
+    "P": 1024 ** 3,
+    "E": 1024 ** 4,
+    "Z": 1024 ** 5,
+    "Y": 1024 ** 6
 }
 
 LIST_KEYS = {'tags', 'spaces'}
@@ -85,3 +88,49 @@ def normalize_value(value):
 def normalize_list_value(value):
     values = value.strip().split(',')
     return [normalize_value(value) for value in values]
+
+
+STORAGE = re.compile(
+    '(?:(?:^|(?<=,))(?:|(?P<pool>[a-zA-Z]+[-?a-zA-Z0-9]*)|(?P<count>-?[0-9]+)|(?:(?P<size>-?[0-9]+(?:\\.[0-9]+)?)(?P<size_exp>[MGTPEZY])(?:i?B)?))(?:$|,))')
+
+
+def parse_storage_constraint(constraint):
+    storage = {'count': 1}
+    for m in STORAGE.finditer(constraint):
+        pool = m.group('pool')
+        if pool:
+            if 'pool' in storage:
+                raise Exception("pool already specified")
+            storage['pool'] = pool
+        count = m.group('count')
+        if count:
+            count = int(count)
+            storage['count'] = count if count > 0 else 1
+        size = m.group('size')
+        if size:
+            storage['size'] = int(float(size) * FACTORS[m.group('size_exp')])
+    return storage
+
+
+DEVICE = re.compile(
+    '^(?P<count>[0-9]+)?(?:^|,)(?P<type>[^,]+)(?:$|,(?!$))(?P<attrs>(?:[^=]+=[^;]+)+)*$')
+ATTR = re.compile(';?(?P<key>[^=]+)=(?P<value>[^;]+)')
+
+
+def parse_device_constraint(constraint):
+    m = DEVICE.match(constraint)
+    if m is None:
+        raise Exception("device constraint does not match")
+    device = {}
+    count = m.group('count')
+    if count:
+        count = int(count)
+        device['count'] = count if count > 0 else 1
+    else:
+        device['count'] = 1
+    device['type'] = m.group('type')
+    attrs = m.group('attrs')
+    if attrs:
+        device['attributes'] = {kv['key']: kv['value']
+                                for kv in ATTR.finditer(attrs)}
+    return device


Test patch:
diff --git a/tests/unit/test_bundle.py b/tests/unit/test_bundle.py
index 6cdfa40..990c70f 100644
--- a/tests/unit/test_bundle.py
+++ b/tests/unit/test_bundle.py
@@ -63,7 +63,7 @@ class TestAddApplicationChange(unittest.TestCase):
                                                      "application",
                                                      "options",
                                                      "constraints",
-                                                     "storage",
+                                                     {"db": "pool,1,1GB"},
                                                      "endpoint_bindings",
                                                      "resources"])
         self.assertEqual({"change_id": 1,
@@ -73,7 +73,7 @@ class TestAddApplicationChange(unittest.TestCase):
                           "application": "application",
                           "options": "options",
                           "constraints": "constraints",
-                          "storage": "storage",
+                          "storage": {"db": {"pool": "pool", "count": 1, "size": 1024}},
                           "endpoint_bindings": "endpoint_bindings",
                           "resources": "resources",
                           "devices": None,
@@ -85,9 +85,9 @@ class TestAddApplicationChange(unittest.TestCase):
                                                      "application",
                                                      "options",
                                                      "constraints",
-                                                     "storage",
+                                                     {"db": "pool,1,1GB"},
+                                                     {"gpu": "1,gpu,attr1=a;attr2=b"},
                                                      "endpoint_bindings",
-                                                     "devices",
                                                      "resources",
                                                      "num_units"])
         self.assertEqual({"change_id": 1,
@@ -97,10 +97,10 @@ class TestAddApplicationChange(unittest.TestCase):
                           "application": "application",
                           "options": "options",
                           "constraints": "constraints",
-                          "storage": "storage",
+                          "storage": {"db": {"pool": "pool", "count": 1, "size": 1024}},
                           "endpoint_bindings": "endpoint_bindings",
                           "resources": "resources",
-                          "devices": "devices",
+                          "devices": {"gpu": {"type": "gpu", "count": 1, "attributes": {"attr1": "a", "attr2": "b"}}},
                           "num_units": "num_units"}, change.__dict__)
 
     def test_dict_params(self):
diff --git a/tests/unit/test_constraints.py b/tests/unit/test_constraints.py
index 3c52090..4d5e26c 100644
--- a/tests/unit/test_constraints.py
+++ b/tests/unit/test_constraints.py
@@ -55,3 +55,76 @@ class TestConstraints(unittest.TestCase):
              "tags": ["tag1"],
              "spaces": ["space1", "space2"]}
         )
+
+    def test_parse_storage_constraint(self):
+        _ = constraints.parse_storage_constraint
+
+        self.assertEqual(
+            _("pool,1M"),
+            {"pool": "pool",
+             "count": 1,
+             "size": 1 * 1024 ** 0}
+        )
+        self.assertEqual(
+            _("pool,"),
+            {"pool": "pool",
+             "count": 1}
+        )
+        self.assertEqual(
+            _("1M"),
+            {"size": 1 * 1024 ** 0,
+             "count": 1}
+        )
+        self.assertEqual(
+            _("p,1G"),
+            {"pool": "p",
+             "count": 1,
+             "size": 1 * 1024 ** 1}
+        )
+        self.assertEqual(
+            _("p,0.5T"),
+            {"pool": "p",
+             "count": 1,
+             "size": 512 * 1024 ** 1}
+        )
+        self.assertEqual(
+            _("3,0.5T"),
+            {"count": 3,
+             "size": 512 * 1024 ** 1}
+        )
+        self.assertEqual(
+            _("0.5T,3"),
+            {"count": 3,
+             "size": 512 * 1024 ** 1}
+        )
+
+    def test_parse_device_constraint(self):
+        _ = constraints.parse_device_constraint
+
+        self.assertEqual(
+            _("nvidia.com/gpu"),
+            {"type": "nvidia.com/gpu",
+             "count": 1}
+        )
+        self.assertEqual(
+            _("2,nvidia.com/gpu"),
+            {"type": "nvidia.com/gpu",
+             "count": 2}
+        )
+        self.assertEqual(
+            _("3,nvidia.com/gpu,gpu=nvidia-tesla-p100"),
+            {"type": "nvidia.com/gpu",
+             "count": 3,
+             "attributes": {
+                 "gpu": "nvidia-tesla-p100"
+             }}
+        )
+        self.assertEqual(
+            _("3,nvidia.com/gpu,gpu=nvidia-tesla-p100;2ndattr=another-attr"),
+            {"type": "nvidia.com/gpu",
+             "count": 3,
+             "attributes": {
+                 "gpu": "nvidia-tesla-p100",
+                 "2ndattr": "another-attr"
+             }}
+        )