Merge pull request #720 from cderici/deploy-attach-storage-param

#720

#### Description

As the name suggests, this adds `--attach-storage` parameter to the `model.deploy()`.

Fixes #716 


#### QA Steps

The following test is added for this. However, see the Notes below.

```
tox -e integration -- tests/integration/test_model.py::test_model_attach_storage_at_deploy
```

All CI tests need to pass.

#### Notes & Discussion

Currently the `attach-storage` and `detach-storage` is not working consistently on the Juju side. So we're skipping the storage tests that need those functions. (Investigating on the Juju side) The reason we need them is that the `--attach-storage` parameter on the `deploy()` expects an already existing storage. And the only way that I know how to get one that's attachable is to `add_storage()` to a unit and `unit.detach_storage()` it, which makes the storage available and retain it in the model. For some reason, however, either the `detach` doesn't detach properly I'm not sure, the `remove_application` wipes the storage out of the model (in the juju cli as well).

Author: jujubot

juju/model.py

@@ -1670,7 +1670,7 @@ async def deploy(
             self, entity_url, application_name=None, bind=None,
             channel=None, config=None, constraints=None, force=False,
             num_units=1, overlays=[], plan=None, resources=None, series=None,
-            storage=None, to=None, devices=None, trust=False):
+            storage=None, to=None, devices=None, trust=False, attach_storage=[]):
         """Deploy a new service or bundle.
 
         :param str entity_url: Charm or bundle url
@@ -1700,6 +1700,8 @@ async def deploy(
             with access to trusted credentials. Hooks run by the charm can access
             cloud credentials and other trusted access credentials.
 
+        :param str[] attach_storage: Existing storage to attach to the deployed unit
+            (not available on k8s models)
         TODO::
 
             - support local file resources
@@ -1713,6 +1715,9 @@ async def deploy(
         if trust and (self.info.agent_version < client.Number.from_json('2.4.0')):
             raise NotImplementedError("trusted is not supported on model version {}".format(self.info.agent_version))
 
+        if not all([type(st) == str for st in attach_storage]):
+            raise JujuError("Expected attach_storage to be a list of strings, given {}".format(attach_storage))
+
         # Ensure what we pass in, is a string.
         entity_url = str(entity_url)
         if is_local_charm(entity_url) and not entity_url.startswith("local:"):
@@ -1820,7 +1825,8 @@ async def deploy(
                 num_units=num_units,
                 placement=parse_placement(to),
                 devices=devices,
-                charm_origin=charm_origin
+                charm_origin=charm_origin,
+                attach_storage=attach_storage,
             )
 
     async def _add_charm(self, charm_url, origin):
@@ -2037,7 +2043,7 @@ async def add_local_resources(self, application, entity_url, metadata, resources
     async def _deploy(self, charm_url, application, series, config,
                       constraints, endpoint_bindings, resources, storage,
                       channel=None, num_units=None, placement=None,
-                      devices=None, charm_origin=None):
+                      devices=None, charm_origin=None, attach_storage=[]):
         """Logic shared between `Model.deploy` and `BundleHandler.deploy`.
         """
         log.info('Deploying %s', charm_url)
@@ -2064,6 +2070,7 @@ async def _deploy(self, charm_url, application, series, config,
             storage=storage,
             placement=placement,
             devices=devices,
+            attach_storage=attach_storage,
         )
         result = await app_facade.Deploy(applications=[app])
         errors = [r.error.message for r in result.results if r.error]

tests/integration/test_model.py

@@ -1077,6 +1077,48 @@ async def test_add_storage(event_loop):
         assert any([tag.storage("pgdata") in s for s in ret])
 
 
+@base.bootstrapped
+@pytest.mark.asyncio
+async def test_model_attach_storage_at_deploy(event_loop):
+    pytest.skip('detach/attach_storage inconsistent on Juju side, unable to test')
+    async with base.CleanModel() as model:
+        # The attach_storage needs to be an existing storage,
+        # so the plan is to:
+        # - Deploy postgresql
+        # - Create and attach a storage
+        # - Detach storage
+        # - Remove app
+        # - Re-deploy with attach_storage parameter
+        # - Make sure the storage is there
+        app = await model.deploy('postgresql')
+        await model.wait_for_idle(status="active")
+
+        unit = app.units[0]
+        ret = await unit.add_storage("pgdata")
+        assert any([tag.storage("pgdata") in s for s in ret])
+        storage_id = ret[0]
+
+        await unit.detach_storage(storage_id, force=True)
+        await jasyncio.sleep(10)
+
+        storages1 = await model.list_storage()
+        assert any([storage_id in s['storage-tag'] for s in storages1])
+
+        # juju remove-application
+        # actually removes the storage even though the destroy_storage=false
+        await app.destroy(destroy_storage=False)
+        await jasyncio.sleep(10)
+
+        storages2 = await model.list_storage()
+        assert any([storage_id in s['storage-tag'] for s in storages2])
+
+        await model.deploy('postgresql', attach_storage=[storage_id])
+        await model.wait_for_idle(status="active")
+
+        storages3 = await model.list_storage()
+        assert any([storage_id in s['storage-tag'] for s in storages3])
+
+
 @base.bootstrapped
 @pytest.mark.asyncio
 async def test_detach_storage(event_loop):