From 50459f42cd33ba5c97bd0b845a2d871f058b86d3 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Wed, 13 Sep 2023 14:32:59 -0400 Subject: [PATCH 1/6] Remove deployedBase from refresher. No longer needed for compatibility with versions of juju prior to 2.9.x as the 3.x clients only will talk to 2.9.latest. Leave 1 source of trueth about the base deployed, in the charm origin which exists even for local charms. --- cmd/juju/application/refresh.go | 6 - cmd/juju/application/refresh_test.go | 119 +++++++++++------ cmd/juju/application/refresher/refresher.go | 57 +++----- .../application/refresher/refresher_test.go | 124 +++++++++--------- 4 files changed, 161 insertions(+), 145 deletions(-) diff --git a/cmd/juju/application/refresh.go b/cmd/juju/application/refresh.go index 64e51391d40..c49b811d345 100644 --- a/cmd/juju/application/refresh.go +++ b/cmd/juju/application/refresh.go @@ -33,7 +33,6 @@ import ( "github.com/juju/juju/cmd/juju/block" "github.com/juju/juju/cmd/juju/common" "github.com/juju/juju/cmd/modelcmd" - corebase "github.com/juju/juju/core/base" corecharm "github.com/juju/juju/core/charm" "github.com/juju/juju/environs/config" "github.com/juju/juju/rpc/params" @@ -408,17 +407,12 @@ func (c *refreshCommand) Run(ctx *cmd.Context) error { } } - chBase, err := corebase.ParseBase(applicationInfo.Base.Name, applicationInfo.Base.Channel) - if err != nil { - return errors.Trace(err) - } cfg := refresher.RefresherConfig{ ApplicationName: c.ApplicationName, CharmURL: oldURL, CharmOrigin: oldOrigin.CoreCharmOrigin(), CharmRef: newRef, Channel: c.Channel, - DeployedBase: chBase, Force: c.Force, ForceBase: c.ForceBase, Switch: c.SwitchURL != "", diff --git a/cmd/juju/application/refresh_test.go b/cmd/juju/application/refresh_test.go index 16ff913f774..de786c58cc0 100644 --- a/cmd/juju/application/refresh_test.go +++ b/cmd/juju/application/refresh_test.go @@ -71,6 +71,9 @@ type BaseRefreshSuite struct { resourceLister mockResourceLister spacesClient mockSpacesClient downloadBundleClient mockDownloadBundleClient + + testPlatform corecharm.Platform + testBase corebase.Base } func (s *BaseRefreshSuite) runRefresh(c *gc.C, args ...string) (*cmd.Context, error) { @@ -98,6 +101,9 @@ func (s *BaseRefreshSuite) setup(c *gc.C, currentCharmURL, latestCharmURL *charm cookieFile := filepath.Join(c.MkDir(), "cookies") s.PatchEnvironment("JUJU_COOKIEFILE", cookieFile) + s.testPlatform = corecharm.MustParsePlatform("amd64/ubuntu/22.04") + s.testBase = corebase.MakeDefaultBase("ubuntu", "22.04") + s.deployResources = func( applicationID string, chID resources.CharmID, @@ -150,9 +156,11 @@ func (s *BaseRefreshSuite) setup(c *gc.C, currentCharmURL, latestCharmURL *charm "": network.AlphaSpaceName, }, charmOrigin: commoncharm.Origin{ - ID: "testing", - Source: schemaToOriginScource(currentCharmURL.Schema), - Risk: "stable", + ID: "testing", + Source: schemaToOriginScource(currentCharmURL.Schema), + Risk: "stable", + Architecture: arch.DefaultArchitecture, + Base: s.testBase, }, } s.modelConfigGetter = newMockModelConfigGetter() @@ -249,9 +257,11 @@ func (s *RefreshSuite) TestStorageConstraints(c *gc.C) { CharmID: application.CharmID{ URL: s.resolvedCharmURL, Origin: commoncharm.Origin{ - ID: "testing", - Source: "charm-hub", - Risk: "stable", + ID: "testing", + Source: "charm-hub", + Risk: "stable", + Architecture: arch.DefaultArchitecture, + Base: s.testBase, }, }, StorageConstraints: map[string]storage.Constraints{ @@ -277,9 +287,11 @@ func (s *RefreshSuite) TestConfigSettings(c *gc.C) { CharmID: application.CharmID{ URL: s.resolvedCharmURL, Origin: commoncharm.Origin{ - ID: "testing", - Source: "charm-hub", - Risk: "stable", + ID: "testing", + Source: "charm-hub", + Risk: "stable", + Architecture: arch.DefaultArchitecture, + Base: s.testBase, }, }, ConfigSettingsYAML: "foo:{}", @@ -298,9 +310,11 @@ func (s *RefreshSuite) TestConfigSettingsWithTrust(c *gc.C) { CharmID: application.CharmID{ URL: s.resolvedCharmURL, Origin: commoncharm.Origin{ - ID: "testing", - Source: "charm-hub", - Risk: "stable", + ID: "testing", + Source: "charm-hub", + Risk: "stable", + Architecture: arch.DefaultArchitecture, + Base: s.testBase, }, }, ConfigSettings: map[string]string{"trust": "true", "foo": "bar"}, @@ -318,9 +332,11 @@ func (s *RefreshSuite) TestConfigSettingsWithTrustFalse(c *gc.C) { CharmID: application.CharmID{ URL: s.resolvedCharmURL, Origin: commoncharm.Origin{ - ID: "testing", - Source: "charm-hub", - Risk: "stable", + ID: "testing", + Source: "charm-hub", + Risk: "stable", + Architecture: arch.DefaultArchitecture, + Base: s.testBase, }, }, ConfigSettings: map[string]string{"trust": "false", "foo": "bar"}, @@ -343,9 +359,11 @@ func (s *RefreshSuite) TestConfigSettingsWithKeyValuesAndFile(c *gc.C) { CharmID: application.CharmID{ URL: s.resolvedCharmURL, Origin: commoncharm.Origin{ - ID: "testing", - Source: "charm-hub", - Risk: "stable", + ID: "testing", + Source: "charm-hub", + Risk: "stable", + Architecture: arch.DefaultArchitecture, + Base: s.testBase, }, }, ConfigSettingsYAML: "foo:{}", @@ -389,9 +407,11 @@ func (s *RefreshSuite) testUpgradeWithBind(c *gc.C, expectedBindings map[string] CharmID: application.CharmID{ URL: s.resolvedCharmURL, Origin: commoncharm.Origin{ - ID: "testing", - Source: "charm-hub", - Risk: "stable", + ID: "testing", + Source: "charm-hub", + Risk: "stable", + Architecture: arch.DefaultArchitecture, + Base: s.testBase, }, }, ConfigSettings: map[string]string{}, @@ -627,7 +647,8 @@ func (s *RefreshSuite) TestUpgradeWithChannel(c *gc.C) { }) origin.ID = "testing" origin.Revision = (*int)(nil) - origin.Architecture = "" + origin.Architecture = arch.DefaultArchitecture + origin.Base = s.testBase s.charmAdder.CheckCall(c, 0, "AddCharm", s.resolvedCharmURL, origin, false) s.charmAPIClient.CheckCallNames(c, "GetCharmURLOrigin", "Get", "SetCharm") s.charmAPIClient.CheckCall(c, 2, "SetCharm", model.GenerationMaster, application.SetCharmConfig{ @@ -635,9 +656,11 @@ func (s *RefreshSuite) TestUpgradeWithChannel(c *gc.C) { CharmID: application.CharmID{ URL: s.resolvedCharmURL, Origin: commoncharm.Origin{ - ID: "testing", - Source: "charm-hub", - Risk: "beta", + ID: "testing", + Source: "charm-hub", + Risk: "beta", + Architecture: arch.DefaultArchitecture, + Base: s.testBase, }, }, ConfigSettings: map[string]string{}, @@ -660,9 +683,11 @@ func (s *RefreshSuite) TestUpgradeWithChannelNoNewCharmURL(c *gc.C) { CharmID: application.CharmID{ URL: s.resolvedCharmURL, Origin: commoncharm.Origin{ - ID: "testing", - Source: "charm-hub", - Risk: "beta", + ID: "testing", + Source: "charm-hub", + Risk: "beta", + Architecture: arch.DefaultArchitecture, + Base: s.testBase, }, }, ConfigSettings: map[string]string{}, @@ -676,10 +701,9 @@ func (s *RefreshSuite) TestRefreshShouldRespectDeployedChannelByDefault(c *gc.C) c.Assert(err, jc.ErrorIsNil) s.charmAdder.CheckCallNames(c, "AddCharm") - origin, _ := utils.DeduceOrigin(s.resolvedCharmURL, charm.Channel{Risk: charm.Beta}, corecharm.Platform{}) + origin, _ := utils.DeduceOrigin(s.resolvedCharmURL, charm.Channel{Risk: charm.Beta}, s.testPlatform) origin.ID = "testing" origin.Revision = (*int)(nil) - origin.Architecture = "" s.charmAdder.CheckCall(c, 0, "AddCharm", s.resolvedCharmURL, origin, false) s.charmAPIClient.CheckCallNames(c, "GetCharmURLOrigin", "Get", "SetCharm") s.charmAPIClient.CheckCall(c, 2, "SetCharm", model.GenerationMaster, application.SetCharmConfig{ @@ -687,9 +711,11 @@ func (s *RefreshSuite) TestRefreshShouldRespectDeployedChannelByDefault(c *gc.C) CharmID: application.CharmID{ URL: s.resolvedCharmURL, Origin: commoncharm.Origin{ - ID: "testing", - Source: "charm-hub", - Risk: "beta", + ID: "testing", + Source: "charm-hub", + Risk: "beta", + Architecture: arch.DefaultArchitecture, + Base: s.testBase, }, }, ConfigSettings: map[string]string{}, @@ -713,7 +739,7 @@ func (s *RefreshSuite) TestSwitch(c *gc.C) { s.charmClient.CheckCallNames(c, "CharmInfo", "CharmInfo") s.charmClient.CheckCall(c, 0, "CharmInfo", s.resolvedCharmURL.String()) s.charmAdder.CheckCallNames(c, "CheckCharmPlacement", "AddCharm") - origin, _ := utils.DeduceOrigin(s.resolvedCharmURL, charm.Channel{Risk: charm.Stable}, corecharm.Platform{}) + origin, _ := utils.DeduceOrigin(s.resolvedCharmURL, charm.Channel{Risk: charm.Stable}, s.testPlatform) parsedSwitchUrl, err := charm.ParseURL("ch:trusty/anotherriak") c.Assert(err, jc.ErrorIsNil) @@ -729,6 +755,7 @@ func (s *RefreshSuite) TestSwitch(c *gc.C) { Source: "charm-hub", Architecture: arch.DefaultArchitecture, Risk: "stable", + Base: s.testBase, }, }, ConfigSettings: map[string]string{}, @@ -967,9 +994,11 @@ func (s *RefreshSuite) TestUpgradeSameVersionWithResourceUpload(c *gc.C) { CharmID: application.CharmID{ URL: s.resolvedCharmURL, Origin: commoncharm.Origin{ - ID: "testing", - Source: "charm-hub", - Risk: "stable", + ID: "testing", + Source: "charm-hub", + Risk: "stable", + Architecture: arch.DefaultArchitecture, + Base: s.testBase, }, }, ConfigSettings: map[string]string{}, @@ -1027,9 +1056,11 @@ func (s *RefreshCharmHubSuite) TestUpgradeResourceRevision(c *gc.C) { s.CheckCall(c, 9, "DeployResources", "foo", resources.CharmID{ URL: s.resolvedCharmURL, Origin: commoncharm.Origin{ - ID: "testing", - Source: "charm-hub", - Risk: "stable"}}, + ID: "testing", + Source: "charm-hub", + Risk: "stable", + Architecture: arch.DefaultArchitecture, + Base: s.testBase}}, map[string]string(nil), map[string]charmresource.Meta{"bar": {Name: "bar", Type: charmresource.TypeFile}}, ) @@ -1068,9 +1099,11 @@ func (s *RefreshCharmHubSuite) TestUpgradeResourceRevisionSupplied(c *gc.C) { s.CheckCall(c, 9, "DeployResources", "foo", resources.CharmID{ URL: s.resolvedCharmURL, Origin: commoncharm.Origin{ - ID: "testing", - Source: "charm-hub", - Risk: "stable"}}, + ID: "testing", + Source: "charm-hub", + Risk: "stable", + Architecture: arch.DefaultArchitecture, + Base: s.testBase}}, map[string]string{"bar": "3"}, map[string]charmresource.Meta{"bar": {Name: "bar", Type: charmresource.TypeFile}}, ) diff --git a/cmd/juju/application/refresher/refresher.go b/cmd/juju/application/refresher/refresher.go index 07220177a4d..75193fd4492 100644 --- a/cmd/juju/application/refresher/refresher.go +++ b/cmd/juju/application/refresher/refresher.go @@ -40,7 +40,6 @@ type RefresherConfig struct { CharmOrigin corecharm.Origin CharmRef string Channel charm.Channel - DeployedBase corebase.Base Force bool ForceBase bool Switch bool @@ -107,14 +106,13 @@ func (d *factory) Run(cfg RefresherConfig) (*CharmID, error) { func (d *factory) maybeReadLocal(charmAdder store.CharmAdder, charmRepo CharmRepository) func(RefresherConfig) (Refresher, error) { return func(cfg RefresherConfig) (Refresher, error) { return &localCharmRefresher{ - charmAdder: charmAdder, - charmOrigin: cfg.CharmOrigin, - charmRepo: charmRepo, - charmURL: cfg.CharmURL, - charmRef: cfg.CharmRef, - deployedBase: cfg.DeployedBase, - force: cfg.Force, - forceBase: cfg.ForceBase, + charmAdder: charmAdder, + charmOrigin: cfg.CharmOrigin, + charmRepo: charmRepo, + charmURL: cfg.CharmURL, + charmRef: cfg.CharmRef, + force: cfg.Force, + forceBase: cfg.ForceBase, }, nil } } @@ -137,7 +135,6 @@ func (d *factory) maybeCharmHub(charmAdder store.CharmAdder, charmResolver Charm charmOrigin: cfg.CharmOrigin, charmRef: cfg.CharmRef, channel: cfg.Channel, - deployedBase: cfg.DeployedBase, switchCharm: cfg.Switch, force: cfg.Force, forceBase: cfg.ForceBase, @@ -148,14 +145,13 @@ func (d *factory) maybeCharmHub(charmAdder store.CharmAdder, charmResolver Charm } type localCharmRefresher struct { - charmAdder store.CharmAdder - charmRepo CharmRepository - charmOrigin corecharm.Origin - charmURL *charm.URL - charmRef string - deployedBase corebase.Base - force bool - forceBase bool + charmAdder store.CharmAdder + charmRepo CharmRepository + charmOrigin corecharm.Origin + charmURL *charm.URL + charmRef string + force bool + forceBase bool } // Allowed will attempt to check if a local charm is allowed to be refreshed. @@ -168,12 +164,9 @@ func (d *localCharmRefresher) Allowed(_ RefresherConfig) (bool, error) { // Refresh a given local charm. // Bundles are not supported as there is no physical representation in Juju. func (d *localCharmRefresher) Refresh() (*CharmID, error) { - var deployedSeries string - if !d.deployedBase.Channel.Empty() { - var err error - if deployedSeries, err = corebase.GetSeriesFromBase(d.deployedBase); err != nil { - return nil, errors.Trace(err) - } + deployedSeries, err := corebase.GetSeriesFromChannel(d.charmOrigin.Platform.OS, d.charmOrigin.Platform.Channel) + if err != nil { + return nil, errors.Trace(err) } ch, newURL, err := d.charmRepo.NewCharmAtPathForceSeries(d.charmRef, deployedSeries, d.forceBase) if err == nil { @@ -236,7 +229,6 @@ type baseRefresher struct { charmOrigin corecharm.Origin charmRef string channel charm.Channel - deployedBase corebase.Base switchCharm bool force bool forceBase bool @@ -268,12 +260,9 @@ func (r baseRefresher) ResolveCharm() (*charm.URL, commoncharm.Origin, error) { if err != nil { return nil, commoncharm.Origin{}, errors.Trace(err) } - - var deployedSeries string - if !r.deployedBase.Channel.Empty() { - if deployedSeries, err = corebase.GetSeriesFromBase(r.deployedBase); err != nil { - return nil, commoncharm.Origin{}, errors.Trace(err) - } + deployedSeries, err := corebase.GetSeriesFromChannel(r.charmOrigin.Platform.OS, r.charmOrigin.Platform.Channel) + if err != nil { + return nil, commoncharm.Origin{}, errors.Trace(err) } _, seriesSupportedErr := corecharm.SeriesForCharm(deployedSeries, supportedSeries) if !r.forceBase && deployedSeries != "" && newURL.Series == "" && seriesSupportedErr != nil { @@ -347,7 +336,7 @@ type charmHubRefresher struct { baseRefresher } -// Allowed will attempt to check if the charm store is allowed to refresh. +// Allowed will attempt to check if the charm is allowed to refresh. // Depending on the charm url, will then determine if that's true or not. func (r *charmHubRefresher) Allowed(cfg RefresherConfig) (bool, error) { path, err := charm.EnsureSchema(cfg.CharmRef, charm.CharmHub) @@ -395,10 +384,6 @@ func (r *charmHubRefresher) Refresh() (*CharmID, error) { return nil, errors.Trace(err) } - if !r.deployedBase.Channel.Empty() { - origin.Base = r.deployedBase - } - curl, actualOrigin, err := store.AddCharmFromURL(r.charmAdder, newURL, origin, r.force) if err != nil { return nil, errors.Trace(err) diff --git a/cmd/juju/application/refresher/refresher_test.go b/cmd/juju/application/refresher/refresher_test.go index bbaeec23a15..3c8562a7679 100644 --- a/cmd/juju/application/refresher/refresher_test.go +++ b/cmd/juju/application/refresher/refresher_test.go @@ -160,7 +160,10 @@ func (s *baseRefresherSuite) TestResolveCharm(c *gc.C) { ref := "meshuggah" curl := charm.MustParseURL(ref) newCurl := charm.MustParseURL(fmt.Sprintf("%s-1", ref)) - origin := commoncharm.Origin{} + origin := commoncharm.Origin{ + Architecture: "amd64", + Base: corebase.MakeDefaultBase("ubuntu", "22.04"), + } charmResolver := NewMockCharmResolver(ctrl) charmResolver.EXPECT().ResolveCharm(curl, origin, false).Return(newCurl, origin, []string{}, nil) @@ -169,13 +172,14 @@ func (s *baseRefresherSuite) TestResolveCharm(c *gc.C) { charmRef: "meshuggah", charmURL: charm.MustParseURL("meshuggah"), charmResolver: charmResolver, + charmOrigin: corecharm.Origin{Platform: corecharm.MustParsePlatform("amd64/ubuntu/22.04")}, resolveOriginFn: charmHubOriginResolver, logger: fakeLogger{}, } - url, origin, err := refresher.ResolveCharm() + url, obtainedOrigin, err := refresher.ResolveCharm() c.Assert(err, jc.ErrorIsNil) c.Assert(url, gc.DeepEquals, charm.MustParseURL("ch:meshuggah-1")) - c.Assert(origin, gc.DeepEquals, commoncharm.Origin{}) + c.Assert(obtainedOrigin, gc.DeepEquals, origin) } func (s *baseRefresherSuite) TestResolveCharmWithSeriesError(c *gc.C) { @@ -185,21 +189,26 @@ func (s *baseRefresherSuite) TestResolveCharmWithSeriesError(c *gc.C) { ref := "meshuggah" curl := charm.MustParseURL(ref) newCurl := charm.MustParseURL(fmt.Sprintf("%s-1", ref)) - origin := commoncharm.Origin{} + origin := commoncharm.Origin{ + Architecture: "amd64", + Base: corebase.MakeDefaultBase("ubuntu", "22.04"), + } charmResolver := NewMockCharmResolver(ctrl) charmResolver.EXPECT().ResolveCharm(curl, origin, false).Return(newCurl, origin, []string{"focal"}, nil) refresher := baseRefresher{ - charmRef: "meshuggah", - deployedBase: corebase.MakeDefaultBase("ubuntu", "18.04"), + charmRef: "meshuggah", + charmOrigin: corecharm.Origin{ + Platform: corecharm.MustParsePlatform("amd64/ubuntu/22.04"), + }, charmURL: charm.MustParseURL("meshuggah"), charmResolver: charmResolver, resolveOriginFn: charmHubOriginResolver, logger: fakeLogger{}, } _, _, err := refresher.ResolveCharm() - c.Assert(err, gc.ErrorMatches, `cannot upgrade from single series "bionic" charm to a charm supporting \["focal"\]. Use --force-series to override.`) + c.Assert(err, gc.ErrorMatches, `cannot upgrade from single series "jammy" charm to a charm supporting \["focal"\]. Use --force-series to override.`) } func (s *baseRefresherSuite) TestResolveCharmWithNoCharmURL(c *gc.C) { @@ -209,7 +218,10 @@ func (s *baseRefresherSuite) TestResolveCharmWithNoCharmURL(c *gc.C) { ref := "meshuggah" curl := charm.MustParseURL(ref) newCurl := charm.MustParseURL(fmt.Sprintf("%s-1", ref)) - origin := commoncharm.Origin{} + origin := commoncharm.Origin{ + Architecture: "amd64", + Base: corebase.MakeDefaultBase("ubuntu", "22.04"), + } charmResolver := NewMockCharmResolver(ctrl) charmResolver.EXPECT().ResolveCharm(curl, origin, false).Return(newCurl, origin, []string{}, nil) @@ -217,6 +229,7 @@ func (s *baseRefresherSuite) TestResolveCharmWithNoCharmURL(c *gc.C) { refresher := baseRefresher{ charmRef: "meshuggah", charmResolver: charmResolver, + charmOrigin: corecharm.Origin{Platform: corecharm.MustParsePlatform("amd64/ubuntu/22.04")}, resolveOriginFn: charmHubOriginResolver, logger: fakeLogger{}, } @@ -244,9 +257,9 @@ func (s *localCharmRefresherSuite) TestRefresh(c *gc.C) { charmAdder.EXPECT().AddLocalCharm(curl, ch, false).Return(curl, nil) charmRepo := NewMockCharmRepository(ctrl) - charmRepo.EXPECT().NewCharmAtPathForceSeries(ref, "", false).Return(ch, curl, nil) + charmRepo.EXPECT().NewCharmAtPathForceSeries(ref, "jammy", false).Return(ch, curl, nil) - cfg := basicRefresherConfig(curl, ref) + cfg := refresherConfigWithOrigin(curl, ref, corecharm.MustParsePlatform("amd64/ubuntu/22.04")) refresher := (&factory{}).maybeReadLocal(charmAdder, charmRepo) task, err := refresher(cfg) @@ -267,9 +280,9 @@ func (s *localCharmRefresherSuite) TestRefreshBecomesExhausted(c *gc.C) { charmAdder := NewMockCharmAdder(ctrl) charmRepo := NewMockCharmRepository(ctrl) - charmRepo.EXPECT().NewCharmAtPathForceSeries(ref, "", false).Return(nil, nil, os.ErrNotExist) + charmRepo.EXPECT().NewCharmAtPathForceSeries(ref, "jammy", false).Return(nil, nil, os.ErrNotExist) - cfg := basicRefresherConfig(curl, ref) + cfg := refresherConfigWithOrigin(curl, ref, corecharm.MustParsePlatform("amd64/ubuntu/22.04")) refresher := (&factory{}).maybeReadLocal(charmAdder, charmRepo) task, err := refresher(cfg) @@ -288,9 +301,9 @@ func (s *localCharmRefresherSuite) TestRefreshDoesNotFindLocal(c *gc.C) { charmAdder := NewMockCharmAdder(ctrl) charmRepo := NewMockCharmRepository(ctrl) - charmRepo.EXPECT().NewCharmAtPathForceSeries(ref, "", false).Return(nil, nil, errors.NotFoundf("fail")) + charmRepo.EXPECT().NewCharmAtPathForceSeries(ref, "jammy", false).Return(nil, nil, errors.NotFoundf("fail")) - cfg := basicRefresherConfig(curl, ref) + cfg := refresherConfigWithOrigin(curl, ref, corecharm.MustParsePlatform("amd64/ubuntu/22.04")) refresher := (&factory{}).maybeReadLocal(charmAdder, charmRepo) task, err := refresher(cfg) @@ -312,8 +325,9 @@ func (s *charmHubCharmRefresherSuite) TestRefresh(c *gc.C) { curl := charm.MustParseURL(ref) newCurl := charm.MustParseURL(fmt.Sprintf("%s-1", ref)) origin := commoncharm.Origin{ - Source: commoncharm.OriginCharmHub, - Base: corebase.MakeDefaultBase("ubuntu", "18.04"), + Source: commoncharm.OriginCharmHub, + Architecture: "amd64", + Base: corebase.MakeDefaultBase("ubuntu", "22.04"), } actualOrigin := origin actualOrigin.ID = "charmid" @@ -324,9 +338,7 @@ func (s *charmHubCharmRefresherSuite) TestRefresh(c *gc.C) { charmResolver := NewMockCharmResolver(ctrl) charmResolver.EXPECT().ResolveCharm(curl, origin, false).Return(newCurl, origin, []string{}, nil) - base := corebase.MakeDefaultBase("ubuntu", "18.04") - cfg := refresherConfigWithOrigin(curl, ref, base) - cfg.DeployedBase = base + cfg := refresherConfigWithOrigin(curl, ref, corecharm.MustParsePlatform("amd64/ubuntu/22.04")) refresher := (&factory{}).maybeCharmHub(charmAdder, charmResolver) task, err := refresher(cfg) @@ -348,8 +360,9 @@ func (s *charmHubCharmRefresherSuite) TestRefreshWithNoOrigin(c *gc.C) { curl := charm.MustParseURL(ref) newCurl := charm.MustParseURL(fmt.Sprintf("%s-1", ref)) origin := commoncharm.Origin{ - Source: commoncharm.OriginCharmHub, - Base: corebase.MakeDefaultBase("ubuntu", "18.04"), + Source: commoncharm.OriginCharmHub, + Architecture: "amd64", + Base: corebase.MakeDefaultBase("ubuntu", "22.04"), } charmAdder := NewMockCharmAdder(ctrl) @@ -358,9 +371,7 @@ func (s *charmHubCharmRefresherSuite) TestRefreshWithNoOrigin(c *gc.C) { charmResolver := NewMockCharmResolver(ctrl) charmResolver.EXPECT().ResolveCharm(curl, origin, false).Return(newCurl, origin, []string{}, nil) - base := corebase.MakeDefaultBase("ubuntu", "18.04") - cfg := refresherConfigWithOrigin(curl, ref, base) - cfg.DeployedBase = base + cfg := refresherConfigWithOrigin(curl, ref, corecharm.MustParsePlatform("amd64/ubuntu/22.04")) refresher := (&factory{}).maybeCharmHub(charmAdder, charmResolver) task, err := refresher(cfg) @@ -381,7 +392,9 @@ func (s *charmHubCharmRefresherSuite) TestRefreshWithNoUpdates(c *gc.C) { ref := "ch:meshuggah" curl := charm.MustParseURL(ref) origin := commoncharm.Origin{ - Source: commoncharm.OriginCharmHub, + Source: commoncharm.OriginCharmHub, + Architecture: "amd64", + Base: corebase.MakeDefaultBase("ubuntu", "22.04"), } charmAdder := NewMockCharmAdder(ctrl) @@ -389,7 +402,7 @@ func (s *charmHubCharmRefresherSuite) TestRefreshWithNoUpdates(c *gc.C) { charmResolver := NewMockCharmResolver(ctrl) charmResolver.EXPECT().ResolveCharm(curl, origin, false).Return(curl, origin, []string{}, nil) - cfg := refresherConfigWithOrigin(curl, ref, corebase.Base{}) + cfg := refresherConfigWithOrigin(curl, ref, corecharm.MustParsePlatform("amd64/ubuntu/22.04")) refresher := (&factory{}).maybeCharmHub(charmAdder, charmResolver) task, err := refresher(cfg) @@ -406,7 +419,9 @@ func (s *charmHubCharmRefresherSuite) TestRefreshWithARevision(c *gc.C) { ref := "ch:meshuggah-1" curl := charm.MustParseURL(ref) origin := commoncharm.Origin{ - Source: commoncharm.OriginCharmHub, + Source: commoncharm.OriginCharmHub, + Architecture: "amd64", + Base: corebase.MakeDefaultBase("ubuntu", "22.04"), } charmAdder := NewMockCharmAdder(ctrl) @@ -414,7 +429,7 @@ func (s *charmHubCharmRefresherSuite) TestRefreshWithARevision(c *gc.C) { charmResolver := NewMockCharmResolver(ctrl) charmResolver.EXPECT().ResolveCharm(curl, origin, false).Return(curl, origin, []string{}, nil) - cfg := refresherConfigWithOrigin(curl, ref, corebase.Base{}) + cfg := refresherConfigWithOrigin(curl, ref, corecharm.MustParsePlatform("amd64/ubuntu/22.04")) refresher := (&factory{}).maybeCharmHub(charmAdder, charmResolver) task, err := refresher(cfg) @@ -431,8 +446,10 @@ func (s *charmHubCharmRefresherSuite) TestRefreshWithOriginChannel(c *gc.C) { ref := "ch:meshuggah-1" curl := charm.MustParseURL(ref) origin := commoncharm.Origin{ - Source: commoncharm.OriginCharmHub, - Risk: "beta", + Source: commoncharm.OriginCharmHub, + Risk: "beta", + Architecture: "amd64", + Base: corebase.MakeDefaultBase("ubuntu", "22.04"), } charmAdder := NewMockCharmAdder(ctrl) @@ -440,13 +457,11 @@ func (s *charmHubCharmRefresherSuite) TestRefreshWithOriginChannel(c *gc.C) { charmResolver := NewMockCharmResolver(ctrl) charmResolver.EXPECT().ResolveCharm(curl, origin, false).Return(curl, origin, []string{}, nil) - cfg := basicRefresherConfig(curl, ref) - cfg.CharmOrigin = corecharm.Origin{ - Source: corecharm.CharmHub, - Channel: &charm.Channel{ - Risk: charm.Edge, - }, + cfg := refresherConfigWithOrigin(curl, ref, corecharm.MustParsePlatform("amd64/ubuntu/22.04")) + cfg.CharmOrigin.Channel = &charm.Channel{ + Risk: charm.Edge, } + cfg.CharmOrigin.Source = corecharm.CharmHub cfg.Channel = charm.Channel{ Risk: charm.Beta, } @@ -470,6 +485,7 @@ func (s *charmHubCharmRefresherSuite) TestRefreshWithCharmSwitch(c *gc.C) { Risk: "beta", Architecture: "amd64", Revision: &curl.Revision, + Base: corebase.MakeDefaultBase("ubuntu", "22.04"), } charmAdder := NewMockCharmAdder(ctrl) @@ -477,14 +493,12 @@ func (s *charmHubCharmRefresherSuite) TestRefreshWithCharmSwitch(c *gc.C) { charmResolver := NewMockCharmResolver(ctrl) charmResolver.EXPECT().ResolveCharm(curl, origin, true).Return(curl, origin, []string{}, nil) - cfg := basicRefresherConfig(curl, ref) + cfg := refresherConfigWithOrigin(curl, ref, corecharm.MustParsePlatform("amd64/ubuntu/22.04")) cfg.Switch = true // flag this as a refresh --switch operation - cfg.CharmOrigin = corecharm.Origin{ - Source: corecharm.CharmHub, - Channel: &charm.Channel{ - Risk: charm.Edge, - }, + cfg.CharmOrigin.Channel = &charm.Channel{ + Risk: charm.Edge, } + cfg.CharmOrigin.Source = corecharm.CharmHub cfg.Channel = charm.Channel{ Risk: charm.Beta, } @@ -507,8 +521,7 @@ func (s *charmHubCharmRefresherSuite) TestAllowed(c *gc.C) { charmAdder := NewMockCharmAdder(ctrl) charmResolver := NewMockCharmResolver(ctrl) - cfg := refresherConfigWithOrigin(curl, ref, corebase.Base{}) - cfg.DeployedBase = corebase.MakeDefaultBase("ubuntu", "18.04") + cfg := refresherConfigWithOrigin(curl, ref, corecharm.MustParsePlatform("amd64/ubuntu/22.04")) refresher := (&factory{}).maybeCharmHub(charmAdder, charmResolver) task, err := refresher(cfg) @@ -531,8 +544,7 @@ func (s *charmHubCharmRefresherSuite) TestAllowedWithSwitch(c *gc.C) { charmResolver := NewMockCharmResolver(ctrl) - cfg := refresherConfigWithOrigin(curl, ref, corebase.Base{}) - cfg.DeployedBase = corebase.MakeDefaultBase("ubuntu", "18.04") + cfg := refresherConfigWithOrigin(curl, ref, corecharm.MustParsePlatform("amd64/ubuntu/22.04")) cfg.Switch = true refresher := (&factory{}).maybeCharmHub(charmAdder, charmResolver) @@ -556,8 +568,7 @@ func (s *charmHubCharmRefresherSuite) TestAllowedError(c *gc.C) { charmResolver := NewMockCharmResolver(ctrl) - cfg := refresherConfigWithOrigin(curl, ref, corebase.Base{}) - cfg.DeployedBase = corebase.MakeDefaultBase("ubuntu", "18.04") + cfg := refresherConfigWithOrigin(curl, ref, corecharm.MustParsePlatform("amd64/ubuntu/22.04")) cfg.Switch = true refresher := (&factory{}).maybeCharmHub(charmAdder, charmResolver) @@ -629,24 +640,17 @@ func (s *charmHubCharmRefresherSuite) TestCharmHubResolveOriginEmptyTrackEmptyCh c.Assert(result, gc.DeepEquals, coreOrigin) } -func basicRefresherConfig(curl *charm.URL, ref string) RefresherConfig { - return RefresherConfig{ +func refresherConfigWithOrigin(curl *charm.URL, ref string, platform corecharm.Platform) RefresherConfig { + rc := RefresherConfig{ ApplicationName: "winnie", CharmURL: curl, CharmRef: ref, Logger: &fakeLogger{}, } -} - -func refresherConfigWithOrigin(curl *charm.URL, ref string, base corebase.Base) RefresherConfig { - rc := basicRefresherConfig(curl, ref) rc.CharmOrigin = corecharm.Origin{ - Source: corecharm.CharmHub, - Channel: &charm.Channel{}, - Platform: corecharm.Platform{ - OS: base.OS, - Channel: base.Channel.String(), - }, + Source: corecharm.CharmHub, + Channel: &charm.Channel{}, + Platform: platform, } return rc } From 1086f9a0b05527a77596cd124fd3129f75042879 Mon Sep 17 00:00:00 2001 From: Nicolas Vinuesa Date: Mon, 4 Sep 2023 18:20:42 +0200 Subject: [PATCH 2/6] Retry transactions when entering scope of a relation unit When using server-side transactions, mgo retries (hard-coded 3 times) transactions that fail with WriteConflict error, and after that it aborts the (mgo) transaction. These retries are seen many times on multiple scenarios in juju, and therefore the business logic decides on each case how to treat the (mgo server-side) aborted transactions. In the case of EnterScope, it is necessary to retry the (mgo server-side) transaction (which includs retrying the mongodb transaction) because EnterScope is called concurrently (via the EnterScope facade) and therefore WriteConflict errors are transient errors. By calling Run(jujutxn.TransactionSource) instead of RunTransaction(ops []txn.Op) we make use of the retries applied to the function (jujutxn.TransactionSource) passed to Run(...). Note: This last observation means that the true solution would be sequential transactions for EnterScope, but since each relationunit has it's own runner, this would mean a huge change. This patch fixes a bug (https://bugs.launchpad.net/juju/+bug/2031631) on which the relation unit doesn't enter scope in the case multiple units and applications are present before creating a relation on them. --- state/relationunit.go | 139 ++++++++++++++++++++----------------- state/relationunit_test.go | 17 +++-- 2 files changed, 84 insertions(+), 72 deletions(-) diff --git a/state/relationunit.go b/state/relationunit.go index b851a179dc1..3bc979e6dae 100644 --- a/state/relationunit.go +++ b/state/relationunit.go @@ -76,79 +76,91 @@ func (ru *RelationUnit) EnterScope(settings map[string]interface{}) error { defer dbCloser() relationScopes, rsCloser := db.GetCollection(relationScopesC) defer rsCloser() - - // Verify that the unit is not already in scope, and abort without error - // if it is. ruKey := ru.key() - if count, err := relationScopes.FindId(ruKey).Count(); err != nil { - return err - } else if count != 0 { - return nil - } - - // Collect the operations necessary to enter scope, as follows: - // * Check unit and relation state, and incref the relation. - // * TODO(fwereade): check unit status == params.StatusActive (this - // breaks a bunch of tests in a boring but noisy-to-fix way, and is - // being saved for a followup). relationDocID := ru.relation.doc.DocID - var ops []txn.Op - if ru.isLocalUnit { + + var settingsChanged func() (bool, error) + + var existingSubName string + + buildTxn := func(attempt int) ([]txn.Op, error) { + + // Verify that the unit is not already in scope, and abort without error + // if it is. + if count, err := relationScopes.FindId(ruKey).Count(); err != nil { + return nil, err + } else if count != 0 { + return nil, nil + } + + // Collect the operations necessary to enter scope, as follows: + // * Check unit and relation state, and incref the relation. + // * TODO(fwereade): check unit status == params.StatusActive (this + // breaks a bunch of tests in a boring but noisy-to-fix way, and is + // being saved for a followup). + var ops []txn.Op + if ru.isLocalUnit { + ops = append(ops, txn.Op{ + C: unitsC, + Id: ru.unitName, + Assert: isAliveDoc, + }) + } ops = append(ops, txn.Op{ - C: unitsC, - Id: ru.unitName, + C: relationsC, + Id: relationDocID, Assert: isAliveDoc, + Update: bson.D{{"$inc", bson.D{{"unitcount", 1}}}}, }) - } - ops = append(ops, txn.Op{ - C: relationsC, - Id: relationDocID, - Assert: isAliveDoc, - Update: bson.D{{"$inc", bson.D{{"unitcount", 1}}}}, - }) - - // * Create the unit settings in this relation, if they do not already - // exist; or completely overwrite them if they do. This must happen - // before we create the scope doc, because the existence of a scope doc - // is considered to be a guarantee of the existence of a settings doc. - settingsChanged := func() (bool, error) { return false, nil } - settingsColl, sCloser := db.GetCollection(settingsC) - defer sCloser() - if count, err := settingsColl.FindId(ruKey).Count(); err != nil { - return err - } else if count == 0 { - ops = append(ops, createSettingsOp(settingsC, ruKey, settings)) - } else { - var rop txn.Op - rop, settingsChanged, err = replaceSettingsOp(ru.st.db(), settingsC, ruKey, settings) - if err != nil { - return err + + // * Create the unit settings in this relation, if they do not already + // exist; or completely overwrite them if they do. This must happen + // before we create the scope doc, because the existence of a scope doc + // is considered to be a guarantee of the existence of a settings doc. + settingsColl, sCloser := db.GetCollection(settingsC) + defer sCloser() + if count, err := settingsColl.FindId(ruKey).Count(); err != nil { + return nil, err + } else if count == 0 { + ops = append(ops, createSettingsOp(settingsC, ruKey, settings)) + settingsChanged = func() (bool, error) { return false, nil } + } else { + var rop txn.Op + rop, settingsChanged, err = replaceSettingsOp(ru.st.db(), settingsC, ruKey, settings) + if err != nil { + return nil, err + } + ops = append(ops, rop) } - ops = append(ops, rop) - } - // * Create the scope doc. - ops = append(ops, txn.Op{ - C: relationScopesC, - Id: ruKey, - Assert: txn.DocMissing, - Insert: relationScopeDoc{ - Key: ruKey, - }, - }) + // * Create the scope doc. + ops = append(ops, txn.Op{ + C: relationScopesC, + Id: ruKey, + Assert: txn.DocMissing, + Insert: relationScopeDoc{ + Key: ruKey, + }, + }) - // * If the unit should have a subordinate, and does not, create it. - var existingSubName string - if subOps, subName, err := ru.subordinateOps(); err != nil { - return err - } else { - existingSubName = subName - ops = append(ops, subOps...) + // * If the unit should have a subordinate, and does not, create it. + if subOps, subName, err := ru.subordinateOps(); err != nil { + return nil, err + } else { + existingSubName = subName + ops = append(ops, subOps...) + } + return ops, nil } + prefix := fmt.Sprintf("cannot enter scope for unit %q in relation %q: ", ru.unitName, ru.relation) // Now run the complete transaction, or figure out why we can't. - if err := ru.st.db().RunTransaction(ops); err != txn.ErrAborted { + if err := ru.st.db().Run(buildTxn); err != txn.ErrAborted { return err + } else if err == txn.ErrAborted { + // Apparently, all our assertions should have passed, but the txn was + // aborted: something is really seriously wrong. + return fmt.Errorf(prefix+"transaction error: %s", err) } if count, err := relationScopes.FindId(ruKey).Count(); err != nil { return err @@ -193,16 +205,13 @@ func (ru *RelationUnit) EnterScope(settings map[string]interface{}) error { // has changed under our feet, preventing us from clearing it properly; if // that is the case, something is seriously wrong (nobody else should be // touching that doc under our feet) and we should bail out. - prefix := fmt.Sprintf("cannot enter scope for unit %q in relation %q: ", ru.unitName, ru.relation) if changed, err := settingsChanged(); err != nil { return err } else if changed { return fmt.Errorf(prefix + "concurrent settings change detected") } - // Apparently, all our assertions should have passed, but the txn was - // aborted: something is really seriously wrong. - return fmt.Errorf(prefix + "inconsistent state in EnterScope") + return nil } // CounterpartApplications returns the slice of application names that are the counterpart of this unit. diff --git a/state/relationunit_test.go b/state/relationunit_test.go index 633fe3ba1e8..7f53a742f16 100644 --- a/state/relationunit_test.go +++ b/state/relationunit_test.go @@ -4,6 +4,7 @@ package state_test import ( + "context" "fmt" "sort" "strconv" @@ -13,6 +14,7 @@ import ( "github.com/juju/errors" "github.com/juju/loggo" jc "github.com/juju/testing/checkers" + "golang.org/x/sync/errgroup" gc "gopkg.in/check.v1" "github.com/juju/juju/core/network" @@ -1045,13 +1047,14 @@ func (prr *ProReqRelation) watches() []*state.RelationScopeWatcher { } func (prr *ProReqRelation) allEnterScope(c *gc.C) { - err := prr.pru0.EnterScope(nil) - c.Assert(err, jc.ErrorIsNil) - err = prr.pru1.EnterScope(nil) - c.Assert(err, jc.ErrorIsNil) - err = prr.rru0.EnterScope(nil) - c.Assert(err, jc.ErrorIsNil) - err = prr.rru1.EnterScope(nil) + g, _ := errgroup.WithContext(context.Background()) + + g.Go(func() error { return prr.pru0.EnterScope(nil) }) + g.Go(func() error { return prr.pru1.EnterScope(nil) }) + g.Go(func() error { return prr.rru0.EnterScope(nil) }) + g.Go(func() error { return prr.rru1.EnterScope(nil) }) + + err := g.Wait() c.Assert(err, jc.ErrorIsNil) } From e1757047f91031600be917adaa7af515159fd6f2 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Thu, 14 Sep 2023 14:21:55 -0400 Subject: [PATCH 3/6] Update the clean target Remove `-n`, which simply prints what would be done, and does not actually do it. Separate `--cache` and `$(PROJECT)...` as to latest go version doesn't allow on the same line and errors. --- Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 033fa651ec3..1ad615006dd 100644 --- a/Makefile +++ b/Makefile @@ -359,7 +359,8 @@ go-install: $(INSTALL_TARGETS) .PHONY: clean clean: ## clean: Clean the cache and test caches - go clean -n -r --cache --testcache $(PROJECT)/... + go clean -r --cache --testcache + go clean $(PROJECT)/... .PHONY: vendor-dependencies vendor-dependencies: From e4c963509e92f6d3515414dfb8afa72d2d65b3f5 Mon Sep 17 00:00:00 2001 From: Nicolas Vinuesa Date: Tue, 5 Sep 2023 13:16:00 +0200 Subject: [PATCH 4/6] Move assertion functions to the transaction builder in EnterScope Previously, there were some assertions that were being done *after* the transaction failed, now that we are creating a buildTxn function to pass to jujutxn.Run(), we can move this assertions inside the buildTxn function. --- state/errors/common.go | 4 +- state/relationunit.go | 126 ++++++++++++++++++------------------- state/relationunit_test.go | 6 +- 3 files changed, 66 insertions(+), 70 deletions(-) diff --git a/state/errors/common.go b/state/errors/common.go index b1277239ba1..d86d7ebbbe9 100644 --- a/state/errors/common.go +++ b/state/errors/common.go @@ -18,12 +18,12 @@ import ( const ( // ErrCannotEnterScope indicates that a relation unit failed to enter its scope // due to either the unit or the relation not being Alive. - ErrCannotEnterScope = errors.ConstError("cannot enter scope: unit or relation is not alive") + ErrCannotEnterScope = errors.ConstError("cannot enter scope") // ErrCannotEnterScopeYet indicates that a relation unit failed to enter its // scope due to a required and pre-existing subordinate unit that is not Alive. // Once that subordinate has been removed, a new one can be created. - ErrCannotEnterScopeYet = errors.ConstError("cannot enter scope yet: non-alive subordinate unit has not been removed") + ErrCannotEnterScopeYet = errors.ConstError("cannot enter scope yet") // ErrCharmRevisionAlreadyModified is returned when a pending or // placeholder charm is no longer pending or a placeholder, signaling diff --git a/state/relationunit.go b/state/relationunit.go index 3bc979e6dae..94021b45a62 100644 --- a/state/relationunit.go +++ b/state/relationunit.go @@ -80,15 +80,68 @@ func (ru *RelationUnit) EnterScope(settings map[string]interface{}) error { relationDocID := ru.relation.doc.DocID var settingsChanged func() (bool, error) - var existingSubName string + prefix := fmt.Sprintf("unit %q in relation %q: ", ru.unitName, ru.relation) buildTxn := func(attempt int) ([]txn.Op, error) { + // Before retrying the transaction, check the following + // assertions: + if attempt > 0 { + if count, err := relationScopes.FindId(ruKey).Count(); err != nil { + return nil, errors.Trace(err) + } else if count != 0 { + // The scope document exists, so we're actually already in scope. + return nil, nil + } + + // The relation or unit might no longer be Alive. (Note that there is no + // need for additional checks if we're trying to create a subordinate + // unit: this could fail due to the subordinate applications not being Alive, + // but this case will always be caught by the check for the relation's + // life (because a relation cannot be Alive if its applications are not).) + relations, rCloser := db.GetCollection(relationsC) + defer rCloser() + if alive, err := isAliveWithSession(relations, relationDocID); err != nil { + return nil, errors.Trace(err) + } else if !alive { + return nil, errors.Annotate(stateerrors.ErrCannotEnterScope, prefix+"relation is no longer alive") + } + if ru.isLocalUnit { + units, uCloser := db.GetCollection(unitsC) + defer uCloser() + if alive, err := isAliveWithSession(units, ru.unitName); err != nil { + return nil, errors.Trace(err) + } else if !alive { + return nil, errors.Annotate(stateerrors.ErrCannotEnterScope, prefix+"unit is no longer alive") + + } + + // Maybe a subordinate used to exist, but is no longer alive. If that is + // case, we will be unable to enter scope until that unit is gone. + if existingSubName != "" { + if alive, err := isAliveWithSession(units, existingSubName); err != nil { + return nil, errors.Trace(err) + } else if !alive { + return nil, errors.Annotatef(stateerrors.ErrCannotEnterScopeYet, prefix+"subordinate %v is no longer alive", existingSubName) + } + } + } - // Verify that the unit is not already in scope, and abort without error + // It's possible that there was a pre-existing settings doc whose version + // has changed under our feet, preventing us from clearing it properly; if + // that is the case, something is seriously wrong (nobody else should be + // touching that doc under our feet) and we should bail out. + if changed, err := settingsChanged(); err != nil { + return nil, errors.Trace(err) + } else if changed { + return nil, fmt.Errorf(prefix + "concurrent settings change detected") + } + } + + // Verify that the unit is not already in scope, and exit without error // if it is. if count, err := relationScopes.FindId(ruKey).Count(); err != nil { - return nil, err + return nil, errors.Trace(err) } else if count != 0 { return nil, nil } @@ -120,7 +173,7 @@ func (ru *RelationUnit) EnterScope(settings map[string]interface{}) error { settingsColl, sCloser := db.GetCollection(settingsC) defer sCloser() if count, err := settingsColl.FindId(ruKey).Count(); err != nil { - return nil, err + return nil, errors.Trace(err) } else if count == 0 { ops = append(ops, createSettingsOp(settingsC, ruKey, settings)) settingsChanged = func() (bool, error) { return false, nil } @@ -128,7 +181,7 @@ func (ru *RelationUnit) EnterScope(settings map[string]interface{}) error { var rop txn.Op rop, settingsChanged, err = replaceSettingsOp(ru.st.db(), settingsC, ruKey, settings) if err != nil { - return nil, err + return nil, errors.Trace(err) } ops = append(ops, rop) } @@ -145,7 +198,7 @@ func (ru *RelationUnit) EnterScope(settings map[string]interface{}) error { // * If the unit should have a subordinate, and does not, create it. if subOps, subName, err := ru.subordinateOps(); err != nil { - return nil, err + return nil, errors.Trace(err) } else { existingSubName = subName ops = append(ops, subOps...) @@ -153,65 +206,8 @@ func (ru *RelationUnit) EnterScope(settings map[string]interface{}) error { return ops, nil } - prefix := fmt.Sprintf("cannot enter scope for unit %q in relation %q: ", ru.unitName, ru.relation) - // Now run the complete transaction, or figure out why we can't. - if err := ru.st.db().Run(buildTxn); err != txn.ErrAborted { - return err - } else if err == txn.ErrAborted { - // Apparently, all our assertions should have passed, but the txn was - // aborted: something is really seriously wrong. - return fmt.Errorf(prefix+"transaction error: %s", err) - } - if count, err := relationScopes.FindId(ruKey).Count(); err != nil { - return err - } else if count != 0 { - // The scope document exists, so we're actually already in scope. - return nil - } - - // The relation or unit might no longer be Alive. (Note that there is no - // need for additional checks if we're trying to create a subordinate - // unit: this could fail due to the subordinate applications not being Alive, - // but this case will always be caught by the check for the relation's - // life (because a relation cannot be Alive if its applications are not).) - relations, rCloser := db.GetCollection(relationsC) - defer rCloser() - if alive, err := isAliveWithSession(relations, relationDocID); err != nil { - return err - } else if !alive { - return stateerrors.ErrCannotEnterScope - } - if ru.isLocalUnit { - units, uCloser := db.GetCollection(unitsC) - defer uCloser() - if alive, err := isAliveWithSession(units, ru.unitName); err != nil { - return err - } else if !alive { - return stateerrors.ErrCannotEnterScope - } - - // Maybe a subordinate used to exist, but is no longer alive. If that is - // case, we will be unable to enter scope until that unit is gone. - if existingSubName != "" { - if alive, err := isAliveWithSession(units, existingSubName); err != nil { - return err - } else if !alive { - return stateerrors.ErrCannotEnterScopeYet - } - } - } - - // It's possible that there was a pre-existing settings doc whose version - // has changed under our feet, preventing us from clearing it properly; if - // that is the case, something is seriously wrong (nobody else should be - // touching that doc under our feet) and we should bail out. - if changed, err := settingsChanged(); err != nil { - return err - } else if changed { - return fmt.Errorf(prefix + "concurrent settings change detected") - } - - return nil + // Now run the complete transaction. + return ru.st.db().Run(buildTxn) } // CounterpartApplications returns the slice of application names that are the counterpart of this unit. diff --git a/state/relationunit_test.go b/state/relationunit_test.go index 7f53a742f16..b89dba47cbb 100644 --- a/state/relationunit_test.go +++ b/state/relationunit_test.go @@ -399,7 +399,7 @@ func (s *RelationUnitSuite) TestContainerCreateSubordinate(c *gc.C) { c.Assert(err, jc.ErrorIsNil) assertNotInScope(c, pru) err = pru.EnterScope(nil) - c.Assert(err, gc.Equals, stateerrors.ErrCannotEnterScopeYet) + c.Assert(err, gc.ErrorMatches, ".*"+stateerrors.ErrCannotEnterScopeYet.Error()) assertNotInScope(c, pru) // Remove the subordinate, and enter scope again; this should work, and @@ -440,7 +440,7 @@ func (s *RelationUnitSuite) TestDestroyRelationWithUnitsInScope(c *gc.C) { // Check that we can't add a new unit now. assertNotInScope(c, pr.ru2) err = pr.ru2.EnterScope(nil) - c.Assert(err, gc.Equals, stateerrors.ErrCannotEnterScope) + c.Assert(err, gc.ErrorMatches, ".*"+stateerrors.ErrCannotEnterScope.Error()) assertNotInScope(c, pr.ru2) // Check that we created no settings for the unit we failed to add. @@ -530,7 +530,7 @@ func (s *RelationUnitSuite) TestAliveRelationScope(c *gc.C) { c.Assert(err, jc.ErrorIsNil) assertNotInScope(c, pr.ru3) err = pr.ru3.EnterScope(nil) - c.Assert(err, gc.Equals, stateerrors.ErrCannotEnterScope) + c.Assert(err, gc.ErrorMatches, ".*"+stateerrors.ErrCannotEnterScope.Error()) assertNotInScope(c, pr.ru3) } From ba73dddd87dc0628508d41711fbceb758a0e113d Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Thu, 14 Sep 2023 14:30:12 -0400 Subject: [PATCH 5/6] Fix for LP1988556, broken refresh by revision for charmhub. This is a patch based on a work around of using the switch flag with the new revision in the charm url for charmhub. The work around is suboptimal, as we want to not encourage charm url use by users directly. It's going away. A real fix will be more possible in juju 3.1 which does not contain charmstore charms. Juju needs to update its use of the charmhub refresh action for the refresh endpoint. However to share deploy/refresh code with charmstore easily, it's necessary to use ResolveCharm too. ResolveCharm does not differentiate between deploy and refresh operations as necessary for charmhub charms, but not charm store charms. --- cmd/juju/application/refresh.go | 6 ++++-- tests/suites/refresh/refresh.sh | 34 +++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/cmd/juju/application/refresh.go b/cmd/juju/application/refresh.go index c3ef3f255e0..12dee40f9a1 100644 --- a/cmd/juju/application/refresh.go +++ b/cmd/juju/application/refresh.go @@ -431,8 +431,10 @@ func (c *refreshCommand) Run(ctx *cmd.Context) error { DeployedBase: chBase, Force: c.Force, ForceSeries: c.ForceSeries, - Switch: c.SwitchURL != "", - Logger: ctx, + // If revision is supplied by the user, treat it as a switch operation, + // the revision has already been added to the "newRef" above. + Switch: c.SwitchURL != "" || c.Revision != -1, + Logger: ctx, } factory, err := c.getRefresherFactory(apiRoot) if err != nil { diff --git a/tests/suites/refresh/refresh.sh b/tests/suites/refresh/refresh.sh index aa60ca57a44..5d999ff7dd9 100644 --- a/tests/suites/refresh/refresh.sh +++ b/tests/suites/refresh/refresh.sh @@ -141,6 +141,39 @@ run_refresh_channel_no_new_revision() { destroy_model "${model_name}" } +run_refresh_revision() { + # Test juju refresh from revision to another + echo + + model_name="test-refresh-revision" + file="${TEST_DIR}/${model_name}.log" + + ensure "${model_name}" "${file}" + + juju deploy juju-qa-test --revision 22 --channel stable --series focal + wait_for "juju-qa-test" "$(idle_condition "juju-qa-test")" + + # refresh to a revision not at the tip of the stable channel + juju refresh juju-qa-test --revision 23 + wait_for "juju-qa-test" "$(charm_rev "juju-qa-test" "23")" + wait_for "juju-qa-test" "$(charm_channel "juju-qa-test" "stable")" + wait_for "juju-qa-test" "$(idle_condition "juju-qa-test")" + + # do a generic refresh, should pick up revision from latest stable + OUT=$(juju refresh juju-qa-test 2>&1 || true) + # shellcheck disable=SC2059 + printf "${OUT}\n" + + # format: Added charm-store charm "ubuntu", revision 21 in channel stable, to the model + revision=$(echo "${OUT}" | awk 'BEGIN{FS=","} {print $2}' | awk 'BEGIN{FS=" "} {print $2}') + + wait_for "juju-qa-test" "$(charm_rev "juju-qa-test" "${revision}")" + wait_for "juju-qa-test" "$(charm_channel "juju-qa-test" "stable")" + wait_for "juju-qa-test" "$(idle_condition "juju-qa-test")" + + destroy_model "${model_name}" +} + test_basic() { if [ "$(skip 'test_basic')" ]; then echo "==> TEST SKIPPED: basic refresh" @@ -157,5 +190,6 @@ test_basic() { run "run_refresh_local_resources" run "run_refresh_channel" run "run_refresh_channel_no_new_revision" + run "run_refresh_revision" ) } From 200af5d8810810b5cc6e9c10c679cc1b729a568e Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Fri, 15 Sep 2023 09:18:27 -0400 Subject: [PATCH 6/6] For target clean, the "r" flag was in the wrong place, only required for clean with $(PROJECT). --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 1ad615006dd..92893b5744f 100644 --- a/Makefile +++ b/Makefile @@ -359,8 +359,8 @@ go-install: $(INSTALL_TARGETS) .PHONY: clean clean: ## clean: Clean the cache and test caches - go clean -r --cache --testcache - go clean $(PROJECT)/... + go clean -x --cache --testcache + go clean -x -r $(PROJECT)/... .PHONY: vendor-dependencies vendor-dependencies: