Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: move parse_storage_constraints to the bundle deployer #1105

Merged
merged 5 commits into from
Oct 17, 2024
Merged

fix: move parse_storage_constraints to the bundle deployer #1105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1bfbb4c
fix: move parse_storage_constraints to the bunlde deployer
Aflynn50 Sep 20, 2024
c53eb98
test: update bundle unit tests with storage constraint parsing
james-garner-canonical Oct 15, 2024
e58e897
style: add type hinting and update docstrings for storage
james-garner-canonical Oct 15, 2024
6f4e8b7
test: add unit test coverage for different storage arguments for AddA…
james-garner-canonical Oct 16, 2024
04f9a1d
fix: manually iterate over cases instead of using pytest.mark.paramet…
james-garner-canonical Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions juju/bundle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,15 @@
import base64
from contextlib import closing
from pathlib import Path
from typing import Dict, Optional

import yaml

from toposort import toposort_flatten

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
Expand Down Expand Up @@ -554,14 +556,20 @@ 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 <https://juju.is/docs/juju/storage-constraint>`_,
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
application's charm.
: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.
Expand Down Expand Up @@ -630,7 +638,11 @@ async def run(self, context):
constraints=self.constraints,
endpoint_bindings=self.endpoint_bindings,
resources=resources,
storage=self.storage,
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,
Expand Down
5 changes: 2 additions & 3 deletions juju/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
james-garner-canonical marked this conversation as resolved.
Show resolved Hide resolved
trust=trust,
base=charm_origin.base,
channel=channel,
Expand Down Expand Up @@ -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,
Expand Down
111 changes: 101 additions & 10 deletions tests/unit/test_bundle.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
# Copyright 2023 Canonical Ltd.
# Licensed under the Apache V2, see LICENCE file for details.

from pathlib import Path
import unittest
from pathlib import Path
from unittest import mock
from mock import patch, Mock, ANY
from typing import Dict, List, Tuple

import yaml

Expand All @@ -23,6 +24,7 @@
SetAnnotationsChange,
)
from juju import charmhub
from juju import constraints
from juju.client import client
from toposort import CircularDependencyError

Expand Down Expand Up @@ -121,14 +123,15 @@ 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",
Expand Down Expand Up @@ -160,22 +163,106 @@ 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,
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 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",
Expand Down Expand Up @@ -208,19 +295,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"})
Expand All @@ -246,19 +335,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"
james-garner-canonical marked this conversation as resolved.
Show resolved Hide resolved
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",
Expand Down Expand Up @@ -289,7 +380,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,
Expand Down
Loading