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

[Bug]: charm upgrade does not work #883

Closed
esunar opened this issue Jun 15, 2023 · 4 comments
Closed

[Bug]: charm upgrade does not work #883

esunar opened this issue Jun 15, 2023 · 4 comments
Assignees
Labels
hint/2.9 going on 2.9 branch kind/bug indicates a bug in the project

Comments

@esunar
Copy link

esunar commented Jun 15, 2023

Description

when I try to run the below code I got the following error.

await model.connect()
app = model.applications["keystone"]
await app.upgrade_charm(channel="victoria/stable")

juju.errors.JujuError: while adding pending resource info for "policyd-override": bad resource metadata: bad info: bad metadata: resource missing filename

Hower below code runs succesfully. What could be the problem?

juju refresh --channel victoria/stable  keystone

log , code, bundle, thread

juju client: 2.9.43-ubuntu-amd64
controller: 2.9.42, python juju==2.9.42.4

@cderici

Urgency

Blocker for our release

Python-libjuju version

2.9.42.4

Juju version

2.9.43-ubuntu-amd64

Reproduce / Test

import asyncio
import logging

from juju.model import Model

JUJU_MAX_FRAME_SIZE = 2**30


def setup_logging(log_level: str = "DEBUG") -> None:
    log_formatter = logging.Formatter(
        fmt="%(asctime)s [%(levelname)s] %(message)s", datefmt="%Y-%m-%d %H:%M:%S"
    )
    root_logger = logging.getLogger()
    root_logger.setLevel(log_level)
    if not root_logger.hasHandlers():
        console_handler = logging.StreamHandler()
        console_handler.setFormatter(log_formatter)
        root_logger.addHandler(console_handler)


async def update():
    model = Model(max_frame_size=JUJU_MAX_FRAME_SIZE)
    await model.connect()
    app = model.applications["keystone"]
    await app.upgrade_charm(channel="victoria/stable")


setup_logging()
loop = asyncio.new_event_loop()
loop.run_until_complete(update())
@esunar esunar added the kind/bug indicates a bug in the project label Jun 15, 2023
@cderici cderici self-assigned this Jun 15, 2023
cderici added a commit to cderici/python-libjuju that referenced this issue Jun 15, 2023
cderici added a commit to cderici/python-libjuju that referenced this issue Jun 15, 2023
jujubot added a commit that referenced this issue Jun 21, 2023
#884

#### Description

This adds a condition to the logic of deciding if we need to upgrade a resource during an `app.refresh()`, analogous to the [`shouldUpgradeResource()` utility](https://github.com/juju/juju/blob/cdda0d771219c9e59bfef4579da8b6389e1b0d72/cmd/juju/application/utils/utils.go#L159) in Juju.

Fixes #883 

#### QA Steps

This adds the use case in #883 as an integration test, so the following should pass:

```
tox -e integration -- tests/integration/test_application.py::test_upgrade_charm_resource_same_rev_no_update
```

Also per #884 (comment), this also splits out the resource refresh decider logic into a separate function and adds unit tests for it, so the following should also pass:

```sh
 tox -e py3 -- tests/unit/test_utils.py::TestShouldUpgradeResource
```
@cderici
Copy link
Contributor

cderici commented Jun 22, 2023

#884 closes this

@cderici cderici closed this as completed Jun 22, 2023
cderici added a commit to cderici/python-libjuju that referenced this issue Jun 26, 2023
cderici added a commit to cderici/python-libjuju that referenced this issue Jun 26, 2023
@agileshaw
Copy link

@cderici I just ran into this issue with ceph-mon upgrade using libjuju 2.9.44.x (which should contain the fix). The error message I got was while adding pending resource info for "alert-rules": bad resource metadata: bad info: bad metadata: resource missing filename.

I was able to produce a similar error for keystone upgrade using the script in bug description as well. My juju controller version is 2.9.44.

Could you to take a look at this issue again?

@cderici
Copy link
Contributor

cderici commented Sep 22, 2023

@agileshaw yeah no problem, I'll re-open the issue and see what's happening as soon as I can 👍

@cderici cderici reopened this Sep 22, 2023
@cderici cderici added the hint/2.9 going on 2.9 branch label Sep 28, 2023
jujubot added a commit that referenced this issue Oct 17, 2023
#960

#### Description

This adds two things:

1. Fixes the error that causes #955 where we were trying to pass a revision for a resource in refresh, can be reproduced with the `juju-cli` as well as follows:

```sh
 $ juju deploy ceph-mon --channel octopus/stable
 $ juju refresh ceph-juju --channel quincy/stable --resource alert-rules=1
ERROR while adding pending resource info for "alert-rules": bad resource metadata: bad info: bad metadata: resource missing filename
```

2. This also implements functionality that allows passing `resources` argument to application refresh for non-local charms.

Fixes #955 and #883 

#### QA Steps

So the fix for the #955 can be tested manually by recreating the scenario above with pylibjuju as follows:

```python
 $ python -m asyncio
>>> import asyncio
>>> from juju import model;m=model.Model();await m.connect()
>>> await m.deploy('ceph-mon', channel='octopus/stable')
```

Check the resources for that using juju-cli:

```sh
 $ juju resources ceph-mon
No resources to display.
```

Go back to the python repl:

```python
>>> await m.applications['ceph-mon'].refresh(channel='quincy/stable')
This should succeed
```

And check the resources again external to pylibjuju, you should see the resource is added:

```sh
 $ juju resources ceph-juju
Resource Supplied by Revision
alert-rules charmstore 1
```

Additionally, an integration test for the second part of the PR that adds the resource argument to refresh is added, so just run that, and maybe play with it manually for different charms:

```
tox -e integration -- tests/integration/test_application.py::test_refresh_with_resource_argument
```

All CI tests need to pass.

#### Notes & Discussion

JUJU-4736
@cderici
Copy link
Contributor

cderici commented Oct 17, 2023

#960 closes this.

@cderici cderici closed this as completed Oct 17, 2023
jujubot added a commit that referenced this issue Oct 26, 2023
#973

#### Description

This is a forward port of #960, bringing support for refreshing application with providing `resources` as arguments.

Fixes #955 and #883 for 3.x track.

#### QA Steps

QA steps should be the same with #960:

So the fix for the #955 can be tested manually by recreating the scenario above with pylibjuju as follows:

```python
 $ python -m asyncio
>>> import asyncio
>>> from juju import model;m=model.Model();await m.connect()
>>> await m.deploy('ceph-mon', channel='octopus/stable')
```

Check the resources for that using juju-cli:

```sh
 $ juju resources ceph-mon
No resources to display.
```

Go back to the python repl:

```python
>>> await m.applications['ceph-mon'].refresh(channel='quincy/stable')
This should succeed
```

And check the resources again external to pylibjuju, you should see the resource is added:

```sh
 $ juju resources ceph-juju
Resource Supplied by Revision
alert-rules charmstore 1
```

Additionally, an integration test for the second part of the PR that adds the resource argument to refresh is added, so just run that, and maybe play with it manually for different charms:

```
tox -e integration -- tests/integration/test_application.py::test_refresh_with_resource_argument
```

All CI tests need to pass.

#### Notes & Discussion

JUJU-4736
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hint/2.9 going on 2.9 branch kind/bug indicates a bug in the project
Projects
None yet
Development

No branches or pull requests

3 participants