all: Add folder pause, make pauses optionally permanent (fixes #3407, fixes #215, fixes #3001) by AudriusButkevicius · Pull Request #3520 · syncthing/syncthing

AudriusButkevicius · 2016-08-16T21:51:42Z

Purpose

Addresses most wanted issue numer 3.

Testing

Not much, pushed some buttons around in the UI, the UI looked the way it was supposed to, saw log messages that I expected to see.

Documentation

Removal of DevicePaused/Unpaused events
Removal of /pause, /resume REST endpoints
Removal of 'paused' in connection info
Addition of unpaused command line option
Addition of UnpauseOnStart option
Addition of Paused attribute on Folder/Device segments

AudriusButkevicius · 2016-08-16T21:56:09Z

+122 −116 - 2 features in 6 lines. If we'd exclude the test changes, that'd be less.

canton7 · 2016-08-16T21:57:47Z

This will screw up many, many things on my side.

In particular, I use device pausing and unpausing to disable devices which are connected to over a metered connection. I use the endpoints and events which you've just removed...

Please don't merge until 0.15!

AudriusButkevicius · 2016-08-16T22:00:37Z

If anything, I think life is easier now, because you just listen to ConfigSaved, and set/read Paused on each device/folder.

Agree, it's not a zero effort change, but I think it's for the better.

canton7 · 2016-08-16T22:09:14Z

Not quite. In order to set the config, I need to model it exactly using my data structures, so that I can parse it in its entirety, then set it back with no loss of information. Since the config format changes often, and I need to be backwards compatible, this would mean a lot of different models, and a lot of very similar-looking code around each, to support. I will also have to start keeping much closer tabs on when the config format changes. In reality, I will probably have stop Syncthing from automatically upgrading, and instead manually vet each new version before allowing users to upgrade. That mechanism will take even more time to get in place...

I've avoided setting the config so far for this reason.

Before you release this, I need the chance add support for setting config, in parallel with the current approach, and add a switch which selects between the two approaches based on the Syncthing version (which means I need to know ahead of time which version will contain these changes). At the very least, I need time to remove my features which depend on device pausing, and push this to the majority of users, which will take several weeks.

If I don't do this, 5000 Windows users will insta-crash, which won't be pretty.

AudriusButkevicius · 2016-08-16T23:16:24Z

We could maintain backwards compatibility, and deprecate the stuff I removed later to make life easier, but not sure how much of a benefit that is, as it just defers work which will have to be done anyway.

calmh · 2016-08-17T04:53:23Z

Could we not do this and retain the REST methods, having them internally just doing the config updates? The difference would be that the actions are persisted, but otherwise the behavior would be the same

canton7 · 2016-08-17T06:58:09Z

Maintaining backwards compatibility, at least for a while, would be a huge help for me. Yes it's just deferring work which needs to be done anyway, but that gives me time to get a fix in place, while allowing you to get your new feature out earlier.

How hard would it be to keep the events for a while? I won't crash if they're removed, bur I'll do strange things if the user manually pauses and unpauses devices.

Nutomic · 2016-08-17T09:52:17Z

I'm not familiar with the Syncthing source code, so I'm just going by the referenced issues, and it sounds really good! Some questions:

To pause Syncthing globally, I will have to iterate over all folders in the config and pause them, right?
Can I add a folder in paused state? So that people can add folders while Syncthing is paused, without transferring data.

AudriusButkevicius · 2016-08-17T09:55:14Z

Answer to both questions is yes. You can also pause syncthing by pausing all devices, not only folders. Or start with -paused command line.

Folder being paused will still makes syncthing connect and exchange pings etc etc, but it will be minimal cost.

Nutomic · 2016-08-17T13:07:55Z

Sounds perfect!

AudriusButkevicius · 2016-08-17T19:14:46Z

Added backwards compatibility. I guess it will be easy just to revert that once we decide to drop 0.15.
Yet pauses are still permanent unlike before, mentioning since something might need to be done from @canton7's side

calmh · 2016-08-18T06:39:56Z

cmd/syncthing/main.go


 	"github.com/thejerf/suture"
+
+	_ "net/http/pprof"


the STPROFILER=":9090" doesn't work if this is not imported.

Right, I had mentally deprecated that, woops

calmh · 2016-08-18T06:45:08Z

I only glanced at the diff briefly now on my phone, so take this with a grain of salt. My previous attempts at folder pausing were shot down for the following reasons:

Pausing a folder required a reconnect, which caused an index resend and consumed data. Not an issue any more.
A paused folder should not receive index updates, so it doesn't consume data for that reason. We can handle this easily by setting a paused flag in the cluster config and the other side won't start an index sender. Not sure if the diff does this?
A paused folder should not receive requests. We can handle this in the same way, by remembering that flag and filtering out the device in the Available() check. Not sure if implemented?

AudriusButkevicius · 2016-08-18T06:58:19Z

Paused folders simply stop existing. So nothing is received, nothing is sent (absent from CC), and nothing is scanned.

calmh · 2016-08-18T12:28:57Z

That doesn't stop another device from requesting files from me for that folder though, if they see that I'm connected and they have an index for it since before?

I also don't think we should remove the folder from the clusterconfig we send. For one thing that will interact badly with your introducer change in the other PR - pause a folder on an introducer and it'll be unshared everywhere... Better to have a Paused bool in the protocol that the other side knows to mean "do not send indexes; do not request files"?

AudriusButkevicius · 2016-08-18T12:44:52Z

You can't request blocks from a paused folder, because it's not running, hence folderSharedWith returns false.

Regarding the introducer issue, good point. Let's decide which one are we merging first and I'll patch up the other

calmh · 2016-08-18T12:49:25Z

I don't understand what you mean about folderSharedWith. That check is just based on the config. If the other device pauses the folder, I'm still sharing it with them so I have reason to request blocks from them if they are connected and the index says they have the blocks I need?

AudriusButkevicius · 2016-08-18T12:52:25Z

folderSharedWith checks m.deviceFolders, which gets populated as folders are started. Paused folders are not started.

calmh · 2016-08-18T12:55:09Z

But I'm not talking about the paused side, I'm talking about the other side that might want to request blocks from the paused side.

AudriusButkevicius · 2016-08-18T12:55:21Z

Yes, requests will go out, but will not be answered, i guess that should be addressed.

AudriusButkevicius · 2016-08-20T10:02:27Z

Yet adding a paused boolean to protocol.Folder breaks backwards compatibility to some extent, as old clients will ignore the fact that it's paused and keep sending requests. Is that fine?

AudriusButkevicius · 2016-08-20T10:03:54Z

perhaps we should add a separate ClusterConfig.PausedFolders and use that in the de-introducer branch to work around the folders being missing in the CM?

calmh · 2016-08-20T12:14:53Z

old clients will ignore the fact that it's paused and keep sending requests. Is that fine?

I think that's fine

AudriusButkevicius · 2016-08-20T15:54:20Z

Actually it's fairly trivial, so I've "just added another map"™ to the soup.
Question, if I have A and B, and B connects paused, yet B has previously advertised to A that it has some files that A doesn't? Will A go into berserk mode trying to download from nowhere?

calmh · 2016-08-21T08:25:07Z

As long as the Available() call doesn't return the device I think it should be fine, it's similar to B not being connected at all

calmh · 2016-08-21T08:32:30Z

lib/config/optionsconfiguration.go

 	OverwriteRemoteDevNames bool     `xml:"overwriteRemoteDeviceNamesOnConnect" json:"overwriteRemoteDeviceNamesOnConnect" default:"false"`
 	TempIndexMinBlocks      int      `xml:"tempIndexMinBlocks" json:"tempIndexMinBlocks" default:"10"`
 	UnackedNotificationIDs  []string `xml:"unackedNotificationID" json:"unackedNotificationIDs"`
+	UnpauseOnStart          bool     `xml:"unpauseOnStart" json:"unpauseOnStart"`


What's the intention for this config? It's essentially a bool you can set to have other bools changed in the config on startup? It's somewhat confusing to me what the results may be of pausing some device, setting this to to true, starting up with -paused etc at the same time so it seems we have too many ways to do the same thing.

The -paused startup flag make senses to me in that I can see a use case for starting Syncthing but having it be passive until you've had a change to do something with the config or whatever.

Not equally sure about the -unpaused?

If this is set and you don't start with -(un)paused, it will unpause everything, just as it does in the current release. This just gives feature parity

canton7 · 2016-12-15T16:28:44Z

so you'll soon have won by wearing me down to a point where I don't really care what API looks like any more

No! Think of the other API consumers! We're relying on you!

…ing#3407, fixes syncthing#215, ref syncthing#3001)

… folders

AudriusButkevicius · 2016-12-18T21:47:54Z

Rebased on master, undeprecated existing stuff, added events for folder pauses, yet no HTTP API for pausing unpausing, so you need to throw the whole config around for that. Reason for that is that the mux we use now does not support extracting arguments from the path which would be the right way to do it /folder/{x}/pause etc. We'll leave that for the future.

calmh · 2016-12-21T12:27:59Z

lib/config/deviceconfiguration.go

 	Introducer               bool                 `xml:"introducer,attr" json:"introducer"`
 	SkipIntroductionRemovals bool                 `xml:"skipIntroductionRemovals,attr" json:"skipIntroductionRemovals"`
 	IntroducedBy             protocol.DeviceID    `xml:"introducedBy,attr" json:"introducedBy"`
+	Paused                   bool                 `xml:"paused" json:"pause"`


paused/pause?

calmh · 2016-12-21T12:31:14Z

Otherwise LGTM, let me take it for a spin as well.

calmh · 2016-12-21T12:35:57Z

lib/model/model.go

+	if !cfg.Paused {
+		m.addFolderLocked(cfg)
+		folderType := m.startFolderLocked(cfg.ID)
+		l.Infoln("Restarted folder", cfg.ID, fmt.Sprintf("(%s)", folderType))


These should use the cfg.Description()

calmh · 2016-12-21T12:37:20Z

lib/model/model.go

+		}
+
+		if toCfg.Paused {
+			l.Infoln("Pausing", deviceID)


Note for the future, we should have something like a deviceCfg.Description() as well with the device ID (perhaps in short form) plus device name. Not in this PR though.

calmh · 2016-12-21T12:37:46Z

Appears to work as designed while clicking around 👍

AudriusButkevicius · 2016-12-21T15:55:07Z

Addressed, merge if happy.

calmh · 2016-12-21T18:41:20Z

@st-review merge the shit out of this

all: Add folder pause, make pauses permanent (fixes #3407, fixes #215, fixes #3001)

st-review · 2016-12-21T18:41:29Z

👌 Merged as bab7c8e. Thanks, @AudriusButkevicius!

…fixes #3001) GitHub-Pull-Request: #3520

calmh · 2016-12-21T18:42:11Z

Good stuff

tobiastom · 2016-12-21T23:47:25Z

Thank you everyone for your work on this!

AudriusButkevicius force-pushed the pfolder branch from 4842840 to 4df5c77 Compare August 16, 2016 21:53

AudriusButkevicius changed the title ~~all: Add folder pause, make pauses optionally permanent (fixes #3407, fixes #215, ref #3001)~~ all: Add folder pause, make pauses optionally permanent (fixes #3407, fixes #215, fixes #3001) Aug 17, 2016

AudriusButkevicius force-pushed the pfolder branch from 568ad03 to dce533d Compare August 17, 2016 19:17

calmh reviewed Aug 18, 2016
View reviewed changes

AudriusButkevicius force-pushed the pfolder branch from c054f04 to f7d8062 Compare August 20, 2016 15:55

calmh reviewed Aug 21, 2016
View reviewed changes

AudriusButkevicius added 8 commits December 18, 2016 20:01

all: Add folder pause, make pauses optionally permanent (fixes syncth…

7d6ef90

…ing#3407, fixes syncthing#215, ref syncthing#3001)

Maintain backwards compatability

bc4c597

Advertise that the folder is paused, do not request files from paused…

85736d7

… folders

Remove UnpauseOnStart

4576a60

starting the folder closes the connections

111dddb

Hide stuff that is not relevant

944f75c

Undeprecate stuff

bd75dd4

Add FolderPaused/Resumed events

527b153

AudriusButkevicius force-pushed the pfolder branch from aefce77 to 527b153 Compare December 18, 2016 21:41

calmh reviewed Dec 21, 2016

View reviewed changes

AudriusButkevicius added 2 commits December 21, 2016 15:43

Merge master

5704094

Address comments

9074adc

st-review closed this Dec 21, 2016

st-review pushed a commit that referenced this pull request Dec 21, 2016

all: Add folder pause, make pauses permanent (fixes #3407, fixes #215, …

bab7c8e

…fixes #3001) GitHub-Pull-Request: #3520

scienmind mentioned this pull request Dec 23, 2016

Random dead-locks #3837

Closed

scienmind mentioned this pull request Feb 6, 2017

GUI: Add "Pause All Folders" and "Resume All Folders" buttons #3964

Closed

calmh mentioned this pull request Feb 7, 2017

Enable/disable a repo? #183

Closed

st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Dec 22, 2017

syncthing locked and limited conversation to collaborators Dec 22, 2017

Uh oh!

Conversation

AudriusButkevicius commented Aug 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Testing

Documentation

Uh oh!

AudriusButkevicius commented Aug 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

canton7 commented Aug 16, 2016

Uh oh!

AudriusButkevicius commented Aug 16, 2016

Uh oh!

canton7 commented Aug 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AudriusButkevicius commented Aug 16, 2016

Uh oh!

calmh commented Aug 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

canton7 commented Aug 17, 2016

Uh oh!

Nutomic commented Aug 17, 2016

Uh oh!

AudriusButkevicius commented Aug 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nutomic commented Aug 17, 2016

Uh oh!

AudriusButkevicius commented Aug 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calmh Aug 18, 2016

Choose a reason for hiding this comment

Uh oh!

AudriusButkevicius Aug 20, 2016

Choose a reason for hiding this comment

Uh oh!

calmh Aug 21, 2016

Choose a reason for hiding this comment

Uh oh!

calmh commented Aug 18, 2016

Uh oh!

AudriusButkevicius commented Aug 18, 2016

Uh oh!

calmh commented Aug 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AudriusButkevicius commented Aug 18, 2016

Uh oh!

calmh commented Aug 18, 2016

Uh oh!

AudriusButkevicius commented Aug 18, 2016

Uh oh!

calmh commented Aug 18, 2016

Uh oh!

AudriusButkevicius commented Aug 18, 2016

Uh oh!

AudriusButkevicius commented Aug 20, 2016

Uh oh!

AudriusButkevicius commented Aug 20, 2016

Uh oh!

calmh commented Aug 20, 2016

Uh oh!

AudriusButkevicius commented Aug 20, 2016

Uh oh!

calmh commented Aug 21, 2016

Uh oh!

calmh Aug 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AudriusButkevicius Aug 21, 2016

Choose a reason for hiding this comment

Uh oh!

canton7 commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AudriusButkevicius commented Aug 16, 2016 •

edited

Loading

AudriusButkevicius commented Aug 16, 2016 •

edited

Loading

canton7 commented Aug 16, 2016 •

edited

Loading

calmh commented Aug 17, 2016 •

edited

Loading

AudriusButkevicius commented Aug 17, 2016 •

edited

Loading

AudriusButkevicius commented Aug 17, 2016 •

edited

Loading

calmh commented Aug 18, 2016 •

edited

Loading

calmh Aug 21, 2016 •

edited

Loading

canton7 commented Dec 15, 2016 •

edited

Loading

AudriusButkevicius commented Dec 18, 2016 •

edited

Loading