lib/model, lib/config: Refactor folder health/error handling (fixes #4445 #4451) #4455
Conversation
…yncthing#4445 syncthing#4451) Implement folder health checks in folder and config instead of model. Changes in behaviour: - Start/stop the fs watcher when leaving/entering error state. - Only log a change in error state once. - Don't say anything about starting/stopping folders, because we don't do that.
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
All of this should live in func (f *FolderConfiguration) CheckFreeSpace() (err error) {, and keep using a type, as a type.
There was a problem hiding this comment.
This code is also needed to get free space of home.
There was a problem hiding this comment.
Fine, so have a utility function or something, don't just wedge something on a type.
There was a problem hiding this comment.
Wedging was more or less a random decision, I just thought this makes sense here as it is used in the config package (before it was wedged on Model, but it has no connection to that at all). Would it make more sense as a utility function in the fs package?
There was a problem hiding this comment.
config fs import cycle...
So either we leave it in config or I move the entire Size thing into the fs package.
There was a problem hiding this comment.
Make a utility fn in config.
lib/config/wrapper.go
Outdated
| func (w *Wrapper) CheckHomeFreeSpace() error { | ||
| w.mut.Lock() | ||
| minFree := w.cfg.Options.MinHomeDiskFree | ||
| w.mut.Unlock() |
There was a problem hiding this comment.
Could just call w.Options()
| if err := f.model.CheckFolderHealth(f.folderID); err != nil { | ||
| l.Infoln("Skipping pull of", f.Description(), "due to folder error:", err) | ||
| if err := f.CheckHealth(); err != nil { | ||
| l.Debugln("Skipping pull of", f.Description(), "due to folder error:", err) |
There was a problem hiding this comment.
We already log this in info or higher, so I'd just remove this
| if err := m.CheckFolderHealth(folder); err != nil { | ||
| l.Infof("Stopping folder %s mid-scan due to folder error: %s", folderCfg.Description(), err) | ||
| if err := runner.CheckHealth(); err != nil { | ||
| l.Debugln("Stopping scan of folder %s due to: %s", folderCfg.Description(), err) |
There was a problem hiding this comment.
We already log this in info or higher, so I'd just remove this
| @@ -2020,13 +2015,13 @@ func (m *Model) internalScanFolderSubdirs(ctx context.Context, folder string, su | |||
| }) | |||
|
|
|||
| if iterError != nil { | |||
| l.Infof("Stopping folder %s mid-scan due to folder error: %s", folderCfg.Description(), iterError) | |||
| l.Debugln("Stopping scan of folder %s due to: %s", folderCfg.Description(), err) | |||
There was a problem hiding this comment.
We already log this in info or higher, so I'd just remove this
| @@ -1914,7 +1909,7 @@ func (m *Model) internalScanFolderSubdirs(ctx context.Context, folder string, su | |||
| // The error we get here is likely an OS level error, which might not be | |||
| // as readable as our health check errors. Check if we can get a health | |||
| // check error first, and use that if it's available. | |||
| if ferr := m.CheckFolderHealth(folder); ferr != nil { | |||
| if ferr := runner.CheckHealth(); ferr != nil { | |||
| err = ferr | |||
| } | |||
| runner.setError(err) | |||
There was a problem hiding this comment.
Isn't this pointless?
There was a problem hiding this comment.
Not sure, the reason given in the comments seems sound enough to me?
There was a problem hiding this comment.
A right, if it's err not ferr, it wouldn't be set otherwise.
| if err := m.CheckFolderHealth(folder); err != nil { | ||
| l.Infof("Stopping folder %s mid-scan due to folder error: %s", folderCfg.Description(), err) | ||
| if err := runner.CheckHealth(); err != nil { | ||
| l.Debugln("Stopping scan of folder %s due to: %s", folderCfg.Description(), err) |
There was a problem hiding this comment.
We already log this in info or higher, so I'd just remove this
There was a problem hiding this comment.
Agreed, the only added information is, that it happened during scan, which should also be clear from context.
There was a problem hiding this comment.
you can do CheckHealth("during scan") or something to explain where the health check is happening, and then log that.
There was a problem hiding this comment.
Actually, we do not necessarily log anything on info or higher unless the error is new or changed. So in a debug scenario, I think it is valid that we log the error anyway and from the position of origin.
| if err := m.CheckFolderHealth(folder); err != nil { | ||
| l.Infof("Stopping folder %s mid-scan due to folder error: %s", folderCfg.Description(), err) | ||
| if err := runner.CheckHealth(); err != nil { | ||
| l.Debugln("Stopping scan of folder %s due to: %s", folderCfg.Description(), err) |
There was a problem hiding this comment.
We already log this in info or higher, so I'd just remove this
|
@st-review lgtm |
|
@AudriusButkevicius: Noted! Need another LGTM or explicit merge command. |
|
@st-review lgtm lib/model, lib/config: Refactor folder health/error handling (fixes #4445, fixes #4451) |
st-review
added
the
pr-merged
st-review
added
the
frozen-due-to-age
While fixing the two linked bugs the result grew uglier and uglier, so I decided to refactor. This splits code regarding folder error/health handling from model.go into folder.go and the config package. The only change in behaviour regards logging and start/stopping fs watch on error state change.