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

Don't reset rclone rate limit when not needed #4119

Closed
Michal-Leszczynski opened this issue Nov 21, 2024 · 0 comments · Fixed by #4120
Closed

Don't reset rclone rate limit when not needed #4119

Michal-Leszczynski opened this issue Nov 21, 2024 · 0 comments · Fixed by #4120
Assignees
Labels
bug Something isn't working
Milestone

Comments

@Michal-Leszczynski
Copy link
Collaborator

Rclone server has globalConfigGuard that should update rate limit / transfers used by the agent only when the update is actually changing the value. Otherwise, we perform unnecessary re-initialization, which might be costly.

Here are logs from an SCT test running 2 backups and restore:

2024-10-28T13:30:00.620+00:00  {"L":"INFO","T":"2024-10-28T13:30:00.163Z","N":"rclone","M":"Location check done"}
2024-10-28T13:30:02.870+00:00  {"L":"INFO","T":"2024-10-28T13:30:02.606Z","N":"rclone","M":"Bandwidth limit set to {100M 100M}"}
2024-10-28T13:30:04.370+00:00  {"L":"INFO","T":"2024-10-28T13:30:04.021Z","N":"rclone","M":"Bandwidth limit set to {100M 100M}"}
2024-10-28T13:32:19.370+00:00  {"L":"INFO","T":"2024-10-28T13:32:19.015Z","N":"rclone","M":"Bandwidth limit set to {100M 100M}"}
2024-10-28T13:32:20.620+00:00  {"L":"INFO","T":"2024-10-28T13:32:20.307Z","N":"rclone","M":"Bandwidth limit set to {100M 100M}"}
2024-10-28T13:32:21.620+00:00  {"L":"INFO","T":"2024-10-28T13:32:21.340Z","N":"rclone","M":"Bandwidth limit set to {100M 100M}"}
2024-10-28T13:32:22.620+00:00  {"L":"INFO","T":"2024-10-28T13:32:22.424Z","N":"rclone","M":"Bandwidth limit set to {100M 100M}"}
2024-10-28T13:32:23.756+00:00  {"L":"INFO","T":"2024-10-28T13:32:23.585Z","N":"rclone","M":"Bandwidth limit set to {100M 100M}"}
2024-10-28T13:33:24.370+00:00  {"L":"INFO","T":"2024-10-28T13:33:23.905Z","N":"rclone","M":"Location check done"}
2024-10-28T13:33:25.620+00:00  {"L":"INFO","T":"2024-10-28T13:33:25.166Z","N":"rclone","M":"Bandwidth limit set to {100M 100M}"}
2024-10-28T13:33:26.790+00:00  {"L":"INFO","T":"2024-10-28T13:33:26.642Z","N":"rclone","M":"Bandwidth limit set to {100M 100M}"}
2024-10-28T13:33:26.790+00:00  {"L":"INFO","T":"2024-10-28T13:33:26.644Z","N":"rclone","M":"There was nothing to transfer"}
2024-10-28T13:33:41.870+00:00  {"L":"INFO","T":"2024-10-28T13:33:41.589Z","N":"rclone","M":"Location check done"}
2024-10-28T13:33:57.621+00:00  {"L":"INFO","T":"2024-10-28T13:33:57.183Z","N":"rclone","M":"Bandwidth limit reset to unlimited"}
2024-10-28T13:34:32.370+00:00  {"L":"INFO","T":"2024-10-28T13:34:32.105Z","N":"rclone","M":"Bandwidth limit reset to unlimited"}
2024-10-28T13:34:59.621+00:00  {"L":"INFO","T":"2024-10-28T13:34:59.513Z","N":"rclone","M":"Bandwidth limit reset to unlimited"}
2024-10-28T13:35:23.370+00:00  {"L":"INFO","T":"2024-10-28T13:35:23.040Z","M":"http: TLS handshake error from 10.12.1.124:45446: EOF"}
2024-10-28T13:35:27.871+00:00  {"L":"INFO","T":"2024-10-28T13:35:27.633Z","N":"rclone","M":"Bandwidth limit reset to unlimited"}
2024-10-28T13:35:57.120+00:00  {"L":"INFO","T":"2024-10-28T13:35:56.914Z","N":"rclone","M":"Bandwidth limit reset to unlimited"}
2024-10-28T13:36:28.621+00:00  {"L":"INFO","T":"2024-10-28T13:36:28.179Z","N":"rclone","M":"Bandwidth limit reset to unlimited"}
2024-10-28T13:36:58.120+00:00  {"L":"INFO","T":"2024-10-28T13:36:57.811Z","N":"rclone","M":"Bandwidth limit reset to unlimited"}
2024-10-28T13:37:33.621+00:00  {"L":"INFO","T":"2024-10-28T13:37:33.341Z","N":"rclone","M":"Bandwidth limit reset to unlimited"}
2024-10-28T13:37:58.620+00:00  {"L":"INFO","T":"2024-10-28T13:37:58.332Z","N":"rclone","M":"Bandwidth limit reset to unlimited"}
2024-10-28T13:38:12.871+00:00  {"L":"INFO","T":"2024-10-28T13:38:12.726Z","N":"rclone","M":"Bandwidth limit reset to unlimited"}

We can see that rate limit has been updated multiple times, even though there was no need for it.

In the implementation we can see:

// SetBandwidthLimit sets global bandwidth limit in token bucket.
func SetBandwidthLimit(limit string) error {
	globalConfigGuard.mu.Lock()
	defer globalConfigGuard.mu.Unlock()
	globalConfigGuard.init()

	if limit == globalConfigGuard.bandwidthLimit {
		return nil
	}

	in := rc.Params{
		"rate": limit,
	}
	_, err := rcCalls.Get("core/bwlimit").Fn(context.Background(), in)
	if err != nil {
		return errors.Wrapf(err, "set bandwidth to %s", limit)
	}
	return nil
}

We simply forgot to update the value cached in globalConfigGuard after setting it with core/bwlimit call.

@Michal-Leszczynski Michal-Leszczynski added the bug Something isn't working label Nov 21, 2024
@Michal-Leszczynski Michal-Leszczynski added this to the 3.4.1 milestone Nov 21, 2024
@Michal-Leszczynski Michal-Leszczynski self-assigned this Nov 21, 2024
Michal-Leszczynski added a commit that referenced this issue Dec 10, 2024
…4120)

* fix(rcserver): don't rely on cached bandwidth limit in configguard

Fixes #4119

* refactor(backup): remove unnecessary calls to RcloneSetBandwidthLimit

Setting bandwidth limit should be handled by specific Rclone calls.

(cherry picked from commit d9dde35)
Michal-Leszczynski added a commit that referenced this issue Dec 10, 2024
…4120)

* fix(rcserver): don't rely on cached bandwidth limit in configguard

Fixes #4119

* refactor(backup): remove unnecessary calls to RcloneSetBandwidthLimit

Setting bandwidth limit should be handled by specific Rclone calls.

(cherry picked from commit d9dde35)
Michal-Leszczynski added a commit that referenced this issue Dec 11, 2024
…4120)

* fix(rcserver): don't rely on cached bandwidth limit in configguard

Fixes #4119

* refactor(backup): remove unnecessary calls to RcloneSetBandwidthLimit

Setting bandwidth limit should be handled by specific Rclone calls.

(cherry picked from commit d9dde35)
Michal-Leszczynski added a commit that referenced this issue Dec 11, 2024
…4120)

* fix(rcserver): don't rely on cached bandwidth limit in configguard

Fixes #4119

* refactor(backup): remove unnecessary calls to RcloneSetBandwidthLimit

Setting bandwidth limit should be handled by specific Rclone calls.

(cherry picked from commit d9dde35)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant