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

fix delete in the owncloud storage driver #1833

Merged
merged 1 commit into from
Jun 28, 2021
Merged

fix delete in the owncloud storage driver #1833

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changelog/unreleased/fix-delete-owncloud-storage-driver.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Bugfix: Properly handle name collisions for deletes in the owncloud driver

In the owncloud storage driver when we delete a file we append the deletion time to the file name.
If two fast consecutive deletes happened, the deletion time would be the same and if the two files had the same name we ended up with only one file in the trashbin.

https://github.com/cs3org/reva/pull/1833
20 changes: 12 additions & 8 deletions pkg/storage/fs/owncloud/owncloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1526,15 +1526,19 @@ func (fs *ocfs) trash(ctx context.Context, ip string, rp string, origin string)
// move to trash location
dtime := time.Now().Unix()
tgt := filepath.Join(rp, fmt.Sprintf("%s.d%d", filepath.Base(ip), dtime))

// The condition reads: "if the file exists"
// I know this check is hard to read because of the double negation
// but this way we avoid to duplicate the code following the if block.
// If two deletes happen fast consecutively they will have the same `dtime`,
// therefore we have to increase the 'dtime' to avoid collisions.
if _, err := os.Stat(tgt); !errors.Is(err, os.ErrNotExist) {
// timestamp collision, try again with higher value:
dtime++
tgt = filepath.Join(rp, fmt.Sprintf("%s.d%d", filepath.Base(ip), dtime))
Copy link
Contributor

@phil-davis phil-davis Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is a good improvement, so we should have it.
But IMO I can write a test where there are lots of textfile.txt in different folders. And I delete then all quickly. The first 2 will delete fine. Then, if the deletes happen more than once per second, there will quickly be a case where the file "exists" in the trashbin with the current Unix second, and one exists for the next Unix second, so the 3rd (or 4th and/or...) delete will fail.

We have this sort of stuff in oC10 where trashbin and versions entries have uses of Unix second in the back-end implementation. And that means there are timing issues when multiple deletes, versions,... happen in the same second. And these things can happen, specially when a sync-client "wakes up" and has a backlog of things to do, or if some automated data-producer is doing "lots of stuff fast".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I completely agree but I am kind of hesitant to spend too much time on the owncloud storage driver since we don't really use it anywhere except for the tests and I still don't know if we even intend to use it productively. Maybe we need to bring this question up again in the standup or some other discussion...

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree - decisions are needed about which storage drivers are to be "long-term" and enhanced to pass "all the tests".

}
if err := os.Rename(ip, tgt); err != nil {
if os.IsExist(err) {
// timestamp collision, try again with higher value:
dtime++
tgt := filepath.Join(rp, fmt.Sprintf("%s.d%d", filepath.Base(ip), dtime))
if err := os.Rename(ip, tgt); err != nil {
return errors.Wrap(err, "ocfs: could not move item to trash")
}
}
return errors.Wrap(err, "ocfs: could not move item to trash")
}

return fs.propagate(ctx, filepath.Dir(ip))
Expand Down
6 changes: 0 additions & 6 deletions tests/acceptance/expected-failures-on-OWNCLOUD-storage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,6 @@ The following scenarios fail on OWNCLOUD storage but not on OCIS storage:
- [apiTrashbin/trashbinFilesFolders.feature:278](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L278)
- [apiTrashbin/trashbinFilesFolders.feature:279](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinFilesFolders.feature#L279)

#### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122)
- [apiTrashbin/trashbinRestore.feature:108](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L108)
- [apiTrashbin/trashbinRestore.feature:109](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L109)
- [apiTrashbin/trashbinRestore.feature:110](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L110)
- [apiTrashbin/trashbinRestore.feature:111](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L111)

#### [href in trashbin PROPFIND response is wrong](https://github.com/owncloud/ocis/issues/1120)
#### [cannot restore to a different file-name](https://github.com/owncloud/ocis/issues/1122)
- [apiTrashbin/trashbinRestore.feature:309](https://github.com/owncloud/core/blob/master/tests/acceptance/features/apiTrashbin/trashbinRestore.feature#L309)
Expand Down