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

SyncEngine: disable heuristics for backup restoration for server >= 9.1 #5263

Merged
merged 1 commit into from
Nov 4, 2016

Conversation

ogoffart
Copy link
Contributor

The ownCloud 9.1 server has a data-fingerprint property that the admin must
change in case of backup restoration. When this change, the client understands
that a backup was restored, and will generate conflict files and re-upload
new files.

The heuristics based system checks that there is at least two files wose mtime
is put back in the past and no files that goes forward. In that case we ask the
user before creating the conflicts.

This commit disable the heuristics for newer server that have the data-fingerpint.
And change the heuristics to two hours because we want to avoid false positive due
to some clock error, and that 2 hours of lost due to backup restoration is probably
not so bad.

We only ask the user in the heuristics based aproach so in practice this mean that
the "backup-detected" dialog will no longer appear with newer server.

Relates issues #5260, #5109

@mention-bot
Copy link

@ogoffart, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dragotin, @jturcotte and @ckamm to be potential reviewers.

@guruz guruz added this to the 2.3.0 milestone Oct 23, 2016
@guruz
Copy link
Contributor

guruz commented Oct 23, 2016

Looks OK except for the comment i had on IRC

// We are going back on time
auto difftime = std::difftime(file->modtime, file->other.modtime);
if (difftime < -3600) {
// We are going back on time (more than two hours to avoid clock skew issues or DST)
Copy link
Contributor

@ckamm ckamm Oct 24, 2016

Choose a reason for hiding this comment

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

It's only more than one hour.

_backInTimeFiles++;
qDebug() << file->path << "has a timestamp earlier than the local file";
} else {
} else if (difftime > 0) {
_hasForwardInTimeFiles = true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it correct to do nothing for the -3600, 0 case? Could you add a comment about that?

@ckamm
Copy link
Contributor

ckamm commented Oct 24, 2016

@ogoffart The idea is that this situation shouldn't happen anymore with new servers because admins should use the new data-fingerprint based backups? I'm a bit worried about disabling our accidental-deletion heuristic like that... Would there be an easy way to disable it only if a data-fingerprint backup restoration is used?

@ogoffart
Copy link
Contributor Author

Would there be an easy way to disable it only if a data-fingerprint backup restoration is used?

No, because by default, data-fingerprint is not set.

The problem i'm trying to fix is users complaining about this message that it appears too often when it shouldn't.
We want to rely on the admin setting the data-fingerprint. This heuristics is already flawed as there might be new files already after the backup was recovered.

The ownCloud 9.1 server has a data-fingerprint property that the admin must
change in case of backup restoration. When this change, the client understands
that a backup was restored, and will generate conflict files and re-upload
new files.

The heuristics based system checks that there is at least two files wose mtime
is put back in the past and no files that goes forward. In that case we ask the
user before creating the conflicts.

This commit disable the heuristics for newer server that have the data-fingerpint.
And change the heuristics to two hours because we want to avoid false positive due
to some clock error, and that 2 hours of lost due to backup restoration is probably
not so bad.

We only ask the user in the heuristics based aproach so in practice this mean that
the "backup-detected" dialog will no longer appear with newer server.

Relates issues #5260, #5109
@ogoffart ogoffart merged commit e3a4c39 into master Nov 4, 2016
@ogoffart ogoffart deleted the restore_backup branch November 4, 2016 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants