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.

Sign up for GitHub

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

Jump to bottom

gui, lib/model: Disable periodic cleanup when versions are kept forever (fixes #8350) #8994

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

gui, lib/model: Disable periodic cleanup when versions are kept forever (fixes #8350) #8994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions gui/default/index.html
View file
Open in desktop
Expand Up @@ -542,8 +542,8 @@ <h4 class="panel-title">
<span ng-if="folder.versioning.type == 'staggered' && folder.versioning.params.maxAge / 86400 != versioningDefaults.staggeredMaxAge" tooltip data-original-title="{{'Maximum Age' | translate}}">
&ensp;<span class="fa fa-calendar"></span>&nbsp;<span ng-if="folder.versioning.params.maxAge == 0" translate>Forever</span><span ng-if="folder.versioning.params.maxAge > 0">{{folder.versioning.params.maxAge | duration}}</span>
</span>
<span ng-if="folder.versioning.cleanupIntervalS != versioningDefaults.cleanupIntervalS" tooltip data-original-title="{{'Cleanup Interval' | translate}}">
&ensp;<span class="fa fa-recycle"></span>&nbsp;<span ng-if="folder.versioning.cleanupIntervalS == 0" translate>Disabled</span><span ng-if="folder.versioning.cleanupIntervalS > 0">{{folder.versioning.cleanupIntervalS | duration}}</span>
<span ng-if="folder.versioning.cleanupIntervalS != versioningDefaults.cleanupIntervalS || (folder.versioning.cleanupIntervalS == versioningDefaults.cleanupIntervalS && (((folder.versioning.type == 'trashcan' || folder.versioning.type == 'simple') && folder.versioning.params.cleanoutDays == 0) || (folder.versioning.type == 'staggered' && folder.versioning.params.maxAge == 0)))" tooltip data-original-title="{{'Cleanup Interval' | translate}}">
&ensp;<span class="fa fa-recycle"></span>&nbsp;<span ng-if="folder.versioning.cleanupIntervalS == 0 || (((folder.versioning.type == 'trashcan' || folder.versioning.type == 'simple') && folder.versioning.params.cleanoutDays == 0) || (folder.versioning.type == 'staggered' && folder.versioning.params.maxAge == 0))" translate>Disabled</span><span ng-if="folder.versioning.cleanupIntervalS > 0 && (((folder.versioning.type == 'trashcan' || folder.versioning.type == 'simple') && folder.versioning.params.cleanoutDays != 0) || (folder.versioning.type == 'staggered' && folder.versioning.params.maxAge != 0))">{{folder.versioning.cleanupIntervalS | duration}}</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is way too much controller logic coded into the GUI presentation for my taste. Please if there is no easier way to determine all these conditions, then at least move it to the controller JS code, encapsulated as a function. Stuff like this tends to get forgotten when the underlying logic changes (such as when a new versioning scheme gets introduced). So more visibility and ideally close to the versioning code itself is better for maintenance.

</span>
<!-- Keep the path last, so that it truncates without pushing other information out of the screen. -->
<span ng-if="folder.versioning.fsPath != ''" tooltip data-original-title="{{folder.versioning.fsPath}}">
Expand Down
6 changes: 5 additions & 1 deletion gui/default/syncthing/folder/editFolderModalView.html
View file
Open in desktop
Expand Up @@ -146,13 +146,17 @@
<div class="form-group" ng-if="internalVersioningEnabled()" ng-class="{'has-error': folderEditor.cleanupIntervalS.$invalid && folderEditor.cleanupIntervalS.$dirty}">
<label translate for="cleanupIntervalS">Cleanup Interval</label>
<div class="input-group">
<input name="cleanupIntervalS" id="cleanupIntervalS" class="form-control text-right" type="number" ng-model="currentFolder._guiVersioning.cleanupIntervalS" required="" min="0" max="31536000" aria-required="true" />
<input name="cleanupIntervalS" id="cleanupIntervalS" class="form-control text-right" type="number" ng-model="currentFolder._guiVersioning.cleanupIntervalS" required="" min="0" max="31536000" aria-required="true" ng-disabled="(((currentFolder._guiVersioning.selector == 'trashcan' || currentFolder._guiVersioning.selector == 'simple') && currentFolder._guiVersioning.trashcanClean == 0) || (currentFolder._guiVersioning.selector == 'staggered' && currentFolder._guiVersioning.staggeredMaxAge == 0))" />
<div class="input-group-addon" translate>seconds</div>
</div>
<p class="help-block">
<span translate ng-if="folderEditor.cleanupIntervalS.$valid || folderEditor.cleanupIntervalS.$pristine"class="help-block">The interval, in seconds, for running cleanup in the versions directory. Zero to disable periodic cleaning.</span>
<span translate ng-if="folderEditor.cleanupIntervalS.$error.required && folderEditor.cleanupIntervalS.$dirty">The cleanup interval cannot be blank.</span>
<span translate ng-if="folderEditor.cleanupIntervalS.$error.min && folderEditor.cleanupIntervalS.$dirty">The interval must be a positive number of seconds.</span>
<span class="text-warning" ng-if="(((currentFolder._guiVersioning.selector == 'trashcan' || currentFolder._guiVersioning.selector == 'simple') && currentFolder._guiVersioning.trashcanClean == 0) || (currentFolder._guiVersioning.selector == 'staggered' && currentFolder._guiVersioning.staggeredMaxAge == 0))">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this logic is repeated in several places. One more reason to factor it out into a controller method.

<span class="fas fa-exclamation-triangle"></span>
<span translate>Cleanup is disabled when versions are kept forever.</span>
</span>
</p>
</div>
</div>
Expand Down
12 changes: 11 additions & 1 deletion lib/model/folder.go
View file
Open in desktop
Expand Up @@ -13,6 +13,7 @@ import (
"math/rand"
"path/filepath"
"sort"
"strconv"
"time"

"github.com/syncthing/syncthing/lib/config"
Expand Down Expand Up @@ -57,8 +58,11 @@ type folder struct {
scanDelay chan time.Duration
initialScanFinished chan struct{}
scanScheduled chan struct{}
versionType string
versionCleanupInterval time.Duration
versionCleanupTimer *time.Timer
versionCleanoutDays int
versionMaxAge int

pullScheduled chan struct{}
pullPause time.Duration
Expand Down Expand Up @@ -96,6 +100,9 @@ type puller interface {
}

func newFolder(model *model, fset *db.FileSet, ignores *ignore.Matcher, cfg config.FolderConfiguration, evLogger events.Logger, ioLimiter *util.Semaphore, ver versioner.Versioner) folder {
cleanoutDays, _ := strconv.Atoi(cfg.Versioning.Params["cleanoutDays"])
maxAge, _ := strconv.Atoi(cfg.Versioning.Params["maxAge"])

f := folder{
stateTracker: newStateTracker(cfg.ID, evLogger),
FolderConfiguration: cfg,
Expand All @@ -115,8 +122,11 @@ func newFolder(model *model, fset *db.FileSet, ignores *ignore.Matcher, cfg conf
scanDelay: make(chan time.Duration),
initialScanFinished: make(chan struct{}),
scanScheduled: make(chan struct{}, 1),
versionType: cfg.Versioning.Type,
versionCleanupInterval: time.Duration(cfg.Versioning.CleanupIntervalS) * time.Second,
versionCleanupTimer: time.NewTimer(time.Duration(cfg.Versioning.CleanupIntervalS) * time.Second),
versionCleanoutDays: cleanoutDays,
versionMaxAge: maxAge,

pullScheduled: make(chan struct{}, 1), // This needs to be 1-buffered so that we queue a pull if we're busy when it comes.

Expand Down Expand Up @@ -161,7 +171,7 @@ func (f *folder) Serve(ctx context.Context) error {

// If we're configured to not do version cleanup, or we don't have a
// versioner, cancel and drain that timer now.
if f.versionCleanupInterval == 0 || f.versioner == nil {
if f.versionCleanupInterval == 0 || ((f.versionType == "trashcan" || f.versionType == "simple") && f.versionCleanoutDays == 0) || (f.versionType == "staggered" && f.versionMaxAge == 0) || f.versionType == "external" || f.versioner == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This stuff could in fact happen in the constructor instead, then we don't need to have instance variables for all this stuff and it would keep things more contained, I think... We could also break up and comment the very long if clause a little bit.

if !f.versionCleanupTimer.Stop() {
<-f.versionCleanupTimer.C
}
Expand Down