[JUJU-3749] Add bases to bundles #15725
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jack-w-shaw
force-pushed
the
JUJU-3749_add_bases_to_bundles
branch
3 times, most recently
from
732eaf4
to
c70222e
Compare
jack-w-shaw
force-pushed
the
JUJU-3749_add_bases_to_bundles
branch
2 times, most recently
from
9d028de
to
82bcd4c
Compare
hmlanigan
requested changes
Jun 15, 2023
There was a problem hiding this comment.
I haven't completed my review yet, but here is the current data
QA:
I can deploy a unit of ubuntu with a base of ubuntu@16.04 from a bundle which is unsupported in juju 3.x, fails deploying the charm only.
$ juju status ubuntu
Model Controller Cloud/Region Version SLA Timestamp
one jack-test localhost/localhost 3.3-beta1.1 unsupported 19:33:42Z
App Version Status Scale Charm Channel Rev Exposed Message
ubuntu waiting 0/1 ubuntu stable 21 no waiting for machine
Unit Workload Agent Machine Public address Ports Message
ubuntu/0 waiting allocating 2 waiting for machine
Machine State Address Inst id Base AZ Message
2 pending juju-4c4e62-2 ubuntu@16.04 Running
$ juju deploy ubuntu --base ubuntu@16.04 xenial
ERROR ubuntu is not available on the following series: xenial not supported
82bcd4c
to
a135751
Compare
|
@jack-w-shaw once a pr is in review. Please add new commits with each change made. The commits can be squashed before merge, but it's difficult to know what's changed for reviewers. I have to start at the top again. |
hmlanigan
reviewed
Jun 23, 2023
|
The QA bug found in juju 3.1 as well - https://bugs.launchpad.net/juju/+bug/2025163 |
hmlanigan
reviewed
Jun 27, 2023
| Name: "a", | ||
| Summary: "b", | ||
| Description: "c", | ||
| Series: []string{"bionic"}, |
There was a problem hiding this comment.
Add a series = "Kubernetes" version of this test.
There was a problem hiding this comment.
Added. in the case of kubernetes series, I assume we want to fallback to series.LegacyKubernetesBase ?
There was a problem hiding this comment.
yes, and at line 82, let's check against series.LegacyKubernetesBase in case it changes.
hmlanigan
reviewed
Jun 28, 2023
core/bundle/changes/changes.go
Outdated
| @@ -225,7 +225,7 @@ type AddCharmChange struct { | |||
| // TODO: since GUIArgs are returned in a magical order, replacing series with base | |||
| // would be a breaking change. Remove GetChanges facade endpoint in Juju 4 | |||
| func (ch *AddCharmChange) GUIArgs() []interface{} { | |||
| return []interface{}{ch.Params.Charm, ch.Params.Series, ch.Params.Channel} | |||
| return []interface{}{ch.Params.Charm, mustBaseToSeries(ch.Params.Base), ch.Params.Channel} | |||
There was a problem hiding this comment.
Add a comment about why this is okay to do. Would prefer using series.GetSeriesFromBase and logging the err if it happens, which it never should. Panics should not be done outside of tests.
Same for all GUIArgs methods
jack-w-shaw
commented
Jun 29, 2023
| @@ -145,6 +150,8 @@ func existingOffersFromModel(ctrlModel *Model) map[string]set.Strings { | |||
| return existingOffers | |||
| } | |||
|
|
|||
| var logger = loggo.GetLogger("juju.core.bundle.changes") | |||
There was a problem hiding this comment.
I know we're trying to move away from this pattern, but changing all the structs and factory signatures to properly inject a logger seems overkill for something that is only a theoretical possibility, and will be dropped in Juju 4 anyway
jack-w-shaw
force-pushed
the
JUJU-3749_add_bases_to_bundles
branch
from
2bf8e6b
to
d7154e8
Compare
hmlanigan
reviewed
Jun 30, 2023
| Name: "a", | ||
| Summary: "b", | ||
| Description: "c", | ||
| Series: []string{"bionic"}, |
There was a problem hiding this comment.
yes, and at line 82, let's check against series.LegacyKubernetesBase in case it changes.
jack-w-shaw
force-pushed
the
JUJU-3749_add_bases_to_bundles
branch
from
26d8a18
to
0aee80a
Compare
Use bases throughout bundle deploy code, parsing immidiately from series if necessary This has meant changing over from SeriesSelector to BaseSelector, which behaves slightly differently. Noteably, it will not select bionic, so a lot of tests have had to change their base from bionic to focal Unfortunately, the least bad option was to keep bases as strings in Change structs. This means the marshalled nicely for Args as a string, and omitempty is respected Also, since GUIArgs output ordering is magic, we cannot replace series with base. GUIArgs and the associated GetChanges facade endpoint should be deprecated in favour of GetChangesMapArgs in Juju 4
jack-w-shaw
force-pushed
the
JUJU-3749_add_bases_to_bundles
branch
from
0aee80a
to
22e3fc0
Compare
|
/merge |
Closed
Merged
jujubot
added a commit
that referenced
this pull request
Jul 11, 2023
#15896 Forward ports: - #15845 - #15725 - #15852 - #15854 - #15849 - #15855 - #15857 - #15861 - #15862 - #15860 - #15864 - #15825 - #15866 - #15863 - #15870 - #15871 - #15872 - #15873 - #15874 - #15876 - #15875 - #15881 - #15727 - #15883 - #15884 - #15880 - #15879 - #15886 - #15887 - #15877 - #15888 - #15893 - #15894 Conflicts: - cmd/juju/ssh/debugcode_test.go - cmd/juju/ssh/debughooks_test.go - cmd/juju/ssh/scp_unix_test.go - cmd/juju/ssh/ssh_machine.go - cmd/juju/ssh/ssh_machine_test.go - cmd/juju/ssh/ssh_unix_test.go - worker/dbaccessor/worker.go - caas/kubernetes/provider/k8s_test.go - caas/kubernetes/provider/operator_test.go - caas/kubernetes/provider/package_test.go - caas/kubernetes/provider/rbac.go - cmd/juju/application/deployer/bundlehandler.go - cmd/juju/application/deployer/bundlehandler_test.go - cmd/jujud/agent/model/manifolds.go - core/bundle/changes/changes.go - worker/caasapplicationprovisioner/application.go - worker/caasapplicationprovisioner/application_test.go - worker/caasapplicationprovisioner/mock_test.go
jack-w-shaw
added a commit
to jack-w-shaw/juju
that referenced
this pull request
Aug 21, 2023
This commit backports _some_ of the changes from the PR juju#15725 However, instead of converting to bases at the edge, we convert to series at the edge. This is because there are a lot of changes in later versions that using bases throughout depends on which are not present in 3.1, which would require expensive backports
jack-w-shaw
added a commit
to jack-w-shaw/juju
that referenced
this pull request
Aug 21, 2023
This commit backports _some_ of the changes from the PR juju#15725 However, instead of converting to bases at the edge, we convert to series at the edge. This is because there are a lot of changes in later versions that using bases throughout depends on which are not present in 3.1, which would require expensive backports We also do NOT backport changes to GetChanges and GetChangesMapArgs, as they are not required to backport this feature. This simplifies the change significantly
jack-w-shaw
added a commit
to jack-w-shaw/juju
that referenced
this pull request
Aug 21, 2023
This commit backports _some_ of the changes from the PR juju#15725 However, instead of converting to bases at the edge, we convert to series at the edge. This is because there are a lot of changes in later versions that using bases throughout depends on which are not present in 3.1, which would require expensive backports We also do NOT backport changes to GetChanges and GetChangesMapArgs, as they are not required to backport this feature. This simplifies the change significantly
jack-w-shaw
added a commit
to jack-w-shaw/juju
that referenced
this pull request
Aug 21, 2023
This commit backports _some_ of the changes from the PR juju#15725 However, instead of converting to bases at the edge, we convert to series at the edge. This is because there are a lot of changes in later versions that using bases throughout depends on which are not present in 3.1, which would require expensive backports We also do NOT backport changes to GetChanges and GetChangesMapArgs, as they are not required to backport this feature. This simplifies the change significantly
5 tasks
jack-w-shaw
added a commit
to jack-w-shaw/juju
that referenced
this pull request
Aug 21, 2023
This commit backports _some_ of the changes from the PR juju#15725 However, instead of converting to bases at the edge, we convert to series at the edge. This is because there are a lot of changes in later versions that using bases throughout depends on which are not present in 3.1, which would require expensive backports We also do NOT backport changes to GetChanges and GetChangesMapArgs, as they are not required to backport this feature. This simplifies the change significantly
5 tasks
jack-w-shaw
added a commit
to jack-w-shaw/juju
that referenced
this pull request
Aug 30, 2023
This commit backports _some_ of the changes from the PR juju#15725 However, instead of converting to bases at the edge, we convert to series at the edge. This is because there are a lot of changes in later versions that using bases throughout depends on which are not present in 3.1, which would require expensive backports We also do NOT backport changes to GetChanges and GetChangesMapArgs, as they are not required to backport this feature. This simplifies the change significantly
jujubot
added a commit
that referenced
this pull request
Aug 31, 2023
…n_bundles #16162 'Backport' the changes required to read bundles which include bases to 3.1. This is so future releass of 3.1 are compatible with default bundles (i.e. without the compatibility option) exported in 3.3 onwards. To do this we need to: - rev the charm package to the most recent version (v11) - 'backport' (and make some _heavy_ modifications) #15725 - backport (with some minor modifications/merge conflict resolutions) #16080 Note: instead of converting to bases at the edge, we convert to series at the edge. This is because there are a lot of changes in later versions that using bases throughout depends on which are not present in 3.1, which would require expensive backports We also do NOT backport changes to GetChanges and GetChangesMapArgs, as they are not required to backport bases in bundles. ## Checklist - [x] Code style: imports ordered, good names, simple structure, etc - [x] Comments saying why design decisions were made - [x] Go unit tests, with comments saying what you're testing - [x] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing - [ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages ## QA steps Ensure unit tests and test_deploy_bundles integration tests pass ``` $ juju deploy kubeflow --trust ``` ``` $ juju add-model m1 $ cat bundle1.yaml name: tiny applications: ubuntu-plus-one: charm: ./testcharms/charms/ubuntu-plus scale: 1 base: "ubuntu@20.04" ubuntu-plus-two: charm: ./testcharms/charms/ubuntu-plus scale: 1 base: "ubuntu@22.04" $ juju deploy ./bundle1.yaml Executing changes: - upload charm /home/jack/juju/juju-core/testcharms/charms/ubuntu-plus for series focal with architecture=amd64 - deploy application ubuntu-plus-one on focal - upload charm /home/jack/juju/juju-core/testcharms/charms/ubuntu-plus for series jammy with architecture=amd64 - deploy application ubuntu-plus-two on jammy - add unit ubuntu-plus-one/0 to new machine 0 - add unit ubuntu-plus-two/0 to new machine 1 Deploy of bundle completed. (wait) $ juju status Model Controller Cloud/Region Version SLA Timestamp m lxd localhost/localhost 3.1.6.1 unsupported 12:02:21+01:00 App Version Status Scale Charm Channel Rev Exposed Message ubuntu-plus-one active 1 ubuntu-plus 0 no Hello from update-status, load: 0.37, 0.79, 1.52 ubuntu-plus-two active 1 ubuntu-plus 0 no Hello from update-status, load: 0.70, 0.83, 1.50 Unit Workload Agent Machine Public address Ports Message ubuntu-plus-one/0* active idle 0 10.219.211.227 Hello from update-status, load: 0.37, 0.79, 1.52 ubuntu-plus-two/0* active idle 1 10.219.211.32 Hello from update-status, load: 0.70, 0.83, 1.50 Machine State Address Inst id Series AZ Message 0 started 10.219.211.227 juju-738e43-0 focal Running 1 started 10.219.211.32 juju-738e43-1 jammy Running ``` ``` $ juju add-model m2 $ cat bundle2.yaml name: tiny applications: ubuntu-plus-one: charm: ./testcharms/charms/ubuntu-plus scale: 1 base: ubuntu@22.04 to: - 0 ubuntu-plus-two: charm: ./testcharms/charms/ubuntu-plus scale: 1 base: ubuntu@20.04 to: - 1 machines: 0: base: ubuntu@22.04 1: base: ubuntu@20.04 $ juju deploy ./bundle2.yaml (wait) $ juju status Model Controller Cloud/Region Version SLA Timestamp m2 lxd localhost/localhost 3.1.6.1 unsupported 12:02:21+01:00 App Version Status Scale Charm Channel Rev Exposed Message ubuntu-plus-one active 1 ubuntu-plus 0 no Hello from update-status, load: 0.37, 0.79, 1.52 ubuntu-plus-two active 1 ubuntu-plus 0 no Hello from update-status, load: 0.70, 0.83, 1.50 Unit Workload Agent Machine Public address Ports Message ubuntu-plus-one/0* active idle 0 10.219.211.227 Hello from update-status, load: 0.37, 0.79, 1.52 ubuntu-plus-two/0* active idle 1 10.219.211.32 Hello from update-status, load: 0.70, 0.83, 1.50 Machine State Address Inst id Series AZ Message 0 started 10.219.211.141 juju-c5c5b8-0 jammy Running 1 started 10.219.211.102 juju-c5c5b8-1 focal Running ``` ``` $ juju add-model m3 $ cat bundle3.yaml name: tiny default-base: ubuntu@20.04 applications: ubuntu-plus-one: charm: ./testcharms/charms/ubuntu-plus scale: 1 ubuntu-plus-two: charm: ./testcharms/charms/ubuntu-plus scale: 1 base: ubuntu@22.04 $ juju deploy ./bundle3.yaml Executing changes: - upload charm /home/jack/juju/juju-core/testcharms/charms/ubuntu-plus for series focal with architecture=amd64 - deploy application ubuntu-plus-one on ubuntu@20.04/stable - upload charm /home/jack/juju/juju-core/testcharms/charms/ubuntu-plus for series jammy with architecture=amd64 - deploy application ubuntu-plus-two on ubuntu@22.04/stable - add unit ubuntu-plus-one/0 to new machine 0 - add unit ubuntu-plus-two/0 to new machine 1 Deploy of bundle completed. (wait) $ juju status Model Controller Cloud/Region Version SLA Timestamp m3 lxd localhost/localhost 3.1.6.1 unsupported 12:02:21+01:00 App Version Status Scale Charm Channel Rev Exposed Message ubuntu-plus-one active 1 ubuntu-plus 0 no Hello from update-status, load: 0.37, 0.79, 1.52 ubuntu-plus-two active 1 ubuntu-plus 0 no Hello from update-status, load: 0.70, 0.83, 1.50 Unit Workload Agent Machine Public address Ports Message ubuntu-plus-one/0* active idle 0 10.219.211.227 Hello from update-status, load: 0.37, 0.79, 1.52 ubuntu-plus-two/0* active idle 1 10.219.211.32 Hello from update-status, load: 0.70, 0.83, 1.50 Machine State Address Inst id Series AZ Message 0 started 10.219.211.227 juju-738e43-0 focal Running 1 started 10.219.211.32 juju-738e43-1 jammy Running ``` ### ALSO Create 3 bundles as follows ``` $ cat bundle1.yaml applications: ubu1: charm: ubuntu base: ubuntu@22.04 series: focal num_units: 1 $ cat bundle2.yaml default-base: ubuntu@22.04 applications: ubu2: charm: ubuntu base: ubuntu@20.04 series: focal num_units: 1 $ cat bundle3.yaml default-base: ubuntu@22.04 applications: ubu3: charm: ubuntu series: focal num_units: 1 ``` Verify: ``` $ juju deploy ./bundle1.yaml ERROR cannot deploy bundle: supplied series "focal" and base "ubuntu@22.04" in application "ubu1" do not match not valid ``` ``` $ juju deploy ./bundle2.yaml Located charm "ubuntu" in charm-hub, channel stable Executing changes: - upload charm ubuntu from charm-hub for base ubuntu@20.04/stable with architecture=amd64 - deploy application ubu2 from charm-hub on ubuntu@20.04/stable using ubuntu - add unit ubu2/0 to new machine 0 Deploy of bundle completed. $ juju status Model Controller Cloud/Region Version SLA Timestamp m1 lxd localhost/localhost 3.1.6.1 unsupported 11:41:06+01:00 App Version Status Scale Charm Channel Rev Exposed Message ubu2 waiting 0/1 ubuntu stable 24 no waiting for machine Unit Workload Agent Machine Public address Ports Message ubu2/0 waiting allocating 0 10.219.211.81 waiting for machine Machine State Address Inst id Base AZ Message 0 pending 10.219.211.81 juju-41adf7-0 ubuntu@20.04 Running ``` ``` $ juju deploy ./bundle3.yaml Located charm "ubuntu" in charm-hub, channel stable Executing changes: - upload charm ubuntu from charm-hub for base ubuntu@20.04/stable with architecture=amd64 - deploy application ubu3 from charm-hub on ubuntu@20.04/stable using ubuntu - add unit ubu3/0 to new machine 0 Deploy of bundle completed. $ juju status Model Controller Cloud/Region Version SLA Timestamp m2 lxd localhost/localhost 3.1.6.1 unsupported 11:41:29+01:00 App Version Status Scale Charm Channel Rev Exposed Message ubu3 waiting 0/1 ubuntu stable 24 no waiting for machine Unit Workload Agent Machine Public address Ports Message ubu3/0 waiting allocating 0 10.219.211.163 waiting for machine Machine State Address Inst id Base AZ Message 0 pending 10.219.211.163 juju-ff74a2-0 ubuntu@20.04 Running ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use bases throughout bundle deploy code, parsing immidiately from series
if necessary
Unfortunately, the least bad option was to keep bases as strings in
Change structs. This means they are marshalled nicely for Args as a string, and
omitempty is respected
Also, since GUIArgs output ordering is magic, we cannot replace series
with base. GUIArgs and the associated GetChanges facade endpoint should
be deprecated in favour of GetChangesMapArgs in Juju 4
Checklist
[ ] doc.go added or updated in changed packagesQA steps
Ensure unit tests and test_deploy_bundles integration tests pass
Documentation changes
Base needs to be added to bundle documentation once 3.3 is released