From 4ba777e0f7ce2198d7d6dd56c639b2126cce511d Mon Sep 17 00:00:00 2001 From: Harry Pidcock Date: Fri, 7 Jun 2024 13:23:56 +1000 Subject: [PATCH 01/10] fix: leadership tracker worker killed by uniter A mistake was made when sidecar charms were introduced since this podspec/operator behaviour of Killing the leadership tracker worker when the uniter exited was enabled for all CAAS units. This is problematic because the leadership tracker worker's lifecycle is already managed by the dependency engine for the unit agent. Since the leadership tracker worker would exit cleanly, the dependency engine would not restart it. With the uniter clearly depending on the leadership tracker, the unit was not able to start again. --- worker/uniter/uniter.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/worker/uniter/uniter.go b/worker/uniter/uniter.go index 849763c4925..3dc8f1bfa2b 100644 --- a/worker/uniter/uniter.go +++ b/worker/uniter/uniter.go @@ -305,8 +305,8 @@ func newUniter(uniterParams *UniterParams) func() (worker.Worker, error) { return u.loop(uniterParams.UnitTag) }, } - if u.modelType == model.CAAS { - // For CAAS models, make sure the leadership tracker is killed when the Uniter + if u.modelType == model.CAAS && !uniterParams.Sidecar { + // For podspec units, make sure the leadership tracker is killed when the Uniter // dies. plan.Init = append(plan.Init, u.leadershipTracker) } From 57bbe3650d028fe6781b61902c7864302d90c399 Mon Sep 17 00:00:00 2001 From: Harry Pidcock Date: Fri, 7 Jun 2024 15:03:50 +1000 Subject: [PATCH 02/10] fix: unit agent lost on migration success During the migration minion's SUCCESS phase, at the point of no return, the api connection can fail. This leads to the doSUCCESS handler being unable to report the SUCCESS phase as being successful to the source controller. When this errors, the target IPs and CA certs are not saved to the agent config. This issue was seen with unit agents failing to use the target controller, but the machine agent using the target controller succesfully. This would show the unit agent on the machine as lost. This fix attempts to make the reporting back to the source controller more robust to connection issues. If for some reason, we fail these retry attempts, we now have a very angry critical log message to inform the admin of this situation. --- worker/migrationminion/manifold.go | 3 + worker/migrationminion/worker.go | 74 ++++++++++++++++++++++-- worker/migrationminion/worker_test.go | 81 ++++++++++++++++++++++++++- 3 files changed, 152 insertions(+), 6 deletions(-) diff --git a/worker/migrationminion/manifold.go b/worker/migrationminion/manifold.go index 629bafe9f44..e856626b279 100644 --- a/worker/migrationminion/manifold.go +++ b/worker/migrationminion/manifold.go @@ -20,6 +20,8 @@ type Logger interface { Errorf(string, ...interface{}) Infof(string, ...interface{}) Debugf(string, ...interface{}) + Warningf(string, ...interface{}) + Criticalf(string, ...interface{}) } // ManifoldConfig defines the names of the manifolds on which a @@ -98,6 +100,7 @@ func (config ManifoldConfig) start(context dependency.Context) (worker.Worker, e Clock: config.Clock, APIOpen: config.APIOpen, ValidateMigration: config.ValidateMigration, + NewFacade: config.NewFacade, Logger: config.Logger, }) if err != nil { diff --git a/worker/migrationminion/worker.go b/worker/migrationminion/worker.go index dc0832fbb36..774accc16ac 100644 --- a/worker/migrationminion/worker.go +++ b/worker/migrationminion/worker.go @@ -4,10 +4,12 @@ package migrationminion import ( + "fmt" "time" "github.com/juju/clock" "github.com/juju/errors" + jujuretry "github.com/juju/retry" "github.com/juju/worker/v3" "github.com/juju/worker/v3/catacomb" "gopkg.in/retry.v1" @@ -19,6 +21,7 @@ import ( "github.com/juju/juju/core/migration" "github.com/juju/juju/core/network" "github.com/juju/juju/core/watcher" + "github.com/juju/juju/rpc" "github.com/juju/juju/rpc/params" "github.com/juju/juju/worker/fortress" ) @@ -52,6 +55,7 @@ type Config struct { Clock clock.Clock APIOpen func(*api.Info, api.DialOpts) (api.Connection, error) ValidateMigration func(base.APICaller) error + NewFacade func(base.APICaller) (Facade, error) Logger Logger } @@ -132,6 +136,7 @@ func (w *Worker) loop() error { return errors.New("watcher channel closed") } if err := w.handle(status); err != nil { + w.config.Logger.Errorf("handling migration phase %s failed: %v", status.Phase, err) return errors.Trace(err) } } @@ -191,7 +196,7 @@ func (w *Worker) doVALIDATION(status watcher.MigrationStatus) error { break } if attempt.More() { - w.config.Logger.Debugf("validation failed (retrying): %v", err) + w.config.Logger.Warningf("validation failed (retrying): %v", err) } } if errors.Is(err, apiservererrors.ErrTryAgain) || params.IsCodeTryAgain(err) { @@ -235,7 +240,15 @@ func (w *Worker) validate(status watcher.MigrationStatus) error { return errors.Trace(err) } -func (w *Worker) doSUCCESS(status watcher.MigrationStatus) error { +func (w *Worker) doSUCCESS(status watcher.MigrationStatus) (err error) { + defer func() { + if err != nil { + cfg := w.config.Agent.CurrentConfig() + w.config.Logger.Criticalf("migration failed for %v: %s/agent.conf left unchanged and pointing to source controller: %v", + cfg.Tag(), cfg.Dir(), err, + ) + } + }() hps, err := network.ParseProviderHostPorts(status.TargetAPIAddrs...) if err != nil { return errors.Annotate(err, "converting API addresses") @@ -243,8 +256,9 @@ func (w *Worker) doSUCCESS(status watcher.MigrationStatus) error { // Report first because the config update that's about to happen // will cause the API connection to drop. The SUCCESS phase is the - // point of no return anyway. - if err := w.report(status, true); err != nil { + // point of no return anyway, so we must retry this step even if + // the api connection dies. + if err := w.robustReport(status, true); err != nil { return errors.Trace(err) } @@ -260,7 +274,57 @@ func (w *Worker) doSUCCESS(status watcher.MigrationStatus) error { } func (w *Worker) report(status watcher.MigrationStatus, success bool) error { - w.config.Logger.Debugf("reporting back for phase %s: %v", status.Phase, success) + w.config.Logger.Infof("reporting back for phase %s: %v", status.Phase, success) err := w.config.Facade.Report(status.MigrationId, status.Phase, success) return errors.Annotate(err, "failed to report phase progress") } + +func (w *Worker) robustReport(status watcher.MigrationStatus, success bool) error { + err := w.report(status, success) + if err != nil && !rpc.IsShutdownErr(err) { + return fmt.Errorf("cannot report migration status %v success=%v: %w", status, success, err) + } else if err == nil { + return nil + } + w.config.Logger.Warningf("report migration status failed: %v", err) + + apiInfo, ok := w.config.Agent.CurrentConfig().APIInfo() + if !ok { + return fmt.Errorf("cannot report migration status %v success=%v: no API connection details", status, success) + } + apiInfo.Addrs = status.SourceAPIAddrs + apiInfo.CACert = status.SourceCACert + + err = jujuretry.Call(jujuretry.CallArgs{ + Func: func() error { + w.config.Logger.Infof("reporting back for phase %s: %v", status.Phase, success) + + conn, err := w.config.APIOpen(apiInfo, api.DialOpts{}) + if err != nil { + return fmt.Errorf("cannot dial source controller: %w", err) + } + defer func() { _ = conn.Close() }() + + facade, err := w.config.NewFacade(conn) + if err != nil { + return err + } + + return facade.Report(status.MigrationId, status.Phase, success) + }, + IsFatalError: func(err error) bool { + return false + }, + NotifyFunc: func(lastError error, attempt int) { + w.config.Logger.Warningf("report migration status failed (attempt %d): %v", attempt, lastError) + }, + Clock: w.config.Clock, + Delay: initialRetryDelay, + Attempts: maxRetries, + BackoffFunc: jujuretry.DoubleDelay, + }) + if err != nil { + return fmt.Errorf("cannot report migration status %v success=%v: %w", status, success, err) + } + return nil +} diff --git a/worker/migrationminion/worker_test.go b/worker/migrationminion/worker_test.go index 53d437028e9..69a12d113e6 100644 --- a/worker/migrationminion/worker_test.go +++ b/worker/migrationminion/worker_test.go @@ -25,6 +25,7 @@ import ( "github.com/juju/juju/core/migration" "github.com/juju/juju/core/network" "github.com/juju/juju/core/watcher" + "github.com/juju/juju/rpc" coretesting "github.com/juju/juju/testing" "github.com/juju/juju/worker/fortress" "github.com/juju/juju/worker/migrationminion" @@ -366,6 +367,73 @@ func (s *Suite) TestSUCCESS(c *gc.C) { s.stub.CheckCall(c, 2, "Report", "id", migration.SUCCESS, true) } +func (s *Suite) TestSUCCESSCantConnectNotReportForTryAgainError(c *gc.C) { + s.client.watcher.changes <- watcher.MigrationStatus{ + MigrationId: "id", + Phase: migration.SUCCESS, + } + s.agent.conf.tag = names.NewUnitTag("app/0") + s.agent.conf.dir = "/var/lib/juju/agents/unit-app-0" + s.config.APIOpen = func(*api.Info, api.DialOpts) (api.Connection, error) { + s.stub.AddCall("API open") + return nil, apiservererrors.ErrTryAgain + } + s.stub.SetErrors(rpc.ErrShutdown) + w, err := migrationminion.New(s.config) + c.Assert(err, jc.ErrorIsNil) + defer workertest.DirtyKill(c, w) + + // Advance time enough for all of the retries to be exhausted. + sleepTime := 100 * time.Millisecond + for i := 0; i < 9; i++ { + err := s.clock.WaitAdvance(sleepTime, coretesting.ShortWait, 1) + c.Assert(err, jc.ErrorIsNil) + sleepTime = sleepTime * 2 + } + + s.waitForStubCalls(c, []string{ + "Watch", + "Lockdown", + "Report", + "API open", + "API open", + "API open", + "API open", + "API open", + "API open", + "API open", + "API open", + "API open", + "API open", + }) +} + +func (s *Suite) TestSUCCESSRetryReport(c *gc.C) { + s.client.watcher.changes <- watcher.MigrationStatus{ + MigrationId: "id", + Phase: migration.SUCCESS, + } + s.agent.conf.tag = names.NewUnitTag("app/0") + s.agent.conf.dir = "/var/lib/juju/agents/unit-app-0" + s.config.NewFacade = func(a base.APICaller) (migrationminion.Facade, error) { + return s.config.Facade, nil + } + + s.stub.SetErrors(rpc.ErrShutdown) + w, err := migrationminion.New(s.config) + c.Assert(err, jc.ErrorIsNil) + defer workertest.CleanKill(c, w) + + s.waitForStubCalls(c, []string{ + "Watch", + "Lockdown", + "Report", + "API open", + "Report", + "API close", + }) +} + func (s *Suite) waitForStubCalls(c *gc.C, expectedCallNames []string) { waitForStubCalls(c, s.stub, expectedCallNames...) } @@ -433,7 +501,7 @@ func (c *stubMinionClient) Watch() (watcher.MigrationStatusWatcher, error) { func (c *stubMinionClient) Report(id string, phase migration.Phase, success bool) error { c.stub.MethodCall(c, "Report", id, phase, success) - return nil + return c.stub.NextErr() } func newStubWatcher() *stubWatcher { @@ -476,6 +544,9 @@ func (ma *stubAgent) ChangeConfig(f agent.ConfigMutator) error { type stubAgentConfig struct { agent.ConfigSetter + tag names.Tag + dir string + mu sync.Mutex addrs []string caCert string @@ -511,6 +582,14 @@ func (mc *stubAgentConfig) APIInfo() (*api.Info, bool) { }, true } +func (mc *stubAgentConfig) Tag() names.Tag { + return mc.tag +} + +func (mc *stubAgentConfig) Dir() string { + return mc.dir +} + type stubConnection struct { api.Connection stub *jujutesting.Stub From 7c8b661086c4d1e1da9ad6def953c4f886ef33b3 Mon Sep 17 00:00:00 2001 From: Kelvin Liu Date: Wed, 12 Jun 2024 10:59:37 +1000 Subject: [PATCH 03/10] Check nil model for non anonymous Login; --- apiserver/admin.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/apiserver/admin.go b/apiserver/admin.go index 6ab7c41de4e..ca0320cb184 100644 --- a/apiserver/admin.go +++ b/apiserver/admin.go @@ -313,8 +313,10 @@ func (a *admin) authenticate(ctx context.Context, req params.LoginRequest) (*aut startPinger = false controllerConn = true } - } else if a.root.model == nil { - // Anonymous login to unknown model. + } + if a.root.model == nil { + // Login to an unknown or migrated model. + // See maybeEmitRedirectError for user logins who are redirected. // Hide the fact that the model does not exist. return nil, errors.Unauthorizedf("invalid entity name or password") } From ab47c58a3500046a26e0d2abddb279265dda12fe Mon Sep 17 00:00:00 2001 From: Simon Richardson Date: Wed, 12 Jun 2024 16:46:33 +0100 Subject: [PATCH 04/10] fix(db): Dqlite HA: too many colons in address If mongodb returns a IPv6 address when clustering, then dqlite will bork on attempting to serialise the address and port. All IPv6 should be included in a `[]`. --- database/node.go | 3 +-- database/node_test.go | 13 ++++++++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/database/node.go b/database/node.go index 51fd8ec0432..09a041dcef1 100644 --- a/database/node.go +++ b/database/node.go @@ -7,7 +7,6 @@ import ( "context" "crypto/tls" "crypto/x509" - "fmt" "io" "net" "os" @@ -322,7 +321,7 @@ func (m *NodeManager) WithTLSOption() (app.Option, error) { // Dqlite as the member of a cluster with peers representing other controllers. func (m *NodeManager) WithClusterOption(addrs []string) app.Option { peerAddrs := transform.Slice(addrs, func(addr string) string { - return fmt.Sprintf("%s:%d", addr, m.port) + return net.JoinHostPort(addr, strconv.Itoa(m.port)) }) m.logger.Debugf("determined Dqlite cluster members: %v", peerAddrs) diff --git a/database/node_test.go b/database/node_test.go index ae801f313e8..59a4a9b877c 100644 --- a/database/node_test.go +++ b/database/node_test.go @@ -339,7 +339,7 @@ func (s *nodeManagerSuite) TestWithTLSOptionSuccess(c *gc.C) { c.Assert(err, jc.ErrorIsNil) } -func (s *nodeManagerSuite) TestWithClusterOptionSuccess(c *gc.C) { +func (s *nodeManagerSuite) TestWithClusterOptionIPv4Success(c *gc.C) { cfg := fakeAgentConfig{} m := NewNodeManager(cfg, true, stubLogger{}, coredatabase.NoopSlowQueryLogger{}) @@ -350,6 +350,17 @@ func (s *nodeManagerSuite) TestWithClusterOptionSuccess(c *gc.C) { c.Assert(err, jc.ErrorIsNil) } +func (s *nodeManagerSuite) TestWithClusterOptionIPv6Success(c *gc.C) { + cfg := fakeAgentConfig{} + m := NewNodeManager(cfg, true, stubLogger{}, coredatabase.NoopSlowQueryLogger{}) + + dqliteApp, err := app.New(c.MkDir(), m.WithClusterOption([]string{"::1"})) + c.Assert(err, jc.ErrorIsNil) + + err = dqliteApp.Close() + c.Assert(err, jc.ErrorIsNil) +} + func (s *nodeManagerSuite) TestWithPreferredCloudLocalAddressOptionNoAddrFallback(c *gc.C) { ctrl := gomock.NewController(c) defer ctrl.Finish() From b75969c6b557b7f9b568b2f0734828a41850f09a Mon Sep 17 00:00:00 2001 From: Nicolas Vinuesa Date: Tue, 11 Jun 2024 12:46:33 +0200 Subject: [PATCH 05/10] fix(offer): improve error message when updating offers with removed endpoints Juju offer is an upsert operation but this might misleading to users. Since we are not changing this behavior right now, this patch at least improves the error message when *updating* an offer and in this update an endpoint (with active consumers) is removed. The new error message should give the user the hint that they are actually updating the offer and therefore if they don't want to remove the previously added endpoints, they should be added as a list in the passed arguments. This behavior is already documented. [squash] fix typo on ensureEndpointsNotInUse args Co-authored-by: Simon Richardson [squash] add test with multiple endpoints offer update [squash] fix cmd status tests --- apiserver/facades/client/client/api_test.go | 1 + cmd/juju/ssh/debughooks_test.go | 2 +- cmd/juju/status/status_internal_test.go | 2 + state/application_test.go | 15 ++++++- state/applicationoffers.go | 12 ++--- state/applicationoffers_test.go | 44 +++++++++++++++++-- .../charm-repo/quantal/mysql/metadata.yaml | 2 + 7 files changed, 67 insertions(+), 11 deletions(-) diff --git a/apiserver/facades/client/client/api_test.go b/apiserver/facades/client/client/api_test.go index 36c0d09cafd..1aca891d4f9 100644 --- a/apiserver/facades/client/client/api_test.go +++ b/apiserver/facades/client/client/api_test.go @@ -224,6 +224,7 @@ var scenarioStatus = ¶ms.FullStatus{ "": network.AlphaSpaceName, "server": network.AlphaSpaceName, "server-admin": network.AlphaSpaceName, + "db-router": network.AlphaSpaceName, "metrics-client": network.AlphaSpaceName, }, }, diff --git a/cmd/juju/ssh/debughooks_test.go b/cmd/juju/ssh/debughooks_test.go index 610af1c5a5a..98e6307f985 100644 --- a/cmd/juju/ssh/debughooks_test.go +++ b/cmd/juju/ssh/debughooks_test.go @@ -97,7 +97,7 @@ var debugHooksTests = []struct { }, { info: `invalid hook`, args: []string{"mysql/0", "invalid-hook"}, - error: `unit "mysql/0" contains neither hook nor action "invalid-hook", valid actions are [anotherfakeaction fakeaction] and valid hooks are [collect-metrics config-changed install juju-info-relation-broken juju-info-relation-changed juju-info-relation-created juju-info-relation-departed juju-info-relation-joined leader-deposed leader-elected leader-settings-changed meter-status-changed metrics-client-relation-broken metrics-client-relation-changed metrics-client-relation-created metrics-client-relation-departed metrics-client-relation-joined post-series-upgrade pre-series-upgrade remove secret-changed secret-expired secret-remove secret-rotate server-admin-relation-broken server-admin-relation-changed server-admin-relation-created server-admin-relation-departed server-admin-relation-joined server-relation-broken server-relation-changed server-relation-created server-relation-departed server-relation-joined start stop update-status upgrade-charm]`, + error: `unit "mysql/0" contains neither hook nor action "invalid-hook", valid actions are [anotherfakeaction fakeaction] and valid hooks are [collect-metrics config-changed db-router-relation-broken db-router-relation-changed db-router-relation-created db-router-relation-departed db-router-relation-joined install juju-info-relation-broken juju-info-relation-changed juju-info-relation-created juju-info-relation-departed juju-info-relation-joined leader-deposed leader-elected leader-settings-changed meter-status-changed metrics-client-relation-broken metrics-client-relation-changed metrics-client-relation-created metrics-client-relation-departed metrics-client-relation-joined post-series-upgrade pre-series-upgrade remove secret-changed secret-expired secret-remove secret-rotate server-admin-relation-broken server-admin-relation-changed server-admin-relation-created server-admin-relation-departed server-admin-relation-joined server-relation-broken server-relation-changed server-relation-created server-relation-departed server-relation-joined start stop update-status upgrade-charm]`, }, { info: `no args at all`, args: nil, diff --git a/cmd/juju/status/status_internal_test.go b/cmd/juju/status/status_internal_test.go index 6b54a523ae3..6465563980b 100644 --- a/cmd/juju/status/status_internal_test.go +++ b/cmd/juju/status/status_internal_test.go @@ -2915,6 +2915,7 @@ var statusTests = []testCase{ "": network.AlphaSpaceName, "server": network.AlphaSpaceName, "server-admin": network.AlphaSpaceName, + "db-router": network.AlphaSpaceName, "metrics-client": network.AlphaSpaceName, }, }), @@ -3047,6 +3048,7 @@ var statusTests = []testCase{ "": network.AlphaSpaceName, "server": network.AlphaSpaceName, "server-admin": network.AlphaSpaceName, + "db-router": network.AlphaSpaceName, "metrics-client": network.AlphaSpaceName, }, }), diff --git a/state/application_test.go b/state/application_test.go index ad5dddc5961..2fdd82ef8e9 100644 --- a/state/application_test.go +++ b/state/application_test.go @@ -457,6 +457,7 @@ func (s *ApplicationSuite) TestMergeBindings(c *gc.C) { "metrics-client": network.AlphaSpaceName, "server": network.AlphaSpaceName, "server-admin": network.AlphaSpaceName, + "db-router": network.AlphaSpaceName, } b, err := s.mysql.EndpointBindings() c.Assert(err, jc.ErrorIsNil) @@ -499,6 +500,7 @@ func (s *ApplicationSuite) TestMergeBindingsWithForce(c *gc.C) { "metrics-client": network.AlphaSpaceName, "server": network.AlphaSpaceName, "server-admin": network.AlphaSpaceName, + "db-router": network.AlphaSpaceName, } b, err := s.mysql.EndpointBindings() c.Assert(err, jc.ErrorIsNil) @@ -2489,6 +2491,17 @@ func (s *ApplicationSuite) TestMysqlEndpoints(c *gc.C) { Scope: charm.ScopeGlobal, }, }) + dbRouterEP, err := s.mysql.Endpoint("db-router") + c.Assert(err, jc.ErrorIsNil) + c.Assert(dbRouterEP, gc.DeepEquals, state.Endpoint{ + ApplicationName: "mysql", + Relation: charm.Relation{ + Interface: "db-router", + Name: "db-router", + Role: charm.RoleProvider, + Scope: charm.ScopeGlobal, + }, + }) monitoringEP, err := s.mysql.Endpoint("metrics-client") c.Assert(err, jc.ErrorIsNil) c.Assert(monitoringEP, gc.DeepEquals, state.Endpoint{ @@ -2503,7 +2516,7 @@ func (s *ApplicationSuite) TestMysqlEndpoints(c *gc.C) { eps, err := s.mysql.Endpoints() c.Assert(err, jc.ErrorIsNil) - c.Assert(eps, jc.SameContents, []state.Endpoint{jiEP, serverEP, serverAdminEP, monitoringEP}) + c.Assert(eps, jc.SameContents, []state.Endpoint{jiEP, serverEP, serverAdminEP, dbRouterEP, monitoringEP}) } func (s *ApplicationSuite) TestRiakEndpoints(c *gc.C) { diff --git a/state/applicationoffers.go b/state/applicationoffers.go index 3f4a64788d3..f019bc56e16 100644 --- a/state/applicationoffers.go +++ b/state/applicationoffers.go @@ -683,7 +683,7 @@ func (s *applicationOffers) UpdateOffer(offerArgs crossmodel.AddApplicationOffer // case. This prevents users from accidentally breaking saas // consumers. goneEndpoints := existingEndpoints.Difference(updatedEndpoints) - if err := s.ensureEndpointsNotInUse(curOfferDoc.ApplicationName, curOfferDoc.OfferUUID, goneEndpoints); err != nil { + if err := s.ensureEndpointsNotInUse(curOfferDoc.ApplicationName, curOfferDoc.OfferUUID, goneEndpoints, updatedEndpoints); err != nil { return nil, err } } @@ -705,8 +705,8 @@ func (s *applicationOffers) UpdateOffer(offerArgs crossmodel.AddApplicationOffer return s.makeApplicationOffer(doc) } -func (s *applicationOffers) ensureEndpointsNotInUse(appName, offerUUID string, endpoints set.Strings) error { - if len(endpoints) == 0 { +func (s *applicationOffers) ensureEndpointsNotInUse(appName, offerUUID string, removedEndpoints, updatedEndpoints set.Strings) error { + if len(removedEndpoints) == 0 { return nil } @@ -723,7 +723,7 @@ func (s *applicationOffers) ensureEndpointsNotInUse(appName, offerUUID string, e return errors.New("malformed relation key") } - if tokens[0] == appName && endpoints.Contains(tokens[1]) { + if tokens[0] == appName && removedEndpoints.Contains(tokens[1]) { inUse.Add(tokens[1]) } } @@ -733,9 +733,9 @@ func (s *applicationOffers) ensureEndpointsNotInUse(appName, offerUUID string, e case 0: return nil case 1: - return errors.Errorf("application endpoint %q has active consumers", inUse.Values()[0]) + return errors.Errorf("updating offer %s:%s would remove endpoint %q which has active consumers", appName, strings.Join(updatedEndpoints.SortedValues(), ", "), inUse.Values()[0]) default: - return errors.Errorf("application endpoints %q have active consumers", strings.Join(inUse.SortedValues(), ", ")) + return errors.Errorf("updating offer %s:%s would remove endpoints %q which have active consumers", appName, strings.Join(updatedEndpoints.SortedValues(), ", "), strings.Join(inUse.SortedValues(), ", ")) } } diff --git a/state/applicationoffers_test.go b/state/applicationoffers_test.go index d7ff7c0280e..89342e3d6bb 100644 --- a/state/applicationoffers_test.go +++ b/state/applicationoffers_test.go @@ -566,7 +566,7 @@ func (s *applicationOffersSuite) addOfferConnection(c *gc.C, offerUUID string) * return app } -func (s *applicationOffersSuite) TestUpdateApplicationOfferRemovingEndpointsInUse(c *gc.C) { +func (s *applicationOffersSuite) TestUpdateApplicationOfferRemovingEndpointInUse(c *gc.C) { owner := s.Factory.MakeUser(c, nil).Name() sd := state.NewApplicationOffers(s.State) offer, err := sd.AddOffer(crossmodel.AddApplicationOfferArgs{ @@ -596,11 +596,49 @@ func (s *applicationOffersSuite) TestUpdateApplicationOfferRemovingEndpointsInUs Endpoints: map[string]string{ // We are attempting to remove the "server" endpoint // from the offer which is currently connected to an - // active consumer + // active consumer. + "server-admin": "server-admin", + }, + }) + c.Assert(err, gc.ErrorMatches, `cannot update application offer "hosted-mysql": updating offer mysql:server-admin would remove endpoint "server" which has active consumers`) +} + +func (s *applicationOffersSuite) TestUpdateApplicationOfferRemovingEndpointsInUse(c *gc.C) { + owner := s.Factory.MakeUser(c, nil).Name() + sd := state.NewApplicationOffers(s.State) + offer, err := sd.AddOffer(crossmodel.AddApplicationOfferArgs{ + OfferName: "hosted-mysql", + ApplicationName: "mysql", + Owner: owner, + Endpoints: map[string]string{ + "server": "server", + "server-admin": "server-admin", + "router": "db-router", + }, + }) + c.Assert(err, jc.ErrorIsNil) + + _, err = s.State.AddOfferConnection(state.AddOfferConnectionParams{ + SourceModelUUID: testing.ModelTag.Id(), + RelationId: 1, + RelationKey: "mysql:db-router mysql:server", + Username: "admin", + OfferUUID: offer.OfferUUID, + }) + c.Assert(err, jc.ErrorIsNil) + + _, err = sd.UpdateOffer(crossmodel.AddApplicationOfferArgs{ + OfferName: "hosted-mysql", + ApplicationName: "mysql", + Owner: owner, + Endpoints: map[string]string{ + // We are attempting to remove the "server" and + // "router" endpoints from the offer which is currently + // connected to an active consumer. "server-admin": "server-admin", }, }) - c.Assert(err, gc.ErrorMatches, `cannot update application offer "hosted-mysql": application endpoint "server" has active consumers`) + c.Assert(err, gc.ErrorMatches, `cannot update application offer "hosted-mysql": updating offer mysql:server-admin would remove endpoints "db-router, server" which have active consumers`) } func (s *applicationOffersSuite) TestUpdateApplicationOfferInvalidApplication(c *gc.C) { diff --git a/testcharms/charm-repo/quantal/mysql/metadata.yaml b/testcharms/charm-repo/quantal/mysql/metadata.yaml index 58a640c817f..3659c281111 100644 --- a/testcharms/charm-repo/quantal/mysql/metadata.yaml +++ b/testcharms/charm-repo/quantal/mysql/metadata.yaml @@ -6,6 +6,8 @@ provides: interface: mysql server-admin: interface: mysql-root + db-router: + interface: db-router requires: metrics-client: interface: metrics From d6e29926cca57112351579b23d24931939c2bbde Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Thu, 13 Jun 2024 10:44:06 +0200 Subject: [PATCH 06/10] fix(upgrade): noble upgrade support The default base was selected by first checking for the latest LTS, then, if that didn't work, trying the offical juju defualt. This commit swaps the order of those, so that the officail juju default version is preferd over the latest LTS. The latest LTS is still used if the charm does not support the juju default version. --- cmd/juju/application/deployer/deployer.go | 2 +- core/base/supported.go | 1 + core/charm/baseselector.go | 19 ++++++++++--------- 3 files changed, 12 insertions(+), 10 deletions(-) diff --git a/cmd/juju/application/deployer/deployer.go b/cmd/juju/application/deployer/deployer.go index 2a40187744a..62a9f3debb1 100644 --- a/cmd/juju/application/deployer/deployer.go +++ b/cmd/juju/application/deployer/deployer.go @@ -91,7 +91,7 @@ func NewDeployerFactory(dep DeployerDependencies) DeployerFactory { } // GetDeployer returns the correct deployer to use based on the cfg provided. -// A ModelConfigGetter needed to find the deployer. +// A ModelConfigGetter is needed to find the deployer. func (d *factory) GetDeployer(cfg DeployerConfig, getter ModelConfigGetter, resolver Resolver) (Deployer, error) { // Determine the type of deploy we have var dk DeployerKind diff --git a/core/base/supported.go b/core/base/supported.go index cb651402a71..ae20ae0afb9 100644 --- a/core/base/supported.go +++ b/core/base/supported.go @@ -387,6 +387,7 @@ var ubuntuSeries = map[SeriesName]seriesVersion{ WorkloadType: ControllerWorkloadType, Version: "24.04", LTS: true, + Supported: true, ESMSupported: true, }, } diff --git a/core/charm/baseselector.go b/core/charm/baseselector.go index ad1f468d89d..58131d40c85 100644 --- a/core/charm/baseselector.go +++ b/core/charm/baseselector.go @@ -132,8 +132,9 @@ func (s BaseSelector) validate(supportedCharmBases, supportedJujuBases []base.Ba // Order of preference is: // - user requested with --base or defined by bundle when deploying // - model default, if set, acts like --base -// - juju default ubuntu LTS from charm manifest -// - first base listed in the charm manifest +// - juju's default supported Ubuntu LTS (if compatible with valid charm bases) +// - the latest known Ubuntu LTS (if compatible with valid charm bases) +// - the first supported base in the charm manifest // - in the case of local charms with no manifest nor base in metadata, // base must be provided by the user. func (s BaseSelector) CharmBase() (selectedBase base.Base, err error) { @@ -156,6 +157,13 @@ func (s BaseSelector) CharmBase() (selectedBase base.Base, err error) { return s.userRequested(s.defaultBase) } + // Try juju's current default supported Ubuntu LTS + jujuDefaultBase, err := BaseForCharm(version.DefaultSupportedLTSBase(), s.supportedBases) + if err == nil { + s.logger.Infof(msgLatestLTSBase, version.DefaultSupportedLTSBase()) + return jujuDefaultBase, nil + } + // Prefer latest Ubuntu LTS. preferredBase, err := BaseForCharm(base.LatestLTSBase(), s.supportedBases) if err == nil { @@ -165,13 +173,6 @@ func (s BaseSelector) CharmBase() (selectedBase base.Base, err error) { return base.Base{}, err } - // Try juju's current default supported Ubuntu LTS - jujuDefaultBase, err := BaseForCharm(version.DefaultSupportedLTSBase(), s.supportedBases) - if err == nil { - s.logger.Infof(msgLatestLTSBase, version.DefaultSupportedLTSBase()) - return jujuDefaultBase, nil - } - // Last chance, the first base in the charm's manifest return BaseForCharm(base.Base{}, s.supportedBases) } From 0d1dd6a5e0bcfcc9c1a4a41d02074e27af1f4ccd Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Thu, 13 Jun 2024 16:11:00 +0200 Subject: [PATCH 07/10] fix: rename ubuntuVersions, remove 24.04 and fix ec2 test metadata The ubuntuVersions var actually listed unsupportedUbuntuVersions. Noble should be supported so it is removed from this list. Change expected base to jujuDefault to reflect change in BaseSelector Update expected latest LTS test Add 24.04 to ec2 testmetadata and fix migrate_test --- .../client/modelupgrader/upgrader_test.go | 18 +++--- core/base/supportedseries_linux_test.go | 2 +- core/charm/baseselector_test.go | 2 +- provider/ec2/series_test.go | 55 +++++++++++++++++++ upgrades/upgradevalidation/migrate_test.go | 5 +- upgrades/upgradevalidation/upgrade_test.go | 6 +- upgrades/upgradevalidation/validation_test.go | 2 +- 7 files changed, 72 insertions(+), 18 deletions(-) diff --git a/apiserver/facades/client/modelupgrader/upgrader_test.go b/apiserver/facades/client/modelupgrader/upgrader_test.go index 416210b8639..08bc2ac10a1 100644 --- a/apiserver/facades/client/modelupgrader/upgrader_test.go +++ b/apiserver/facades/client/modelupgrader/upgrader_test.go @@ -42,7 +42,8 @@ var winVersions = []string{ "win2016", "win2016hv", "win2019", "win7", "win8", "win81", "win10", } -var ubuntuVersions = []string{ +// unsupportedUbuntuVersions are the ubuntu versions that juju does not support. +var unsupportedUbuntuVersions = []string{ "12.04", "12.10", "13.04", @@ -65,7 +66,6 @@ var ubuntuVersions = []string{ "22.10", "23.04", "23.10", - "24.04", } var controllerCfg = controller.Config{ @@ -257,7 +257,7 @@ func (s *modelUpgradeSuite) assertUpgradeModelForControllerModelJuju3(c *gc.C, d // - check if the model has win machines; ctrlState.EXPECT().MachineCountForBase(makeBases("windows", winVersions)).Return(nil, nil) // - check if the model has deprecated ubuntu machines; - ctrlState.EXPECT().MachineCountForBase(makeBases("ubuntu", ubuntuVersions)).Return(nil, nil) + ctrlState.EXPECT().MachineCountForBase(makeBases("ubuntu", unsupportedUbuntuVersions)).Return(nil, nil) // - check LXD version. // - check if model has charm store charms; ctrlState.EXPECT().AllCharmURLs().Return(nil, errors.NotFoundf("charms")) @@ -277,7 +277,7 @@ func (s *modelUpgradeSuite) assertUpgradeModelForControllerModelJuju3(c *gc.C, d // - check if the model has win machines; state1.EXPECT().MachineCountForBase(makeBases("windows", winVersions)).Return(nil, nil) // - check if the model has deprecated ubuntu machines; - state1.EXPECT().MachineCountForBase(makeBases("ubuntu", ubuntuVersions)).Return(nil, nil) + state1.EXPECT().MachineCountForBase(makeBases("ubuntu", unsupportedUbuntuVersions)).Return(nil, nil) // - check if model has charm store charms; state1.EXPECT().AllCharmURLs().Return(nil, errors.NotFoundf("charms")) // - check LXD version. @@ -383,7 +383,7 @@ func (s *modelUpgradeSuite) TestUpgradeModelForControllerDyingHostedModelJuju3(c // - check if the model has win machines; ctrlState.EXPECT().MachineCountForBase(makeBases("windows", winVersions)).Return(nil, nil) // - check if the model has deprecated ubuntu machines; - ctrlState.EXPECT().MachineCountForBase(makeBases("ubuntu", ubuntuVersions)).Return(nil, nil) + ctrlState.EXPECT().MachineCountForBase(makeBases("ubuntu", unsupportedUbuntuVersions)).Return(nil, nil) // - check LXD version. // - check if model has charm store charms; ctrlState.EXPECT().AllCharmURLs().Return(nil, errors.NotFoundf("charms")) @@ -488,7 +488,7 @@ func (s *modelUpgradeSuite) TestUpgradeModelForControllerModelJuju3Failed(c *gc. // - check if the model has win machines; ctrlState.EXPECT().MachineCountForBase(makeBases("windows", winVersions)).Return(map[string]int{"win10": 1, "win7": 2}, nil) // - check if the model has deprecated ubuntu machines; - ctrlState.EXPECT().MachineCountForBase(makeBases("ubuntu", ubuntuVersions)).Return(map[string]int{"xenial": 2}, nil) + ctrlState.EXPECT().MachineCountForBase(makeBases("ubuntu", unsupportedUbuntuVersions)).Return(map[string]int{"xenial": 2}, nil) // - check if model has charm store charms; ctrlState.EXPECT().AllCharmURLs().Return(nil, errors.NotFoundf("charms")) // - check LXD version. @@ -509,7 +509,7 @@ func (s *modelUpgradeSuite) TestUpgradeModelForControllerModelJuju3Failed(c *gc. // - check if the model has win machines; state1.EXPECT().MachineCountForBase(makeBases("windows", winVersions)).Return(map[string]int{"win10": 1, "win7": 3}, nil) // - check if the model has deprecated ubuntu machines; - state1.EXPECT().MachineCountForBase(makeBases("ubuntu", ubuntuVersions)).Return(map[string]int{ + state1.EXPECT().MachineCountForBase(makeBases("ubuntu", unsupportedUbuntuVersions)).Return(map[string]int{ "artful": 1, "cosmic": 2, "disco": 3, "eoan": 4, "groovy": 5, "hirsute": 6, "impish": 7, "precise": 8, "quantal": 9, "raring": 10, "saucy": 11, "trusty": 12, "utopic": 13, "vivid": 14, "wily": 15, @@ -597,7 +597,7 @@ func (s *modelUpgradeSuite) assertUpgradeModelJuju3(c *gc.C, ctrlModelVers strin // - check if the model has win machines; st.EXPECT().MachineCountForBase(makeBases("windows", winVersions)).Return(nil, nil) // - check if the model has deprecated ubuntu machines; - st.EXPECT().MachineCountForBase(makeBases("ubuntu", ubuntuVersions)).Return(nil, nil) + st.EXPECT().MachineCountForBase(makeBases("ubuntu", unsupportedUbuntuVersions)).Return(nil, nil) // - check LXD version. serverFactory.EXPECT().RemoteServer(s.cloudSpec).Return(server, nil) server.EXPECT().ServerVersion().Return("5.2") @@ -678,7 +678,7 @@ func (s *modelUpgradeSuite) TestUpgradeModelJuju3Failed(c *gc.C) { // - check if the model has win machines; st.EXPECT().MachineCountForBase(makeBases("windows", winVersions)).Return(map[string]int{"win10": 1, "win7": 3}, nil) // - check if the model has deprecated ubuntu machines; - st.EXPECT().MachineCountForBase(makeBases("ubuntu", ubuntuVersions)).Return(map[string]int{ + st.EXPECT().MachineCountForBase(makeBases("ubuntu", unsupportedUbuntuVersions)).Return(map[string]int{ "artful": 1, "cosmic": 2, "disco": 3, "eoan": 4, "groovy": 5, "hirsute": 6, "impish": 7, "precise": 8, "quantal": 9, "raring": 10, "saucy": 11, "trusty": 12, "utopic": 13, "vivid": 14, "wily": 15, diff --git a/core/base/supportedseries_linux_test.go b/core/base/supportedseries_linux_test.go index 01ede6f4bc7..a420b5266dc 100644 --- a/core/base/supportedseries_linux_test.go +++ b/core/base/supportedseries_linux_test.go @@ -33,7 +33,7 @@ func (s *SupportedSeriesLinuxSuite) TestLatestLts(c *gc.C) { latest, want string }{ {"testseries", "testseries"}, - {"", "jammy"}, + {"", "noble"}, } for _, test := range table { SetLatestLtsForTesting(test.latest) diff --git a/core/charm/baseselector_test.go b/core/charm/baseselector_test.go index d86d54d039e..1fc07e058f0 100644 --- a/core/charm/baseselector_test.go +++ b/core/charm/baseselector_test.go @@ -146,7 +146,7 @@ func (s *baseSelectorSuite) TestCharmBase(c *gc.C) { selector: BaseSelector{ supportedBases: []base.Base{utopic, vivid, jujuDefault, latest}, }, - expectedBase: latest, + expectedBase: jujuDefault, }, } diff --git a/provider/ec2/series_test.go b/provider/ec2/series_test.go index ca12ccda864..4e487670984 100644 --- a/provider/ec2/series_test.go +++ b/provider/ec2/series_test.go @@ -134,6 +134,61 @@ const testImageMetadataProduct = ` { "content_id": "com.ubuntu.cloud:released:aws", "products": { + "com.ubuntu.cloud:server:24.04:amd64": { + "release": "noble", + "version": "24.04", + "arch": "amd64", + "versions": { + "20121218": { + "items": { + "usee1pi": { + "root_store": "instance", + "virt": "pv", + "region": "us-east-1", + "id": "ami-02204111" + }, + "usww1pe": { + "root_store": "ssd", + "virt": "pv", + "region": "eu-west-1", + "id": "ami-02204116" + }, + "apne1pe": { + "root_store": "ssd", + "virt": "pv", + "region": "ap-northeast-1", + "id": "ami-02204126" + }, + "apne1he": { + "root_store": "ssd", + "virt": "hvm", + "region": "ap-northeast-1", + "id": "ami-02204187" + }, + "test1peebs": { + "root_store": "ssd", + "virt": "pv", + "region": "test", + "id": "ami-02204133" + }, + "test1pessd": { + "root_store": "ebs", + "virt": "pv", + "region": "test", + "id": "ami-02204139" + }, + "test1he": { + "root_store": "ssd", + "virt": "hvm", + "region": "test", + "id": "ami-02204135" + } + }, + "pubname": "ubuntu-noble-24.04-amd64-server-20121218", + "label": "release" + } + } + }, "com.ubuntu.cloud:server:22.04:amd64": { "release": "jammy", "version": "22.04", diff --git a/upgrades/upgradevalidation/migrate_test.go b/upgrades/upgradevalidation/migrate_test.go index 857415d886a..95f1cd62cae 100644 --- a/upgrades/upgradevalidation/migrate_test.go +++ b/upgrades/upgradevalidation/migrate_test.go @@ -23,7 +23,7 @@ var winVersions = []string{ "win2016", "win2016hv", "win2019", "win7", "win8", "win81", "win10", } -var ubuntuVersions = []string{ +var unsupportedUbuntuVersions = []string{ "12.04", "12.10", "13.04", @@ -46,7 +46,6 @@ var ubuntuVersions = []string{ "22.10", "23.04", "23.10", - "24.04", } func makeBases(os string, vers []string) []state.Base { @@ -119,7 +118,7 @@ func (s *migrateSuite) setupMocks(c *gc.C) (*gomock.Controller, environscloudspe s.st.EXPECT().HasUpgradeSeriesLocks().Return(false, nil) // - check if the model has win machines; s.st.EXPECT().MachineCountForBase(makeBases("windows", winVersions)).Return(nil, nil) - s.st.EXPECT().MachineCountForBase(makeBases("ubuntu", ubuntuVersions)).Return(nil, nil) + s.st.EXPECT().MachineCountForBase(makeBases("ubuntu", unsupportedUbuntuVersions)).Return(nil, nil) // - check no charm store charms s.st.EXPECT().AllCharmURLs().Return([]*string{}, errors.NotFoundf("charm urls")) diff --git a/upgrades/upgradevalidation/upgrade_test.go b/upgrades/upgradevalidation/upgrade_test.go index 848ad3005fa..d2d94435365 100644 --- a/upgrades/upgradevalidation/upgrade_test.go +++ b/upgrades/upgradevalidation/upgrade_test.go @@ -74,7 +74,7 @@ func (s *upgradeValidationSuite) TestValidatorsForControllerUpgradeJuju3(c *gc.C statePool.EXPECT().MongoVersion().Return("4.4", nil) // - check if the model has win machines; ctrlState.EXPECT().MachineCountForBase(makeBases("windows", winVersions)).Return(nil, nil) - ctrlState.EXPECT().MachineCountForBase(makeBases("ubuntu", ubuntuVersions)).Return(nil, nil) + ctrlState.EXPECT().MachineCountForBase(makeBases("ubuntu", unsupportedUbuntuVersions)).Return(nil, nil) // - check no charm store charms ctrlState.EXPECT().AllCharmURLs().Return([]*string{}, errors.NotFoundf("charm urls")) // - check LXD version. @@ -87,7 +87,7 @@ func (s *upgradeValidationSuite) TestValidatorsForControllerUpgradeJuju3(c *gc.C model1.EXPECT().MigrationMode().Return(state.MigrationModeNone) // - check if the model has win machines; state1.EXPECT().MachineCountForBase(makeBases("windows", winVersions)).Return(nil, nil) - state1.EXPECT().MachineCountForBase(makeBases("ubuntu", ubuntuVersions)).Return(nil, nil) + state1.EXPECT().MachineCountForBase(makeBases("ubuntu", unsupportedUbuntuVersions)).Return(nil, nil) // - check no charm store charms state1.EXPECT().AllCharmURLs().Return([]*string{}, errors.NotFoundf("charm urls")) // - check LXD version. @@ -130,7 +130,7 @@ func (s *upgradeValidationSuite) TestValidatorsForModelUpgradeJuju3(c *gc.C) { state.EXPECT().HasUpgradeSeriesLocks().Return(false, nil) // - check if the model has win machines. state.EXPECT().MachineCountForBase(makeBases("windows", winVersions)).Return(nil, nil) - state.EXPECT().MachineCountForBase(makeBases("ubuntu", ubuntuVersions)).Return(nil, nil) + state.EXPECT().MachineCountForBase(makeBases("ubuntu", unsupportedUbuntuVersions)).Return(nil, nil) // - check LXD version. serverFactory.EXPECT().RemoteServer(cloudSpec).Return(server, nil) server.EXPECT().ServerVersion().Return("5.2") diff --git a/upgrades/upgradevalidation/validation_test.go b/upgrades/upgradevalidation/validation_test.go index 73ba1003ca3..7c70082f05e 100644 --- a/upgrades/upgradevalidation/validation_test.go +++ b/upgrades/upgradevalidation/validation_test.go @@ -134,7 +134,7 @@ func (s *upgradeValidationSuite) TestCheckForDeprecatedUbuntuSeriesForModel(c *g defer ctrl.Finish() st := mocks.NewMockState(ctrl) - st.EXPECT().MachineCountForBase(makeBases("ubuntu", ubuntuVersions)).Return(map[string]int{"xenial": 1, "vivid": 2, "trusty": 3}, nil) + st.EXPECT().MachineCountForBase(makeBases("ubuntu", unsupportedUbuntuVersions)).Return(map[string]int{"xenial": 1, "vivid": 2, "trusty": 3}, nil) blocker, err := upgradevalidation.CheckForDeprecatedUbuntuSeriesForModel("", nil, st, nil) c.Assert(err, jc.ErrorIsNil) From f9be7989d147b7ca789ed6ab00ba75199d04e7aa Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Thu, 13 Jun 2024 16:16:50 +0200 Subject: [PATCH 08/10] fix: stop setting a different LatestLTS to test. There was a bug in this test where the "latest" variable used in the table driven tests was set to LastestLTS before the mock value was set. This meant it was 24.04 not the mocked value of 20.04. This caused the tests to fail. By removing this mocking we avoid this problem. The latest value is the same globally and locally --- core/charm/baseselector_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/core/charm/baseselector_test.go b/core/charm/baseselector_test.go index 1fc07e058f0..5fb597de512 100644 --- a/core/charm/baseselector_test.go +++ b/core/charm/baseselector_test.go @@ -150,10 +150,6 @@ func (s *baseSelectorSuite) TestCharmBase(c *gc.C) { }, } - // Use bionic for LTS for all calls. - previous := base.SetLatestLtsForTesting("focal") - defer base.SetLatestLtsForTesting(previous) - for i, test := range deployBasesTests { c.Logf("test %d [%s]", i, test.title) test.selector.logger = s.logger From 30ddcc87dde320d44891cc9e3b34aace9b4f3353 Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Thu, 13 Jun 2024 16:19:41 +0200 Subject: [PATCH 09/10] fix: add 24.04 to the faked versions TestSystemdBootstrapInstanceUserDataAndState uses these faked versions with the latest LTS which is 24.04. It not being here caused the test to fail. --- testing/environ.go | 1 + 1 file changed, 1 insertion(+) diff --git a/testing/environ.go b/testing/environ.go index 7d4692b9f52..af985f29e89 100644 --- a/testing/environ.go +++ b/testing/environ.go @@ -42,6 +42,7 @@ var ( FakeSupportedJujuBases = []corebase.Base{ corebase.MustParseBaseFromString("ubuntu@20.04"), corebase.MustParseBaseFromString("ubuntu@22.04"), + corebase.MustParseBaseFromString("ubuntu@24.04"), jujuversion.DefaultSupportedLTSBase(), } ) From 0e91e6e462d07f11ee9d2bca490d6fc47868da48 Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Mon, 17 Jun 2024 16:27:13 +0200 Subject: [PATCH 10/10] fix: default to deploying to latest LTS After discussion, we decided that it is better to actually default to the latest LTS if the charm author has eplicitly decided to support it (i.e. has put it as a supported version in charmcraft.yaml) and we should't try to second guess them --- .../deployer/bundlehandler_test.go | 26 +++++++++---------- core/charm/baseselector.go | 16 ++++++------ core/charm/baseselector_test.go | 2 +- .../charms/juju-qa-test/charmcraft.yaml | 12 ++++----- 4 files changed, 27 insertions(+), 29 deletions(-) diff --git a/cmd/juju/application/deployer/bundlehandler_test.go b/cmd/juju/application/deployer/bundlehandler_test.go index 385c7bd4c2c..29e8f4145a5 100644 --- a/cmd/juju/application/deployer/bundlehandler_test.go +++ b/cmd/juju/application/deployer/bundlehandler_test.go @@ -267,8 +267,8 @@ func (s *BundleDeployRepositorySuite) TestDeployKubernetesBundleSuccess(c *gc.C) s.expectEmptyModelToStart(c) s.expectWatchAll() - mariadbCurl := charm.MustParseURL("ch:jammy/mariadb-k8s") - gitlabCurl := charm.MustParseURL("ch:jammy/gitlab-k8s") + mariadbCurl := charm.MustParseURL("ch:noble/mariadb-k8s") + gitlabCurl := charm.MustParseURL("ch:noble/gitlab-k8s") chUnits := []charmUnit{ { charmMetaSeries: []string{"kubernetes"}, @@ -287,8 +287,8 @@ func (s *BundleDeployRepositorySuite) TestDeployKubernetesBundleSuccess(c *gc.C) s.runDeploy(c, kubernetesGitlabBundle) c.Assert(s.deployArgs, gc.HasLen, 2) - s.assertDeployArgs(c, gitlabCurl.String(), "gitlab", "ubuntu", "22.04") - s.assertDeployArgs(c, mariadbCurl.String(), "mariadb", "ubuntu", "22.04") + s.assertDeployArgs(c, gitlabCurl.String(), "gitlab", "ubuntu", "24.04") + s.assertDeployArgs(c, mariadbCurl.String(), "mariadb", "ubuntu", "24.04") s.assertDeployArgsStorage(c, "mariadb", map[string]storage.Constraints{"database": {Pool: "mariadb-pv", Size: 0x14, Count: 0x1}}) s.assertDeployArgsConfig(c, "mariadb", map[string]interface{}{"dataset-size": "70%"}) @@ -1256,7 +1256,7 @@ func (s *BundleDeployRepositorySuite) TestDeployBundleUnitPlacedInApplication(c s.expectWatchAll() s.expectResolveCharm(nil) - wordpressCurl := charm.MustParseURL("ch:jammy/wordpress") + wordpressCurl := charm.MustParseURL("ch:noble/wordpress") s.expectResolveCharm(nil) charmInfo := &apicharms.CharmInfo{ Revision: wordpressCurl.Revision, @@ -1277,7 +1277,7 @@ func (s *BundleDeployRepositorySuite) TestDeployBundleUnitPlacedInApplication(c {Entity: ¶ms.UnitInfo{Name: "wordpress/1", MachineId: "1"}}, }, nil) - djangoCurl := charm.MustParseURL("ch:jammy/django") + djangoCurl := charm.MustParseURL("ch:noble/django") s.expectResolveCharm(nil) charmInfo2 := &apicharms.CharmInfo{ Revision: djangoCurl.Revision, @@ -1327,7 +1327,7 @@ func (s *BundleDeployRepositorySuite) TestDeployBundlePeerContainer(c *gc.C) { s.expectAddContainer("0", "0/lxd/1", "", "lxd") s.expectAddContainer("1", "1/lxd/1", "", "lxd") - wordpressCurl := charm.MustParseURL("ch:jammy/wordpress") + wordpressCurl := charm.MustParseURL("ch:noble/wordpress") s.expectResolveCharm(nil) charmInfo := &apicharms.CharmInfo{ URL: wordpressCurl.String(), @@ -1341,7 +1341,7 @@ func (s *BundleDeployRepositorySuite) TestDeployBundlePeerContainer(c *gc.C) { s.expectAddOneUnit("wordpress", "0/lxd/0", "0") s.expectAddOneUnit("wordpress", "1/lxd/0", "1") - djangoCurl := charm.MustParseURL("ch:jammy/django") + djangoCurl := charm.MustParseURL("ch:noble/django") s.expectResolveCharm(nil) charmInfo2 := &apicharms.CharmInfo{ URL: djangoCurl.String(), @@ -1543,8 +1543,8 @@ func (s *BundleDeployRepositorySuite) TestDeployBundleAnnotations(c *gc.C) { s.expectEmptyModelToStart(c) s.expectWatchAll() - djangoCurl := charm.MustParseURL("ch:jammy/django") - memCurl := charm.MustParseURL("ch:jammy/mem") + djangoCurl := charm.MustParseURL("ch:noble/django") + memCurl := charm.MustParseURL("ch:noble/mem") chUnits := []charmUnit{ { curl: memCurl, @@ -1761,12 +1761,12 @@ func (s *BundleDeployRepositorySuite) TestDeployBundleExpose(c *gc.C) { s.expectEmptyModelToStart(c) s.expectWatchAll() - wordpressCurl := charm.MustParseURL("ch:jammy/wordpress") + wordpressCurl := charm.MustParseURL("ch:noble/wordpress") chUnits := []charmUnit{ { charmMetaSeries: []string{"jammy", "focal"}, curl: wordpressCurl, - machineUbuntuVersion: "22.04", + machineUbuntuVersion: "24.04", }, } s.setupCharmUnits(chUnits) @@ -1781,7 +1781,7 @@ applications: ` s.runDeploy(c, content) - s.assertDeployArgs(c, wordpressCurl.String(), "wordpress", "ubuntu", "22.04") + s.assertDeployArgs(c, wordpressCurl.String(), "wordpress", "ubuntu", "24.04") c.Check(s.output.String(), gc.Equals, ""+ "Located charm \"wordpress\" in charm-hub\n"+ "Executing changes:\n"+ diff --git a/core/charm/baseselector.go b/core/charm/baseselector.go index 58131d40c85..9fcb25b4ff8 100644 --- a/core/charm/baseselector.go +++ b/core/charm/baseselector.go @@ -132,8 +132,8 @@ func (s BaseSelector) validate(supportedCharmBases, supportedJujuBases []base.Ba // Order of preference is: // - user requested with --base or defined by bundle when deploying // - model default, if set, acts like --base -// - juju's default supported Ubuntu LTS (if compatible with valid charm bases) // - the latest known Ubuntu LTS (if compatible with valid charm bases) +// - juju's default supported Ubuntu LTS (if compatible with valid charm bases) // - the first supported base in the charm manifest // - in the case of local charms with no manifest nor base in metadata, // base must be provided by the user. @@ -157,13 +157,6 @@ func (s BaseSelector) CharmBase() (selectedBase base.Base, err error) { return s.userRequested(s.defaultBase) } - // Try juju's current default supported Ubuntu LTS - jujuDefaultBase, err := BaseForCharm(version.DefaultSupportedLTSBase(), s.supportedBases) - if err == nil { - s.logger.Infof(msgLatestLTSBase, version.DefaultSupportedLTSBase()) - return jujuDefaultBase, nil - } - // Prefer latest Ubuntu LTS. preferredBase, err := BaseForCharm(base.LatestLTSBase(), s.supportedBases) if err == nil { @@ -173,6 +166,13 @@ func (s BaseSelector) CharmBase() (selectedBase base.Base, err error) { return base.Base{}, err } + // Try juju's current default supported Ubuntu LTS + jujuDefaultBase, err := BaseForCharm(version.DefaultSupportedLTSBase(), s.supportedBases) + if err == nil { + s.logger.Infof(msgLatestLTSBase, version.DefaultSupportedLTSBase()) + return jujuDefaultBase, nil + } + // Last chance, the first base in the charm's manifest return BaseForCharm(base.Base{}, s.supportedBases) } diff --git a/core/charm/baseselector_test.go b/core/charm/baseselector_test.go index 5fb597de512..7f8e7a68849 100644 --- a/core/charm/baseselector_test.go +++ b/core/charm/baseselector_test.go @@ -146,7 +146,7 @@ func (s *baseSelectorSuite) TestCharmBase(c *gc.C) { selector: BaseSelector{ supportedBases: []base.Base{utopic, vivid, jujuDefault, latest}, }, - expectedBase: jujuDefault, + expectedBase: latest, }, } diff --git a/testcharms/charm-hub/charms/juju-qa-test/charmcraft.yaml b/testcharms/charm-hub/charms/juju-qa-test/charmcraft.yaml index 26272195fd4..565f3175ba7 100644 --- a/testcharms/charm-hub/charms/juju-qa-test/charmcraft.yaml +++ b/testcharms/charm-hub/charms/juju-qa-test/charmcraft.yaml @@ -11,19 +11,17 @@ parts: - actions.yaml - metadata.yaml - config.yaml + bases: - build-on: - - name: "ubuntu" - channel: "20.04" - name: "ubuntu" channel: "22.04" - run-on: - name: "ubuntu" - channel: "16.04" - architectures: ["amd64", "arm64"] + channel: "24.04" + run-on: - name: "ubuntu" - channel: "18.04" + channel: "22.04" architectures: ["amd64", "arm64"] - name: "ubuntu" - channel: "20.04" + channel: "24.04" architectures: ["amd64", "arm64"]