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.

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

Fail to deploy charm with storage parameter #1075

Closed
dragomirp opened this issue Jul 19, 2024 · 2 comments
Closed

Fail to deploy charm with storage parameter #1075

dragomirp opened this issue Jul 19, 2024 · 2 comments
Assignees
Labels
area/application hint/3.x going on main branch kind/bug indicates a bug in the project state/in-progress currently being worked on

Comments

@dragomirp
Copy link

Description

Trying to update libjuju from 3.5.0.0 to 3.5.2.0 breaks tests setting the storage parameter during deploy. Example CI run can be seen here. Deploying a locally build charm with:

        await ops_test.model.deploy(
            charm,
            application_name=FIRST_APPLICATION,
            num_units=3,
            series=CHARM_SERIES,
            storage={"pgdata": {"pool": "lxd-btrfs", "size": 2048}},
            config={"profile": "testing"},
        )

Will result in:

  File "/home/runner/work/postgresql-operator/postgresql-operator/tests/integration/ha_tests/test_restore_cluster.py", line 40, in test_build_and_deploy
    await ops_test.model.deploy(
  File "/home/runner/work/postgresql-operator/postgresql-operator/.tox/integration/lib/python3.10/site-packages/juju/model.py", line 1841, in deploy
    return await self._deploy(
  File "/home/runner/work/postgresql-operator/postgresql-operator/.tox/integration/lib/python3.10/site-packages/juju/model.py", line 2154, in _deploy
    storage=***k: parse_storage_constraint(v) for k, v in (storage or dict()).items()***,
  File "/home/runner/work/postgresql-operator/postgresql-operator/.tox/integration/lib/python3.10/site-packages/juju/model.py", line 2154, in <dictcomp>
    storage=***k: parse_storage_constraint(v) for k, v in (storage or dict()).items()***,
  File "/home/runner/work/postgresql-operator/postgresql-operator/.tox/integration/lib/python3.10/site-packages/juju/constraints.py", line 131, in parse_storage_constraint
    for m in STORAGE.finditer(constraint):
TypeError: expected string or bytes-like object

Trying to deploy something like:

await model.deploy("postgresql", storage={"pgdata": "lxd-btrfs,1,2G"}, config={"profile": "testing"})

Fails different validation:

TypeError: juju.client._definitions.Constraints() argument after ** must be a mapping, not str

I think the issue is caused by the additional validation added in model.py L2119

Urgency

Annoying bug in our test suite

Python-libjuju version

3.5.2.0

Juju version

3.4.4

Reproduce / Test

>>> import asyncio
>>> from juju.model import Model
>>> model = Model()
>>> await model.connect_current()
>>> await model.deploy("postgresql", storage={"pgdata": {"pool": "lxd-btrfs", "size": 2048}}, config={"profile": "testing"})
@addyess
Copy link
Contributor

addyess commented Jul 26, 2024

@cderici -- i'm also affected by this

i tries switching to the new style of arguments:
using

storage = {
    "osd-devices": "8G,1",
    "osd-journals": "8G,1",
}

and that breaks when it tries parse these as client.Contraints in model.py

        if storage:
            storage = {
                k: client.Constraints(**v)
                for k, v in storage.items()
            }

@hmlanigan hmlanigan added kind/bug indicates a bug in the project area/application in progress hint/3.x going on main branch labels Aug 1, 2024
@james-garner-canonical james-garner-canonical added state/in-progress currently being worked on and removed in progress labels Sep 23, 2024
jujubot added a commit that referenced this issue Nov 26, 2024
…fy-handling-of-storage-constraints-in-deploy

#1213

#### Description

This PR unifies storage constraint parsing into a single method (`juju.constraints.parse_storage_constraints`), which is called in a single place (`juju.model.Model._deploy`), allowing both bundle and model deployments to specify storage constraints using either the [juju storage constraint directive format](https://juju.is/docs/juju/storage-constraint) (e.g. `{'label': 'ebs,100G'}`) or pre-parsed dictionaries (e.g. `{'label': {'count': 1, 'pool': 'ebs', 'size': 102400}`).

#### QA Steps

Unit tests have been updated to reflect the new parsing location. Integration tests have been added to verify that model deployment can request storage using either format. The existing bundle integration tests should continue to pass.


#### Notes & Discussion

This PR resolves the issues with storage constraint parsing identified in:
- #1052
- #1075

#1052 was initially addressed in #1053, which was included in the [3.5.2.0](https://github.com/juju/python-libjuju/releases/tag/3.5.2.0) release. This allowed bundle deployments (using `juju.bundle.AddApplicationChange.run`) to correctly handle storage constraints specified as strings in the [juju storage constraint directive format](https://juju.is/docs/juju/storage-constraint).

Unfortunately, this erroneously required that model deployments (using `juju.model.Model.deploy`) also use this string format, instead of the parsed dictionary format that was previously accepted. This was noticed in #1075 and initially fixed in #1105, which was merged into `main` but never released. This fix moved parsing of storage constraint strings to bundle deployment exclusively.

Due to the interim period in which `3.5.2` has (incorrectly) required model deployments to use the string format, I think the best fix at this point is to allow both bundle deployments and model deployments to use either format, and parse them into the parsed dictionary format in a single place in `juju.model.Model._deploy` (the private method called by both bundle and model deployments).

After merging, let's look at getting these changes out in a `3.5.2.2` bugfix release.
@james-garner-canonical
Copy link
Contributor

Hopefully fixed by #1105 and #1213. Please re-open this issue if things aren't fixed for you in the next release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/application hint/3.x going on main branch kind/bug indicates a bug in the project state/in-progress currently being worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants