Skip to content

Commit

Permalink
Merge pull request #1105 from Aflynn50/fix-parse-storage-constraints
Browse files Browse the repository at this point in the history 
#1105

It seems a previous change for allow bundle storage constraints was done at the wrong level and affected regular storage constraints as well. This fix moves the previous change to only affect bundle storage constraints.

#### QA Steps

TODO
  • Loading branch information
@jujubot
jujubot authored Oct 17, 2024
2 parents 3911d1f + 04f9a1d commit 0e8c45f
Show file tree
Hide file tree
Showing 3 changed files with 117 additions and 15 deletions.
16 changes: 14 additions & 2 deletions juju/bundle.py
View file
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
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,
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
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"
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

0 comments on commit 0e8c45f

Please sign in to comment.