From 578f1d9dc6c316354889b5b1b8a0e5c4cdc60289 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 25 Nov 2024 15:46:58 +1300 Subject: [PATCH 1/2] feat(storage-constraints): unify parsing under Model._deploy This allows both bundle.AddApplicationChange and model.Model.deploy to use either storage constraints in the form of {label: constraint} where constraint can be either a juju style storage constraints string, or a python dict with a 'count' entry and optionally 'pool' and 'size' entries. --- juju/bundle.py | 29 ++++++++++++++++------------- juju/constraints.py | 21 ++++++++++++++++++++- juju/model.py | 25 +++++++++++++++++++------ 3 files changed, 55 insertions(+), 20 deletions(-) diff --git a/juju/bundle.py b/juju/bundle.py index 6f136ec77..ad6600b36 100644 --- a/juju/bundle.py +++ b/juju/bundle.py @@ -1,5 +1,6 @@ # Copyright 2023 Canonical Ltd. # Licensed under the Apache V2, see LICENCE file for details. +from __future__ import annotations import base64 import io @@ -8,7 +9,7 @@ import zipfile from contextlib import closing from pathlib import Path -from typing import Dict, Optional +from typing import TYPE_CHECKING, Mapping, cast import requests import yaml @@ -17,12 +18,15 @@ from . import jasyncio, utils from .client import client from .constraints import parse as parse_constraints -from .constraints import parse_storage_constraint from .errors import JujuError from .origin import Channel, Source from .url import URL, Schema from .utils import get_base_from_origin_or_channel +if TYPE_CHECKING: + from .constraints import StorageConstraintDict + from .model import Model + log = logging.getLogger(__name__) @@ -599,7 +603,8 @@ class AddApplicationChange(ChangeInfo): :options: holds application options. :constraints: optional application constraints. :storage: optional storage constraints, in the form of `{label: constraint}`. - The label is a string specified by the charm, while the constraint is a string following + The label is a string specified by the charm, while the constraint is + either a constraints.StorageConstraintDict, or a string following `the juju storage constraint directive format `_, specifying the storage pool, number of volumes, and size of each volume. :devices: optional devices constraints. @@ -610,7 +615,7 @@ class AddApplicationChange(ChangeInfo): application's charm. """ - storage: Optional[Dict[str, str]] + storage: Mapping[str, str | StorageConstraintDict] | None = None @staticmethod def method(): @@ -627,7 +632,8 @@ async def run(self, context): # NB: this should really be handled by the controller when generating the # bundle change plan, and this short-term workaround may be missing some # aspects of the logic which the CLI client contains to handle edge cases. - if self.application in context.model.applications: + model = cast("Model", context.model) # pyright: ignore[reportUnknownMemberType] + if self.application in model.applications: log.debug("Skipping %s; already in model", self.application) return self.application @@ -637,9 +643,9 @@ async def run(self, context): if self.options is not None: options = self.options if context.trusted: - if context.model.info.agent_version < client.Number.from_json("2.4.0"): + if model.info.agent_version < client.Number.from_json("2.4.0"): raise NotImplementedError( - f"trusted is not supported on model version {context.model.info.agent_version}" + f"trusted is not supported on model version {model.info.agent_version}" ) options["trust"] = "true" @@ -676,11 +682,11 @@ async def run(self, context): .get("resources", {}) ) if Schema.CHARM_HUB.matches(url.schema): - resources = await context.model._add_charmhub_resources( + resources = await model._add_charmhub_resources( self.application, charm, origin, overrides=self.resources ) - await context.model._deploy( + await model._deploy( charm_url=charm, application=self.application, series=self.series, @@ -688,10 +694,7 @@ async def run(self, context): constraints=self.constraints, endpoint_bindings=self.endpoint_bindings, resources=resources, - storage={ - label: parse_storage_constraint(constraint) - for label, constraint in (self.storage or {}).items() - }, + storage=self.storage, channel=self.channel, devices=self.devices, num_units=self.num_units, diff --git a/juju/constraints.py b/juju/constraints.py index 885dcfb47..246bd4c78 100644 --- a/juju/constraints.py +++ b/juju/constraints.py @@ -19,7 +19,7 @@ # import re -from typing import Dict, List, Optional, TypedDict, Union +from typing import Dict, List, Mapping, Optional, TypedDict, Union from typing_extensions import NotRequired, Required @@ -190,6 +190,25 @@ def parse_storage_constraint(constraint: str) -> StorageConstraintDict: return storage +def parse_storage_constraints( + constraints: Optional[Mapping[str, Union[str, StorageConstraintDict]]] = None, +) -> Dict[str, StorageConstraintDict]: + if constraints is None: + return {} + parsed: dict[str, StorageConstraintDict] = {} + for label, storage_constraint in constraints.items(): + if isinstance(storage_constraint, str): + parsed[label] = parse_storage_constraint(storage_constraint) + elif isinstance(storage_constraint, dict): # pyright: ignore[reportUnnecessaryIsInstance] + parsed[label] = storage_constraint + else: + raise ValueError( + f"Unexpected constraint {storage_constraint!r}" + f" for label {label!r} in {constraints}" + ) + return parsed + + DEVICE = re.compile( # original regex: # '^(?P[0-9]+)?(?:^|,)(?P[^,]+)(?:$|,(?!$))(?P(?:[^=]+=[^;]+)+)*$' diff --git a/juju/model.py b/juju/model.py index 8dd7d747d..856eb28fa 100644 --- a/juju/model.py +++ b/juju/model.py @@ -32,6 +32,7 @@ from .client import client, connection, connector from .client.overrides import Caveat, Macaroon from .constraints import parse as parse_constraints +from .constraints import parse_storage_constraints from .controller import ConnectedController, Controller from .delta import get_entity_class, get_entity_delta from .errors import ( @@ -61,6 +62,7 @@ if TYPE_CHECKING: from .application import Application from .client._definitions import FullStatus + from .constraints import StorageConstraintDict from .machine import Machine from .relation import Relation from .remoteapplication import ApplicationOffer, RemoteApplication @@ -1788,7 +1790,7 @@ async def deploy( resources=None, series=None, revision=None, - storage=None, + storage: Mapping[str, str | StorageConstraintDict] | None = None, to=None, devices=None, trust=False, @@ -1813,7 +1815,11 @@ async def deploy( :param str series: Series on which to deploy DEPRECATED: use --base (with Juju 3.1) :param int revision: specifying a revision requires a channel for future upgrades for charms. For bundles, revision and channel are mutually exclusive. - :param dict storage: Storage constraints TODO how do these look? + :param dict storage: optional storage constraints, in the form of `{label: constraint}`. + The label is a string specified by the charm, while the constraint is + a constraints.StorageConstraintsDict, or a string following + `the juju storage constraint directive format `_, + specifying the storage pool, number of volumes, and size of each volume. :param to: Placement directive as a string. For example: '23' - place on machine 23 @@ -1829,8 +1835,6 @@ async def deploy( :param str[] attach_storage: Existing storage to attach to the deployed unit (not available on k8s models) """ - if storage: - storage = {k: client.Constraints(**v) for k, v in storage.items()} if trust and (self.info.agent_version < client.Number.from_json("2.4.0")): raise NotImplementedError( f"trusted is not supported on model version {self.info.agent_version}" @@ -2251,7 +2255,7 @@ async def _deploy( constraints, endpoint_bindings, resources, - storage, + storage: Mapping[str, str | StorageConstraintDict] | None, channel=None, num_units=None, placement=None, @@ -2261,9 +2265,18 @@ async def _deploy( force=False, server_side_deploy=False, ): - """Logic shared between `Model.deploy` and `BundleHandler.deploy`.""" + """Logic shared between `Model.deploy` and `BundleHandler.deploy`. + + :param dict storage: optional storage constraints, in the form of `{label: constraint}`. + The label is a string specified by the charm, while the constraint is + either a constraints.StorageConstraintDict, or a string following + `the juju storage constraint directive format `_, + specifying the storage pool, number of volumes, and size of each volume. + """ log.info("Deploying %s", charm_url) + storage = parse_storage_constraints(storage) + trust = config.get("trust", False) # stringify all config values for API, and convert to YAML config = {k: str(v) for k, v in config.items()} From 69aa0527259eecd0920aa698045a306afac36492 Mon Sep 17 00:00:00 2001 From: James Garner Date: Mon, 25 Nov 2024 17:25:38 +1300 Subject: [PATCH 2/2] test(storage-constraints): update tests to reflect new logic Unit tests have been updated to reflect the fact that parsing of storage constraints no longer happens before calling Model._deploy. Due to the difficulty in mocking the _deploy internals, the bundle test of parsing many storage variations has been moved to test_parse_storage_constraints in test_constraints. Two integration tests have been added to test deployment with parsed and unparsed storage arguments respectively. --- tests/integration/test_model.py | 36 ++++++++++ tests/unit/test_bundle.py | 117 ++------------------------------ tests/unit/test_constraints.py | 52 ++++++++++++++ 3 files changed, 93 insertions(+), 112 deletions(-) diff --git a/tests/integration/test_model.py b/tests/integration/test_model.py index e46594807..2bc7c7393 100644 --- a/tests/integration/test_model.py +++ b/tests/integration/test_model.py @@ -45,6 +45,42 @@ async def test_model_name(): await model.disconnect() +@base.bootstrapped +async def test_deploy_with_storage_unparsed(): + async with base.CleanModel() as model: + await model.deploy( + "postgresql", + storage={"pgdata": "1G"}, + ) + await model.wait_for_idle(status="active") + storages = await model.list_storage() + assert len(storages) == 1 + [storage] = storages + # size information isn't exposed, so can't assert on that + assert storage["owner-tag"].startswith(tag.unit("postgresql")) + assert storage["storage-tag"].startswith(tag.storage("pgdata")) + assert storage["life"] == "alive" + assert storage["status"].status == "attached" + + +@base.bootstrapped +async def test_deploy_with_storage_preparsed(): + async with base.CleanModel() as model: + await model.deploy( + "postgresql", + storage={"pgdata": {"size": 1024, "count": 1}}, + ) + await model.wait_for_idle(status="active") + storages = await model.list_storage() + assert len(storages) == 1 + [storage] = storages + # size information isn't exposed, so can't assert on that + assert storage["owner-tag"].startswith(tag.unit("postgresql")) + assert storage["storage-tag"].startswith(tag.storage("pgdata")) + assert storage["life"] == "alive" + assert storage["status"].status == "attached" + + @base.bootstrapped @pytest.mark.bundle async def test_deploy_local_bundle_dir(): diff --git a/tests/unit/test_bundle.py b/tests/unit/test_bundle.py index b5f92756e..3a9b1537f 100644 --- a/tests/unit/test_bundle.py +++ b/tests/unit/test_bundle.py @@ -3,14 +3,13 @@ import unittest from pathlib import Path -from typing import Dict, List, Tuple from unittest import mock from unittest.mock import ANY, Mock, patch import yaml from toposort import CircularDependencyError -from juju import charmhub, constraints +from juju import charmhub from juju.bundle import ( AddApplicationChange, AddCharmChange, @@ -189,113 +188,13 @@ async def test_run_with_charmhub_charm(self): constraints="constraints", endpoint_bindings="endpoint_bindings", resources=["resource1"], - storage={ - storage_label: constraints.parse_storage_constraint(storage_constraint) - }, + storage={storage_label: storage_constraint}, devices="devices", channel="channel", charm_origin=ANY, num_units="num_units", ) - async def test_run_with_storage_variations(self): - """Test that various valid storage constraints are parsed as expected - before model._deploy is called. - - Uses the mock call logic from test_run_with_charmhub_charm, - which will run before this test. - """ - storage_arg_pairs: List[ - Tuple[Dict[str, str], Dict[str, constraints.StorageConstraintDict]] - ] = [ - # (storage_arg_for_change, storage_arg_for_deploy) - ( - {"some-label": "ebs,100G,1"}, - {"some-label": {"count": 1, "pool": "ebs", "size": 102400}}, - ), - ( - {"some-label": "ebs,2.1G,3"}, - {"some-label": {"count": 3, "pool": "ebs", "size": 2150}}, - ), - ( - {"some-label": "ebs,100G"}, - {"some-label": {"count": 1, "pool": "ebs", "size": 102400}}, - ), - ({"some-label": "ebs,2"}, {"some-label": {"count": 2, "pool": "ebs"}}), - ({"some-label": "200G,7"}, {"some-label": {"count": 7, "size": 204800}}), - ({"some-label": "ebs"}, {"some-label": {"count": 1, "pool": "ebs"}}), - ( - {"some-label": "10YB"}, - {"some-label": {"count": 1, "size": 11529215046068469760}}, - ), - ({"some-label": "1"}, {"some-label": {"count": 1}}), - ({"some-label": "-1"}, {"some-label": {"count": 1}}), - ({"some-label": ""}, {"some-label": {"count": 1}}), - ( - { - "some-label": "2.1G,3", - "data": "1MiB,70", - "logs": "ebs,-1", - }, - { - "some-label": {"count": 3, "size": 2150}, - "data": {"count": 70, "size": 1}, - "logs": {"count": 1, "pool": "ebs"}, - }, - ), - ] - for storage_arg_for_change, storage_arg_for_deploy in storage_arg_pairs: - change = AddApplicationChange( - 1, - [], - params={ - "charm": "charm", - "series": "series", - "application": "application", - "options": "options", - "constraints": "constraints", - "storage": storage_arg_for_change, - "endpoint-bindings": "endpoint_bindings", - "resources": "resources", - "devices": "devices", - "num-units": "num_units", - "channel": "channel", - }, - ) - # mock model - model = Mock() - model._deploy = mock.AsyncMock(return_value=None) - model._add_charmhub_resources = mock.AsyncMock(return_value=["resource1"]) - model.applications = {} - # mock context - context = Mock() - context.resolve.return_value = "ch:charm1" - context.origins = {"ch:charm1": Mock()} - context.trusted = False - context.model = model - # mock info_func - info_func = mock.AsyncMock(return_value=["12345", "name"]) - # patch and call - with patch.object(charmhub.CharmHub, "get_charm_id", info_func): - result = await change.run(context) - assert result == "application" - # asserts - model._deploy.assert_called_once() - model._deploy.assert_called_with( - charm_url="ch:charm1", - application="application", - series="series", - config="options", - constraints="constraints", - endpoint_bindings="endpoint_bindings", - resources=["resource1"], - storage=storage_arg_for_deploy, # we're testing this - devices="devices", - channel="channel", - charm_origin=ANY, - num_units="num_units", - ) - async def test_run_with_charmhub_charm_no_channel(self): """Test to verify if when the given channel is None, the channel defaults to "local/stable", which is the default channel value for the @@ -347,9 +246,7 @@ async def test_run_with_charmhub_charm_no_channel(self): constraints="constraints", endpoint_bindings="endpoint_bindings", resources=["resource1"], - storage={ - storage_label: constraints.parse_storage_constraint(storage_constraint) - }, + storage={storage_label: storage_constraint}, devices="devices", channel="latest/stable", charm_origin=ANY, @@ -397,9 +294,7 @@ async def test_run_local(self): constraints="constraints", endpoint_bindings="endpoint_bindings", resources={}, - storage={ - storage_label: constraints.parse_storage_constraint(storage_constraint) - }, + storage={storage_label: storage_constraint}, devices="devices", num_units="num_units", channel="", @@ -452,9 +347,7 @@ async def test_run_no_series(self): constraints="constraints", endpoint_bindings="endpoint_bindings", resources=["resource1"], - storage={ - storage_label: constraints.parse_storage_constraint(storage_constraint) - }, + storage={storage_label: storage_constraint}, devices="devices", channel="channel", charm_origin=ANY, diff --git a/tests/unit/test_constraints.py b/tests/unit/test_constraints.py index f437f58f6..79c3c86f0 100644 --- a/tests/unit/test_constraints.py +++ b/tests/unit/test_constraints.py @@ -4,6 +4,7 @@ # # Test our constraints parser # +from __future__ import annotations import unittest @@ -79,6 +80,57 @@ def test_parse_storage_constraint(self): 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_storage_constraints(self): + """Test that various valid storage constraints are parsed as expected.""" + storage_arg_pairs: list[ + tuple[dict[str, str], dict[str, constraints.StorageConstraintDict]] + ] = [ + # (storage_arg, parsed_storage_arg) + ( + {"some-label": "ebs,100G,1"}, + {"some-label": {"count": 1, "pool": "ebs", "size": 102400}}, + ), + ( + {"some-label": "ebs,2.1G,3"}, + {"some-label": {"count": 3, "pool": "ebs", "size": 2150}}, + ), + ( + {"some-label": "ebs,100G"}, + {"some-label": {"count": 1, "pool": "ebs", "size": 102400}}, + ), + ({"some-label": "ebs,2"}, {"some-label": {"count": 2, "pool": "ebs"}}), + ({"some-label": "200G,7"}, {"some-label": {"count": 7, "size": 204800}}), + ({"some-label": "ebs"}, {"some-label": {"count": 1, "pool": "ebs"}}), + ( + {"some-label": "10YB"}, + {"some-label": {"count": 1, "size": 11529215046068469760}}, + ), + ({"some-label": "1"}, {"some-label": {"count": 1}}), + ({"some-label": "-1"}, {"some-label": {"count": 1}}), + ({"some-label": ""}, {"some-label": {"count": 1}}), + ( + { + "some-label": "2.1G,3", + "data": "1MiB,70", + "logs": "ebs,-1", + }, + { + "some-label": {"count": 3, "size": 2150}, + "data": {"count": 70, "size": 1}, + "logs": {"count": 1, "pool": "ebs"}, + }, + ), + ] + for storage_arg, parsed_storage_constraint in storage_arg_pairs: + self.assertEqual( + constraints.parse_storage_constraints(storage_arg), + parsed_storage_constraint, + ) + self.assertEqual( + constraints.parse_storage_constraints(parsed_storage_constraint), + parsed_storage_constraint, + ) + def test_parse_device_constraint(self): _ = constraints.parse_device_constraint