Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore improvement: transfers #4054

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Michal-Leszczynski
Copy link
Collaborator

@Michal-Leszczynski Michal-Leszczynski commented Oct 2, 2024

This PR will need to be split into 2 separate ones, as it modifies both scylla manager pkg and swagger pkg.
For now I used go.mod replace for testing purposes, but with 2 separate PRs it won't be needed anymore.

This PR also contains other changes, as it is rebased on #4044 and other restore related feature branches.

This PR allows for setting transfers in the same fashion as setting rate limit.
There is a dedicated endpoint /rclone/core/transfers which sets the amount of transfers used during upload/download by all the following API calls.
Fixed issue describes the need to allow for setting transfers only during restore, but because of the above, transfers also need be re-set during backup (so that backup does not use the transfers set by restore). Because of that, I decided to make transfers configurable not only in restore, but also in backup.

For the backup, the transfers need to be greater than 0, and the default value is 2 - the same as the default value in rclone config.

For restore, the transfers can be equal to 0 (special value describing transfers=2*node_shard_count) and this is their default value, as we want to make unconfigured restore as fast as possible.

Fixes #3948

@Michal-Leszczynski Michal-Leszczynski changed the title Restore: allow for setting transfers Restore improvement: allow for setting transfers Oct 2, 2024
@Michal-Leszczynski Michal-Leszczynski changed the title Restore improvement: allow for setting transfers Restore improvement: transfers Oct 2, 2024
@Michal-Leszczynski Michal-Leszczynski force-pushed the ml/restore-transfers branch 4 times, most recently from ba26e48 to 2962fa8 Compare October 8, 2024 12:50
This endpoint will be used to control the amount of transfers during backup/restore.
It does so by changing the transfer value in global config and clearing file system
cache, so that the file systems can be regenerated again with correct transfers value.
This commit exposes the /rclone/core/transfers agent endpoint via agent client.
This is the first step to control transfers in the context of restore.
This commit allows user to control the amount of rclone transfers used during restore.
…t default

Our experiments showed that the fastest file download
was achieved for transfers=2*cpu_cnt. In order to make
it easier for the user to use, a new, special value
of --transfers=0 flag will take care of that.
It is set as the default, because we aim to make
not configured restore as fast as possible.
This is the first step to control transfers in the context of backup.
We need to explicitly set transfers before backup.
Otherwise, a backup running after restore would inherit
its transfers setting, which might not be desirable.
Future commits will also make this configurable
by a dedicated --transfers flag.
This commit allows user to control the amount of rclone transfers used during backup.
…th transfers

This way this test also checks transfers before and after backup.
It also checks transfers before, in the middle, when paused,
when resumed, and after restore.
@Michal-Leszczynski
Copy link
Collaborator Author

Michal-Leszczynski commented Oct 8, 2024

@karol-kokoszka This PR is ready for review!

I have one question - I added transfers as a singular int to both backup and restore tasks.
The current version of this PR always sets transfers before backup/restore - meaning that the default value set in rclone yaml config is overwritten. This creates a regression, as now user can't specify different transfer values for different nodes (except for the special --transfers=0 available only for restore).

It might be more idiomatic for it to be []string defining transfer setting per dc - so that it has the same syntax like backup's --rate-limit, --snapshot-parallel, --upload-parallel.
On the other hand, I'm not sure how useful would be the per dc (not per node) configuration.

I could extend --transfers=X semantic to:

  • if X>0 - set transfers to X on all nodes
  • if X=0 - set transfers to 2*shard_cnt (default, fastest setting like with --parallel flag)
  • if X=-1 - set transfers to the value from rclone config file (allows for more detailed manual configuration)

What are your thoughts on this?

@karol-kokoszka
Copy link
Collaborator

This creates a regression, as now user can't specify different transfer values for different nodes (except for the special --transfers=0 available only for restore)

I'm not sure if I understand. So now, the transfers from scylla-manager-agent.yaml are not respected ?

The goal is to change the number of transfer only in context of the given task (restore).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Control Transfers parameter from Manager Server
2 participants