Handle retention locks on SM side#4834
Conversation
Parallel manifest upload is limited to 12 to avoid memory issues caused by storing all manifests in memory at the same time. This is a sane guardrail that should be exported and used whenever we are worried about going OOM on SM side. Note that this limitation is not really respected in other places in the code (we store snapshotDirs for all nodes in memory, restore loads all manifests to memory), but we should aim not to increase the potential risk of running OOM if we can prevent it, and we don't expect big performance penalty. One such case could be creating retention locks on backed up files, where we expect this operation to be way shorter than the upload of backed up files, so it shouldn't introduce noticeable performance regression.
As discovered in: #4659 (comment), rclone server reads the whole req body into memory before calling specific handlers. Because of that, there is a limit to a single req body size, so that agent does not go OOM. This commit adds a guardrail describing max safe sstable component file cnt that can be sent in a single req body, so that it doesn't hit the rclone side limit. It should be used in all places which call rclone endpoints accepting sstable component list of potentially any size. One of such places will be the new clone/operations/retention-lock endpoint used for locking backup files in batches. Refs #4659
This commit adds a separate backup stage that will be responsible for applying retention locks on all files of and already finalized backup.
This commit adds new backup metric responsible for tracking progress of already locked backup files.
Michal-Leszczynski
changed the title
2a13aff to
0fae07d
Compare
This commit introduces retention lock configuration to backup properties and target. It also defines their validation and default values.
This commit adds StageRetentionLock, which is responsible for creating retention locks on finalized backup files. To make it resumable, it can't rely on existing snapshotDirs, as they might be empty or incomplete after resume, as they only represent the files that were uploaded in the current task run. This means it needs to fetch the already uploaded manifests from backup location and discover the files that needs to be locked from them. The SM side implementation is works on a fail fast basis, so it works differently than the SM agent side implementation of retention lock endpoint which works on best effort basis. We lack a mechanism for resuming the retention lock progress on resume, so the best effort approach does not give us that many benefits for now. It doesn't seem important to align them on SM and SM agent side for now, but it's something to take into consideration. SM memory consideration: Retention locks operate on per-file basis, so at some point we need store file names in memory. The current most memory safe way of iterating over manifest files is by downloading manifest Index into a tmp file and iterating over FilesMeta one by one. FilesMeta contains all files from a single node from a single table, so it's potentially still in millions. That's why the cross node parallelism is limited to 12 (as when uploading the manifests), so that we don't end up storing all file names from all nodes in memory at the same time, which could lead into memory issues for bigger clusters. This can be improved by refactoring ForEachIndexIterWithError (or just adding new iter method), so that it would allow to call callback on a batch of read files, but this requires additional work and should be done as a separate effort if needed. Then, we could either remove the limitation or greatly increase it. SM agent memory considerations: Since rclone server reads the whole req body into memory before processing the req, it limits the max body size to 1MB. We need to be aware of this on SM side, as sending all file names of given table in a single req body could easily go over such limit and fail, as it was shown in #4659. Because of that, retention lock jobs are batched to send up to 10k files in a single req. If ForEachIndexIterWithError is ever changed to work per batch basis, we should use the same batch size here as well. Fixes https://scylladb.atlassian.net/browse/CLOUD-1915
Since we lack rclone endpoint for checking file retention lock configuration, we will need to check it directly from fake GCS server with GCS SDK. This commit adds it to our vendored deps. The hidden import is temporarily added so that 'go mod tidy' does not remove this dep from our go.mod. As it turned out, our *.dev .gitignore rule prevented us from vendoring the new deps referring to packages like cel.dev/expr. To fix that, .gitignore has been updated to always include entire vendor dir.
0fae07d to
bc398f0
Compare
This commit adds the following tests validating retention lock backup: - TestBackupRetentionLockIntegration - checking that consecutive backups succeeds and apply correct locks - TestBackupResumeOnRetentionLockStageIntegration - checking that backup interrupted when applying locks can be safely resumed and succeed - TestRetentionLockUntil - simple unit test checking that the retention period is calculated correctly
This commit modifies smoke tables and schema restore tests so that they operate on backup with retention locks to verify that they don't impact restore procedure in any way.
bc398f0 to
5b09a7c
Compare
|
@copilot review this PR |
Here's my review of the PR: Overall: The implementation is well-structured and follows the existing patterns in the codebase. The new Positive aspects:
Potential issues / suggestions:
No blocking issues found. The code is clean, well-tested, and follows existing patterns. |
This PR adds new
StageRetentionLockto SM backup procedure.This stage is configured according to
RetentionLockMode,OverrideRetentionLock,RetentionDaysbackup Target fields (not yet exposed viasctoolflags).Most things are straight forward and described in commit messages, but here are a few considerations that could be discussed:
StageRetentionLock- it always starts from scratchsctool progressdisplay forStageRetentionLock- progress can be tracked only via metricsStageRetentionLockoperates for up to 12 nodes in parallel to limit the amount of manifests stored in memory - can be fixed in the future, but is this a problem?Fixes https://scylladb.atlassian.net/browse/CLOUD-1915