From 323f8dd90b95ce8b1b21a9ec3b6c7af95f246a91 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Tue, 3 Dec 2024 16:11:28 +0100 Subject: [PATCH 01/17] remove controller watcher --- cmd/jimmsrv/main.go | 6 +- cmd/jimmsrv/service/service.go | 27 +- internal/jimm/controller.go | 46 -- internal/jimm/export_test.go | 4 - internal/jimm/model_poller.go | 58 ++ internal/jimm/watcher.go | 350 ----------- internal/jimm/watcher_test._go | 1070 -------------------------------- internal/jimm/watcher_test.go | 360 +++++++++++ 8 files changed, 437 insertions(+), 1484 deletions(-) create mode 100644 internal/jimm/model_poller.go delete mode 100644 internal/jimm/watcher_test._go create mode 100644 internal/jimm/watcher_test.go diff --git a/cmd/jimmsrv/main.go b/cmd/jimmsrv/main.go index 30beec722..6e4c59e11 100644 --- a/cmd/jimmsrv/main.go +++ b/cmd/jimmsrv/main.go @@ -189,9 +189,6 @@ func start(ctx context.Context, s *service.Service) error { } isLeader := os.Getenv("JIMM_IS_LEADER") != "" - if isLeader { - s.Go(func() error { return jimmsvc.WatchControllers(ctx) }) // Deletes dead/dying models, updates model config. - } s.Go(func() error { return jimmsvc.WatchModelSummaries(ctx) }) if isLeader { @@ -206,6 +203,9 @@ func start(ctx context.Context, s *service.Service) error { s.Go(func() error { return jimmsvc.OpenFGACleanup(ctx, time.NewTicker(6*time.Hour).C) }) + s.Go(func() error { + return jimmsvc.WatchModelsDying(ctx, time.NewTicker(time.Minute).C) + }) } if isLeader { diff --git a/cmd/jimmsrv/service/service.go b/cmd/jimmsrv/service/service.go index 232f1a980..ae4dc3186 100644 --- a/cmd/jimmsrv/service/service.go +++ b/cmd/jimmsrv/service/service.go @@ -209,17 +209,6 @@ func (s *Service) ServeHTTP(w http.ResponseWriter, req *http.Request) { s.mux.ServeHTTP(w, req) } -// WatchControllers connects to all controllers and starts an AllWatcher -// monitoring all changes to models. WatchControllers finishes when the -// given context is canceled, or there is a fatal error watching models. -func (s *Service) WatchControllers(ctx context.Context) error { - w := jimm.Watcher{ - Database: s.jimm.Database, - Dialer: s.jimm.Dialer, - } - return w.Watch(ctx, 10*time.Minute) -} - // WatchModelSummaries connects to all controllers and starts a // ModelSummaryWatcher for all models. WatchModelSummaries finishes when // the given context is canceled, or there is a fatal error watching model @@ -272,6 +261,22 @@ func (s *Service) OpenFGACleanup(ctx context.Context, trigger <-chan time.Time) } } +// OpenFGACleanup starts a goroutine that cleans up any orphaned tuples from OpenFGA. +func (s *Service) WatchModelsDying(ctx context.Context, trigger <-chan time.Time) error { + for { + select { + case <-trigger: + err := s.jimm.WatchModelsDying(ctx) + if err != nil { + zapctx.Error(ctx, "openfga cleanup", zap.Error(err)) + continue + } + case <-ctx.Done(): + return nil + } + } +} + // Cleanup cleans up resources that need to be released on shutdown. func (s *Service) Cleanup() { // Iterating over clean up function in reverse-order to avoid early clean ups. diff --git a/internal/jimm/controller.go b/internal/jimm/controller.go index db8673951..6ac686754 100644 --- a/internal/jimm/controller.go +++ b/internal/jimm/controller.go @@ -629,52 +629,6 @@ func (j *JIMM) ImportModel(ctx context.Context, user *openfga.User, controllerNa return errors.E(op, err) } - return importer.handleModelDeltas(ctx) -} - -func (m *modelImporter) handleModelDeltas(ctx context.Context) error { - const op = errors.Op("jimm.getModelDeltas") - - modelAPI, err := m.jimm.dialModel(ctx, &m.model.Controller, m.model.ResourceTag()) - if err != nil { - return errors.E(op, err) - } - defer modelAPI.Close() - - watcherID, err := modelAPI.WatchAll(ctx) - if err != nil { - return errors.E(op, err) - } - defer func() { - if err := modelAPI.ModelWatcherStop(ctx, watcherID); err != nil { - zapctx.Error(ctx, "failed to stop model watcher", zap.Error(err)) - } - }() - - deltas, err := modelAPI.ModelWatcherNext(ctx, watcherID) - if err != nil { - return errors.E(op, err) - } - - modelIDf := func(uuid string) *modelState { - if uuid == m.model.UUID.String { - return &modelState{ - id: m.model.ID, - machines: make(map[string]int64), - units: make(map[string]bool), - } - } - return nil - } - - w := &Watcher{ - Database: m.jimm.Database, - } - for _, d := range deltas { - if err := w.handleDelta(ctx, modelIDf, d); err != nil { - return errors.E(op, err) - } - } return nil } diff --git a/internal/jimm/export_test.go b/internal/jimm/export_test.go index 6e3b27b63..a0043087f 100644 --- a/internal/jimm/export_test.go +++ b/internal/jimm/export_test.go @@ -24,10 +24,6 @@ var ( ResolveTag = resolveTag ) -func WatchController(w *Watcher, ctx context.Context, ctl *dbmodel.Controller) error { - return w.watchController(ctx, ctl) -} - func NewWatcherWithControllerUnavailableChan(db db.Database, dialer Dialer, pubsub Publisher, testChannel chan error) *Watcher { return &Watcher{ Pubsub: pubsub, diff --git a/internal/jimm/model_poller.go b/internal/jimm/model_poller.go new file mode 100644 index 000000000..365d16503 --- /dev/null +++ b/internal/jimm/model_poller.go @@ -0,0 +1,58 @@ +// Copyright 2024 Canonical. +package jimm + +import ( + "context" + + jujuparams "github.com/juju/juju/rpc/params" + "github.com/juju/juju/state" + "github.com/juju/zaputil/zapctx" + "go.uber.org/zap" + + "github.com/canonical/jimm/v3/internal/dbmodel" + "github.com/canonical/jimm/v3/internal/errors" +) + +func (j *JIMM) WatchModelsDying(ctx context.Context) error { + const op = errors.Op("jimm.WatchModelsDying") + + // Ensure that if the watcher stops because of a database error all + // the controller connections get closed. + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + adminUser := j.everyoneUser() + adminUser.JimmAdmin = true + err := j.ForEachModel(ctx, adminUser, func(m *dbmodel.Model, _ jujuparams.UserAccessPermission) error { + if m.Life == state.Dying.String() { + mt := m.ResourceTag() + // if the model is dying and not found by querying the controller we can assume it is dead. + // And safely delete the reference from our db. + j.doModelAdmin(ctx, adminUser, mt, func(m *dbmodel.Model, api API) error { + if err := api.ModelInfo(ctx, &jujuparams.ModelInfo{}); err != nil { + // Some versions of juju return unauthorized for models that cannot be found. + if errors.ErrorCode(err) == errors.CodeNotFound || errors.ErrorCode(err) == errors.CodeUnauthorized { + if err := j.DB().DeleteModel(ctx, m); err != nil { + return errors.E(op, err) + } else { + return nil + } + } else { + return errors.E(op, err) + } + } + return nil + }) + } + + return nil + }) + if err != nil { + // Ignore temporary database errors. + if errors.ErrorCode(err) != errors.CodeDatabaseLocked { + return errors.E(op, err) + } + zapctx.Warn(ctx, "temporary error polling for controllers", zap.Error(err)) + } + return nil +} diff --git a/internal/jimm/watcher.go b/internal/jimm/watcher.go index ff07dfe12..b81e368d4 100644 --- a/internal/jimm/watcher.go +++ b/internal/jimm/watcher.go @@ -7,8 +7,6 @@ import ( "database/sql" "time" - jujuparams "github.com/juju/juju/rpc/params" - "github.com/juju/juju/state" "github.com/juju/names/v5" "github.com/juju/zaputil/zapctx" "go.uber.org/zap" @@ -16,7 +14,6 @@ import ( "github.com/canonical/jimm/v3/internal/db" "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/errors" - "github.com/canonical/jimm/v3/internal/servermon" ) // Publisher defines the interface used by the Watcher @@ -42,49 +39,6 @@ type Watcher struct { deltaProcessedChan chan bool } -// Watch starts the watcher which connects to all known controllers and -// monitors them for updates. Watch polls the database at the given -// interval to find any new controllers to watch. Watch blocks until either -// the given context is closed, or there is an error querying the database. -func (w *Watcher) Watch(ctx context.Context, interval time.Duration) error { - const op = errors.Op("jimm.Watch") - - r := newRunner() - // Ensure that all started goroutines are completed before we return. - defer r.wait() - - // Ensure that if the watcher stops because of a database error all - // the controller connections get closed. - ctx, cancel := context.WithCancel(ctx) - defer cancel() - - ticker := time.NewTicker(interval) - defer ticker.Stop() - for { - err := w.Database.ForEachController(ctx, func(ctl *dbmodel.Controller) error { - ctx := zapctx.WithFields(ctx, zap.String("controller", ctl.Name)) - r.run(ctl.Name, func() { - zapctx.Info(ctx, "starting controller watcher") - err := w.watchController(ctx, ctl) - zapctx.Error(ctx, "controller watcher stopped", zap.Error(err)) - }) - return nil - }) - if err != nil { - // Ignore temporary database errors. - if errors.ErrorCode(err) != errors.CodeDatabaseLocked { - return errors.E(op, err) - } - zapctx.Warn(ctx, "temporary error polling for controllers", zap.Error(err)) - } - select { - case <-ctx.Done(): - return ctx.Err() - case <-ticker.C: - } - } -} - // WatchAllModelSummaries starts the watcher which connects to all known // controllers and monitors them for model summary updates. // WatchAllModelSummaries polls the database at the given @@ -179,187 +133,6 @@ type modelState struct { units map[string]bool } -func (w *Watcher) checkControllerModels(ctx context.Context, ctl *dbmodel.Controller, checks ...func(*dbmodel.Model) error) (map[string]*modelState, error) { - const op = errors.Op("jimm.checkControllerModels") - - // modelIDs contains the set of models running on the - // controller that JIMM is interested in. - modelStates := make(map[string]*modelState) - // find all the models we expect to get deltas from initially. - err := w.Database.ForEachControllerModel(ctx, ctl, func(m *dbmodel.Model) error { - // models without a UUID are currently being initialised - // and we don't want to check for those yet. - if !m.UUID.Valid { - return nil - } - - for _, check := range checks { - err := check(m) - if err != nil { - return errors.E(op, err) - } - } - modelStates[m.UUID.String] = &modelState{ - id: m.ID, - machines: make(map[string]int64), - units: make(map[string]bool), - } - return nil - }) - if err != nil { - return nil, errors.E(op, err) - } - return modelStates, nil -} - -func (w *Watcher) deltaProcessedNotification() { - if w.deltaProcessedChan != nil { - select { - case w.deltaProcessedChan <- true: - default: - } - } -} - -// watchController connects to the given controller and watches for model -// changes on the controller. -// -// nolint:gocognit // We ignore watch as watchers are removed in Juju 4.0. -func (w *Watcher) watchController(ctx context.Context, ctl *dbmodel.Controller) error { - const op = errors.Op("jimm.watchController") - - // connect to the controller - api, err := w.dialController(ctx, ctl) - if err != nil { - return errors.E(op, err) - } - defer api.Close() - // start the all watcher - id, err := api.WatchAllModels(ctx) - if err != nil { - return errors.E(op, err) - } - defer func() { - if err := api.AllModelWatcherStop(ctx, id); err != nil { - zapctx.Error(ctx, "failed to stop all model watcher", zap.Error(err)) - } - }() - - checkDyingModel := func(m *dbmodel.Model) error { - if m.Life == state.Dying.String() || m.Life == state.Dead.String() { - // models that were in the dying state may no - // longer be on the controller, check if it should - // be immediately deleted. - mi := jujuparams.ModelInfo{ - UUID: m.UUID.String, - } - if err := api.ModelInfo(ctx, &mi); err != nil { - // Some versions of juju return unauthorized for models that cannot be found. - if errors.ErrorCode(err) == errors.CodeNotFound || errors.ErrorCode(err) == errors.CodeUnauthorized { - if err := w.Database.DeleteModel(ctx, m); err != nil { - return errors.E(op, err) - } else { - return nil - } - } else { - return errors.E(op, err) - } - } - } - return nil - } - - // modelStates contains the set of models running on the - // controller that JIMM is interested in. The function also - // check for any dying models and deletes them where necessary. - modelStates, err := w.checkControllerModels(ctx, ctl, checkDyingModel) - if err != nil { - return errors.E(op, err) - } - - modelStatef := func(uuid string) *modelState { - state, ok := modelStates[uuid] - if ok { - return state - } - m := dbmodel.Model{ - UUID: sql.NullString{ - String: uuid, - Valid: true, - }, - ControllerID: ctl.ID, - } - err := w.Database.GetModel(ctx, &m) - switch { - case err == nil: - st := modelState{ - id: m.ID, - machines: make(map[string]int64), - units: make(map[string]bool), - } - modelStates[uuid] = &st - case errors.ErrorCode(err) == errors.CodeNotFound: - modelStates[uuid] = nil - default: - zapctx.Error(ctx, "cannot get model", zap.Error(err)) - } - return modelStates[uuid] - } - - for { - // wait for updates from the all watcher. - deltas, err := api.AllModelWatcherNext(ctx, id) - if err != nil { - return errors.E(op, err) - } - servermon.MonitorDeltasReceivedCount.WithLabelValues(ctl.UUID).Add(float64(len(deltas))) - for _, d := range deltas { - eid := d.Entity.EntityId() - ctx := zapctx.WithFields(ctx, zap.String("model-uuid", eid.ModelUUID), zap.String("kind", eid.Kind), zap.String("id", eid.Id)) - zapctx.Debug(ctx, "processing delta") - if err := w.handleDelta(ctx, modelStatef, d); err != nil { - return errors.E(op, err) - } - } - for k, v := range modelStates { - if v == nil { - // If we have cached not to process a model - // remove it so we check again next time. - delete(modelStates, k) - continue - } - if v.changed { - v.changed = false - // Update changed model. - err := w.Database.Transaction(func(tx *db.Database) error { - m := dbmodel.Model{ - ID: v.id, - } - if err := tx.GetModel(ctx, &m); err != nil { - return err - } - var machines, cores int64 - for _, n := range v.machines { - machines++ - cores += n - } - // m.Cores = cores - // m.Machines = machines - // m.Units = int64(len(v.units)) - if err := tx.UpdateModel(ctx, &m); err != nil { - return err - } - return nil - }) - if err != nil { - zapctx.Error(ctx, "cannot get model for update", zap.Error(err)) - continue - } - } - } - } -} - // watchAllModelSummaries connects to the given controller and watches the // summary updates. func (w *Watcher) watchAllModelSummaries(ctx context.Context, ctl *dbmodel.Controller) error { @@ -387,36 +160,6 @@ func (w *Watcher) watchAllModelSummaries(ctx context.Context, ctl *dbmodel.Contr } }() - // modelIDs contains the set of models running on the - // controller that JIMM is interested in. - modelStates, err := w.checkControllerModels(ctx, ctl) - if err != nil { - return errors.E(op, err) - } - - modelIDf := func(uuid string) uint { - state, ok := modelStates[uuid] - if ok { - return state.id - } - m := dbmodel.Model{ - UUID: sql.NullString{ - String: uuid, - Valid: true, - }, - ControllerID: ctl.ID, - } - err := w.Database.GetModel(ctx, &m) - if err == nil || errors.ErrorCode(err) == errors.CodeNotFound { - modelStates[uuid] = &modelState{ - id: m.ID, - } - return m.ID - } - zapctx.Error(ctx, "cannot get model", zap.Error(err)) - return 0 - } - for { select { case <-ctx.Done(): @@ -430,12 +173,6 @@ func (w *Watcher) watchAllModelSummaries(ctx context.Context, ctl *dbmodel.Contr } // Sanitize the model abstracts. for _, summary := range modelSummaries { - modelID := modelIDf(summary.UUID) - if modelID == 0 { - // skip unknown models - continue - } - summary := summary admins := make([]string, 0, len(summary.Admins)) for _, admin := range summary.Admins { if names.NewUserTag(admin).IsLocal() { @@ -449,90 +186,3 @@ func (w *Watcher) watchAllModelSummaries(ctx context.Context, ctl *dbmodel.Contr } } } - -func (w *Watcher) handleDelta(ctx context.Context, modelIDf func(string) *modelState, d jujuparams.Delta) error { - defer w.deltaProcessedNotification() - eid := d.Entity.EntityId() - state := modelIDf(eid.ModelUUID) - if state == nil { - return nil - } - switch eid.Kind { - case "machine": - if d.Removed { - state.changed = true - delete(state.machines, eid.Id) - return nil - } - var cores int64 - machine := d.Entity.(*jujuparams.MachineInfo) - if machine.HardwareCharacteristics != nil && machine.HardwareCharacteristics.CpuCores != nil { - //nolint:gosec // We expect cpu cores to fit into int64. - cores = int64(*machine.HardwareCharacteristics.CpuCores) - } - sCores, ok := state.machines[eid.Id] - if !ok || sCores != cores { - state.machines[eid.Id] = cores - state.changed = true - } - case "model": - model := dbmodel.Model{ - ID: state.id, - } - if d.Removed { - return w.deleteModel(ctx, &model) - } - return w.updateModel(ctx, &model, d.Entity.(*jujuparams.ModelUpdate)) - case "unit": - if d.Removed { - state.changed = true - delete(state.units, eid.Id) - return nil - } - if !state.units[eid.Id] { - state.changed = true - state.units[eid.Id] = true - } - default: - } - return nil -} - -func (w *Watcher) deleteModel(ctx context.Context, model *dbmodel.Model) error { - const op = errors.Op("watcher.deleteModel") - - err := w.Database.Transaction(func(db *db.Database) error { - if err := db.GetModel(ctx, model); err != nil { - if errors.ErrorCode(err) != errors.CodeNotFound { - return err - } - } - if !(model.Life == state.Dying.String() || model.Life == state.Dead.String()) { - // If the model hasn't been marked as dying, don't remove it. - return nil - } - return db.DeleteModel(ctx, model) - }) - if err != nil { - return errors.E(op, err) - } - return nil -} - -func (w *Watcher) updateModel(ctx context.Context, model *dbmodel.Model, info *jujuparams.ModelUpdate) error { - const op = errors.Op("watcher.updateModel") - - err := w.Database.Transaction(func(db *db.Database) error { - if err := db.GetModel(ctx, model); err != nil { - if errors.ErrorCode(err) != errors.CodeNotFound { - return err - } - } - model.FromJujuModelUpdate(*info) - return db.UpdateModel(ctx, model) - }) - if err != nil { - return errors.E(op, err) - } - return nil -} diff --git a/internal/jimm/watcher_test._go b/internal/jimm/watcher_test._go deleted file mode 100644 index ba8b0c2a6..000000000 --- a/internal/jimm/watcher_test._go +++ /dev/null @@ -1,1070 +0,0 @@ -// Copyright 2024 Canonical. - -package jimm_test - -import ( - "context" - "database/sql" - "sync" - "sync/atomic" - "testing" - "time" - - qt "github.com/frankban/quicktest" - "github.com/juju/juju/core/instance" - "github.com/juju/juju/core/life" - jujuparams "github.com/juju/juju/rpc/params" - "github.com/juju/juju/state" - - "github.com/canonical/jimm/v3/internal/db" - "github.com/canonical/jimm/v3/internal/dbmodel" - "github.com/canonical/jimm/v3/internal/errors" - "github.com/canonical/jimm/v3/internal/jimm" - "github.com/canonical/jimm/v3/internal/testutils/jimmtest" -) - -const testWatcherEnv = `clouds: -- name: test-cloud - type: test-provider - regions: - - name: test-cloud-region -cloud-credentials: -- owner: alice@canonical.com - name: cred-1 - cloud: test-cloud -controllers: -- name: controller-1 - uuid: 00000001-0000-0000-0000-000000000001 - cloud: test-cloud - region: test-cloud-region -models: -- name: model-1 - type: iaas - uuid: 00000002-0000-0000-0000-000000000001 - controller: controller-1 - default-series: warty - cloud: test-cloud - region: test-cloud-region - cloud-credential: cred-1 - owner: alice@canonical.com - life: alive - status: - status: available - info: "OK!" - since: 2020-02-20T20:02:20Z - users: - - user: alice@canonical.com - access: admin - - user: bob@canonical.com - access: write - - user: charlie@canonical.com - access: read - sla: - level: unsupported - agent-version: 1.2.3 -- name: model-2 - type: iaas - uuid: 00000002-0000-0000-0000-000000000002 - controller: controller-1 - default-series: warty - cloud: test-cloud - region: test-cloud-region - cloud-credential: cred-1 - owner: alice@canonical.com - life: dying -- name: model-3 - type: iaas - uuid: 00000002-0000-0000-0000-000000000003 - controller: controller-1 - default-series: warty - cloud: test-cloud - region: test-cloud-region - cloud-credential: cred-1 - owner: alice@canonical.com - life: dead -` - -var watcherTests = []struct { - name string - initDB func(*qt.C, db.Database) - deltas [][]jujuparams.Delta - checkDB func(*qt.C, db.Database) -}{{ - name: "AddMachine", - deltas: [][]jujuparams.Delta{ - {{ - Entity: &jujuparams.MachineInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Id: "2", - InstanceId: "machine-2", - HardwareCharacteristics: &instance.HardwareCharacteristics{ - CpuCores: newUint64(2), - }, - }, - }}, - nil, - }, - checkDB: func(c *qt.C, db db.Database) { - ctx := context.Background() - - model := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000001", - Valid: true, - }, - } - err := db.GetModel(ctx, &model) - c.Assert(err, qt.IsNil) - - c.Check(model.Machines, qt.Equals, int64(1)) - c.Check(model.Cores, qt.Equals, int64(2)) - }, -}, { - name: "UpdateMachine", - deltas: [][]jujuparams.Delta{ - {{ - Entity: &jujuparams.MachineInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Id: "0", - InstanceId: "machine-0", - }, - }}, {{ - Entity: &jujuparams.MachineInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Id: "0", - InstanceId: "machine-0", - HardwareCharacteristics: &instance.HardwareCharacteristics{ - CpuCores: newUint64(4), - }, - }, - }}, - nil, - }, - checkDB: func(c *qt.C, db db.Database) { - ctx := context.Background() - - model := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000001", - Valid: true, - }, - } - err := db.GetModel(ctx, &model) - c.Assert(err, qt.IsNil) - - c.Check(model.Machines, qt.Equals, int64(1)) - c.Check(model.Cores, qt.Equals, int64(4)) - }, -}, { - name: "DeleteMachine", - deltas: [][]jujuparams.Delta{ - {{ - Entity: &jujuparams.MachineInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Id: "0", - InstanceId: "machine-0", - HardwareCharacteristics: &instance.HardwareCharacteristics{ - CpuCores: newUint64(3), - }, - }, - }}, {{ - Removed: true, - Entity: &jujuparams.MachineInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Id: "0", - }, - }}, - nil, - }, - checkDB: func(c *qt.C, db db.Database) { - ctx := context.Background() - - model := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000001", - Valid: true, - }, - } - err := db.GetModel(ctx, &model) - c.Assert(err, qt.IsNil) - - c.Check(model.Machines, qt.Equals, int64(0)) - c.Check(model.Cores, qt.Equals, int64(0)) - }, -}, { - name: "AddUnit", - deltas: [][]jujuparams.Delta{ - {{ - Entity: &jujuparams.UnitInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Name: "app-1/2", - }, - }}, - nil, - }, - checkDB: func(c *qt.C, db db.Database) { - ctx := context.Background() - - model := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000001", - Valid: true, - }, - } - err := db.GetModel(ctx, &model) - c.Assert(err, qt.IsNil) - - c.Check(model.Units, qt.Equals, int64(1)) - }, -}, { - name: "UpdateUnit", - deltas: [][]jujuparams.Delta{ - {{ - Entity: &jujuparams.UnitInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Name: "app-1/2", - }, - }}, - {{ - Entity: &jujuparams.UnitInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Name: "app-1/2", - }, - }}, - nil, - }, - checkDB: func(c *qt.C, db db.Database) { - ctx := context.Background() - - model := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000001", - Valid: true, - }, - } - err := db.GetModel(ctx, &model) - c.Assert(err, qt.IsNil) - - c.Check(model.Units, qt.Equals, int64(1)) - }, -}, { - name: "DeleteUnit", - deltas: [][]jujuparams.Delta{ - {{ - Entity: &jujuparams.UnitInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Name: "app-1/0", - }, - }}, - {{ - Removed: true, - Entity: &jujuparams.UnitInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Name: "app-1/0", - }, - }}, - nil, - }, - checkDB: func(c *qt.C, db db.Database) { - ctx := context.Background() - - model := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000001", - Valid: true, - }, - } - err := db.GetModel(ctx, &model) - c.Assert(err, qt.IsNil) - - c.Check(model.Units, qt.Equals, int64(0)) - }, -}, { - name: "UnknownModelsIgnored", - deltas: [][]jujuparams.Delta{ - {{ - Entity: &jujuparams.ModelUpdate{ - ModelUUID: "00000002-0000-0000-0000-000000000004", - Name: "new-model", - Owner: "charlie@canonical.com", - Life: "starting", - }, - }}, - nil, - }, - checkDB: func(c *qt.C, db db.Database) { - ctx := context.Background() - - model := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000004", - Valid: true, - }, - } - err := db.GetModel(ctx, &model) - c.Check(err, qt.ErrorMatches, `model not found`) - c.Check(errors.ErrorCode(err), qt.Equals, errors.CodeNotFound) - }, -}, { - name: "UpdateModel", - deltas: [][]jujuparams.Delta{ - {{ - Entity: &jujuparams.ModelUpdate{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Name: "model-1", - Owner: "alice@canonical.com", - Life: life.Value(state.Alive.String()), - ControllerUUID: "00000001-0000-0000-0000-000000000001", - Status: jujuparams.StatusInfo{ - Current: "available", - Message: "updated status message", - Version: "1.2.3", - }, - SLA: jujuparams.ModelSLAInfo{ - Level: "1", - Owner: "me", - }, - }, - }}, - nil, - }, - checkDB: func(c *qt.C, db db.Database) { - ctx := context.Background() - - model := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000001", - Valid: true, - }, - } - err := db.GetModel(ctx, &model) - c.Assert(err, qt.IsNil) - // zero any uninteresting associations - // TODO(mhilton) don't fetch these in the first place. - model.Owner = dbmodel.Identity{} - model.CloudCredential = dbmodel.CloudCredential{} - model.CloudRegion = dbmodel.CloudRegion{} - model.Controller = dbmodel.Controller{} - c.Check(model, jimmtest.DBObjectEquals, dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000001", - Valid: true, - }, - Name: "model-1", - Type: "iaas", - DefaultSeries: "warty", - Life: state.Alive.String(), - Status: dbmodel.Status{ - Status: "available", - Info: "updated status message", - Version: "1.2.3", - }, - SLA: dbmodel.SLA{ - Level: "1", - Owner: "me", - }, - }) - }, -}, { - name: "DeleteDyingModel", - deltas: [][]jujuparams.Delta{ - {{ - Removed: true, - Entity: &jujuparams.ModelUpdate{ - ModelUUID: "00000002-0000-0000-0000-000000000002", - }, - }}, - nil, - }, - checkDB: func(c *qt.C, db db.Database) { - ctx := context.Background() - - model := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000002", - Valid: true, - }, - } - err := db.GetModel(ctx, &model) - c.Check(err, qt.ErrorMatches, `model not found`) - c.Check(errors.ErrorCode(err), qt.Equals, errors.CodeNotFound) - }, -}, { - name: "DeleteDeadModel", - deltas: [][]jujuparams.Delta{ - {{ - Removed: true, - Entity: &jujuparams.ModelUpdate{ - ModelUUID: "00000002-0000-0000-0000-000000000003", - }, - }}, - nil, - }, - checkDB: func(c *qt.C, db db.Database) { - ctx := context.Background() - - model := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000003", - Valid: true, - }, - } - err := db.GetModel(ctx, &model) - c.Check(err, qt.ErrorMatches, `model not found`) - c.Check(errors.ErrorCode(err), qt.Equals, errors.CodeNotFound) - }, -}, { - name: "DeleteLivingModelFails", - deltas: [][]jujuparams.Delta{ - {{ - Removed: true, - Entity: &jujuparams.ModelUpdate{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - }, - }}, - nil, - }, - checkDB: func(c *qt.C, db db.Database) { - ctx := context.Background() - - model := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000001", - Valid: true, - }, - } - err := db.GetModel(ctx, &model) - c.Assert(err, qt.IsNil) - // zero any uninteresting associations - // TODO(mhilton) don't fetch these in the first place. - model.Owner = dbmodel.Identity{} - model.CloudCredential = dbmodel.CloudCredential{} - model.CloudRegion = dbmodel.CloudRegion{} - model.Controller = dbmodel.Controller{} - c.Check(model, jimmtest.DBObjectEquals, dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000001", - Valid: true, - }, - Name: "model-1", - Type: "iaas", - DefaultSeries: "warty", - Life: state.Alive.String(), - Status: dbmodel.Status{ - Status: "available", - Info: "OK!", - Since: sql.NullTime{ - Time: time.Date(2020, 2, 20, 20, 2, 20, 0, time.UTC), - Valid: true, - }, - Version: "1.2.3", - }, - SLA: dbmodel.SLA{ - Level: "unsupported", - }, - }) - }, -}} - -//nolint:gocognit -func TestWatcher(t *testing.T) { - c := qt.New(t) - - for _, test := range watcherTests { - c.Run(test.name, func(c *qt.C) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - nextC := make(chan []jujuparams.Delta, len(test.deltas)) - var stopped uint32 - - deltaProcessedChannel := make(chan bool, len(test.deltas)) - - w := jimm.NewWatcherWithDeltaProcessedChannel( - db.Database{ - DB: jimmtest.PostgresDB(c, nil), - }, - &jimmtest.Dialer{ - API: &jimmtest.API{ - AllModelWatcherNext_: func(ctx context.Context, id string) ([]jujuparams.Delta, error) { - if id != test.name { - return nil, errors.E("incorrect id") - } - - select { - case <-ctx.Done(): - return nil, ctx.Err() - case d, ok := <-nextC: - c.Logf("AllModelWatcherNext received %#v, %v", d, ok) - if ok { - return d, nil - } - cancel() - <-ctx.Done() - return nil, ctx.Err() - } - }, - AllModelWatcherStop_: func(ctx context.Context, id string) error { - if id != test.name { - return errors.E("incorrect id") - } - atomic.StoreUint32(&stopped, 1) - return nil - }, - WatchAllModels_: func(context.Context) (string, error) { - return test.name, nil - }, - ModelInfo_: func(_ context.Context, info *jujuparams.ModelInfo) error { - switch info.UUID { - case "00000002-0000-0000-0000-000000000002": - return errors.E(errors.CodeNotFound) - case "00000002-0000-0000-0000-000000000003": - return errors.E(errors.CodeUnauthorized) - default: - c.Errorf("unexpected model uuid: %s", info.UUID) - return errors.E("unexpected API call") - } - - }, - }, - }, - nil, - deltaProcessedChannel, - ) - - env := jimmtest.ParseEnvironment(c, testWatcherEnv) - err := w.Database.Migrate(ctx, false) - c.Assert(err, qt.IsNil) - env.PopulateDB(c, w.Database) - - if test.initDB != nil { - test.initDB(c, w.Database) - } - - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - err := w.Watch(ctx, time.Millisecond) - checkIfContextCanceled(c, ctx, err) - }() - - validDeltas := 0 - for _, d := range test.deltas { - select { - case nextC <- d: - if d != nil { - validDeltas++ - } - case <-ctx.Done(): - c.Fatal("context closed prematurely") - } - } - - for i := 0; i < validDeltas; i++ { - select { - case <-deltaProcessedChannel: - case <-ctx.Done(): - c.Fatal("context closed prematurely") - } - } - - close(nextC) - wg.Wait() - - test.checkDB(c, w.Database) - }) - } -} - -var modelSummaryWatcherTests = []struct { - name string - summaries [][]jujuparams.ModelAbstract - checkPublisher func(*qt.C, *testPublisher) -}{{ - name: "ModelSummaries", - summaries: [][]jujuparams.ModelAbstract{ - {{ - UUID: "00000002-0000-0000-0000-000000000001", - Status: "test status", - Size: jujuparams.ModelSummarySize{ - Applications: 1, - Machines: 2, - Containers: 3, - Units: 4, - Relations: 12, - }, - Admins: []string{"alice@canonical.com", "bob"}, - }, { - // this is a summary for an model unknown to jimm - // meaning its summary will not be published - // to the pubsub hub. - UUID: "00000002-0000-0000-0000-000000000004", - Status: "test status 2", - Size: jujuparams.ModelSummarySize{ - Applications: 5, - Machines: 4, - Containers: 3, - Units: 2, - Relations: 1, - }, - Admins: []string{"bob@canonical.com"}, - }}, - nil, - }, - checkPublisher: func(c *qt.C, publisher *testPublisher) { - c.Assert(publisher.messages, qt.DeepEquals, []interface{}{ - jujuparams.ModelAbstract{ - UUID: "00000002-0000-0000-0000-000000000001", - Status: "test status", - Size: jujuparams.ModelSummarySize{ - Applications: 1, - Machines: 2, - Containers: 3, - Units: 4, - Relations: 12, - }, - Admins: []string{"alice@canonical.com"}, - }, - }) - }, -}} - -func TestModelSummaryWatcher(t *testing.T) { - c := qt.New(t) - - for _, test := range modelSummaryWatcherTests { - c.Run(test.name, func(c *qt.C) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - nextC := make(chan []jujuparams.ModelAbstract) - var stopped uint32 - - publisher := &testPublisher{} - - w := &jimm.Watcher{ - Pubsub: publisher, - Database: db.Database{ - DB: jimmtest.PostgresDB(c, nil), - }, - Dialer: &jimmtest.Dialer{ - API: &jimmtest.API{ - WatchAllModelSummaries_: func(_ context.Context) (string, error) { - return test.name, nil - }, - ModelSummaryWatcherNext_: func(ctx context.Context, id string) ([]jujuparams.ModelAbstract, error) { - if id != test.name { - return nil, errors.E("incorrect id") - } - - select { - case <-ctx.Done(): - return nil, ctx.Err() - case summaries, ok := <-nextC: - c.Logf("ModelSummaryWatcherNext received %#v, %v", summaries, ok) - if ok { - return summaries, nil - } - cancel() - <-ctx.Done() - return nil, ctx.Err() - } - }, - ModelSummaryWatcherStop_: func(_ context.Context, id string) error { - if id != test.name { - return errors.E("incorrect id") - } - atomic.StoreUint32(&stopped, 1) - return nil - }, - SupportsModelSummaryWatcher_: true, - ModelInfo_: func(_ context.Context, info *jujuparams.ModelInfo) error { - switch info.UUID { - default: - c.Errorf("unexpected model uuid: %s", info.UUID) - case "00000002-0000-0000-0000-000000000002": - case "00000002-0000-0000-0000-000000000003": - } - return errors.E(errors.CodeNotFound) - }, - }, - }, - } - - env := jimmtest.ParseEnvironment(c, testWatcherEnv) - err := w.Database.Migrate(ctx, false) - c.Assert(err, qt.IsNil) - env.PopulateDB(c, w.Database) - - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - err := w.WatchAllModelSummaries(ctx, time.Millisecond) - checkIfContextCanceled(c, ctx, err) - }() - - for _, summary := range test.summaries { - select { - case nextC <- summary: - case <-ctx.Done(): - c.Fatal("context closed prematurely") - } - } - close(nextC) - wg.Wait() - - test.checkPublisher(c, publisher) - }) - } -} - -func TestWatcherSetsControllerUnavailable(t *testing.T) { - c := qt.New(t) - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - controllerUnavailableChannel := make(chan error, 1) - w := jimm.NewWatcherWithControllerUnavailableChan( - db.Database{ - DB: jimmtest.PostgresDB(c, nil), - }, - &jimmtest.Dialer{ - Err: errors.E("test error"), - }, - &testPublisher{}, - controllerUnavailableChannel, - ) - - env := jimmtest.ParseEnvironment(c, testWatcherEnv) - err := w.Database.Migrate(ctx, false) - c.Assert(err, qt.IsNil) - env.PopulateDB(c, w.Database) - - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - err := w.Watch(ctx, time.Millisecond) - checkIfContextCanceled(c, ctx, err) - }() - - // it appears that the jimm code does not treat failing to - // set a controller as unavailable as an error - so - // the test will not treat it as one either. - cerr := <-controllerUnavailableChannel - if cerr != nil { - ctl := dbmodel.Controller{ - Name: "controller-1", - } - err = w.Database.GetController(ctx, &ctl) - c.Assert(err, qt.IsNil) - c.Check(ctl.UnavailableSince.Valid, qt.Equals, true) - } - cancel() - wg.Wait() -} - -func TestWatcherClearsControllerUnavailable(t *testing.T) { - c := qt.New(t) - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - w := jimm.Watcher{ - Database: db.Database{ - DB: jimmtest.PostgresDB(c, nil), - }, - Dialer: &jimmtest.Dialer{ - API: &jimmtest.API{ - AllModelWatcherNext_: func(_ context.Context, _ string) ([]jujuparams.Delta, error) { - cancel() - <-ctx.Done() - return nil, ctx.Err() - }, - ModelInfo_: func(_ context.Context, info *jujuparams.ModelInfo) error { - switch info.UUID { - default: - c.Errorf("unexpected model uuid: %s", info.UUID) - case "00000002-0000-0000-0000-000000000002": - case "00000002-0000-0000-0000-000000000003": - } - return errors.E(errors.CodeNotFound) - }, - WatchAllModels_: func(ctx context.Context) (string, error) { - return "1234", nil - }, - }, - }, - Pubsub: &testPublisher{}, - } - - env := jimmtest.ParseEnvironment(c, testWatcherEnv) - err := w.Database.Migrate(ctx, false) - c.Assert(err, qt.IsNil) - env.PopulateDB(c, w.Database) - - // update controller's UnavailableSince field - ctl := dbmodel.Controller{ - Name: "controller-1", - } - err = w.Database.GetController(ctx, &ctl) - c.Assert(err, qt.IsNil) - ctl.UnavailableSince = sql.NullTime{ - Time: time.Now(), - Valid: true, - } - err = w.Database.UpdateController(ctx, &ctl) - c.Assert(err, qt.IsNil) - - // start the watcher - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - err := w.Watch(ctx, time.Millisecond) - checkIfContextCanceled(c, ctx, err) - }() - wg.Wait() - - // check that the unavailable since time has been cleared - ctl = dbmodel.Controller{ - Name: "controller-1", - } - err = w.Database.GetController(context.Background(), &ctl) - c.Assert(err, qt.IsNil) - c.Assert(ctl.UnavailableSince.Valid, qt.IsFalse) -} - -func TestWatcherRemoveDyingModelsOnStartup(t *testing.T) { - c := qt.New(t) - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - w := &jimm.Watcher{ - Pubsub: &testPublisher{}, - Database: db.Database{ - DB: jimmtest.PostgresDB(c, nil), - }, - Dialer: &jimmtest.Dialer{ - API: &jimmtest.API{ - AllModelWatcherNext_: func(_ context.Context, _ string) ([]jujuparams.Delta, error) { - cancel() - <-ctx.Done() - return nil, ctx.Err() - }, - ModelInfo_: func(_ context.Context, info *jujuparams.ModelInfo) error { - switch info.UUID { - default: - c.Errorf("unexpected model uuid: %s", info.UUID) - case "00000002-0000-0000-0000-000000000002": - case "00000002-0000-0000-0000-000000000003": - } - return errors.E(errors.CodeNotFound) - }, - WatchAllModels_: func(ctx context.Context) (string, error) { - return "1234", nil - }, - }, - }, - } - env := jimmtest.ParseEnvironment(c, testWatcherEnv) - err := w.Database.Migrate(ctx, false) - c.Assert(err, qt.IsNil) - env.PopulateDB(c, w.Database) - - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - err := w.Watch(ctx, time.Millisecond) - checkIfContextCanceled(c, ctx, err) - }() - wg.Wait() - - m := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000002", - Valid: true, - }, - } - err = w.Database.GetModel(context.Background(), &m) - c.Check(errors.ErrorCode(err), qt.Equals, errors.CodeNotFound) -} - -const testWatcherIgnoreDeltasForModelsFromIncorrectControllerEnv = `clouds: -- name: test-cloud - type: test-provider - regions: - - name: test-cloud-region -cloud-credentials: -- owner: alice@canonical.com - name: cred-1 - cloud: test-cloud -controllers: -- name: controller-1 - uuid: 00000001-0000-0000-0000-000000000001 - cloud: test-cloud - region: test-cloud-region -- name: controller-2 - uuid: 00000001-0000-0000-0000-000000000002 - cloud: test-cloud - region: test-cloud-region -models: -- name: model-1 - type: iaas - uuid: 00000002-0000-0000-0000-000000000001 - controller: controller-1 - default-series: warty - cloud: test-cloud - region: test-cloud-region - cloud-credential: cred-1 - owner: alice@canonical.com - life: alive -` - -func TestWatcherIgnoreDeltasForModelsFromIncorrectController(t *testing.T) { - c := qt.New(t) - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - nextC := make(chan []jujuparams.Delta) - w := &jimm.Watcher{ - Pubsub: &testPublisher{}, - Database: db.Database{ - DB: jimmtest.PostgresDB(c, nil), - }, - Dialer: jimmtest.DialerMap{ - "controller-1": &jimmtest.Dialer{ - API: &jimmtest.API{ - AllModelWatcherNext_: func(_ context.Context, _ string) ([]jujuparams.Delta, error) { - <-ctx.Done() - return nil, ctx.Err() - }, - WatchAllModels_: func(ctx context.Context) (string, error) { - return "1234", nil - }, - }, - }, - "controller-2": &jimmtest.Dialer{ - API: &jimmtest.API{ - AllModelWatcherNext_: func(_ context.Context, _ string) ([]jujuparams.Delta, error) { - select { - case <-ctx.Done(): - return nil, ctx.Err() - case d, ok := <-nextC: - if ok { - return d, nil - } - cancel() - <-ctx.Done() - return nil, ctx.Err() - } - - }, - WatchAllModels_: func(ctx context.Context) (string, error) { - return "1234", nil - }, - }, - }, - }, - } - env := jimmtest.ParseEnvironment(c, testWatcherIgnoreDeltasForModelsFromIncorrectControllerEnv) - err := w.Database.Migrate(ctx, false) - c.Assert(err, qt.IsNil) - env.PopulateDB(c, w.Database) - - m1 := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000001", - Valid: true, - }, - } - err = w.Database.GetModel(context.Background(), &m1) - c.Assert(err, qt.IsNil) - - var wg sync.WaitGroup - wg.Add(1) - go func() { - defer wg.Done() - err := w.Watch(ctx, time.Millisecond) - checkIfContextCanceled(c, ctx, err) - }() - - nextC <- []jujuparams.Delta{{ - Entity: &jujuparams.ModelUpdate{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Name: "model-1", - Owner: "alice@canonical.com", - Life: life.Value(state.Alive.String()), - Status: jujuparams.StatusInfo{ - Current: "busy", - }, - }, - }} - nextC <- []jujuparams.Delta{{ - Entity: &jujuparams.MachineInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Id: "0", - }, - }} - nextC <- []jujuparams.Delta{{ - Entity: &jujuparams.ApplicationInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Name: "app-1", - }, - }} - nextC <- []jujuparams.Delta{{ - Entity: &jujuparams.UnitInfo{ - ModelUUID: "00000002-0000-0000-0000-000000000001", - Name: "app-1/0", - }, - }} - close(nextC) - - wg.Wait() - m2 := dbmodel.Model{ - UUID: sql.NullString{ - String: "00000002-0000-0000-0000-000000000001", - Valid: true, - }, - } - err = w.Database.GetModel(context.Background(), &m2) - c.Assert(err, qt.IsNil) - c.Check(m2, qt.DeepEquals, m1) -} - -func checkIfContextCanceled(c *qt.C, ctx context.Context, err error) { - errorToCheck := err - if ctx.Err() != nil { - errorToCheck = ctx.Err() - } - c.Check( - errorToCheck, - qt.ErrorMatches, - `.*(context canceled|operation was canceled).*`, qt.Commentf("unexpected error %s (%#v)", err, err), - ) -} - -type testPublisher struct { - mu sync.Mutex - messages []interface{} -} - -func (p *testPublisher) Publish(model string, content interface{}) <-chan struct{} { - p.mu.Lock() - defer p.mu.Unlock() - p.messages = append(p.messages, content) - - done := make(chan struct{}) - close(done) - return done -} - -func newUint64(i uint64) *uint64 { - return &i -} diff --git a/internal/jimm/watcher_test.go b/internal/jimm/watcher_test.go new file mode 100644 index 000000000..d43bf6ba9 --- /dev/null +++ b/internal/jimm/watcher_test.go @@ -0,0 +1,360 @@ +// Copyright 2024 Canonical. + +package jimm_test + +import ( + "context" + "database/sql" + "sync" + "sync/atomic" + "testing" + "time" + + qt "github.com/frankban/quicktest" + jujuparams "github.com/juju/juju/rpc/params" + + "github.com/canonical/jimm/v3/internal/db" + "github.com/canonical/jimm/v3/internal/dbmodel" + "github.com/canonical/jimm/v3/internal/errors" + "github.com/canonical/jimm/v3/internal/jimm" + "github.com/canonical/jimm/v3/internal/testutils/jimmtest" +) + +const testWatcherEnv = `clouds: +- name: test-cloud + type: test-provider + regions: + - name: test-cloud-region +cloud-credentials: +- owner: alice@canonical.com + name: cred-1 + cloud: test-cloud +controllers: +- name: controller-1 + uuid: 00000001-0000-0000-0000-000000000001 + cloud: test-cloud + region: test-cloud-region +models: +- name: model-1 + type: iaas + uuid: 00000002-0000-0000-0000-000000000001 + controller: controller-1 + cloud: test-cloud + region: test-cloud-region + cloud-credential: cred-1 + owner: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + access: write + - user: charlie@canonical.com + access: read +- name: model-2 + type: iaas + uuid: 00000002-0000-0000-0000-000000000002 + controller: controller-1 + cloud: test-cloud + region: test-cloud-region + cloud-credential: cred-1 + owner: alice@canonical.com + life: dying +- name: model-3 + type: iaas + uuid: 00000002-0000-0000-0000-000000000003 + controller: controller-1 + cloud: test-cloud + region: test-cloud-region + cloud-credential: cred-1 + owner: alice@canonical.com + life: dead +` + +var modelSummaryWatcherTests = []struct { + name string + summaries [][]jujuparams.ModelAbstract + checkPublisher func(*qt.C, *testPublisher) +}{{ + name: "ModelSummaries", + summaries: [][]jujuparams.ModelAbstract{ + {{ + UUID: "00000002-0000-0000-0000-000000000001", + Status: "test status", + Size: jujuparams.ModelSummarySize{ + Applications: 1, + Machines: 2, + Containers: 3, + Units: 4, + Relations: 12, + }, + Admins: []string{"alice@canonical.com", "bob"}, + }, { + // this is a summary for an model unknown to jimm + // meaning its summary will not be published + // to the pubsub hub. + UUID: "00000002-0000-0000-0000-000000000004", + Status: "test status 2", + Size: jujuparams.ModelSummarySize{ + Applications: 5, + Machines: 4, + Containers: 3, + Units: 2, + Relations: 1, + }, + Admins: []string{"bob@canonical.com"}, + }}, + nil, + }, + checkPublisher: func(c *qt.C, publisher *testPublisher) { + c.Assert(publisher.messages, qt.DeepEquals, []interface{}{ + jujuparams.ModelAbstract{ + UUID: "00000002-0000-0000-0000-000000000001", + Status: "test status", + Size: jujuparams.ModelSummarySize{ + Applications: 1, + Machines: 2, + Containers: 3, + Units: 4, + Relations: 12, + }, + Admins: []string{"alice@canonical.com"}, + }, + }) + }, +}} + +func TestModelSummaryWatcher(t *testing.T) { + c := qt.New(t) + + for _, test := range modelSummaryWatcherTests { + c.Run(test.name, func(c *qt.C) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + nextC := make(chan []jujuparams.ModelAbstract) + var stopped uint32 + + publisher := &testPublisher{} + + w := &jimm.Watcher{ + Pubsub: publisher, + Database: db.Database{ + DB: jimmtest.PostgresDB(c, nil), + }, + Dialer: &jimmtest.Dialer{ + API: &jimmtest.API{ + WatchAllModelSummaries_: func(_ context.Context) (string, error) { + return test.name, nil + }, + ModelSummaryWatcherNext_: func(ctx context.Context, id string) ([]jujuparams.ModelAbstract, error) { + if id != test.name { + return nil, errors.E("incorrect id") + } + + select { + case <-ctx.Done(): + return nil, ctx.Err() + case summaries, ok := <-nextC: + c.Logf("ModelSummaryWatcherNext received %#v, %v", summaries, ok) + if ok { + return summaries, nil + } + cancel() + <-ctx.Done() + return nil, ctx.Err() + } + }, + ModelSummaryWatcherStop_: func(_ context.Context, id string) error { + if id != test.name { + return errors.E("incorrect id") + } + atomic.StoreUint32(&stopped, 1) + return nil + }, + SupportsModelSummaryWatcher_: true, + ModelInfo_: func(_ context.Context, info *jujuparams.ModelInfo) error { + switch info.UUID { + default: + c.Errorf("unexpected model uuid: %s", info.UUID) + case "00000002-0000-0000-0000-000000000002": + case "00000002-0000-0000-0000-000000000003": + } + return errors.E(errors.CodeNotFound) + }, + }, + }, + } + + env := jimmtest.ParseEnvironment(c, testWatcherEnv) + err := w.Database.Migrate(ctx, false) + c.Assert(err, qt.IsNil) + env.PopulateDB(c, w.Database) + + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + err := w.WatchAllModelSummaries(ctx, time.Millisecond) + checkIfContextCanceled(c, ctx, err) + }() + + for _, summary := range test.summaries { + select { + case nextC <- summary: + case <-ctx.Done(): + c.Fatal("context closed prematurely") + } + } + close(nextC) + wg.Wait() + + test.checkPublisher(c, publisher) + }) + } +} + +func TestWatcherSetsControllerUnavailable(t *testing.T) { + c := qt.New(t) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + controllerUnavailableChannel := make(chan error, 1) + w := jimm.NewWatcherWithControllerUnavailableChan( + db.Database{ + DB: jimmtest.PostgresDB(c, nil), + }, + &jimmtest.Dialer{ + Err: errors.E("test error"), + }, + &testPublisher{}, + controllerUnavailableChannel, + ) + + env := jimmtest.ParseEnvironment(c, testWatcherEnv) + err := w.Database.Migrate(ctx, false) + c.Assert(err, qt.IsNil) + env.PopulateDB(c, w.Database) + + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + err := w.WatchAllModelSummaries(ctx, time.Millisecond) + checkIfContextCanceled(c, ctx, err) + }() + + // it appears that the jimm code does not treat failing to + // set a controller as unavailable as an error - so + // the test will not treat it as one either. + cerr := <-controllerUnavailableChannel + if cerr != nil { + ctl := dbmodel.Controller{ + Name: "controller-1", + } + err = w.Database.GetController(ctx, &ctl) + c.Assert(err, qt.IsNil) + c.Check(ctl.UnavailableSince.Valid, qt.Equals, true) + } + cancel() + wg.Wait() +} + +func TestWatcherClearsControllerUnavailable(t *testing.T) { + c := qt.New(t) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + w := jimm.Watcher{ + Database: db.Database{ + DB: jimmtest.PostgresDB(c, nil), + }, + Dialer: &jimmtest.Dialer{ + API: &jimmtest.API{ + AllModelWatcherNext_: func(_ context.Context, _ string) ([]jujuparams.Delta, error) { + cancel() + <-ctx.Done() + return nil, ctx.Err() + }, + ModelInfo_: func(_ context.Context, info *jujuparams.ModelInfo) error { + switch info.UUID { + default: + c.Errorf("unexpected model uuid: %s", info.UUID) + case "00000002-0000-0000-0000-000000000002": + case "00000002-0000-0000-0000-000000000003": + } + return errors.E(errors.CodeNotFound) + }, + WatchAllModels_: func(ctx context.Context) (string, error) { + return "1234", nil + }, + }, + }, + Pubsub: &testPublisher{}, + } + + env := jimmtest.ParseEnvironment(c, testWatcherEnv) + err := w.Database.Migrate(ctx, false) + c.Assert(err, qt.IsNil) + env.PopulateDB(c, w.Database) + + // update controller's UnavailableSince field + ctl := dbmodel.Controller{ + Name: "controller-1", + } + err = w.Database.GetController(ctx, &ctl) + c.Assert(err, qt.IsNil) + ctl.UnavailableSince = sql.NullTime{ + Time: time.Now(), + Valid: true, + } + err = w.Database.UpdateController(ctx, &ctl) + c.Assert(err, qt.IsNil) + + // start the watcher + var wg sync.WaitGroup + wg.Add(1) + go func() { + defer wg.Done() + err := w.WatchAllModelSummaries(ctx, time.Millisecond) + checkIfContextCanceled(c, ctx, err) + }() + wg.Wait() + + // check that the unavailable since time has been cleared + ctl = dbmodel.Controller{ + Name: "controller-1", + } + err = w.Database.GetController(context.Background(), &ctl) + c.Assert(err, qt.IsNil) + c.Assert(ctl.UnavailableSince.Valid, qt.IsFalse) +} + +func checkIfContextCanceled(c *qt.C, ctx context.Context, err error) { + errorToCheck := err + if ctx.Err() != nil { + errorToCheck = ctx.Err() + } + c.Check( + errorToCheck, + qt.ErrorMatches, + `.*(context canceled|operation was canceled).*`, qt.Commentf("unexpected error %s (%#v)", err, err), + ) +} + +type testPublisher struct { + mu sync.Mutex + messages []interface{} +} + +func (p *testPublisher) Publish(model string, content interface{}) <-chan struct{} { + p.mu.Lock() + defer p.mu.Unlock() + p.messages = append(p.messages, content) + + done := make(chan struct{}) + close(done) + return done +} From b3752af868f989c90aec43667556e25951079a75 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Tue, 3 Dec 2024 16:42:50 +0100 Subject: [PATCH 02/17] fix lint errors --- internal/jimm/model_poller.go | 6 ++++-- internal/jimm/utils.go | 10 ---------- internal/jimm/watcher.go | 14 -------------- 3 files changed, 4 insertions(+), 26 deletions(-) diff --git a/internal/jimm/model_poller.go b/internal/jimm/model_poller.go index 365d16503..047942d88 100644 --- a/internal/jimm/model_poller.go +++ b/internal/jimm/model_poller.go @@ -28,7 +28,7 @@ func (j *JIMM) WatchModelsDying(ctx context.Context) error { mt := m.ResourceTag() // if the model is dying and not found by querying the controller we can assume it is dead. // And safely delete the reference from our db. - j.doModelAdmin(ctx, adminUser, mt, func(m *dbmodel.Model, api API) error { + err := j.doModelAdmin(ctx, adminUser, mt, func(m *dbmodel.Model, api API) error { if err := api.ModelInfo(ctx, &jujuparams.ModelInfo{}); err != nil { // Some versions of juju return unauthorized for models that cannot be found. if errors.ErrorCode(err) == errors.CodeNotFound || errors.ErrorCode(err) == errors.CodeUnauthorized { @@ -43,8 +43,10 @@ func (j *JIMM) WatchModelsDying(ctx context.Context) error { } return nil }) + if err != nil { + return err + } } - return nil }) if err != nil { diff --git a/internal/jimm/utils.go b/internal/jimm/utils.go index c97cdf225..1e8387178 100644 --- a/internal/jimm/utils.go +++ b/internal/jimm/utils.go @@ -68,13 +68,3 @@ func (j *JIMM) dialController(ctx context.Context, ctl *dbmodel.Controller) (API } return api, nil } - -// dialModel dials a model. -func (j *JIMM) dialModel(ctx context.Context, ctl *dbmodel.Controller, mt names.ModelTag) (API, error) { - api, err := j.dial(ctx, ctl, mt) - if err != nil { - zapctx.Error(ctx, "failed to dial the controller", zaputil.Error(err)) - return nil, err - } - return api, nil -} diff --git a/internal/jimm/watcher.go b/internal/jimm/watcher.go index b81e368d4..e7414881e 100644 --- a/internal/jimm/watcher.go +++ b/internal/jimm/watcher.go @@ -119,20 +119,6 @@ func (w *Watcher) dialController(ctx context.Context, ctl *dbmodel.Controller) ( return api, nil } -// A modelState holds the in-memory state of a model for the watcher. -type modelState struct { - // id is the database id of the model. - id uint - changed bool - - // machines maps the Id of all the machines that have been seen to - // the number of cores reported. - machines map[string]int64 - - // units stores the ids of all units that have been seen. - units map[string]bool -} - // watchAllModelSummaries connects to the given controller and watches the // summary updates. func (w *Watcher) watchAllModelSummaries(ctx context.Context, ctl *dbmodel.Controller) error { From 2d4864ca145c0b66d4abecf1f50f0a87eb1f04f4 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Tue, 3 Dec 2024 16:46:21 +0100 Subject: [PATCH 03/17] simplify function --- internal/jimm/model_poller.go | 31 +++++++++++++------------------ internal/jimm/utils.go | 10 ++++++++++ 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/internal/jimm/model_poller.go b/internal/jimm/model_poller.go index 047942d88..c9668d520 100644 --- a/internal/jimm/model_poller.go +++ b/internal/jimm/model_poller.go @@ -21,30 +21,25 @@ func (j *JIMM) WatchModelsDying(ctx context.Context) error { ctx, cancel := context.WithCancel(ctx) defer cancel() - adminUser := j.everyoneUser() - adminUser.JimmAdmin = true - err := j.ForEachModel(ctx, adminUser, func(m *dbmodel.Model, _ jujuparams.UserAccessPermission) error { + err := j.DB().ForEachModel(ctx, func(m *dbmodel.Model) error { if m.Life == state.Dying.String() { - mt := m.ResourceTag() // if the model is dying and not found by querying the controller we can assume it is dead. // And safely delete the reference from our db. - err := j.doModelAdmin(ctx, adminUser, mt, func(m *dbmodel.Model, api API) error { - if err := api.ModelInfo(ctx, &jujuparams.ModelInfo{}); err != nil { - // Some versions of juju return unauthorized for models that cannot be found. - if errors.ErrorCode(err) == errors.CodeNotFound || errors.ErrorCode(err) == errors.CodeUnauthorized { - if err := j.DB().DeleteModel(ctx, m); err != nil { - return errors.E(op, err) - } else { - return nil - } - } else { + api, err := j.dialModel(ctx, &m.Controller, m.ResourceTag()) + if err != nil { + return err + } + if err := api.ModelInfo(ctx, &jujuparams.ModelInfo{}); err != nil { + // Some versions of juju return unauthorized for models that cannot be found. + if errors.ErrorCode(err) == errors.CodeNotFound || errors.ErrorCode(err) == errors.CodeUnauthorized { + if err := j.DB().DeleteModel(ctx, m); err != nil { return errors.E(op, err) + } else { + return nil } + } else { + return errors.E(op, err) } - return nil - }) - if err != nil { - return err } } return nil diff --git a/internal/jimm/utils.go b/internal/jimm/utils.go index 1e8387178..c97cdf225 100644 --- a/internal/jimm/utils.go +++ b/internal/jimm/utils.go @@ -68,3 +68,13 @@ func (j *JIMM) dialController(ctx context.Context, ctl *dbmodel.Controller) (API } return api, nil } + +// dialModel dials a model. +func (j *JIMM) dialModel(ctx context.Context, ctl *dbmodel.Controller, mt names.ModelTag) (API, error) { + api, err := j.dial(ctx, ctl, mt) + if err != nil { + zapctx.Error(ctx, "failed to dial the controller", zaputil.Error(err)) + return nil, err + } + return api, nil +} From e0a6d881d00ec5d93aa32baa8d595dbbf35c6527 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Tue, 3 Dec 2024 17:45:37 +0100 Subject: [PATCH 04/17] first test for model poller --- cmd/jimmsrv/service/service.go | 2 +- internal/jimm/model_poller.go | 4 +- internal/jimm/model_poller_test.go | 156 +++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+), 3 deletions(-) create mode 100644 internal/jimm/model_poller_test.go diff --git a/cmd/jimmsrv/service/service.go b/cmd/jimmsrv/service/service.go index ae4dc3186..c6dbde850 100644 --- a/cmd/jimmsrv/service/service.go +++ b/cmd/jimmsrv/service/service.go @@ -266,7 +266,7 @@ func (s *Service) WatchModelsDying(ctx context.Context, trigger <-chan time.Time for { select { case <-trigger: - err := s.jimm.WatchModelsDying(ctx) + err := s.jimm.PollModelsDying(ctx) if err != nil { zapctx.Error(ctx, "openfga cleanup", zap.Error(err)) continue diff --git a/internal/jimm/model_poller.go b/internal/jimm/model_poller.go index c9668d520..12d142462 100644 --- a/internal/jimm/model_poller.go +++ b/internal/jimm/model_poller.go @@ -13,7 +13,7 @@ import ( "github.com/canonical/jimm/v3/internal/errors" ) -func (j *JIMM) WatchModelsDying(ctx context.Context) error { +func (j *JIMM) PollModelsDying(ctx context.Context) error { const op = errors.Op("jimm.WatchModelsDying") // Ensure that if the watcher stops because of a database error all @@ -29,7 +29,7 @@ func (j *JIMM) WatchModelsDying(ctx context.Context) error { if err != nil { return err } - if err := api.ModelInfo(ctx, &jujuparams.ModelInfo{}); err != nil { + if err := api.ModelInfo(ctx, &jujuparams.ModelInfo{UUID: m.UUID.String}); err != nil { // Some versions of juju return unauthorized for models that cannot be found. if errors.ErrorCode(err) == errors.CodeNotFound || errors.ErrorCode(err) == errors.CodeUnauthorized { if err := j.DB().DeleteModel(ctx, m); err != nil { diff --git a/internal/jimm/model_poller_test.go b/internal/jimm/model_poller_test.go new file mode 100644 index 000000000..aea971c65 --- /dev/null +++ b/internal/jimm/model_poller_test.go @@ -0,0 +1,156 @@ +// Copyright 2024 Canonical. + +package jimm_test + +import ( + "context" + "database/sql" + "testing" + + qt "github.com/frankban/quicktest" + "github.com/google/uuid" + "github.com/juju/juju/core/life" + jujuparams "github.com/juju/juju/rpc/params" + + "github.com/canonical/jimm/v3/internal/db" + "github.com/canonical/jimm/v3/internal/dbmodel" + "github.com/canonical/jimm/v3/internal/errors" + "github.com/canonical/jimm/v3/internal/jimm" + "github.com/canonical/jimm/v3/internal/testutils/jimmtest" +) + +const modelPollerTestEnv = `clouds: +- name: test-cloud + type: test-provider + regions: + - name: test-cloud-region +cloud-credentials: +- owner: alice@canonical.com + name: cred-1 + cloud: test-cloud +controllers: +- name: controller-1 + uuid: 00000001-0000-0000-0000-000000000001 + cloud: test-cloud + region: test-cloud-region +models: +- name: model-1 + uuid: 00000002-0000-0000-0000-000000000001 + controller: controller-1 + cloud: test-cloud + region: test-cloud-region + cloud-credential: cred-1 + owner: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + access: admin +- name: model-2 + uuid: 00000002-0000-0000-0000-000000000002 + controller: controller-1 + cloud: test-cloud + region: test-cloud-region + cloud-credential: cred-1 + owner: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + access: write +users: +- username: alice@canonical.com + controller-access: superuser +` + +func TestPollModelsDying(t *testing.T) { + c := qt.New(t) + ctx := context.Background() + + client, _, _, err := jimmtest.SetupTestOFGAClient(c.Name()) + c.Assert(err, qt.IsNil) + j := &jimm.JIMM{ + UUID: uuid.NewString(), + OpenFGAClient: client, + Database: db.Database{ + DB: jimmtest.PostgresDB(c, nil), + }, + } + err = j.Database.Migrate(ctx, false) + c.Assert(err, qt.IsNil) + + tests := []struct { + description string + controllerModelInfo func(ctx context.Context, mi *jujuparams.ModelInfo) error + initEnv func() *jimmtest.Environment + checkDb func(env *jimmtest.Environment) + expectedSummariesSize int + }{ + { + description: "jimm start and some model set to dying ", + initEnv: func() *jimmtest.Environment { + + env := jimmtest.ParseEnvironment(c, modelPollerTestEnv) + env.PopulateDBAndPermissions(c, j.ResourceTag(), j.Database, client) + j.Dialer = &jimmtest.Dialer{ + API: &jimmtest.API{ + ModelInfo_: func(ctx context.Context, mi *jujuparams.ModelInfo) error { + switch mi.UUID { + case env.Models[0].UUID: + return errors.E(errors.CodeNotFound) + case env.Models[1].UUID: + mi = &jujuparams.ModelInfo{ + Life: life.Dying, + } + return nil + default: + return errors.E("new error") + } + }, + }, + } + model := dbmodel.Model{ + UUID: sql.NullString{ + String: env.Models[0].UUID, + Valid: true, + }, + } + err := j.DB().GetModel(ctx, &model) + c.Assert(err, qt.IsNil) + model.Life = string(life.Dying) + err = j.DB().UpdateModel(ctx, &model) + c.Assert(err, qt.IsNil) + return env + }, + checkDb: func(env *jimmtest.Environment) { + model := dbmodel.Model{ + UUID: sql.NullString{ + String: env.Models[0].UUID, + Valid: true, + }, + } + err := j.DB().GetModel(ctx, &model) + c.Assert(err, qt.ErrorMatches, "model not found") + + model = dbmodel.Model{ + UUID: sql.NullString{ + String: env.Models[1].UUID, + Valid: true, + }, + } + err = j.DB().GetModel(ctx, &model) + c.Assert(err, qt.IsNil) + }, + }, + } + for _, t := range tests { + c.Run(t.description, func(c *qt.C) { + env := t.initEnv() + err := j.PollModelsDying(ctx) + c.Check(err, qt.IsNil) + t.checkDb(env) + }) + } +} From 59165869776879628bcf231ec6550f27a24e626d Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Wed, 4 Dec 2024 12:07:10 +0100 Subject: [PATCH 05/17] rename and update tests --- cmd/jimmsrv/main.go | 2 +- cmd/jimmsrv/service/service.go | 4 +- .../{model_poller.go => model_cleanup.go} | 11 +- internal/jimm/model_cleanup_test.go | 195 ++++++++++++++++++ internal/jimm/model_poller_test.go | 156 -------------- 5 files changed, 205 insertions(+), 163 deletions(-) rename internal/jimm/{model_poller.go => model_cleanup.go} (80%) create mode 100644 internal/jimm/model_cleanup_test.go delete mode 100644 internal/jimm/model_poller_test.go diff --git a/cmd/jimmsrv/main.go b/cmd/jimmsrv/main.go index 6e4c59e11..4dfd8e047 100644 --- a/cmd/jimmsrv/main.go +++ b/cmd/jimmsrv/main.go @@ -204,7 +204,7 @@ func start(ctx context.Context, s *service.Service) error { return jimmsvc.OpenFGACleanup(ctx, time.NewTicker(6*time.Hour).C) }) s.Go(func() error { - return jimmsvc.WatchModelsDying(ctx, time.NewTicker(time.Minute).C) + return jimmsvc.CleanupModelsDying(ctx, time.NewTicker(time.Minute).C) }) } diff --git a/cmd/jimmsrv/service/service.go b/cmd/jimmsrv/service/service.go index c6dbde850..c4082a0ee 100644 --- a/cmd/jimmsrv/service/service.go +++ b/cmd/jimmsrv/service/service.go @@ -262,11 +262,11 @@ func (s *Service) OpenFGACleanup(ctx context.Context, trigger <-chan time.Time) } // OpenFGACleanup starts a goroutine that cleans up any orphaned tuples from OpenFGA. -func (s *Service) WatchModelsDying(ctx context.Context, trigger <-chan time.Time) error { +func (s *Service) CleanupModelsDying(ctx context.Context, trigger <-chan time.Time) error { for { select { case <-trigger: - err := s.jimm.PollModelsDying(ctx) + err := s.jimm.CleanupModelsDying(ctx) if err != nil { zapctx.Error(ctx, "openfga cleanup", zap.Error(err)) continue diff --git a/internal/jimm/model_poller.go b/internal/jimm/model_cleanup.go similarity index 80% rename from internal/jimm/model_poller.go rename to internal/jimm/model_cleanup.go index 12d142462..1219814a3 100644 --- a/internal/jimm/model_poller.go +++ b/internal/jimm/model_cleanup.go @@ -3,6 +3,7 @@ package jimm import ( "context" + "fmt" jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/juju/state" @@ -13,7 +14,7 @@ import ( "github.com/canonical/jimm/v3/internal/errors" ) -func (j *JIMM) PollModelsDying(ctx context.Context) error { +func (j *JIMM) CleanupModelsDying(ctx context.Context) error { const op = errors.Op("jimm.WatchModelsDying") // Ensure that if the watcher stops because of a database error all @@ -27,18 +28,20 @@ func (j *JIMM) PollModelsDying(ctx context.Context) error { // And safely delete the reference from our db. api, err := j.dialModel(ctx, &m.Controller, m.ResourceTag()) if err != nil { - return err + zapctx.Error(ctx, fmt.Sprintf("Cannot dial model %s: %s\n", m.UUID.String, err)) + return nil } if err := api.ModelInfo(ctx, &jujuparams.ModelInfo{UUID: m.UUID.String}); err != nil { // Some versions of juju return unauthorized for models that cannot be found. if errors.ErrorCode(err) == errors.CodeNotFound || errors.ErrorCode(err) == errors.CodeUnauthorized { if err := j.DB().DeleteModel(ctx, m); err != nil { - return errors.E(op, err) + zapctx.Error(ctx, fmt.Sprintf("Cannot delete model %s: %s\n", m.UUID.String, err)) } else { return nil } } else { - return errors.E(op, err) + zapctx.Error(ctx, fmt.Sprintf("Cannot get ModelInfo for model %s: %s\n", m.UUID.String, err)) + return nil } } } diff --git a/internal/jimm/model_cleanup_test.go b/internal/jimm/model_cleanup_test.go new file mode 100644 index 000000000..c3537d439 --- /dev/null +++ b/internal/jimm/model_cleanup_test.go @@ -0,0 +1,195 @@ +// Copyright 2024 Canonical. + +package jimm_test + +import ( + "context" + "database/sql" + "testing" + "time" + + qt "github.com/frankban/quicktest" + "github.com/google/uuid" + jujuparams "github.com/juju/juju/rpc/params" + "github.com/juju/juju/state" + "github.com/juju/names/v5" + + "github.com/canonical/jimm/v3/internal/db" + "github.com/canonical/jimm/v3/internal/dbmodel" + "github.com/canonical/jimm/v3/internal/errors" + "github.com/canonical/jimm/v3/internal/jimm" + "github.com/canonical/jimm/v3/internal/testutils/jimmtest" +) + +const modelPollerTestEnv = `clouds: +- name: test-cloud + type: test-provider + regions: + - name: test-cloud-region +cloud-credentials: +- owner: alice@canonical.com + name: cred-1 + cloud: test-cloud +controllers: +- name: controller-1 + uuid: 00000001-0000-0000-0000-000000000001 + cloud: test-cloud + region: test-cloud-region +models: +- name: model-1 + uuid: 00000002-0000-0000-0000-000000000001 + controller: controller-1 + cloud: test-cloud + region: test-cloud-region + cloud-credential: cred-1 + owner: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + access: admin +- name: model-2 + uuid: 00000002-0000-0000-0000-000000000002 + controller: controller-1 + cloud: test-cloud + region: test-cloud-region + cloud-credential: cred-1 + owner: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + access: write +- name: model-3 + uuid: 00000002-0000-0000-0000-000000000003 + controller: controller-1 + cloud: test-cloud + region: test-cloud-region + cloud-credential: cred-1 + owner: alice@canonical.com + life: alive + users: + - user: alice@canonical.com + access: admin + - user: bob@canonical.com + access: write +users: +- username: alice@canonical.com + controller-access: superuser +` + +func TestPollModelsDying(t *testing.T) { + // init + c := qt.New(t) + ctx := context.Background() + + client, _, _, err := jimmtest.SetupTestOFGAClient(c.Name()) + c.Assert(err, qt.IsNil) + j := &jimm.JIMM{ + UUID: uuid.NewString(), + OpenFGAClient: client, + Database: db.Database{ + DB: jimmtest.PostgresDB(c, nil), + }, + } + err = j.Database.Migrate(ctx, false) + c.Assert(err, qt.IsNil) + jimmAdmin, err := j.GetUser(ctx, "alice@canonical.com") + c.Assert(err, qt.IsNil) + + env := jimmtest.ParseEnvironment(c, modelPollerTestEnv) + env.PopulateDBAndPermissions(c, j.ResourceTag(), j.Database, client) + + j.Dialer = &jimmtest.Dialer{ + API: &jimmtest.API{ + ModelInfo_: func(ctx context.Context, mi *jujuparams.ModelInfo) error { + switch mi.UUID { + case env.Models[0].UUID: + return errors.E(errors.CodeNotFound) + case env.Models[1].UUID: + return nil + default: + return errors.E("new error") + } + }, + DestroyModel_: func(ctx context.Context, mt names.ModelTag, b1, b2 *bool, d1, d2 *time.Duration) error { + return nil + }, + }, + } + err = j.DestroyModel(ctx, jimmAdmin, names.NewModelTag(env.Models[0].UUID), nil, nil, nil, nil) + c.Assert(err, qt.IsNil) + + // test + err = j.CleanupModelsDying(ctx) + c.Assert(err, qt.IsNil) + + model := dbmodel.Model{ + UUID: sql.NullString{ + String: env.Models[0].UUID, + Valid: true, + }, + } + err = j.DB().GetModel(ctx, &model) + c.Assert(err, qt.ErrorMatches, "model not found") + + model = dbmodel.Model{ + UUID: sql.NullString{ + String: env.Models[1].UUID, + Valid: true, + }, + } + err = j.DB().GetModel(ctx, &model) + c.Assert(err, qt.IsNil) +} + +func TestPollModelsDyingControllerErrors(t *testing.T) { + // init + c := qt.New(t) + ctx := context.Background() + + client, _, _, err := jimmtest.SetupTestOFGAClient(c.Name()) + c.Assert(err, qt.IsNil) + j := &jimm.JIMM{ + UUID: uuid.NewString(), + OpenFGAClient: client, + Database: db.Database{ + DB: jimmtest.PostgresDB(c, nil), + }, + } + err = j.Database.Migrate(ctx, false) + c.Assert(err, qt.IsNil) + + env := jimmtest.ParseEnvironment(c, modelPollerTestEnv) + env.PopulateDBAndPermissions(c, j.ResourceTag(), j.Database, client) + jimmAdmin, err := j.GetUser(ctx, "alice@canonical.com") + c.Assert(err, qt.IsNil) + j.Dialer = &jimmtest.Dialer{ + API: &jimmtest.API{ + ModelInfo_: func(ctx context.Context, mi *jujuparams.ModelInfo) error { + return errors.E("controller not available") + }, + DestroyModel_: func(ctx context.Context, mt names.ModelTag, b1, b2 *bool, d1, d2 *time.Duration) error { + return nil + }, + }, + } + err = j.DestroyModel(ctx, jimmAdmin, names.NewModelTag(env.Models[0].UUID), nil, nil, nil, nil) + c.Assert(err, qt.IsNil) + + // test + err = j.CleanupModelsDying(ctx) + c.Assert(err, qt.IsNil) + + model := dbmodel.Model{ + UUID: sql.NullString{ + String: env.Models[0].UUID, + Valid: true, + }, + } + err = j.DB().GetModel(ctx, &model) + c.Assert(err, qt.IsNil) + c.Assert(model.Life, qt.Equals, state.Dying.String()) +} diff --git a/internal/jimm/model_poller_test.go b/internal/jimm/model_poller_test.go deleted file mode 100644 index aea971c65..000000000 --- a/internal/jimm/model_poller_test.go +++ /dev/null @@ -1,156 +0,0 @@ -// Copyright 2024 Canonical. - -package jimm_test - -import ( - "context" - "database/sql" - "testing" - - qt "github.com/frankban/quicktest" - "github.com/google/uuid" - "github.com/juju/juju/core/life" - jujuparams "github.com/juju/juju/rpc/params" - - "github.com/canonical/jimm/v3/internal/db" - "github.com/canonical/jimm/v3/internal/dbmodel" - "github.com/canonical/jimm/v3/internal/errors" - "github.com/canonical/jimm/v3/internal/jimm" - "github.com/canonical/jimm/v3/internal/testutils/jimmtest" -) - -const modelPollerTestEnv = `clouds: -- name: test-cloud - type: test-provider - regions: - - name: test-cloud-region -cloud-credentials: -- owner: alice@canonical.com - name: cred-1 - cloud: test-cloud -controllers: -- name: controller-1 - uuid: 00000001-0000-0000-0000-000000000001 - cloud: test-cloud - region: test-cloud-region -models: -- name: model-1 - uuid: 00000002-0000-0000-0000-000000000001 - controller: controller-1 - cloud: test-cloud - region: test-cloud-region - cloud-credential: cred-1 - owner: alice@canonical.com - life: alive - users: - - user: alice@canonical.com - access: admin - - user: bob@canonical.com - access: admin -- name: model-2 - uuid: 00000002-0000-0000-0000-000000000002 - controller: controller-1 - cloud: test-cloud - region: test-cloud-region - cloud-credential: cred-1 - owner: alice@canonical.com - life: alive - users: - - user: alice@canonical.com - access: admin - - user: bob@canonical.com - access: write -users: -- username: alice@canonical.com - controller-access: superuser -` - -func TestPollModelsDying(t *testing.T) { - c := qt.New(t) - ctx := context.Background() - - client, _, _, err := jimmtest.SetupTestOFGAClient(c.Name()) - c.Assert(err, qt.IsNil) - j := &jimm.JIMM{ - UUID: uuid.NewString(), - OpenFGAClient: client, - Database: db.Database{ - DB: jimmtest.PostgresDB(c, nil), - }, - } - err = j.Database.Migrate(ctx, false) - c.Assert(err, qt.IsNil) - - tests := []struct { - description string - controllerModelInfo func(ctx context.Context, mi *jujuparams.ModelInfo) error - initEnv func() *jimmtest.Environment - checkDb func(env *jimmtest.Environment) - expectedSummariesSize int - }{ - { - description: "jimm start and some model set to dying ", - initEnv: func() *jimmtest.Environment { - - env := jimmtest.ParseEnvironment(c, modelPollerTestEnv) - env.PopulateDBAndPermissions(c, j.ResourceTag(), j.Database, client) - j.Dialer = &jimmtest.Dialer{ - API: &jimmtest.API{ - ModelInfo_: func(ctx context.Context, mi *jujuparams.ModelInfo) error { - switch mi.UUID { - case env.Models[0].UUID: - return errors.E(errors.CodeNotFound) - case env.Models[1].UUID: - mi = &jujuparams.ModelInfo{ - Life: life.Dying, - } - return nil - default: - return errors.E("new error") - } - }, - }, - } - model := dbmodel.Model{ - UUID: sql.NullString{ - String: env.Models[0].UUID, - Valid: true, - }, - } - err := j.DB().GetModel(ctx, &model) - c.Assert(err, qt.IsNil) - model.Life = string(life.Dying) - err = j.DB().UpdateModel(ctx, &model) - c.Assert(err, qt.IsNil) - return env - }, - checkDb: func(env *jimmtest.Environment) { - model := dbmodel.Model{ - UUID: sql.NullString{ - String: env.Models[0].UUID, - Valid: true, - }, - } - err := j.DB().GetModel(ctx, &model) - c.Assert(err, qt.ErrorMatches, "model not found") - - model = dbmodel.Model{ - UUID: sql.NullString{ - String: env.Models[1].UUID, - Valid: true, - }, - } - err = j.DB().GetModel(ctx, &model) - c.Assert(err, qt.IsNil) - }, - }, - } - for _, t := range tests { - c.Run(t.description, func(c *qt.C) { - env := t.initEnv() - err := j.PollModelsDying(ctx) - c.Check(err, qt.IsNil) - t.checkDb(env) - }) - } -} From 303896b6c203c69379cdad914024f969bfd82381 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Wed, 4 Dec 2024 12:36:57 +0100 Subject: [PATCH 06/17] remove all watcher from juju client --- internal/jimm/controller_test.go | 15 ---- internal/jimm/jimm.go | 20 ----- internal/jimm/model_cleanup.go | 2 - internal/jimm/watcher_test.go | 6 +- internal/jujuclient/allwatcher.go | 46 ---------- internal/jujuclient/allwatcher_test.go | 52 ------------ internal/jujuclient/modelwatcher_test.go | 102 ----------------------- internal/testutils/jimmtest/api.go | 48 ----------- 8 files changed, 4 insertions(+), 287 deletions(-) delete mode 100644 internal/jujuclient/allwatcher.go delete mode 100644 internal/jujuclient/allwatcher_test.go delete mode 100644 internal/jujuclient/modelwatcher_test.go diff --git a/internal/jimm/controller_test.go b/internal/jimm/controller_test.go index 2fc9732d4..9e1775e28 100644 --- a/internal/jimm/controller_test.go +++ b/internal/jimm/controller_test.go @@ -953,21 +953,6 @@ func TestImportModel(t *testing.T) { c.Run(test.about, func(c *qt.C) { api := &jimmtest.API{ ModelInfo_: test.modelInfo, - ModelWatcherNext_: func(ctx context.Context, id string) ([]jujuparams.Delta, error) { - if id != test.about { - return nil, errors.E("incorrect id") - } - return test.deltas, nil - }, - ModelWatcherStop_: func(ctx context.Context, id string) error { - if id != test.about { - return errors.E("incorrect id") - } - return nil - }, - WatchAll_: func(context.Context) (string, error) { - return test.about, nil - }, ListApplicationOffers_: func(ctx context.Context, of []jujuparams.OfferFilter) ([]jujuparams.ApplicationOfferAdminDetailsV5, error) { return test.offers, nil }, diff --git a/internal/jimm/jimm.go b/internal/jimm/jimm.go index 88c219189..a41e30bc9 100644 --- a/internal/jimm/jimm.go +++ b/internal/jimm/jimm.go @@ -241,13 +241,6 @@ type API interface { // AddCloud adds a new cloud. AddCloud(context.Context, names.CloudTag, jujuparams.Cloud, bool) error - // AllModelWatcherNext returns the next set of deltas from an - // all-model watcher. - AllModelWatcherNext(context.Context, string) ([]jujuparams.Delta, error) - - // AllModelWatcherStop stops an all-model watcher. - AllModelWatcherStop(context.Context, string) error - // ChangeModelCredential replaces cloud credential for a given model with the provided one. ChangeModelCredential(context.Context, names.ModelTag, names.CloudCredentialTag) error @@ -338,13 +331,6 @@ type API interface { // ModelSummaryWatcherStop stops a model summary watcher. ModelSummaryWatcherStop(context.Context, string) error - // ModelWatcherNext receives the next set of results from the model - // watcher with the given id. - ModelWatcherNext(ctx context.Context, id string) ([]jujuparams.Delta, error) - - // ModelWatcherStop stops the model watcher with the given id. - ModelWatcherStop(ctx context.Context, id string) error - // Offer creates a new application-offer. Offer(context.Context, crossmodel.OfferURL, jujuparams.AddApplicationOffer) error @@ -387,15 +373,9 @@ type API interface { // ValidateModelUpgrade validates that a model can be upgraded. ValidateModelUpgrade(context.Context, names.ModelTag, bool) error - // WatchAll creates a watcher that reports deltas for a specific model. - WatchAll(context.Context) (string, error) - // WatchAllModelSummaries creates a ModelSummaryWatcher. WatchAllModelSummaries(context.Context) (string, error) - // WatchAllModels creates a megawatcher. - WatchAllModels(context.Context) (string, error) - // ListFilesystems lists filesystems for desired machines. // If no machines provided, a list of all filesystems is returned. ListFilesystems(ctx context.Context, machines []string) ([]jujuparams.FilesystemDetailsListResult, error) diff --git a/internal/jimm/model_cleanup.go b/internal/jimm/model_cleanup.go index 1219814a3..2ceabee8f 100644 --- a/internal/jimm/model_cleanup.go +++ b/internal/jimm/model_cleanup.go @@ -17,8 +17,6 @@ import ( func (j *JIMM) CleanupModelsDying(ctx context.Context) error { const op = errors.Op("jimm.WatchModelsDying") - // Ensure that if the watcher stops because of a database error all - // the controller connections get closed. ctx, cancel := context.WithCancel(ctx) defer cancel() diff --git a/internal/jimm/watcher_test.go b/internal/jimm/watcher_test.go index d43bf6ba9..c293a1a97 100644 --- a/internal/jimm/watcher_test.go +++ b/internal/jimm/watcher_test.go @@ -215,6 +215,7 @@ func TestModelSummaryWatcher(t *testing.T) { } func TestWatcherSetsControllerUnavailable(t *testing.T) { + t.Skip() c := qt.New(t) ctx, cancel := context.WithCancel(context.Background()) @@ -262,6 +263,7 @@ func TestWatcherSetsControllerUnavailable(t *testing.T) { } func TestWatcherClearsControllerUnavailable(t *testing.T) { + t.Skip() c := qt.New(t) ctx, cancel := context.WithCancel(context.Background()) @@ -273,7 +275,7 @@ func TestWatcherClearsControllerUnavailable(t *testing.T) { }, Dialer: &jimmtest.Dialer{ API: &jimmtest.API{ - AllModelWatcherNext_: func(_ context.Context, _ string) ([]jujuparams.Delta, error) { + ModelSummaryWatcherNext_: func(ctx context.Context, s string) ([]jujuparams.ModelAbstract, error) { cancel() <-ctx.Done() return nil, ctx.Err() @@ -287,7 +289,7 @@ func TestWatcherClearsControllerUnavailable(t *testing.T) { } return errors.E(errors.CodeNotFound) }, - WatchAllModels_: func(ctx context.Context) (string, error) { + WatchAllModelSummaries_: func(ctx context.Context) (string, error) { return "1234", nil }, }, diff --git a/internal/jujuclient/allwatcher.go b/internal/jujuclient/allwatcher.go deleted file mode 100644 index b1ecc55db..000000000 --- a/internal/jujuclient/allwatcher.go +++ /dev/null @@ -1,46 +0,0 @@ -// Copyright 2024 Canonical. - -package jujuclient - -import ( - "context" - - jujuerrors "github.com/juju/errors" - jujuparams "github.com/juju/juju/rpc/params" - - "github.com/canonical/jimm/v3/internal/errors" -) - -// WatchAllModels initialises a new AllModelWatcher. On success the watcher -// ID is returned. This uses the WatchAllModels method on the Controller -// facade. -func (c Connection) WatchAllModels(ctx context.Context) (string, error) { - const op = errors.Op("jujuclient.WatchAllModels") - var resp jujuparams.SummaryWatcherID - if err := c.CallHighestFacadeVersion(ctx, "Controller", []int{11, 7}, "", "WatchAllModels", nil, &resp); err != nil { - return "", errors.E(op, jujuerrors.Cause(err)) - } - return resp.WatcherID, nil -} - -// AllModelWatcherNext receives the next set of results from the all-model -// watcher with the given id. This uses the Next method on the -// AllModelWatcher facade. -func (c Connection) AllModelWatcherNext(ctx context.Context, id string) ([]jujuparams.Delta, error) { - const op = errors.Op("jujuclient.AllModelWatcherNext") - var resp jujuparams.AllWatcherNextResults - if err := c.CallHighestFacadeVersion(ctx, "AllModelWatcher", []int{4, 2}, id, "Next", nil, &resp); err != nil { - return nil, errors.E(op, jujuerrors.Cause(err)) - } - return resp.Deltas, nil -} - -// AllModelWatcherStop stops the all-model watcher with the given id. This -// uses the Stop method on the AllModelWatcher facade. -func (c Connection) AllModelWatcherStop(ctx context.Context, id string) error { - const op = errors.Op("jujuclient.AllModelWatcherStop") - if err := c.CallHighestFacadeVersion(ctx, "AllModelWatcher", []int{4, 2}, id, "Stop", nil, nil); err != nil { - return errors.E(op, jujuerrors.Cause(err)) - } - return nil -} diff --git a/internal/jujuclient/allwatcher_test.go b/internal/jujuclient/allwatcher_test.go deleted file mode 100644 index b04b28d5b..000000000 --- a/internal/jujuclient/allwatcher_test.go +++ /dev/null @@ -1,52 +0,0 @@ -// Copyright 2024 Canonical. - -package jujuclient_test - -import ( - "context" - - jujuparams "github.com/juju/juju/rpc/params" - gc "gopkg.in/check.v1" -) - -type allModelWatcherSuite struct { - jujuclientSuite -} - -var _ = gc.Suite(&allModelWatcherSuite{}) - -func (s *allModelWatcherSuite) TestWatchAllModels(c *gc.C) { - ctx := context.Background() - - id, err := s.API.WatchAllModels(ctx) - c.Assert(err, gc.Equals, nil) - c.Assert(id, gc.Not(gc.Equals), "") - - err = s.API.AllModelWatcherStop(ctx, id) - c.Assert(err, gc.Equals, nil) -} - -func (s *allModelWatcherSuite) TestAllModelWatcherNext(c *gc.C) { - ctx := context.Background() - - id, err := s.API.WatchAllModels(ctx) - c.Assert(err, gc.Equals, nil) - - _, err = s.API.AllModelWatcherNext(ctx, id) - c.Assert(err, gc.Equals, nil) - - err = s.API.AllModelWatcherStop(ctx, id) - c.Assert(err, gc.Equals, nil) -} - -func (s *allModelWatcherSuite) TestAllModelWatcherNextError(c *gc.C) { - _, err := s.API.AllModelWatcherNext(context.Background(), "invalid-watcher") - c.Check(jujuparams.ErrCode(err), gc.Equals, jujuparams.CodeNotFound) - c.Check(err, gc.ErrorMatches, `unknown watcher id \(not found\)`) -} - -func (s *allModelWatcherSuite) TestAllModelWatcherStopError(c *gc.C) { - err := s.API.AllModelWatcherStop(context.Background(), "invalid-watcher") - c.Check(jujuparams.ErrCode(err), gc.Equals, jujuparams.CodeNotFound) - c.Check(err, gc.ErrorMatches, `unknown watcher id \(not found\)`) -} diff --git a/internal/jujuclient/modelwatcher_test.go b/internal/jujuclient/modelwatcher_test.go deleted file mode 100644 index bb12a8f77..000000000 --- a/internal/jujuclient/modelwatcher_test.go +++ /dev/null @@ -1,102 +0,0 @@ -// Copyright 2024 Canonical. - -package jujuclient_test - -import ( - "context" - - "github.com/juju/juju/core/network" - jujuparams "github.com/juju/juju/rpc/params" - gc "gopkg.in/check.v1" - - "github.com/canonical/jimm/v3/internal/dbmodel" - "github.com/canonical/jimm/v3/internal/jimm" - "github.com/canonical/jimm/v3/internal/jujuclient" - "github.com/canonical/jimm/v3/internal/testutils/jimmtest" -) - -type modelWatcherSuite struct { - jimmtest.JujuSuite - - Dialer jimm.Dialer - API jimm.API -} - -func (s *modelWatcherSuite) SetUpTest(c *gc.C) { - s.JujuSuite.SetUpTest(c) - - s.Dialer = &jujuclient.Dialer{ - JWTService: s.JIMM.JWTService, - } - var err error - info := s.APIInfo(c) - hpss := make(dbmodel.HostPorts, 0, len(info.Addrs)) - for _, addr := range info.Addrs { - hp, err := network.ParseMachineHostPort(addr) - if err != nil { - continue - } - hpss = append(hpss, []jujuparams.HostPort{{ - Address: jujuparams.FromMachineAddress(hp.MachineAddress), - Port: hp.Port(), - }}) - } - ctl := dbmodel.Controller{ - UUID: s.ControllerConfig.ControllerUUID(), - Name: s.ControllerConfig.ControllerName(), - CACertificate: info.CACert, - AdminIdentityName: info.Tag.Id(), - AdminPassword: info.Password, - Addresses: hpss, - } - - s.API, err = s.Dialer.Dial(context.Background(), &ctl, s.Model.ModelTag(), nil) - c.Assert(err, gc.Equals, nil) -} - -func (s *modelWatcherSuite) TearDownTest(c *gc.C) { - if s.API != nil { - err := s.API.Close() - s.API = nil - c.Assert(err, gc.Equals, nil) - } - s.JujuSuite.TearDownTest(c) -} - -var _ = gc.Suite(&modelWatcherSuite{}) - -func (s *modelWatcherSuite) TestWatchAll(c *gc.C) { - ctx := context.Background() - - id, err := s.API.WatchAll(ctx) - c.Assert(err, gc.Equals, nil) - c.Assert(id, gc.Not(gc.Equals), "") - - err = s.API.ModelWatcherStop(ctx, id) - c.Assert(err, gc.Equals, nil) -} - -func (s *modelWatcherSuite) TestModelWatcherNext(c *gc.C) { - ctx := context.Background() - - id, err := s.API.WatchAll(ctx) - c.Assert(err, gc.Equals, nil) - - _, err = s.API.ModelWatcherNext(ctx, id) - c.Assert(err, gc.Equals, nil) - - err = s.API.ModelWatcherStop(ctx, id) - c.Assert(err, gc.Equals, nil) -} - -func (s *modelWatcherSuite) TestModelWatcherNextError(c *gc.C) { - _, err := s.API.ModelWatcherNext(context.Background(), "invalid-watcher") - c.Check(jujuparams.ErrCode(err), gc.Equals, jujuparams.CodeNotFound) - c.Check(err, gc.ErrorMatches, `unknown watcher id \(not found\)`) -} - -func (s *modelWatcherSuite) TestModelWatcherStopError(c *gc.C) { - err := s.API.ModelWatcherStop(context.Background(), "invalid-watcher") - c.Check(jujuparams.ErrCode(err), gc.Equals, jujuparams.CodeNotFound) - c.Check(err, gc.ErrorMatches, `unknown watcher id \(not found\)`) -} diff --git a/internal/testutils/jimmtest/api.go b/internal/testutils/jimmtest/api.go index 3abdc11e5..d2851027c 100644 --- a/internal/testutils/jimmtest/api.go +++ b/internal/testutils/jimmtest/api.go @@ -122,8 +122,6 @@ type API struct { base.APICaller AddCloud_ func(context.Context, names.CloudTag, jujuparams.Cloud, bool) error - AllModelWatcherNext_ func(context.Context, string) ([]jujuparams.Delta, error) - AllModelWatcherStop_ func(context.Context, string) error ChangeModelCredential_ func(context.Context, names.ModelTag, names.CloudCredentialTag) error CheckCredentialModels_ func(context.Context, jujuparams.TaggedCredential) ([]jujuparams.UpdateCredentialModelResult, error) Close_ func() error @@ -150,8 +148,6 @@ type API struct { ModelSummaryWatcherNext_ func(context.Context, string) ([]jujuparams.ModelAbstract, error) ModelSummaryWatcherStop_ func(context.Context, string) error ListModelSummaries_ func(context.Context, jujuparams.ModelSummariesRequest) (jujuparams.ModelSummaryResults, error) - ModelWatcherNext_ func(ctx context.Context, id string) ([]jujuparams.Delta, error) - ModelWatcherStop_ func(ctx context.Context, id string) error Offer_ func(context.Context, crossmodel.OfferURL, jujuparams.AddApplicationOffer) error Ping_ func(context.Context) error RemoveCloud_ func(context.Context, names.CloudTag) error @@ -165,9 +161,7 @@ type API struct { UpdateCloud_ func(context.Context, names.CloudTag, jujuparams.Cloud) error UpdateCredential_ func(context.Context, jujuparams.TaggedCredential) ([]jujuparams.UpdateCredentialModelResult, error) ValidateModelUpgrade_ func(context.Context, names.ModelTag, bool) error - WatchAll_ func(context.Context) (string, error) WatchAllModelSummaries_ func(context.Context) (string, error) - WatchAllModels_ func(context.Context) (string, error) ListFilesystems_ func(ctx context.Context, machines []string) ([]jujuparams.FilesystemDetailsListResult, error) ListVolumes_ func(ctx context.Context, machines []string) ([]jujuparams.VolumeDetailsListResult, error) ListStorageDetails_ func(ctx context.Context) ([]jujuparams.StorageDetails, error) @@ -180,20 +174,6 @@ func (a *API) AddCloud(ctx context.Context, tag names.CloudTag, cld jujuparams.C return a.AddCloud_(ctx, tag, cld, force) } -func (a *API) AllModelWatcherNext(ctx context.Context, id string) ([]jujuparams.Delta, error) { - if a.AllModelWatcherNext_ == nil { - return nil, errors.E(errors.CodeNotImplemented) - } - return a.AllModelWatcherNext_(ctx, id) -} - -func (a *API) AllModelWatcherStop(ctx context.Context, id string) error { - if a.AllModelWatcherStop_ == nil { - return errors.E(errors.CodeNotImplemented) - } - return a.AllModelWatcherStop_(ctx, id) -} - func (a *API) CheckCredentialModels(ctx context.Context, cred jujuparams.TaggedCredential) ([]jujuparams.UpdateCredentialModelResult, error) { if a.CheckCredentialModels_ == nil { return nil, errors.E(errors.CodeNotImplemented) @@ -458,13 +438,6 @@ func (a *API) WatchAllModelSummaries(ctx context.Context) (string, error) { return a.WatchAllModelSummaries_(ctx) } -func (a *API) WatchAllModels(ctx context.Context) (string, error) { - if a.WatchAllModels_ == nil { - return "", errors.E(errors.CodeNotImplemented) - } - return a.WatchAllModels_(ctx) -} - func (a *API) ChangeModelCredential(ctx context.Context, model names.ModelTag, cred names.CloudCredentialTag) error { if a.ChangeModelCredential_ == nil { return errors.E(errors.CodeNotImplemented) @@ -472,27 +445,6 @@ func (a *API) ChangeModelCredential(ctx context.Context, model names.ModelTag, c return a.ChangeModelCredential_(ctx, model, cred) } -func (a *API) ModelWatcherNext(ctx context.Context, id string) ([]jujuparams.Delta, error) { - if a.ModelWatcherNext_ == nil { - return nil, errors.E(errors.CodeNotImplemented) - } - return a.ModelWatcherNext_(ctx, id) -} - -func (a *API) ModelWatcherStop(ctx context.Context, id string) error { - if a.ModelWatcherStop_ == nil { - return errors.E(errors.CodeNotImplemented) - } - return a.ModelWatcherStop_(ctx, id) -} - -func (a *API) WatchAll(ctx context.Context) (string, error) { - if a.WatchAll_ == nil { - return "", errors.E(errors.CodeNotImplemented) - } - return a.WatchAll_(ctx) -} - func (a *API) ListFilesystems(ctx context.Context, machines []string) ([]jujuparams.FilesystemDetailsListResult, error) { if a.ListFilesystems_ == nil { return nil, errors.E(errors.CodeNotImplemented) From 464ee530cb5a7dbe84e43f59654ccb0bd224d026 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Wed, 4 Dec 2024 15:58:56 +0100 Subject: [PATCH 07/17] fix tests for model summaries watcher --- internal/jimm/watcher.go | 12 ++++++++++++ internal/jimm/watcher_test.go | 3 +-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/internal/jimm/watcher.go b/internal/jimm/watcher.go index e7414881e..8edea47cc 100644 --- a/internal/jimm/watcher.go +++ b/internal/jimm/watcher.go @@ -159,6 +159,18 @@ func (w *Watcher) watchAllModelSummaries(ctx context.Context, ctl *dbmodel.Contr } // Sanitize the model abstracts. for _, summary := range modelSummaries { + m := dbmodel.Model{ + UUID: sql.NullString{ + String: summary.UUID, + Valid: true, + }, + ControllerID: ctl.ID, + } + err := w.Database.GetModel(ctx, &m) + if err != nil { + // skip summaries for model not present in JIMM's db + continue + } admins := make([]string, 0, len(summary.Admins)) for _, admin := range summary.Admins { if names.NewUserTag(admin).IsLocal() { diff --git a/internal/jimm/watcher_test.go b/internal/jimm/watcher_test.go index c293a1a97..7a7798f98 100644 --- a/internal/jimm/watcher_test.go +++ b/internal/jimm/watcher_test.go @@ -215,7 +215,6 @@ func TestModelSummaryWatcher(t *testing.T) { } func TestWatcherSetsControllerUnavailable(t *testing.T) { - t.Skip() c := qt.New(t) ctx, cancel := context.WithCancel(context.Background()) @@ -263,7 +262,6 @@ func TestWatcherSetsControllerUnavailable(t *testing.T) { } func TestWatcherClearsControllerUnavailable(t *testing.T) { - t.Skip() c := qt.New(t) ctx, cancel := context.WithCancel(context.Background()) @@ -292,6 +290,7 @@ func TestWatcherClearsControllerUnavailable(t *testing.T) { WatchAllModelSummaries_: func(ctx context.Context) (string, error) { return "1234", nil }, + SupportsModelSummaryWatcher_: true, }, }, Pubsub: &testPublisher{}, From 741725106ac9d87d20827521a557594230e440ad Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Wed, 4 Dec 2024 15:59:34 +0100 Subject: [PATCH 08/17] remove model watcher client --- internal/jujuclient/modelwatcher.go | 45 ----------------------------- 1 file changed, 45 deletions(-) delete mode 100644 internal/jujuclient/modelwatcher.go diff --git a/internal/jujuclient/modelwatcher.go b/internal/jujuclient/modelwatcher.go deleted file mode 100644 index 00137b572..000000000 --- a/internal/jujuclient/modelwatcher.go +++ /dev/null @@ -1,45 +0,0 @@ -// Copyright 2024 Canonical. - -package jujuclient - -import ( - "context" - - jujuerrors "github.com/juju/errors" - jujuparams "github.com/juju/juju/rpc/params" - - "github.com/canonical/jimm/v3/internal/errors" -) - -// WatchAll initialises a new ModelWatcher. On success the watcher -// ID is returned. This uses the WatchAll method on the Client. -func (c Connection) WatchAll(ctx context.Context) (string, error) { - const op = errors.Op("jujuclient.WatchAll") - var resp jujuparams.AllWatcherId - if err := c.CallHighestFacadeVersion(ctx, "Client", []int{6, 1}, "", "WatchAll", nil, &resp); err != nil { - return "", errors.E(op, jujuerrors.Cause(err)) - } - return resp.AllWatcherId, nil -} - -// ModelWatcherNext receives the next set of results from the model -// watcher with the given id. This uses the Next method on the -// AllWatcher facade version 1. -func (c Connection) ModelWatcherNext(ctx context.Context, id string) ([]jujuparams.Delta, error) { - const op = errors.Op("jujuclient.ModelWatcherNext") - var resp jujuparams.AllWatcherNextResults - if err := c.CallHighestFacadeVersion(ctx, "AllWatcher", []int{3, 2, 1}, id, "Next", nil, &resp); err != nil { - return nil, errors.E(op, jujuerrors.Cause(err)) - } - return resp.Deltas, nil -} - -// ModelWatcherStop stops the model watcher with the given id. This -// uses the Stop method on the AllWatcher facade version 1. -func (c Connection) ModelWatcherStop(ctx context.Context, id string) error { - const op = errors.Op("jujuclient.ModelWatcherStop") - if err := c.CallHighestFacadeVersion(ctx, "AllWatcher", []int{3, 2, 1}, id, "Stop", nil, nil); err != nil { - return errors.E(op, jujuerrors.Cause(err)) - } - return nil -} From 2cd933f9a7715a5281f0a0ae28ce647215b393e0 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Wed, 4 Dec 2024 16:24:45 +0100 Subject: [PATCH 09/17] fix minor --- cmd/jimmsrv/service/service.go | 4 ++-- internal/jimm/model_cleanup.go | 3 +++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/cmd/jimmsrv/service/service.go b/cmd/jimmsrv/service/service.go index c4082a0ee..9d7aad8fe 100644 --- a/cmd/jimmsrv/service/service.go +++ b/cmd/jimmsrv/service/service.go @@ -261,14 +261,14 @@ func (s *Service) OpenFGACleanup(ctx context.Context, trigger <-chan time.Time) } } -// OpenFGACleanup starts a goroutine that cleans up any orphaned tuples from OpenFGA. +// CleanupModelsDying triggers every `trigger` time and calls the jimm methods to cleanup dying models. func (s *Service) CleanupModelsDying(ctx context.Context, trigger <-chan time.Time) error { for { select { case <-trigger: err := s.jimm.CleanupModelsDying(ctx) if err != nil { - zapctx.Error(ctx, "openfga cleanup", zap.Error(err)) + zapctx.Error(ctx, "dying models cleanup", zap.Error(err)) continue } case <-ctx.Done(): diff --git a/internal/jimm/model_cleanup.go b/internal/jimm/model_cleanup.go index 2ceabee8f..bcfac00d7 100644 --- a/internal/jimm/model_cleanup.go +++ b/internal/jimm/model_cleanup.go @@ -1,4 +1,5 @@ // Copyright 2024 Canonical. + package jimm import ( @@ -14,6 +15,8 @@ import ( "github.com/canonical/jimm/v3/internal/errors" ) +// CleanupModelsDying loops over dying models, contacting the respective controller. +// And deleting the model from our database if the error is `NotFound` which means the model was successfully deleted. func (j *JIMM) CleanupModelsDying(ctx context.Context) error { const op = errors.Op("jimm.WatchModelsDying") From daaf82f0fff122a9cf280a4517965f38373a4a41 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Wed, 4 Dec 2024 16:38:02 +0100 Subject: [PATCH 10/17] fix --- internal/jimm/model_cleanup.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/jimm/model_cleanup.go b/internal/jimm/model_cleanup.go index bcfac00d7..ee62c33d2 100644 --- a/internal/jimm/model_cleanup.go +++ b/internal/jimm/model_cleanup.go @@ -18,7 +18,7 @@ import ( // CleanupModelsDying loops over dying models, contacting the respective controller. // And deleting the model from our database if the error is `NotFound` which means the model was successfully deleted. func (j *JIMM) CleanupModelsDying(ctx context.Context) error { - const op = errors.Op("jimm.WatchModelsDying") + const op = errors.Op("jimm.CleanupModelsDying") ctx, cancel := context.WithCancel(ctx) defer cancel() From 40972c35dffffe66e5061390dcf21a5f12904d52 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Fri, 6 Dec 2024 11:13:20 +0100 Subject: [PATCH 11/17] pr comments --- cmd/jimmsrv/main.go | 2 +- cmd/jimmsrv/service/service.go | 6 ++-- internal/jimm/model_cleanup.go | 49 ++++++++++++++--------------- internal/jimm/model_cleanup_test.go | 4 +-- 4 files changed, 29 insertions(+), 32 deletions(-) diff --git a/cmd/jimmsrv/main.go b/cmd/jimmsrv/main.go index 4dfd8e047..127183aa2 100644 --- a/cmd/jimmsrv/main.go +++ b/cmd/jimmsrv/main.go @@ -204,7 +204,7 @@ func start(ctx context.Context, s *service.Service) error { return jimmsvc.OpenFGACleanup(ctx, time.NewTicker(6*time.Hour).C) }) s.Go(func() error { - return jimmsvc.CleanupModelsDying(ctx, time.NewTicker(time.Minute).C) + return jimmsvc.CleanupDyingModels(ctx, time.NewTicker(time.Minute).C) }) } diff --git a/cmd/jimmsrv/service/service.go b/cmd/jimmsrv/service/service.go index 9d7aad8fe..f5ffa3449 100644 --- a/cmd/jimmsrv/service/service.go +++ b/cmd/jimmsrv/service/service.go @@ -261,12 +261,12 @@ func (s *Service) OpenFGACleanup(ctx context.Context, trigger <-chan time.Time) } } -// CleanupModelsDying triggers every `trigger` time and calls the jimm methods to cleanup dying models. -func (s *Service) CleanupModelsDying(ctx context.Context, trigger <-chan time.Time) error { +// CleanupDyingModels triggers every `trigger` time and calls the jimm methods to cleanup dying models. +func (s *Service) CleanupDyingModels(ctx context.Context, trigger <-chan time.Time) error { for { select { case <-trigger: - err := s.jimm.CleanupModelsDying(ctx) + err := s.jimm.CleanupDyingModels(ctx) if err != nil { zapctx.Error(ctx, "dying models cleanup", zap.Error(err)) continue diff --git a/internal/jimm/model_cleanup.go b/internal/jimm/model_cleanup.go index ee62c33d2..67b7adaec 100644 --- a/internal/jimm/model_cleanup.go +++ b/internal/jimm/model_cleanup.go @@ -9,51 +9,48 @@ import ( jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/juju/state" "github.com/juju/zaputil/zapctx" - "go.uber.org/zap" "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/errors" ) -// CleanupModelsDying loops over dying models, contacting the respective controller. +// CleanupDyingModels loops over dying models, contacting the respective controller. // And deleting the model from our database if the error is `NotFound` which means the model was successfully deleted. -func (j *JIMM) CleanupModelsDying(ctx context.Context) error { - const op = errors.Op("jimm.CleanupModelsDying") +func (j *JIMM) CleanupDyingModels(ctx context.Context) error { + const op = errors.Op("jimm.CleanupDyingModels") + zapctx.Info(ctx, string(op)) ctx, cancel := context.WithCancel(ctx) defer cancel() err := j.DB().ForEachModel(ctx, func(m *dbmodel.Model) error { - if m.Life == state.Dying.String() { - // if the model is dying and not found by querying the controller we can assume it is dead. - // And safely delete the reference from our db. - api, err := j.dialModel(ctx, &m.Controller, m.ResourceTag()) - if err != nil { - zapctx.Error(ctx, fmt.Sprintf("Cannot dial model %s: %s\n", m.UUID.String, err)) - return nil - } - if err := api.ModelInfo(ctx, &jujuparams.ModelInfo{UUID: m.UUID.String}); err != nil { - // Some versions of juju return unauthorized for models that cannot be found. - if errors.ErrorCode(err) == errors.CodeNotFound || errors.ErrorCode(err) == errors.CodeUnauthorized { - if err := j.DB().DeleteModel(ctx, m); err != nil { - zapctx.Error(ctx, fmt.Sprintf("Cannot delete model %s: %s\n", m.UUID.String, err)) - } else { - return nil - } + if m.Life != state.Dying.String() { + return nil + } + // if the model is dying and not found by querying the controller we can assume it is dead. + // And safely delete the reference from our db. + api, err := j.dialController(ctx, &m.Controller) + if err != nil { + zapctx.Error(ctx, fmt.Sprintf("Cannot dial controller %s: %s\n", m.Controller.UUID, err)) + return nil + } + if err := api.ModelInfo(ctx, &jujuparams.ModelInfo{UUID: m.UUID.String}); err != nil { + // Some versions of juju return unauthorized for models that cannot be found. + if errors.ErrorCode(err) == errors.CodeNotFound || errors.ErrorCode(err) == errors.CodeUnauthorized { + if err := j.DB().DeleteModel(ctx, m); err != nil { + zapctx.Error(ctx, fmt.Sprintf("Cannot delete model %s: %s\n", m.UUID.String, err)) } else { - zapctx.Error(ctx, fmt.Sprintf("Cannot get ModelInfo for model %s: %s\n", m.UUID.String, err)) return nil } + } else { + zapctx.Error(ctx, fmt.Sprintf("Cannot get ModelInfo for model %s: %s\n", m.UUID.String, err)) + return nil } } return nil }) if err != nil { - // Ignore temporary database errors. - if errors.ErrorCode(err) != errors.CodeDatabaseLocked { - return errors.E(op, err) - } - zapctx.Warn(ctx, "temporary error polling for controllers", zap.Error(err)) + return errors.E(op, err) } return nil } diff --git a/internal/jimm/model_cleanup_test.go b/internal/jimm/model_cleanup_test.go index c3537d439..6efb90588 100644 --- a/internal/jimm/model_cleanup_test.go +++ b/internal/jimm/model_cleanup_test.go @@ -123,7 +123,7 @@ func TestPollModelsDying(t *testing.T) { c.Assert(err, qt.IsNil) // test - err = j.CleanupModelsDying(ctx) + err = j.CleanupDyingModels(ctx) c.Assert(err, qt.IsNil) model := dbmodel.Model{ @@ -180,7 +180,7 @@ func TestPollModelsDyingControllerErrors(t *testing.T) { c.Assert(err, qt.IsNil) // test - err = j.CleanupModelsDying(ctx) + err = j.CleanupDyingModels(ctx) c.Assert(err, qt.IsNil) model := dbmodel.Model{ From 10111ef76a23897e814d3c3b1e6e159e2cbc3d90 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Fri, 6 Dec 2024 11:22:40 +0100 Subject: [PATCH 12/17] remove dial model function --- internal/jimm/utils.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/internal/jimm/utils.go b/internal/jimm/utils.go index c97cdf225..1e8387178 100644 --- a/internal/jimm/utils.go +++ b/internal/jimm/utils.go @@ -68,13 +68,3 @@ func (j *JIMM) dialController(ctx context.Context, ctl *dbmodel.Controller) (API } return api, nil } - -// dialModel dials a model. -func (j *JIMM) dialModel(ctx context.Context, ctl *dbmodel.Controller, mt names.ModelTag) (API, error) { - api, err := j.dial(ctx, ctl, mt) - if err != nil { - zapctx.Error(ctx, "failed to dial the controller", zaputil.Error(err)) - return nil, err - } - return api, nil -} From f29457e0addb8e1d5e362b0780a08901a4da0d1c Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Fri, 6 Dec 2024 12:10:46 +0100 Subject: [PATCH 13/17] set dying only if juju api don't fail --- internal/jimm/model.go | 16 ++++++++---- internal/jimm/model_test.go | 49 +++++++++++++++++++++---------------- 2 files changed, 39 insertions(+), 26 deletions(-) diff --git a/internal/jimm/model.go b/internal/jimm/model.go index d3122b67a..4e5d7d751 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1165,15 +1165,21 @@ func (j *JIMM) DestroyModel(ctx context.Context, user *openfga.User, mt names.Mo zapctx.Info(ctx, string(op)) err := j.doModelAdmin(ctx, user, mt, func(m *dbmodel.Model, api API) error { - if err := api.DestroyModel(ctx, mt, destroyStorage, force, maxWait, timeout); err != nil { - return err - } m.Life = state.Dying.String() if err := j.Database.UpdateModel(ctx, m); err != nil { - // If the database fails to update don't worry too much the - // monitor should catch it. zapctx.Error(ctx, "failed to store model change", zaputil.Error(err)) + return err } + if err := api.DestroyModel(ctx, mt, destroyStorage, force, maxWait, timeout); err != nil { + zapctx.Error(ctx, "failed to call destroy juju api", zaputil.Error(err)) + // this is a manual way of restoring the life state to alive if the JUJU api fails. + m.Life = state.Alive.String() + if err := j.Database.UpdateModel(ctx, m); err != nil { + zapctx.Error(ctx, "failed to store model change", zaputil.Error(err)) + } + return err + } + return nil }) if err != nil { diff --git a/internal/jimm/model_test.go b/internal/jimm/model_test.go index f7d76265c..111a9378e 100644 --- a/internal/jimm/model_test.go +++ b/internal/jimm/model_test.go @@ -3141,6 +3141,7 @@ var destroyModelTests = []struct { timeout *time.Duration expectError string expectErrorCode errors.Code + expectedLife string }{{ name: "NotFound", env: destroyModelTestEnv, @@ -3182,30 +3183,34 @@ var destroyModelTests = []struct { force: newBool(false), maxWait: newDuration(time.Second), timeout: newDuration(time.Second), + expectedLife: "dying", }, { name: "SuperuserSuccess", env: destroyModelTestEnv, destroyModel: func(_ context.Context, _ names.ModelTag, _, _ *bool, _, _ *time.Duration) error { return nil }, - username: "charlie@canonical.com", - uuid: "00000002-0000-0000-0000-000000000001", + username: "charlie@canonical.com", + uuid: "00000002-0000-0000-0000-000000000001", + expectedLife: "dying", }, { - name: "DialError", - env: destroyModelTestEnv, - dialError: errors.E("dial error"), - username: "alice@canonical.com", - uuid: "00000002-0000-0000-0000-000000000001", - expectError: `dial error`, + name: "DialError", + env: destroyModelTestEnv, + dialError: errors.E("dial error"), + username: "alice@canonical.com", + uuid: "00000002-0000-0000-0000-000000000001", + expectError: `dial error`, + expectedLife: "alive", }, { name: "APIError", env: destroyModelTestEnv, destroyModel: func(_ context.Context, _ names.ModelTag, _, _ *bool, _, _ *time.Duration) error { return errors.E("api error") }, - username: "charlie@canonical.com", - uuid: "00000002-0000-0000-0000-000000000001", - expectError: `api error`, + username: "charlie@canonical.com", + uuid: "00000002-0000-0000-0000-000000000001", + expectError: `api error`, + expectedLife: "alive", }} func TestDestroyModel(t *testing.T) { @@ -3249,18 +3254,20 @@ func TestDestroyModel(t *testing.T) { if test.expectErrorCode != "" { c.Check(errors.ErrorCode(err), qt.Equals, test.expectErrorCode) } - return + } else { + c.Assert(err, qt.IsNil) } - c.Assert(err, qt.IsNil) - m := dbmodel.Model{ - UUID: sql.NullString{ - String: test.uuid, - Valid: true, - }, + if test.expectedLife != "" { + m := dbmodel.Model{ + UUID: sql.NullString{ + String: test.uuid, + Valid: true, + }, + } + err = j.Database.GetModel(ctx, &m) + c.Assert(err, qt.IsNil) + c.Assert(m.Life, qt.Equals, test.expectedLife) } - err = j.Database.GetModel(ctx, &m) - c.Assert(err, qt.IsNil) - c.Check(m.Life, qt.Equals, state.Dying.String()) }) } } From 7400a848b160702e9261ccc2a3bf751dc0adc908 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Fri, 6 Dec 2024 13:59:54 +0100 Subject: [PATCH 14/17] remove context --- internal/jimm/model_cleanup.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/jimm/model_cleanup.go b/internal/jimm/model_cleanup.go index 67b7adaec..ab735f5e1 100644 --- a/internal/jimm/model_cleanup.go +++ b/internal/jimm/model_cleanup.go @@ -20,9 +20,6 @@ func (j *JIMM) CleanupDyingModels(ctx context.Context) error { const op = errors.Op("jimm.CleanupDyingModels") zapctx.Info(ctx, string(op)) - ctx, cancel := context.WithCancel(ctx) - defer cancel() - err := j.DB().ForEachModel(ctx, func(m *dbmodel.Model) error { if m.Life != state.Dying.String() { return nil From 31151989d52b87bb8d65c53232df5289f88c002c Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Mon, 9 Dec 2024 09:23:51 +0100 Subject: [PATCH 15/17] pr suggestion --- internal/jimm/model.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/jimm/model.go b/internal/jimm/model.go index 4e5d7d751..53e85e869 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1174,8 +1174,8 @@ func (j *JIMM) DestroyModel(ctx context.Context, user *openfga.User, mt names.Mo zapctx.Error(ctx, "failed to call destroy juju api", zaputil.Error(err)) // this is a manual way of restoring the life state to alive if the JUJU api fails. m.Life = state.Alive.String() - if err := j.Database.UpdateModel(ctx, m); err != nil { - zapctx.Error(ctx, "failed to store model change", zaputil.Error(err)) + if uerr := j.Database.UpdateModel(ctx, m); uerr != nil { + zapctx.Error(ctx, "failed to store model change", zaputil.Error(uerr)) } return err } From 46a19508db40d521bf3ce0e25f9d09b7a4464a4c Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Tue, 10 Dec 2024 10:50:00 +0100 Subject: [PATCH 16/17] fix pr comments --- internal/jimm/model.go | 2 +- internal/jimm/model_cleanup.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/jimm/model.go b/internal/jimm/model.go index 53e85e869..0da4ccc8e 100644 --- a/internal/jimm/model.go +++ b/internal/jimm/model.go @@ -1171,7 +1171,7 @@ func (j *JIMM) DestroyModel(ctx context.Context, user *openfga.User, mt names.Mo return err } if err := api.DestroyModel(ctx, mt, destroyStorage, force, maxWait, timeout); err != nil { - zapctx.Error(ctx, "failed to call destroy juju api", zaputil.Error(err)) + zapctx.Error(ctx, "failed to call DestroyModel juju api", zaputil.Error(err)) // this is a manual way of restoring the life state to alive if the JUJU api fails. m.Life = state.Alive.String() if uerr := j.Database.UpdateModel(ctx, m); uerr != nil { diff --git a/internal/jimm/model_cleanup.go b/internal/jimm/model_cleanup.go index ab735f5e1..f4c040b07 100644 --- a/internal/jimm/model_cleanup.go +++ b/internal/jimm/model_cleanup.go @@ -28,19 +28,19 @@ func (j *JIMM) CleanupDyingModels(ctx context.Context) error { // And safely delete the reference from our db. api, err := j.dialController(ctx, &m.Controller) if err != nil { - zapctx.Error(ctx, fmt.Sprintf("Cannot dial controller %s: %s\n", m.Controller.UUID, err)) + zapctx.Error(ctx, fmt.Sprintf("cannot dial controller %s: %s\n", m.Controller.UUID, err)) return nil } if err := api.ModelInfo(ctx, &jujuparams.ModelInfo{UUID: m.UUID.String}); err != nil { // Some versions of juju return unauthorized for models that cannot be found. if errors.ErrorCode(err) == errors.CodeNotFound || errors.ErrorCode(err) == errors.CodeUnauthorized { if err := j.DB().DeleteModel(ctx, m); err != nil { - zapctx.Error(ctx, fmt.Sprintf("Cannot delete model %s: %s\n", m.UUID.String, err)) + zapctx.Error(ctx, fmt.Sprintf("cannot delete model %s: %s\n", m.UUID.String, err)) } else { return nil } } else { - zapctx.Error(ctx, fmt.Sprintf("Cannot get ModelInfo for model %s: %s\n", m.UUID.String, err)) + zapctx.Error(ctx, fmt.Sprintf("cannot get ModelInfo for model %s: %s\n", m.UUID.String, err)) return nil } } From 0f493bac2d3c1784f132ba232d8980870382a1c1 Mon Sep 17 00:00:00 2001 From: SimoneDutto Date: Tue, 10 Dec 2024 11:51:32 +0100 Subject: [PATCH 17/17] move to qt suite --- internal/jimm/model_cleanup_test.go | 88 ++++++++++++++--------------- 1 file changed, 43 insertions(+), 45 deletions(-) diff --git a/internal/jimm/model_cleanup_test.go b/internal/jimm/model_cleanup_test.go index 6efb90588..2d647e3f6 100644 --- a/internal/jimm/model_cleanup_test.go +++ b/internal/jimm/model_cleanup_test.go @@ -9,6 +9,7 @@ import ( "time" qt "github.com/frankban/quicktest" + "github.com/frankban/quicktest/qtsuite" "github.com/google/uuid" jujuparams "github.com/juju/juju/rpc/params" "github.com/juju/juju/state" @@ -18,6 +19,7 @@ import ( "github.com/canonical/jimm/v3/internal/dbmodel" "github.com/canonical/jimm/v3/internal/errors" "github.com/canonical/jimm/v3/internal/jimm" + "github.com/canonical/jimm/v3/internal/openfga" "github.com/canonical/jimm/v3/internal/testutils/jimmtest" ) @@ -80,35 +82,45 @@ users: controller-access: superuser ` -func TestPollModelsDying(t *testing.T) { - // init - c := qt.New(t) - ctx := context.Background() +type modelCleanupSuite struct { + jimm *jimm.JIMM + jimmAdmin *openfga.User + ofgaClient *openfga.OFGAClient + env *jimmtest.Environment +} - client, _, _, err := jimmtest.SetupTestOFGAClient(c.Name()) +func (s *modelCleanupSuite) Init(c *qt.C) { + ctx := context.Background() + // Setup DB + var err error + s.ofgaClient, _, _, err = jimmtest.SetupTestOFGAClient(c.Name()) c.Assert(err, qt.IsNil) - j := &jimm.JIMM{ + s.jimm = &jimm.JIMM{ UUID: uuid.NewString(), - OpenFGAClient: client, + OpenFGAClient: s.ofgaClient, Database: db.Database{ - DB: jimmtest.PostgresDB(c, nil), + DB: jimmtest.PostgresDB(c, time.Now), }, } - err = j.Database.Migrate(ctx, false) + err = s.jimm.Database.Migrate(ctx, false) c.Assert(err, qt.IsNil) - jimmAdmin, err := j.GetUser(ctx, "alice@canonical.com") + s.jimmAdmin, err = s.jimm.GetUser(ctx, "alice@canonical.com") c.Assert(err, qt.IsNil) - env := jimmtest.ParseEnvironment(c, modelPollerTestEnv) - env.PopulateDBAndPermissions(c, j.ResourceTag(), j.Database, client) + s.env = jimmtest.ParseEnvironment(c, modelPollerTestEnv) + s.env.PopulateDBAndPermissions(c, s.jimm.ResourceTag(), s.jimm.Database, s.ofgaClient) +} + +func (s *modelCleanupSuite) TestPollModelsDying(c *qt.C) { + ctx := context.Background() - j.Dialer = &jimmtest.Dialer{ + s.jimm.Dialer = &jimmtest.Dialer{ API: &jimmtest.API{ ModelInfo_: func(ctx context.Context, mi *jujuparams.ModelInfo) error { switch mi.UUID { - case env.Models[0].UUID: + case s.env.Models[0].UUID: return errors.E(errors.CodeNotFound) - case env.Models[1].UUID: + case s.env.Models[1].UUID: return nil default: return errors.E("new error") @@ -119,54 +131,36 @@ func TestPollModelsDying(t *testing.T) { }, }, } - err = j.DestroyModel(ctx, jimmAdmin, names.NewModelTag(env.Models[0].UUID), nil, nil, nil, nil) + err := s.jimm.DestroyModel(ctx, s.jimmAdmin, names.NewModelTag(s.env.Models[0].UUID), nil, nil, nil, nil) c.Assert(err, qt.IsNil) // test - err = j.CleanupDyingModels(ctx) + err = s.jimm.CleanupDyingModels(ctx) c.Assert(err, qt.IsNil) model := dbmodel.Model{ UUID: sql.NullString{ - String: env.Models[0].UUID, + String: s.env.Models[0].UUID, Valid: true, }, } - err = j.DB().GetModel(ctx, &model) + err = s.jimm.DB().GetModel(ctx, &model) c.Assert(err, qt.ErrorMatches, "model not found") model = dbmodel.Model{ UUID: sql.NullString{ - String: env.Models[1].UUID, + String: s.env.Models[1].UUID, Valid: true, }, } - err = j.DB().GetModel(ctx, &model) + err = s.jimm.DB().GetModel(ctx, &model) c.Assert(err, qt.IsNil) } -func TestPollModelsDyingControllerErrors(t *testing.T) { - // init - c := qt.New(t) +func (s *modelCleanupSuite) TestPollModelsDyingControllerErrors(c *qt.C) { ctx := context.Background() - client, _, _, err := jimmtest.SetupTestOFGAClient(c.Name()) - c.Assert(err, qt.IsNil) - j := &jimm.JIMM{ - UUID: uuid.NewString(), - OpenFGAClient: client, - Database: db.Database{ - DB: jimmtest.PostgresDB(c, nil), - }, - } - err = j.Database.Migrate(ctx, false) - c.Assert(err, qt.IsNil) - - env := jimmtest.ParseEnvironment(c, modelPollerTestEnv) - env.PopulateDBAndPermissions(c, j.ResourceTag(), j.Database, client) - jimmAdmin, err := j.GetUser(ctx, "alice@canonical.com") - c.Assert(err, qt.IsNil) - j.Dialer = &jimmtest.Dialer{ + s.jimm.Dialer = &jimmtest.Dialer{ API: &jimmtest.API{ ModelInfo_: func(ctx context.Context, mi *jujuparams.ModelInfo) error { return errors.E("controller not available") @@ -176,20 +170,24 @@ func TestPollModelsDyingControllerErrors(t *testing.T) { }, }, } - err = j.DestroyModel(ctx, jimmAdmin, names.NewModelTag(env.Models[0].UUID), nil, nil, nil, nil) + err := s.jimm.DestroyModel(ctx, s.jimmAdmin, names.NewModelTag(s.env.Models[0].UUID), nil, nil, nil, nil) c.Assert(err, qt.IsNil) // test - err = j.CleanupDyingModels(ctx) + err = s.jimm.CleanupDyingModels(ctx) c.Assert(err, qt.IsNil) model := dbmodel.Model{ UUID: sql.NullString{ - String: env.Models[0].UUID, + String: s.env.Models[0].UUID, Valid: true, }, } - err = j.DB().GetModel(ctx, &model) + err = s.jimm.DB().GetModel(ctx, &model) c.Assert(err, qt.IsNil) c.Assert(model.Life, qt.Equals, state.Dying.String()) } + +func TestDyingModelsCleanup(t *testing.T) { + qtsuite.Run(qt.New(t), &modelCleanupSuite{}) +}