From 9001c8d16a260b3224f1554836cb2be8bfab15ca Mon Sep 17 00:00:00 2001 From: Jack Shaw Date: Mon, 2 Oct 2023 15:41:36 +0100 Subject: [PATCH] Ensure charm origins have a revision This will be useful for a future change, since we will now be able to use the charm origin to get a revision, rather than being force to parse it from the charm url The client always fills in a revision when deploying. Also the apiserver parses from the provided charm url is a revision is not provided. This is for compatibility with old clients. In future, we will likely enforce an origin is provided in validation steps Add upgrade steps which add a revision to charm origins if it doesn't have one. Parsed from the charm url Do the same for migration on import As a flyby, on migrations make sure to export -1 as the revision for charm origins without a revision, because 0 is a valid revision for local charms. --- .../facades/client/application/application.go | 10 ++ .../application/application_unit_test.go | 21 +++-- cmd/juju/application/utils/origin.go | 8 +- state/migration_export.go | 6 +- state/migration_import.go | 11 +++ state/migration_import_test.go | 24 +++++ state/upgrades.go | 40 ++++++++ state/upgrades_test.go | 92 +++++++++++++++++++ upgrades/backend.go | 5 + upgrades/operations.go | 2 + upgrades/steps_317.go | 20 ++++ upgrades/steps_317_test.go | 26 ++++++ upgrades/upgrade_test.go | 5 +- 13 files changed, 255 insertions(+), 15 deletions(-) create mode 100644 upgrades/steps_317.go create mode 100644 upgrades/steps_317_test.go diff --git a/apiserver/facades/client/application/application.go b/apiserver/facades/client/application/application.go index cab19a6d1821..67eb269a8e53 100644 --- a/apiserver/facades/client/application/application.go +++ b/apiserver/facades/client/application/application.go @@ -287,6 +287,16 @@ func (api *APIBase) Deploy(args params.ApplicationsDeploy) (params.ErrorResults, result.Results[i].Error = apiservererrors.ServerError(err) continue } + // Fill in the charm origin revision from the charm url if it's absent + if arg.CharmOrigin.Revision == nil { + curl, err := charm.ParseURL(arg.CharmURL) + if err != nil { + result.Results[i].Error = apiservererrors.ServerError(err) + continue + } + rev := curl.Revision + arg.CharmOrigin.Revision = &rev + } err := deployApplication( api.backend, api.model, diff --git a/apiserver/facades/client/application/application_unit_test.go b/apiserver/facades/client/application/application_unit_test.go index d7058cd8737f..babf532221fc 100644 --- a/apiserver/facades/client/application/application_unit_test.go +++ b/apiserver/facades/client/application/application_unit_test.go @@ -1405,7 +1405,7 @@ func (s *ApplicationSuite) TestDeployCharmOrigin(c *gc.C) { args := params.ApplicationsDeploy{ Applications: []params.ApplicationDeploy{{ ApplicationName: "foo", - CharmURL: "local:foo-0", + CharmURL: "local:foo-4", CharmOrigin: ¶ms.CharmOrigin{Source: "local", Base: params.Base{Name: "ubuntu", Channel: "20.04/stable"}}, NumUnits: 1, }, { @@ -1419,7 +1419,7 @@ func (s *ApplicationSuite) TestDeployCharmOrigin(c *gc.C) { NumUnits: 1, }, { ApplicationName: "hub", - CharmURL: "hub-0", + CharmURL: "hub-7", CharmOrigin: ¶ms.CharmOrigin{ Source: "charm-hub", Risk: "stable", @@ -1438,6 +1438,10 @@ func (s *ApplicationSuite) TestDeployCharmOrigin(c *gc.C) { c.Assert(s.deployParams["foo"].CharmOrigin.Source, gc.Equals, corecharm.Source("local")) c.Assert(s.deployParams["hub"].CharmOrigin.Source, gc.Equals, corecharm.Source("charm-hub")) + + // assert revision is filled in from the charm url + c.Assert(*s.deployParams["foo"].CharmOrigin.Revision, gc.Equals, 4) + c.Assert(*s.deployParams["hub"].CharmOrigin.Revision, gc.Equals, 7) } func createCharmOriginFromURL(curl *charm.URL) *params.CharmOrigin { @@ -1626,13 +1630,18 @@ func (s *ApplicationSuite) TestApplicationDeploymentRemovesPendingResourcesOnFai resourcesManager.EXPECT().RemovePendingAppResources("my-app", resources).Return(nil) s.backend.EXPECT().Resources().Return(resourcesManager) + rev := 8 results, err := s.api.Deploy(params.ApplicationsDeploy{ Applications: []params.ApplicationDeploy{{ - // CharmURL is missing to ensure deployApplication fails - // so that we can assert pending app resources are removed + // CharmURL is missing & revision included to ensure deployApplication + // fails sufficiently far through execution so that we can assert pending + // app resources are removed ApplicationName: "my-app", - CharmOrigin: validCharmOriginForTest(), - Resources: resources, + CharmOrigin: ¶ms.CharmOrigin{ + Source: "charm-hub", + Revision: &rev, + }, + Resources: resources, }}, }) c.Assert(err, jc.ErrorIsNil) diff --git a/cmd/juju/application/utils/origin.go b/cmd/juju/application/utils/origin.go index 4a08ed37dc80..27f3d9343a6f 100644 --- a/cmd/juju/application/utils/origin.go +++ b/cmd/juju/application/utils/origin.go @@ -55,19 +55,17 @@ func DeduceOrigin(url *charm.URL, channel charm.Channel, platform corecharm.Plat if channel.Branch != "" { branch = &channel.Branch } - var revision *int - if url.Revision != -1 { - revision = &url.Revision - } origin = commoncharm.Origin{ Source: commoncharm.OriginCharmHub, - Revision: revision, Risk: string(channel.Risk), Track: track, Branch: branch, Architecture: architecture, } } + if url.Revision != -1 { + origin.Revision = &url.Revision + } if platform.OS != "" && platform.Channel != "" { base, err := corebase.ParseBase(platform.OS, platform.Channel) if err != nil { diff --git a/state/migration_export.go b/state/migration_export.go index 61a7a2426c51..864825fe4284 100644 --- a/state/migration_export.go +++ b/state/migration_export.go @@ -2220,7 +2220,11 @@ func (e *exporter) getCharmOrigin(doc applicationDoc, defaultArch string) (descr origin := doc.CharmOrigin // If the channel is empty, then we fall back to the Revision. - var revision int + // Set default revision to -1. This is because a revision of 0 is + // a valid revision for local charms which we need to be able to + // from. On import, in the -1 case we grab the revision by parsing + // the charm url. + revision := -1 if rev := origin.Revision; rev != nil { revision = *rev } diff --git a/state/migration_import.go b/state/migration_import.go index b3fee01c895b..0c0229ce421b 100644 --- a/state/migration_import.go +++ b/state/migration_import.go @@ -1419,6 +1419,17 @@ func (i *importer) makeCharmOrigin(a description.Application) (*CharmOrigin, err co := a.CharmOrigin() rev := co.Revision() + // If revision is empty we export revision -1. In this case + // parse the revision from the url + // NOTE: We use <= 0 because some old versions of juju default + // revision to 0 when it's empty + if rev <= 0 { + curl, err := charm.ParseURL(a.CharmURL()) + if err != nil { + return nil, errors.Trace(err) + } + rev = curl.Revision + } var channel *Channel // Only charmhub charms will have a channel. Local charms diff --git a/state/migration_import_test.go b/state/migration_import_test.go index a2d4d93cfe27..ba7eee912daf 100644 --- a/state/migration_import_test.go +++ b/state/migration_import_test.go @@ -697,6 +697,30 @@ func (s *MigrationImportSuite) TestApplicationsUpdateSeriesNotPlatform(c *gc.C) c.Assert(obtainedOrigin.Platform.Channel, gc.Equals, "20.04/stable") } +func (s *MigrationImportSuite) TestApplicationCharmOriginFilledIn(c *gc.C) { + platform := &state.Platform{Architecture: corearch.DefaultArchitecture, OS: "ubuntu", Channel: "12.10/stable"} + f := factory.NewFactory(s.State, s.StatePool) + + testCharm := f.MakeCharm(c, &factory.CharmParams{Revision: "8"}) + _ = f.MakeApplication(c, &factory.ApplicationParams{ + Charm: testCharm, + Name: "mysql", + CharmOrigin: &state.CharmOrigin{ + Source: "charm-hub", + Type: "charm", + Channel: &state.Channel{ + Risk: "edge", + }, + Platform: platform, + }, + }) + + _, newSt := s.importModel(c, s.State) + newApp, err := newSt.Application("mysql") + c.Assert(err, jc.ErrorIsNil) + c.Assert(*newApp.CharmOrigin().Revision, gc.Equals, 8) +} + func (s *MigrationImportSuite) TestApplicationStatus(c *gc.C) { cons := constraints.MustParse("arch=amd64 mem=8G") platform := &state.Platform{Architecture: corearch.DefaultArchitecture, OS: "ubuntu", Channel: "12.10/stable"} diff --git a/state/upgrades.go b/state/upgrades.go index fb11a8a9957d..f95bcfc1afde 100644 --- a/state/upgrades.go +++ b/state/upgrades.go @@ -4,6 +4,7 @@ package state import ( + "github.com/juju/charm/v11" "github.com/juju/errors" "github.com/juju/mgo/v3/bson" "github.com/juju/mgo/v3/txn" @@ -253,3 +254,42 @@ func EnsureInitalRefCountForExternalSecretBackends(pool *StatePool) error { } return nil } + +func EnsureApplicationCharmOriginsHaveRevisions(pool *StatePool) error { + return errors.Trace(runForAllModelStates(pool, func(st *State) error { + allApps, err := st.AllApplications() + if err != nil { + return errors.Annotate(err, "loading applications") + } + + var ops []txn.Op + + for _, app := range allApps { + if app.CharmOrigin().Revision != nil { + continue + } + curlStr, _ := app.CharmURL() + if curlStr == nil { + logger.Warningf("Application %q has no charm url", app.Name()) + continue + } + curl, err := charm.ParseURL(*curlStr) + if err != nil { + return errors.Annotatef(err, "parsing charm url %q", *curlStr) + } + if curl.Revision == -1 { + logger.Warningf("Charm url %q has no revision, defaulting to -1", curl.String()) + } + ops = append(ops, txn.Op{ + C: applicationsC, + Id: app.doc.DocID, + Assert: bson.D{{"charm-origin.revision", bson.D{{"$exists", false}}}}, + Update: bson.D{{"$set", bson.D{{"charm-origin.revision", curl.Revision}}}}, + }) + } + if len(ops) > 0 { + return errors.Trace(st.runRawTransaction(ops)) + } + return nil + })) +} diff --git a/state/upgrades_test.go b/state/upgrades_test.go index e3b88635ad8a..c330a493004f 100644 --- a/state/upgrades_test.go +++ b/state/upgrades_test.go @@ -6,6 +6,7 @@ package state import ( "sort" + "github.com/juju/charm/v11" "github.com/juju/errors" "github.com/juju/mgo/v3" "github.com/juju/mgo/v3/bson" @@ -370,3 +371,94 @@ func (s *upgradesSuite) TestEnsureInitalRefCountForExternalSecretBackends(c *gc. expectedData.filter = bson.D{{"_id", bson.M{"$regex": "secretbackend#revisions#.*"}}} s.assertUpgradedData(c, EnsureInitalRefCountForExternalSecretBackends, expectedData) } + +func (s *upgradesSuite) TestEnsureApplicationCharmOriginsHaveRevisions(c *gc.C) { + ch := AddTestingCharm(c, s.state, "dummy") + rev := 8 + platform := Platform{OS: "ubuntu", Channel: "20.04"} + _ = addTestingApplication(c, addTestingApplicationParams{ + st: s.state, + name: "my-app", + ch: ch, + origin: &CharmOrigin{ + Source: charm.CharmHub.String(), + Platform: &platform, + Revision: &rev, + }, + numUnits: 1, + }) + _ = addTestingApplication(c, addTestingApplicationParams{ + st: s.state, + name: "my-local-app", + ch: ch, + origin: &CharmOrigin{ + Source: charm.Local.String(), + Platform: &platform, + }, + numUnits: 1, + }) + + apps, err := s.state.AllApplications() + c.Assert(err, jc.ErrorIsNil) + c.Assert(apps, gc.HasLen, 2) + c.Assert(*apps[0].CharmOrigin().Revision, gc.Equals, 8) + c.Assert(apps[1].CharmOrigin().Revision, gc.IsNil) + + chBson := bson.M{ + "_id": s.state.docID("my-app"), + "charm-origin": bson.M{ + "hash": "", + "id": "", + "platform": bson.M{"os": "ubuntu", "channel": "20.04"}, + "source": "ch", + "revision": 8, + }, + "charmmodifiedversion": 0, + "charmurl": "local:quantal/quantal-dummy-1", + "exposed": false, + "forcecharm": false, + "life": 0, + "metric-credentials": []uint8{}, + "minunits": 0, + "model-uuid": s.state.ModelUUID(), + "name": "my-app", + "passwordhash": "", + "provisioning-state": nil, + "relationcount": 0, + "scale": 0, + "subordinate": false, + "unitcount": 1, + } + localChBson := bson.M{ + "_id": s.state.docID("my-local-app"), + "charm-origin": bson.M{ + "hash": "", + "id": "", + "platform": bson.M{"os": "ubuntu", "channel": "20.04"}, + "source": "local", + "revision": 1, + }, + "charmmodifiedversion": 0, + "charmurl": "local:quantal/quantal-dummy-1", + "exposed": false, + "forcecharm": false, + "life": 0, + "metric-credentials": []uint8{}, + "minunits": 0, + "model-uuid": s.state.ModelUUID(), + "name": "my-local-app", + "passwordhash": "", + "provisioning-state": nil, + "relationcount": 0, + "scale": 0, + "subordinate": false, + "unitcount": 1, + } + expected := bsonMById{chBson, localChBson} + + appColl, closer := s.state.db().GetRawCollection(applicationsC) + defer closer() + + expectedData := upgradedData(appColl, expected) + s.assertUpgradedData(c, EnsureApplicationCharmOriginsHaveRevisions, expectedData) +} diff --git a/upgrades/backend.go b/upgrades/backend.go index 7b17a04b461d..3dc52a3dd0e1 100644 --- a/upgrades/backend.go +++ b/upgrades/backend.go @@ -14,6 +14,7 @@ type StateBackend interface { RemoveOrphanedSecretPermissions() error MigrateApplicationOpenedPortsToUnitScope() error EnsureInitalRefCountForExternalSecretBackends() error + EnsureApplicationCharmOriginsHaveRevisions() error } // Model is an interface providing access to the details of a model within the @@ -43,3 +44,7 @@ func (s stateBackend) MigrateApplicationOpenedPortsToUnitScope() error { func (s stateBackend) EnsureInitalRefCountForExternalSecretBackends() error { return state.EnsureInitalRefCountForExternalSecretBackends(s.pool) } + +func (s stateBackend) EnsureApplicationCharmOriginsHaveRevisions() error { + return state.EnsureApplicationCharmOriginsHaveRevisions(s.pool) +} diff --git a/upgrades/operations.go b/upgrades/operations.go index 6d90d230ef6a..24d834b69392 100644 --- a/upgrades/operations.go +++ b/upgrades/operations.go @@ -21,6 +21,7 @@ var stateUpgradeOperations = func() []Operation { steps := []Operation{ upgradeToVersion{version.MustParse("3.1.1"), stateStepsFor311()}, upgradeToVersion{version.MustParse("3.1.3"), stateStepsFor313()}, + upgradeToVersion{version.MustParse("3.1.7"), stateStepsFor317()}, } return steps } @@ -32,6 +33,7 @@ var upgradeOperations = func() []Operation { steps := []Operation{ upgradeToVersion{version.MustParse("3.1.1"), stepsFor311()}, upgradeToVersion{version.MustParse("3.1.3"), stepsFor313()}, + upgradeToVersion{version.MustParse("3.1.7"), stepsFor317()}, } return steps } diff --git a/upgrades/steps_317.go b/upgrades/steps_317.go new file mode 100644 index 000000000000..4a2101fc10fd --- /dev/null +++ b/upgrades/steps_317.go @@ -0,0 +1,20 @@ +// Copyright 2023 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package upgrades + +func stepsFor317() []Step { + return []Step{} +} + +func stateStepsFor317() []Step { + return []Step{ + &upgradeStep{ + description: "ensure application charm origins have revisions", + targets: []Target{DatabaseMaster}, + run: func(context Context) error { + return context.State().EnsureApplicationCharmOriginsHaveRevisions() + }, + }, + } +} diff --git a/upgrades/steps_317_test.go b/upgrades/steps_317_test.go new file mode 100644 index 000000000000..dbcc4e06ac0c --- /dev/null +++ b/upgrades/steps_317_test.go @@ -0,0 +1,26 @@ +// Copyright 2023 Canonical Ltd. +// Licensed under the AGPLv3, see LICENCE file for details. + +package upgrades_test + +import ( + jc "github.com/juju/testing/checkers" + "github.com/juju/version/v2" + gc "gopkg.in/check.v1" + + "github.com/juju/juju/testing" + "github.com/juju/juju/upgrades" +) + +var v317 = version.MustParse("3.1.7") + +type steps317Suite struct { + testing.BaseSuite +} + +var _ = gc.Suite(&steps317Suite{}) + +func (s *steps317Suite) TestEnsureApplicationCharmOriginsHaveRevisions(c *gc.C) { + step := findStateStep(c, v317, "ensure application charm origins have revisions") + c.Assert(step.Targets(), jc.DeepEquals, []upgrades.Target{upgrades.DatabaseMaster}) +} diff --git a/upgrades/upgrade_test.go b/upgrades/upgrade_test.go index 09f59104e92c..42384d7d2861 100644 --- a/upgrades/upgrade_test.go +++ b/upgrades/upgrade_test.go @@ -31,9 +31,6 @@ func TestPackage(t *stdtesting.T) { gc.TestingT(t) } -// Untl we add upgrade steps for 3.0, keep static analysis happy. -var _ = findStateStep - func findStateStep(c *gc.C, ver version.Number, description string) upgrades.Step { for _, op := range (*upgrades.StateUpgradeOperations)() { if op.TargetVersion() == ver { @@ -599,6 +596,7 @@ func (s *upgradeSuite) TestStateUpgradeOperationsVersions(c *gc.C) { c.Assert(versions, gc.DeepEquals, []string{ "3.1.1", "3.1.3", + "3.1.7", }) } @@ -607,6 +605,7 @@ func (s *upgradeSuite) TestUpgradeOperationsVersions(c *gc.C) { c.Assert(versions, gc.DeepEquals, []string{ "3.1.1", "3.1.3", + "3.1.7", }) }