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

[JUJU-1628] Deploy by revision #830

Merged
merged 17 commits into from
Apr 26, 2023
Merged

Conversation

cderici
Copy link
Contributor

@cderici cderici commented Apr 17, 2023

Description

This adds the feature that allows deploying by revision, by passing through the revision info into the ResolveCharms API call.

Fixes #690

QA Steps

An example is added, so it should work:

 $ python examples/deploywithrevision.py
  • Along with that example, I need to write some integration tests for this before landing (see discussion below).
tox -e integration -- tests/integration/test_model.py::test_deploy_by_revision
tox -e integration -- tests/integration/test_model.py::test_deploy_by_revision_validate_flags

All CI tests need to pass.

Notes & Discussion

  • Before landing, we need to make sure that the input validation is being handled gracefully. For regular charms, --revision requires --channel, however for bundles, --channel and --revision are mutually exclusive. The ResolveCharms call should be getting us the right errors for those, we just need to make sure those errors are passed through correctly. (there should be integration tests for these validations)

cderici added 2 commits April 17, 2023 16:54
by passing the revision info through the ResolveCharm api call.

fixes juju#690
that deploys juju-qa-test --channel 2.0/stable --revision 22
@cderici cderici added the hint/do-not-merge requires additional efforts, no merge label Apr 17, 2023
Note that --revision is only used for charmhub charms, so we're not
passing this info into the CharmOrigin for local charms.
cderici added 4 commits April 19, 2023 15:02
This is needed because in libjuju we default to latest/stable if no
channel is specified. It is dangerous if we don't add this check
because without it the libjuju would deploy the wrong revision without
an exception if --revision is used but no --channel is given.
Otherwise fail early without making any additional API calls.
flags are validated before deploying charms or bundles
@cderici cderici removed the hint/do-not-merge requires additional efforts, no merge label Apr 19, 2023
@juanmanuel-tirado
Copy link
Contributor

Manual QA is not passing.

Using Juju:

juju deploy juju-qa-test test1 --revision=19 --channel=latest/edge
Located charm "juju-qa-test" in charm-hub, revision 19
Deploying "test1" from charm-hub charm "juju-qa-test", revision 19 in channel latest/edge on [email protected]/stable
juju status test1
Model  Controller  Cloud/Region         Version  SLA          Timestamp
test   lxd312      localhost/localhost  3.1.2    unsupported  11:18:59+02:00

App    Version  Status   Scale  Charm         Channel      Rev  Exposed  Message
test1           waiting    0/1  juju-qa-test  latest/edge   19  no       waiting for machine

Unit     Workload  Agent       Machine  Public address  Ports  Message
test1/1  waiting   allocating  27                              waiting for machine

Machine  State    Address  Inst id  Base          AZ  Message
27       pending           pending  [email protected]      

We get revision 19. When using python-libjuju:

from juju.model import Model
m = Model(); await m.connect()
await m.deploy("juju-qa-test", "test21python", revision=19, channel="latest/edge")

We don't get the expected revision:

 juju status test2
Model  Controller  Cloud/Region         Version  SLA          Timestamp
test   lxd312      localhost/localhost  3.1.2    unsupported  11:19:50+02:00

App    Version  Status   Scale  Charm         Channel      Rev  Exposed  Message
test2           waiting    0/1  juju-qa-test  latest/edge   21  no       waiting for machine

Unit     Workload  Agent       Machine  Public address  Ports  Message
test2/0  waiting   allocating  28       10.220.18.224          waiting for machine

Machine  State    Address        Inst id         Base          AZ  Message
28       pending  10.220.18.224  juju-a38396-28  [email protected]      Running

The returned revision should be 19.

Copy link
Contributor

@juanmanuel-tirado juanmanuel-tirado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the test, we need to deploy a revision previous to the latest one to really check the solution.

juanmanuel-tirado and others added 10 commits April 25, 2023 14:06
* Propagate force parameter to resolve function to enable force for remote bundles.
Without the series the ResolveCharm will select the latest base for
the charm. E.g. even if we want focal, the ResolveCharm will return
22.04 (jammy) for the base channel in the resulting origin if the
charm supports both jammy and focal.

This communicates the series info with the ResolveCharm via the
inputted origin so the resulting origin will have the correct base
channel.

Fixes juju#822
That are accidentally introduced in juju#825.

* _resolve_charm's force parameter is made optional
* fixed the _resolve_charm calls in bundle.py to have less number of
variables to unpack onto
This selector will be used after the ResolveCharms api call, and use
the supported series and the base information returned to select the
appropriate series to construct the correct base for the charm. Then
it's given to the AddCharm in normal the deploy process.
The main change is in the _resolve_charm that's used in the model and
bundle deploy code path. We pass in the series info whenever we can
into the ResolveCharm call, then use its result to run the series
selector, then construct the base using that and finally pass the
origin to the AddCharm. Note that the ResolveCharm API call is made no
more than once.
…t revision

This is basically testing what is manually reported by
@juanmanuel-tirado in the following review:

juju#830 (comment)
@juanmanuel-tirado
Copy link
Contributor

Manual QA works. Part of the CI integration tests fails because the ubuntu charm in version 22 requests storage. That's OK. However, this solution does not mimic the Juju CLI when deploying bundles. For example:

await m.deploy("anbox-cloud-core", "test", revision=60)
...
JujuError: specifying a revision requires a channel for future upgrades. Please use --channel

if using --channel

await m.deploy("anbox-cloud-core", "testb", revision=60, channel="stable")
....
JujuError: revision and channel are mutually exclusive when deploying a bundle. Please choose one.

With the Juju CLI deploying bundles can be done without setting the channel.

juju deploy anbox-cloud-core --revision=60 

I think we can push this one. However, we have to address this in a separated PR.

Copy link
Contributor

@juanmanuel-tirado juanmanuel-tirado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG2M

We expect some CI to fail due to the ubuntu 22 charm requiring storage.

@juanmanuel-tirado
Copy link
Contributor

/merge

@jujubot jujubot merged commit 96b8b07 into juju:master Apr 26, 2023
juanmanuel-tirado added a commit that referenced this pull request May 5, 2023
* [JUJU-3202] Add facades for 3.1.1. (#807)

* Add facades for 3.1.1.

* Update secrets backend test.

* Add destroy-units to destroy several units at once

Fixes #811

* Add integration test for destroy_units

* [JUJU-3517] Revisit _build_facades in connection (#826)

The main change here is that we no longer raise an exception in the
case of an unknown facade.

* Added 3.1.2 and 3.2-beta2 schemas.

* Add capability for deploying by revision

by passing the revision info through the ResolveCharm api call.

fixes #690

* Add example for deploy with revision

that deploys juju-qa-test --channel 2.0/stable --revision 22

* Add revision parameter in LocalDeployType.resolve

Note that --revision is only used for charmhub charms, so we're not
passing this info into the CharmOrigin for local charms.

* Add integration test for deploy by revision success

* Add validation for --revision flag to require --channel flag

This is needed because in libjuju we default to latest/stable if no
channel is specified. It is dangerous if we don't add this check
because without it the libjuju would deploy the wrong revision without
an exception if --revision is used but no --channel is given.

* Add validation for bundles to require either --revision or --channel

Otherwise fail early without making any additional API calls.

* Add integration test for making sure the --required and --channel
flags are validated before deploying charms or bundles

* [JUJU-3253] add missing force in bundle deployment (#815)

* Propagate force parameter to resolve function to enable force for remote bundles.

* Pass series info into origin for ResolveCharm

Without the series the ResolveCharm will select the latest base for
the charm. E.g. even if we want focal, the ResolveCharm will return
22.04 (jammy) for the base channel in the resulting origin if the
charm supports both jammy and focal.

This communicates the series info with the ResolveCharm via the
inputted origin so the resulting origin will have the correct base
channel.

Fixes #822

* Fix _resolve_charm errors

That are accidentally introduced in #825.

* _resolve_charm's force parameter is made optional
* fixed the _resolve_charm calls in bundle.py to have less number of
variables to unpack onto

* Change charm channel in bundle for test

* Rename example with consistent name

* Implement series selector for charm resolution

This selector will be used after the ResolveCharms api call, and use
the supported series and the base information returned to select the
appropriate series to construct the correct base for the charm. Then
it's given to the AddCharm in normal the deploy process.

* Add unit tests for series selector functionality

* Utilize the series selector to construct the correct base for charms

The main change is in the _resolve_charm that's used in the model and
bundle deploy code path. We pass in the series info whenever we can
into the ResolveCharm call, then use its result to run the series
selector, then construct the base using that and finally pass the
origin to the AddCharm. Note that the ResolveCharm API call is made no
more than once.

* Fix unit tests for bundle change runs

* Add integration test to challenge the resolver to find an old corrrect revision

This is basically testing what is manually reported by
@juanmanuel-tirado in the following review:

#830 (comment)

* [JUJU-3552] Prepare 3.1.2.1 release (#836)

* Add juju 3.1.2 missing facades. 
* Fix secrets-bakend-lifecycle test.
* Increase testing timeouts.
* Revisit postgresql charm to be downloaded.
---------

Co-authored-by: Juju bot <[email protected]>
Co-authored-by: Caner Derici <[email protected]>

* Prepare release notes for 3.1.2.0. (#843)

* Prepare release notes for 3.1.2.0.

---------

Co-authored-by: Caner Derici <[email protected]>
Co-authored-by: Juju bot <[email protected]>
@cderici cderici mentioned this pull request Oct 5, 2023
jujubot added a commit that referenced this pull request Oct 13, 2023
#957

#### Description

This backports the support for deploy by revision from master branch (3.x track) (#830) onto the 2.9 track. 

#### QA Steps

So we have 1 example and 2 integration tests for this, all of them should work well (I tested it against 2.9.45 on lxd).

```sh
 $ python examples/deploywithrevision.py
```

```
tox -e integration -- tests/integration/test_model.py::test_deploy_by_revision
```

```
tox -e integration -- tests/integration/test_model.py::test_deploy_by_revision_validate_flags
```

All the rest of the CI tests need to pass.

#### Notes & Discussion

JUJU-4716
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploying a specific revision of a charm
3 participants