remove controller watcher #1478
Conversation
There was a problem hiding this comment.
in the previous pr i've renamed the watcher_test.go file to avoid fixing useless tests failing, but now this commit seems to add new test in this watcher_test.go
It is not, I've just removed all tests regarding ModelWatcher and readded the ModelSummaryWatcher tests.
| ctx, cancel := context.WithCancel(ctx) | ||
| defer cancel() | ||
|
|
||
| err := j.DB().ForEachModel(ctx, func(m *dbmodel.Model) error { |
There was a problem hiding this comment.
Instead of for each models, rather list models that are dying.
There was a problem hiding this comment.
yep, i was battled because i feel like creating a method in the database called GetDyingModels() is not very clever. I'd rather add a List(filter) method in our db methods.
But i remember last time there was some opposition to that, and rather just using "ForEach" to keep it consistent.
There was a problem hiding this comment.
I think foreach is fine honestly and dialing the controllers makes a lot of sense to me. Why would we have a db method?
internal/jimm/model_cleanup.go
Outdated
| 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()) |
There was a problem hiding this comment.
What about following the same pattern as the previous code. Something like:
- Start a routine for each controller.
- Open API connection to controller.
- GetDyingModels(ThisController)
- Query controller for model status.
If the routine returns with an error, the caller can reschedule after e.g. 30s. It just saves us for making a new connection for each model. Granted this may be a premature optimisation but it feels cleaner and aligns with how the watcher worked before anyway.
There was a problem hiding this comment.
Potentially we could do something like this.
However I though starting a routine for each controller feels like a useless optimization because we run this cleanup every 1 minute, so do we care how long the cleanup is? No user is waiting for this to finish.
Saving the connection for each model is fine, if we move towards listing the dying model.
internal/jimm/model_cleanup.go
Outdated
| 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. |
There was a problem hiding this comment.
can you please explain which versions return which error?
There was a problem hiding this comment.
i copy/pasted this from the watcher code. I honestly have no idea.
There was a problem hiding this comment.
it would be nice to verify
There was a problem hiding this comment.
2d232e1
it's a 3 years old comment from Martin
* remove controller watcher
Description
In this PR we remove the Model Watcher controller from JIMM.
To do so:
Removed:
Fixes JIRA/GitHub issue number
Engineering checklist
Check only items that apply
Test instructions
Notes for code reviewers