csync: mtime diff < 1s => conflict not detected #5589
Comments
|
I "fixed" the test for the moment by this workaround (you need to undo it to reproduce the problem): https://github.com/cernbox/smashbox/blob/master/lib/test_basicSync.py#L181 |
|
the "mtime" resolution of the csync primitive is indeed in second. |
|
This has been like that from the beginning, and so far, nobody asked for a better resolution. |
|
or ultimately checksums?
…On Thu, Mar 9, 2017 at 9:32 AM, Olivier Goffart ***@***.***> wrote:
the "mtime" resolution of the csync primitive is indeed in second.
The other way to cause a change to be detected is to change the size of
the file.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5589 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAl9jUk7cqqlHOWsYuvg9GoLMvWfurbNks5rj7kEgaJpZM4MXDFY>
.
--
---
Best regards,
Kuba
|
|
We need to think about it at a later stage. With human interaction it seems very unlikely to generate this, when the sync client is used my more machines we need to come up with a better architecture resolution eventually. Do understand that the limits are that changes occur in the same second and that the size of the file has also not changed. |
|
Okey, no problem but would it be possible please to quantify "later stage"?
Please keep in mind that this synchronization system is not only used for
"human interaction" in the sense of opening one office file at a time in a
group of 5 people. People are (trying to) synchronize lots of machine
generated content with thousands of files across several 10s or even 100s
of users. Is owncloud not suitable for this kind of usage?
…On Thu, Mar 9, 2017 at 2:14 PM, hodyroff ***@***.***> wrote:
We need to think about it at a later stage. With human interaction it
seems very unlikely to generate this, when the sync client is used my more
machines we need to come up with a better architecture resolution
eventually. Do understand that the limits are that changes occur in the
same second and that the size of the file has also not changed.
@settermjd <https://github.com/settermjd> for documentation (desktop
client limitations or sync algorithm architecture).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#5589 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAl9jcaqO67NUiG74cRs8VqCVijmROfiks5rj_sdgaJpZM4MXDFY>
.
--
---
Best regards,
Kuba
|
|
If this is a real live scenario we would need to put a larger prio on it. Can this happen without a size change - which would also trigger the right behaviour? I understood right now you did hit it based on a testing scenario. |
|
The problem with real life scenario is that we do not have means to detect this problem other than a user actively looking into the content and noticing the difference. So there is no way for me to say how often (if at all) someone in a sufficiently large user base if affected. Would it be possible to at least add a warning or error? |
|
Yes. An error sounds like a good idea for now. We will discuss it for 2.4. reminds me that I wanted to ask for a new error tab so the errors can easily seen on the client side ... and not go away after a short second or so ... |
|
To log/report an error the code will have to know that the problem happened. At the moment it thinks that the files at client and server ends are the same (=same size and "close enough" mtime). So I don't see how there is a viable interim stage that just logs/reports an error. I think the issue with the checksum thing is that for big files it takes time/resources to calculate the checksum. If the code only asks for it when this occasional mtime/size match happens, then the checksum does not have to be calculated (and stored) for every file. But of course if a check is needed on a big file then there will be delay. If checksums are stored for every file, then the check is quick, but the system always has overhead of calculating and storing the checksums. |
|
I think it would be possible to use higher-resolution mtimes locally where they are available, but that doesn't help with your test case since local and remote mtimes are compared. On casual inspection, it looks like the server mtimes have second resolution ( To address this particular case (NEW locally, NEW remotely, same mtime/size but different content) the checksum comparison @phil-davis mentions would make sense. That would work if the server is aware of a checksum for the file. Then the local checksum could be computed in this edge case only and used as an extra criterion. The case is detected at https://github.com/owncloud/client/blob/master/csync/src/csync_reconcile.c#L275 |
|
@ogoffart From an implementation point of view, I expect we could mark the file as CONFLICT and create PropagateDownloadFile jobs instead of skipping files in this situation. Then we'd calculate the checksum in these jobs and skip the download if it matches the remote checksum. The main potential issue I see is that NEW/NEW with identical size/mtime happens for all files on db-loss. Previously this would be a really fast operation, but now it'd need to calculate the checksum for each file. |
|
@ckamm: we need to keep the current behavior for NEW/NEW. for SYNC/SYNC, we might change the behavior and download the server version. we already do a byte to byte comparison of the two files before marking the conflict. |
|
Of course, if the server were to give us a content checksum before we download, that would solve the issue as well. |
|
@ogoffart @ckamm FYI update on checksums by @IljaN owncloud/core#26655 |
Given that we want to check checksums in NEW/NEW situations.
|
checksum actually is not enough. We need a collision safe hash. |
|
@ogoffart Yes. We'd need to verify whether what the server returns is usable for this check. It currently looks like the oc server will maintain SHA1 checksums, that'd be suitable. |
|
PR #5838 |
* For conflicts where mtime and size are identical:
a) If there's no remote checksum, skip (unchanged)
b) If there's a remote checksum, create a PropagateDownload
and compute the local checksum. If the checksums are identical,
don't download the file and just update metadata.
* Avoid exposing the existence of checksumTypeId beyond the database
layer. This makes handling checksums easier in general because they
can usually be treated as a single blob.
This change was prompted by the difficulty of producing file_stat_t
entries uniformly from PROPFINDs and the database.
* For conflicts where mtime and size are identical:
a) If there's no remote checksum, skip (unchanged)
b) If there's a remote checksum that's a useful hash, create a
PropagateDownload job and compute the local hash. If the hashes
are identical, don't download the file and just update metadata.
* Avoid exposing the existence of checksumTypeId beyond the database
layer. This makes handling checksums easier in general because they
can usually be treated as a single blob.
This change was prompted by the difficulty of producing file_stat_t
entries uniformly from PROPFINDs and the database.
|
@moscicki Would you be able to test the PR with your server setup? |
|
Yes. Please send me a pointer where to find packages with this fix.
…On Thu, Jun 15, 2017 at 3:40 PM, Markus Goetz ***@***.***> wrote:
@moscicki <https://github.com/moscicki> Would you be able to test the PR
with your server setup?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5589 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAl9jWxKZR8D8K2nBHSVOzzCaWhlRIDuks5sETRbgaJpZM4MXDFY>
.
--
---
Best regards,
Kuba
|
|
@moscicki Note that there's only a behavior change if the server sends MD5 or SHA1 hashes in the |
|
@moscicki Any news? |
|
Will test next week. Sorry.
…On Thu, Jul 27, 2017 at 10:36 AM, Markus Goetz ***@***.***> wrote:
@moscicki <https://github.com/moscicki> Any news?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5589 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAl9jR3Qb2bQ5H8u2FniSyNsP-uSqmqWks5sSEwSgaJpZM4MXDFY>
.
--
---
Best regards,
Kuba
|
|
@moscicki Have you had a chance to test this? |
|
Working on it. Please allow few more days. @giorgosalex is having a look. |
|
You can try the 2.4.0 rc1 @giorgosalex @moscicki https://owncloud.org/changelog/desktop/ |
|
Assuming it works.. |
Seen on versions 2.2.4, 2.1.1, 1.7. This is likely a csync issue.
Reproducer, depends how quick is the sync on this line: https://github.com/cernbox/smashbox/blob/master/lib/test_basicSync.py#L179
./bin/smash -t 4 ./lib/test_basicSync.py
When two clients (loser and winner) create the conflicting files within 1s then the synchronization of the client that comes second (loser) does not detect the conflict and the file is left unsynchronized.
In this case what's get created on disk is this:
As you can see below, the content with the checksum cac0c3f51680222f14025641074e35fa from the loser is not synchronized nor turned into conflict file:
The test ends with failed checks:
At the level of the sync client log, the conflict is silently skipped:
If you compare it to the correct run (when timing > 1s):
The text was updated successfully, but these errors were encountered: