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

Add a max session time limit #3371

Open
wants to merge 2 commits into
base: master
from

Conversation

@boosh
Copy link

commented Jul 23, 2019

What is the purpose of this change?

It allows users to specify the maximum duration of a transfer session sort of as requested in #985, making it easier to script rclone to e.g. backup during the night. The setting I've added introduces a duration flag. After rclone has been running for that amount of time, no new transfers will be initiated, but those already started will be able allowed to complete. It may be useful to implement a further flag for a hard limit that will terminate in-progress transfers once the session limit is reached, but in another PR.

Note - Since --max-transfer doesn't appear to have tests, neither does this feature. I have tested the non-server copy manually though - I've never used server mode so would need some pointers to test that.

Was the change discussed in an issue or in the forum before?

Sort of in #985

Checklist

  • I have read the contribution guidelines.
  • I have added tests for all changes in this PR if appropriate.
  • I have added documentation for the changes if appropriate.
  • All commit messages are in house style.
  • I'm done, this Pull Request is ready for review :-)

@boosh boosh force-pushed the boosh:time-limits branch 3 times, most recently from 4019a0b to a138c25 Jul 23, 2019

@yparitcher
Copy link
Contributor

left a comment

@boosh

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

OK thanks. Let me know if you want me to squash the commits.

@boosh

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

Ah it doesn't work if I move it to those places. I'll revert the change.

@boosh boosh force-pushed the boosh:time-limits branch from ef6a8d7 to a138c25 Jul 24, 2019

@boosh

This comment has been minimized.

Copy link
Author

commented Jul 24, 2019

sync/run is too high-level. I need to break during the transfer loop. Perhaps moveOrCopy didn't work because I tried syncing a directory? I expect that'd be the primary usecase since right now in-progress transfers aren't terminated once the session limit is reached so I doubt it'd make sense to call it with a list of files (which even if it was would still be caught).

Is there anywhere else it needs adding to given it's mainly to help script backups (so several operations wouldn't be relevant)? Perhaps it should be added to operations/Move though...

@ncw

This comment has been minimized.

Copy link
Collaborator

commented Jul 24, 2019

Note that MaxTransfer does have tests here: fs/accounting/accounting_test.go :-)

I think that breaking the transfer in Copy is probably the wrong place.

What you want to do is cancel the context passed to the march routine.

I think using a deadline should work

newCtx, cancel := context.WithDeadline(ctx, 100*time.Seconds) // config var goes here
// pass newCtx to march
// do the march

This will cause march routine to exit and cause the sync to come to a stop gracefully.

@boosh boosh force-pushed the boosh:time-limits branch from a138c25 to 34503f1 Jul 25, 2019

@@ -16,6 +12,9 @@ import (
"github.com/ncw/rclone/fs/march"
"github.com/ncw/rclone/fs/operations"
"github.com/pkg/errors"
"path"

This comment has been minimized.

Copy link
@golangcibot

golangcibot Jul 25, 2019

File is not goimports-ed (from goimports)

@boosh boosh force-pushed the boosh:time-limits branch 3 times, most recently from b118ac1 to 52e5878 Jul 25, 2019

@boosh

This comment has been minimized.

Copy link
Author

commented Jul 25, 2019

OK I've changed to add a deadline to the context. I've added a test but it'll be quite brittle. Is there any way of throttling transfers during testing to e.g. simulate large files, slow connections etc? At the moment I've just created 10000 test files. The test works on my mac today but obviously as computers get faster it'll break one day... It'd be better to only create a handful of files with a function that just makes each transfer sleep for a while to make them safely reproducible.

@boosh boosh force-pushed the boosh:time-limits branch from 52e5878 to 43cd6ab Jul 25, 2019

@boosh

This comment has been minimized.

Copy link
Author

commented Jul 25, 2019

Hmm the appveyor test failed - perhaps because it's on Windows? On my Mac no context.DeadlineExceeded is ever thrown so I can't require an error in the tests. I assume it's something to do with the test filesystem abstraction. For now I won't add an assertion to the error returned from Sync unless there's a cross-platform solution. The test makes sure that not all files were transferred so that should be good enough.

Show resolved Hide resolved fs/sync/sync_test.go Outdated
fs: Add a flag to allow specifying the maximum duration of a transfer…
… session

This PR gives you more control over how long rclone will run for, making it easier to script backups, e.g. via cron. Once the `--max-session` time limit is reached, no new transfers will be initiated, but those already in-flight will be allowed to complete.

@boosh boosh force-pushed the boosh:time-limits branch from 43cd6ab to f5b8de6 Jul 25, 2019

@ncw
Copy link
Collaborator

left a comment

See comments inline

Show resolved Hide resolved fs/sync/sync.go Outdated
Show resolved Hide resolved fs/sync/sync_test.go

fs.Config.MaxSession = maxSession

testFiles := make([]fstest.Item, 10000)

This comment has been minimized.

Copy link
@ncw

ncw Jul 28, 2019

Collaborator

Making 10000 files will kill the integration tests with other backends for certain! (These tests are run against all the backends once per day).

How about just using a few filesa nd temporarily setting a bwlimit to 1b/s or something like that?

This comment has been minimized.

Copy link
@boosh

boosh Jul 29, 2019

Author

They're still flaky on my Mac - I thought I'd got down to only needing 5 test files but sometimes it can get through a few hundred :-(

@ncw

This comment has been minimized.

Copy link
Collaborator

commented Jul 28, 2019

Hmm the appveyor test failed - perhaps because it's on Windows?

I think that is because you didn't reset the deadline in your tests see the deadline exceeded messages.

On my Mac no context.DeadlineExceeded is ever thrown so I can't require an error in the tests.

You should find the error in fs.Stats.GetLastError()

This should work on all platforms

@boosh

This comment has been minimized.

Copy link
Author

commented Jul 29, 2019

Am I missing some kind of initialisation? I think I'm reaching the limit of the time I can spend on this...

@ncw

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

I merged a lot of things which changes the stats interface which has caused the breakage.

If you want me to finish it off then I can - it is very nearly there.

If you want to have a go yourself, then rebase it onto master and change the uses of fs.Config.Stats to accounting.GlobalStats()

@boosh

This comment has been minimized.

Copy link
Author

commented Jul 30, 2019

@ncw It'd be great if you could finish it off please. I'm running into problems rebasing. Thanks.

@ncw

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

@ncw It'd be great if you could finish it off please. I'm running into problems rebasing. Thanks.

No problems, will do :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.