backupccl: incremental schedules always wait on_previous_running #98249
Conversation
|
This change is |
|
An alternative suggested by @shermanCRL is that we stop accepting |
| @@ -384,6 +387,30 @@ func doCreateBackupSchedules( | |||
| kmsURIs, nil, resultsCh) | |||
| } | |||
|
|
|||
| func maybeOverrideIncrementalScheduleDetails(ctx context.Context, details *jobspb.ScheduleDetails) { | |||
There was a problem hiding this comment.
It might be better to error and fail rather than overriding with a warning?
There was a problem hiding this comment.
I don't think we can do that because that would mean CREATE SCHEDULE with an on_previous_running = start would fail as a whole. Since we do not have a way to express full backup and inc backup schedules separately we still want start to apply to the full schedule but want to override what applies to the incremental schedule. As mentioned in my comment above and in the slack thread I think we'll change this to unconditionally set inc schedule to have the WAIT option.
There was a problem hiding this comment.
Right 🤦 In that case, maybe the warning could include that? on_previous_running=start only applies to full backups. incremental backups will use on_previous_running=wait or something similar?
There was a problem hiding this comment.
After an internal chat we decided that we should simplify the user story by making on_previous_running a configurable option for only full backup schedules. Incrementals will always wait for previous running jobs to complete before spawning a new incremental job. I've updated the commit message and the code with this new implementation. I don't think we even need to log a warning anymore.
adityamaru
force-pushed
the
schedule-fixup
branch
from
86c9c13 to
7987f56
Compare
adityamaru
changed the title
on_previous_running = start
|
cc: @kathancox for the required docs changes to make |
An incremental backup schedule must always wait if there is a running job that was previously scheduled by this incremental schedule. This is because until the previous incremental backup job completes, all future incremental jobs will attempt to backup data from the same `StartTime` corresponding to the `EndTime` of the last incremental layer. In this case only the first incremental job to complete will succeed, while the remaining jobs will either be rejected or worse corrupt the chain of backups. This change overrides the Wait behaviour for an incremental schedule to always default to `wait` during schedule creation or in an alter statement. Note the user specified value will still be applied to the full backup schedule. Ideally we'd have a way to configure options for both the full and incremental schedule separately, in which case we could reject the `on_previous_running` configuration for incremental schedules. Until then this workaround will have to do and we should call out this known limitation. Release note (enterprise change): backup schedules created or altered to have the option `on_previous_running` will have the full backup schedule created with the user specified option, but will override the incremental backup schedule to always default to `on_previous_running = wait`. This ensures correctness of the backup chains created by the incremental schedule by preventing duplicate incremental jobs from racing against each other.
adityamaru
force-pushed
the
schedule-fixup
branch
from
7987f56 to
e22626f
Compare
|
TFTR! bors r=benbardin |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from e22626f to blathers/backport-release-22.1-98249: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 22.1.x failed. See errors above. error creating merge commit from e22626f to blathers/backport-release-22.2-98249: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 22.2.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
An incremental backup schedule must always wait if there is a running job
that was previously scheduled by this incremental schedule. This is
because until the previous incremental backup job completes, all future
incremental jobs will attempt to backup data from the same
StartTimecorresponding to the
EndTimeof the last incremental layer. In thiscase only the first incremental job to complete will succeed, while the
remaining jobs will either be rejected or worse corrupt the chain of
backups.
This change overrides the Wait behaviour for an incremental schedule to
always default to
waitduring schedule creation or in an alter statement.Note the user specified value will still be applied to the full backup schedule.
Ideally we'd have a way to configure options for both the full and
incremental schedule separately, in which case we could reject the
on_previous_runningconfiguration for incremental schedules.Until then this workaround will have to do and we should call out this
known limitation.
Fixes: #96110
Release note (enterprise change): backup schedules created or altered to
have the option
on_previous_runningwill have the full backupschedule created with the user specified option, but will override the
incremental backup schedule to always default to
on_previous_running = wait.This ensures correctness of the backup chains created by the incremental
schedule by preventing duplicate incremental jobs from racing against each
other.