From 1bfbb4c23bb32e6b350d772c3edbf72b5568946a Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Fri, 20 Sep 2024 17:23:34 +0100 Subject: [PATCH 1/5] fix: move parse_storage_constraints to the bunlde deployer It seems a previous change for allow bundle storage constraints was done at the wrong level and affected regualr storage constraints as well. This fix moves the previous change to only affect bunlde storage constraints. --- juju/bundle.py | 3 ++- juju/model.py | 5 ++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/juju/bundle.py b/juju/bundle.py index 9c1c8cca..5c7af072 100644 --- a/juju/bundle.py +++ b/juju/bundle.py @@ -16,6 +16,7 @@ from .client import client from .constraints import parse as parse_constraints +from .constraints import parse_storage_constraint from .errors import JujuError from . import utils, jasyncio from .origin import Channel, Source @@ -630,7 +631,7 @@ async def run(self, context): constraints=self.constraints, endpoint_bindings=self.endpoint_bindings, resources=resources, - storage=self.storage, + storage={k: parse_storage_constraint(v) for k, v in (self.storage or dict()).items()}, channel=self.channel, devices=self.devices, num_units=self.num_units, diff --git a/juju/model.py b/juju/model.py index f3b274b1..2d2cd40b 100644 --- a/juju/model.py +++ b/juju/model.py @@ -29,7 +29,6 @@ from .client import client, connector from .client.overrides import Caveat, Macaroon from .constraints import parse as parse_constraints -from .constraints import parse_storage_constraint from .controller import Controller, ConnectedController from .delta import get_entity_class, get_entity_delta from .errors import JujuAPIError, JujuError, JujuModelConfigError, JujuBackupError @@ -2116,7 +2115,7 @@ async def _deploy(self, charm_url, application, series, config, devices=devices, dryrun=False, placement=placement, - storage={k: parse_storage_constraint(v) for k, v in (storage or dict()).items()}, + storage=storage, trust=trust, base=charm_origin.base, channel=channel, @@ -2151,7 +2150,7 @@ async def _deploy(self, charm_url, application, series, config, endpoint_bindings=endpoint_bindings, num_units=num_units, resources=resources, - storage={k: parse_storage_constraint(v) for k, v in (storage or dict()).items()}, + storage=storage, placement=placement, devices=devices, attach_storage=attach_storage, From c53eb98469777dfafa021676e84020875c97aab3 Mon Sep 17 00:00:00 2001 From: James Garner Date: Tue, 15 Oct 2024 14:33:27 +1300 Subject: [PATCH 2/5] test: update bundle unit tests with storage constraint parsing --- tests/unit/test_bundle.py | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/tests/unit/test_bundle.py b/tests/unit/test_bundle.py index 5140dfd3..0aba34c5 100644 --- a/tests/unit/test_bundle.py +++ b/tests/unit/test_bundle.py @@ -23,6 +23,7 @@ SetAnnotationsChange, ) from juju import charmhub +from juju import constraints from juju.client import client from toposort import CircularDependencyError @@ -123,12 +124,14 @@ def test_dict_params_missing_data(self): class TestAddApplicationChangeRun: async def test_run_with_charmhub_charm(self): + storage_label = "some-label" + storage_constraint = "ebs,100G,1" change = AddApplicationChange(1, [], params={"charm": "charm", "series": "series", "application": "application", "options": "options", "constraints": "constraints", - "storage": "storage", + "storage": {storage_label: storage_constraint}, "endpoint-bindings": "endpoint_bindings", "resources": "resources", "devices": "devices", @@ -160,7 +163,7 @@ async def test_run_with_charmhub_charm(self): constraints="constraints", endpoint_bindings="endpoint_bindings", resources=["resource1"], - storage="storage", + storage={storage_label: constraints.parse_storage_constraint(storage_constraint)}, devices="devices", channel="channel", charm_origin=ANY, @@ -170,12 +173,14 @@ 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 Charm Hub """ + storage_label = "some-label" + storage_constraint = "ebs,100G,1" change = AddApplicationChange(1, [], params={"charm": "charm", "series": "series", "application": "application", "options": "options", "constraints": "constraints", - "storage": "storage", + "storage": {storage_label: storage_constraint}, "endpoint-bindings": "endpoint_bindings", "resources": "resources", "devices": "devices", @@ -208,19 +213,21 @@ async def test_run_with_charmhub_charm_no_channel(self): constraints="constraints", endpoint_bindings="endpoint_bindings", resources=["resource1"], - storage="storage", + storage={storage_label: constraints.parse_storage_constraint(storage_constraint)}, devices="devices", channel="latest/stable", charm_origin=ANY, num_units="num_units") async def test_run_local(self): + storage_label = "some-label" + storage_constraint = "ebs,100G,1" change = AddApplicationChange(1, [], params={"charm": "local:charm", "series": "series", "application": "application", "options": "options", "constraints": "constraints", - "storage": "storage", + "storage": {storage_label: storage_constraint}, "endpoint-bindings": "endpoint_bindings", "devices": "devices", "num-units": "num_units"}) @@ -246,19 +253,21 @@ async def test_run_local(self): constraints="constraints", endpoint_bindings="endpoint_bindings", resources={}, - storage="storage", + storage={storage_label: constraints.parse_storage_constraint(storage_constraint)}, devices="devices", num_units="num_units", channel="", charm_origin=ANY) async def test_run_no_series(self): + storage_label = "some-label" + storage_constraint = "ebs,100G,1" change = AddApplicationChange(1, [], params={"charm": "ch:charm1", "series": "", "application": "application", "options": "options", "constraints": "constraints", - "storage": "storage", + "storage": {storage_label: storage_constraint}, "endpoint-bindings": "endpoint_bindings", "resources": "resources", "devices": "devices", @@ -289,7 +298,7 @@ async def test_run_no_series(self): constraints="constraints", endpoint_bindings="endpoint_bindings", resources=["resource1"], - storage="storage", + storage={storage_label: constraints.parse_storage_constraint(storage_constraint)}, devices="devices", channel="channel", charm_origin=ANY, From e58e897228336d4d8f43181cbc1f540234492289 Mon Sep 17 00:00:00 2001 From: James Garner Date: Tue, 15 Oct 2024 15:39:24 +1300 Subject: [PATCH 3/5] style: add type hinting and update docstrings for storage --- juju/bundle.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/juju/bundle.py b/juju/bundle.py index 5c7af072..99439744 100644 --- a/juju/bundle.py +++ b/juju/bundle.py @@ -9,6 +9,7 @@ import base64 from contextlib import closing from pathlib import Path +from typing import Dict, Optional import yaml @@ -555,7 +556,10 @@ class AddApplicationChange(ChangeInfo): this will be used to scale the application. :options: holds application options. :constraints: optional application constraints. - :storage: optional storage 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 juju storage constraint directive format `_, + specifying the storage pool, number of volumes, and size of each volume. :devices: optional devices constraints. :endpoint_bindings: optional endpoint bindings :resources: identifies the revision to use for each resource of the @@ -563,6 +567,9 @@ class AddApplicationChange(ChangeInfo): :local_resources: identifies the path to the local resource of the application's charm. """ + + storage: Optional[Dict[str, str]] + @staticmethod def method(): """method returns an associated ID for the Juju API call. @@ -631,7 +638,11 @@ async def run(self, context): constraints=self.constraints, endpoint_bindings=self.endpoint_bindings, resources=resources, - storage={k: parse_storage_constraint(v) for k, v in (self.storage or dict()).items()}, + storage={ + label: parse_storage_constraint(constraint) + for label, constraint + in (self.storage or {}).items() + }, channel=self.channel, devices=self.devices, num_units=self.num_units, From 6f4e8b74c8bce6f2e4b1ac81fd451e73b58abc54 Mon Sep 17 00:00:00 2001 From: James Garner Date: Wed, 16 Oct 2024 15:58:57 +1300 Subject: [PATCH 4/5] test: add unit test coverage for different storage arguments for AddApplicationChange --- tests/unit/test_bundle.py | 92 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 2 deletions(-) diff --git a/tests/unit/test_bundle.py b/tests/unit/test_bundle.py index 0aba34c5..187ab1c6 100644 --- a/tests/unit/test_bundle.py +++ b/tests/unit/test_bundle.py @@ -1,10 +1,12 @@ # Copyright 2023 Canonical Ltd. # Licensed under the Apache V2, see LICENCE file for details. -from pathlib import Path +import pytest import unittest +from pathlib import Path from unittest import mock from mock import patch, Mock, ANY +from typing import Dict, Union import yaml @@ -122,7 +124,6 @@ def test_dict_params_missing_data(self): class TestAddApplicationChangeRun: - async def test_run_with_charmhub_charm(self): storage_label = "some-label" storage_constraint = "ebs,100G,1" @@ -169,6 +170,93 @@ async def test_run_with_charmhub_charm(self): charm_origin=ANY, num_units="num_units") + @pytest.mark.parametrize( + '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'} + }, + ), + ] + ) + async def test_run_with_storage_variations( + self, + storage_arg_for_change: Dict[str, str], + storage_arg_for_deploy: Dict[str, Dict[str, Union[int, str]]], + ): + """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. + """ + 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 Charm Hub From 04f9a1dd98e88a76f0f8283025c39619c0ec63ad Mon Sep 17 00:00:00 2001 From: James Garner Date: Thu, 17 Oct 2024 12:25:04 +1300 Subject: [PATCH 5/5] fix: manually iterate over cases instead of using pytest.mark.parametrize --- tests/unit/test_bundle.py | 124 ++++++++++++++++++-------------------- 1 file changed, 59 insertions(+), 65 deletions(-) diff --git a/tests/unit/test_bundle.py b/tests/unit/test_bundle.py index 187ab1c6..a6f69a80 100644 --- a/tests/unit/test_bundle.py +++ b/tests/unit/test_bundle.py @@ -1,12 +1,11 @@ # Copyright 2023 Canonical Ltd. # Licensed under the Apache V2, see LICENCE file for details. -import pytest import unittest from pathlib import Path from unittest import mock from mock import patch, Mock, ANY -from typing import Dict, Union +from typing import Dict, List, Tuple import yaml @@ -170,9 +169,13 @@ async def test_run_with_charmhub_charm(self): charm_origin=ANY, num_units="num_units") - @pytest.mark.parametrize( - 'storage_arg_for_change,storage_arg_for_deploy', - [ + 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}}), @@ -196,66 +199,57 @@ async def test_run_with_charmhub_charm(self): }, ), ] - ) - async def test_run_with_storage_variations( - self, - storage_arg_for_change: Dict[str, str], - storage_arg_for_deploy: Dict[str, Dict[str, Union[int, str]]], - ): - """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. - """ - 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", - ) + 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