CSS-6435 update migrated models #1109
Conversation
kian99
requested review from
alesstimec,
ale8k,
babakks and
mina1460
as code owners
kian99
force-pushed
the
CSS-6435-update-migrated-models
branch
2 times, most recently
from
afcf3bc to
256ec9b
Compare
kian99
force-pushed
the
CSS-6435-update-migrated-models
branch
from
9dba5b5 to
1855829
Compare
| model.Life = constants.MIGRATING_INTERNAL.String() | ||
| model.NewControllerID = sql.NullInt32{Int32: int32(newControllerID), Valid: true} | ||
| } | ||
| err = j.Database.UpdateModel(ctx, &model) |
There was a problem hiding this comment.
a-ha! another on for the distributed transactions
There was a problem hiding this comment.
Indeed, in this case we'd want to put the model into a "pending_start_migration" state and then a worker picks up on that and calls Juju to starts the migration then we can update the state to "pending_complete_migration" or something like that.
| checkMigratingModel := func(m *dbmodel.Model) error { | ||
| // models that were in the migrating state may no longer be on | ||
| // the controller, here we will update them once migration has completed. | ||
| modelMigrating := m.Life == constants.MIGRATING_AWAY.String() || m.Life == constants.MIGRATING_INTERNAL.String() |
There was a problem hiding this comment.
wouldn't this case be caught by the check on L242? what if we failed to update our DB but managed to initiate migration on the controller..
There was a problem hiding this comment.
If we removed this check then we would be calling ModelInfo on all models on all controllers which was not the intention. As for "if we failed to update our DB but managed to initiate migration on the controller." that's the same problem the checkDyingModel function faces and is a more general problem that can't be solved cleanly without something like distributed transactions.
| if !misingOrRedirectError { | ||
| return nil | ||
| } | ||
| // Model undergoing internal migration needs an update to its parent controller. |
There was a problem hiding this comment.
does this if/else mean that juju controller will return a not-found error when migration completes?
There was a problem hiding this comment.
It first returns a redirect error, you can sort of see that here https://github.com/juju/juju/blob/3.3/cmd/modelcmd/base.go#L265 and then from my testing locally that redirect error eventually becomes a not-found error.
| err = w.Database.GetModel(context.Background(), &modelInternalMigrated) | ||
| c.Assert(err, qt.IsNil) | ||
| c.Assert(modelInternalMigrated.Controller.Name, qt.Equals, "controller-1") | ||
| c.Assert(modelInternalMigrated.Life, qt.Equals, "alive") |
There was a problem hiding this comment.
constants.ALIVE.String()
There was a problem hiding this comment.
I haven't changed the magic strings in all tests, should I?
There was a problem hiding this comment.
Sure, I'll change them in the tests too.
There was a problem hiding this comment.
In fact, I'll do this in a follow-up so that it's clearer.
| if i >= 0 && i < len(lifeNames) { | ||
| return lifeNames[i] | ||
| } | ||
| return "unknown" |
There was a problem hiding this comment.
logging here might be very useful because this should never happen
There was a problem hiding this comment.
I don't think this package should be logging, otherwise we need to provide a context.
| // String returns the name of an model life constant. | ||
| func (p ModelLife) String() string { | ||
| i := int(p) | ||
| if i >= 0 && i < len(lifeNames) { |
There was a problem hiding this comment.
isn't 0 the "unknown"?
Should we check for >= 1 ?
There was a problem hiding this comment.
0 is the value for the first enum so I think it's valid to check for that. The fact that 0 is "unknown" and the return value, if out of bounds, also being "unknown" is okay I think.
| return ModelLife(p), true | ||
| } | ||
| } | ||
| return UNKNOWN, false |
There was a problem hiding this comment.
I believe this should return an error instead of the boolean.
Also, why allow the parser to continue when the model life is unknown? I would have just returned an error instead of allowing the "unknown" to propagate.
If you decide to keep it, then an error/warn log would be good and useful here.
There was a problem hiding this comment.
This is a common pattern to return a bool like checking if a key exists in a map. The only error we'd ever return is "String not parseable/not found" which is the same as returning false.
| // to its constant value. | ||
| func ParseModelLife(target string) (ModelLife, bool) { | ||
| for p, name := range lifeNames { | ||
| if target == name { |
There was a problem hiding this comment.
You might want to lowercase both before comparison or use strings.EqualFold instead
There was a problem hiding this comment.
I think the exact match is more correct because this should be used generally when you're exchanging a ModelLife value over a network request where the type is lost and then want to convert it back into a concrete type. So the case should also match.
| err = w.Database.GetModel(context.Background(), &modelInternalMigrated) | ||
| c.Assert(err, qt.IsNil) | ||
| c.Assert(modelInternalMigrated.Controller.Name, qt.Equals, "controller-1") | ||
| c.Assert(modelInternalMigrated.Life, qt.Equals, "alive") |
Description
This PR builds on #1107 and should be reviewed after that lands.
This PR handles updating JIMM's database when a model migration takes place. After a successful migration the database needs to either reflect the new controller a model is under (in the case of migrating to a controller within JIMM) or the model needs to be deleted from JIMM's database (when migrating to a controller not managed by JIMM).
We utilise JIMM's watch method which runs periodically to poll all models on each controller, at which point we check if the model is in the migrating state and then check if the model still exists on its original controller.
Fixes CSS-6435
Engineering checklist
Check only items that apply