fix: cron removed on module change #3512
Conversation
stuartwdouglas
force-pushed
the
stuartwdouglas/3503
branch
4 times, most recently
from
69863ae
to
fc03977
Compare
|
Can you describe the actual change here? Or preferably, add comments to the PR. The code doesn't make sense to me on the face of it, but that's probably because I don't understand the problem. |
stuartwdouglas
force-pushed
the
stuartwdouglas/3503
branch
from
fc03977
to
7fe8e20
Compare
|
I added some comments. Basically you see the 'added' message for the new deployment before you see the 'removed' message. The code the way it was would end up deleting a modules cron jobs when the old module was removed. |
backend/cron/service.go
Outdated
| @@ -78,6 +78,7 @@ func run(ctx context.Context, verbClient CallClient, changes chan *ftlv1.PullSch | |||
| logger := log.FromContext(ctx).Scope("cron") | |||
| // Map of cron jobs for each module. | |||
| cronJobs := map[string][]cronJob{} | |||
| currentDeploymentForModule := map[string]string{} | |||
There was a problem hiding this comment.
What I don't understand is that it looks like keys are never removed from this map?
There was a problem hiding this comment.
oops, that was an oversight, fixed. It would not have caused any actual issues in practice, unless you added and removed a huge number of modules.
stuartwdouglas
force-pushed
the
stuartwdouglas/3503
branch
from
7fe8e20
to
13136c3
Compare
| switch resp.ChangeType { | ||
| case ftlv1.DeploymentChangeType_DEPLOYMENT_REMOVED: | ||
| // We see the new state of the module before we see the removed deployment. |
There was a problem hiding this comment.
Ah I see, that makes sense. Though if it's immediately ADDED, this shouldn't actually stop cron jobs from working as the ticket describes right?
There was a problem hiding this comment.
It is ADDED then REMOVED, so we first get a message with the new cron schema, then get a message that the old one is removed. With the code the way it currently is all the cron jobs after the first deployment basically get removed immediately.
|
The order of the updates is correct though, both deployments are running for a while for rolling deployments, and canaries will only make this worse. |
stuartwdouglas
force-pushed
the
stuartwdouglas/3503
branch
from
13136c3
to
d059442
Compare
stuartwdouglas
requested review from
wesbillman
and removed request for
a team
| @@ -100,13 +100,18 @@ func Start(ctx context.Context, config Config, pullSchemaClient PullSchemaClient | |||
| newState.protoSchema.Modules = append(newState.protoSchema.Modules, resp.Schema) | |||
| } | |||
| } else if existing != nil { | |||
| // We see the new state of the module before we see the removed deployment. | |||
| // We only want to actually remove if it was not replaced by a new deployment. | |||
| if !resp.ModuleRemoved { | |||
fixes #3503