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

make NextCloud WORM file system friendly #24596

Merged
merged 3 commits into from
Sep 16, 2022
Merged

make NextCloud WORM file system friendly #24596

merged 3 commits into from
Sep 16, 2022

Conversation

kofemann
Copy link
Member

@kofemann kofemann commented Dec 7, 2020

No description provided.

@kofemann kofemann changed the title Dcache worm make NextCloud WORM file system friendly Dec 7, 2020
@kofemann kofemann mentioned this pull request Dec 7, 2020
Open
@kofemann kofemann force-pushed the dcache-worm branch 2 times, most recently from 8136009 to 75b3261 Compare December 22, 2020 10:28
@rullzer
Copy link
Member

rullzer commented Feb 11, 2021

@kofemann as far I know you have been running with this in production for some time now right?
Any reason this PR is still a draft?

@kofemann
Copy link
Member Author

@rullzer We would like to run some tests to validate that other things (like apps) are not broken. Do you have a testsuite that we can run locally?

@PVince81
Copy link
Member

the code looks fine to me in general, I'm hoping that the extra unlink will not cause much extra overhead, but we're talking about local storage anyway so should be fine. @icewind1991 thoughts ?

@PVince81
Copy link
Member

@kofemann please rebase and resolve the conflicts

@PVince81
Copy link
Member

regarding tests, at least the ones with regular storage, are already running in our CI so if something is broken we'll likely see this immediately

@kofemann
Copy link
Member Author

@PVince81 Thanks for the update. Actually, since 2 weeks we run this code with nextcloud 22.2.3 in a production. Up to now we haven't seen any issues.

@PVince81
Copy link
Member

@PVince81 Thanks for the update. Actually, since 2 weeks we run this code with nextcloud 22.2.3 in a production. Up to now we haven't seen any issues.

that's great to hear! I took the liberty to click "ready for review"

@PVince81 PVince81 marked this pull request as ready for review January 28, 2022 14:38
@PVince81 PVince81 self-assigned this Jan 28, 2022
@szaimen szaimen added 3. to review Waiting for reviews enhancement labels Jan 31, 2022
@szaimen szaimen added this to the Nextcloud 24 milestone Jan 31, 2022
@PVince81
Copy link
Member

PVince81 commented Feb 7, 2022

some strange errors in multiple places:

1) Test\Cache\FileCacheTest::testGarbageCollectLeaveRecentKeys
InvalidArgumentException: Username is invalid because files already exist for this user

I'm wondering if the change in this PR somehow interferes with test cleanup, but I can't see how

@kofemann
Copy link
Member Author

kofemann commented Feb 7, 2022

This is obviously something that I have to look at. Is it possible to run the test manually?

@PVince81
Copy link
Member

PVince81 commented Feb 7, 2022

This is obviously something that I have to look at. Is it possible to run the test manually?

see https://docs.nextcloud.com/server/latest/developer_manual/core/unit-testing.html#bootstrapping-nextcloud

this is how I usually test locally:
./autotest.sh sqlite tests/lib/Cache/FileCacheTest.php

@icewind1991
Copy link
Member

I would expect a filesystem to be smart enough to handle the "truncate an existing file and write to it" case, but I'm fine adding some logic for it.

It should only unlink if the file already exists though.

icewind1991
icewind1991 requested changes Feb 15, 2022
View reviewed changes
Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

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

Please add a check if the file exists before deleting it

@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
Merged
@blizzz blizzz mentioned this pull request Mar 31, 2022
Merged
@kofemann
Copy link
Member Author

I have added a coifing option control this behavior. A check before unlink will issue unnecessary stat call.

@blizzz blizzz mentioned this pull request Apr 7, 2022
Merged
@blizzz blizzz mentioned this pull request Apr 13, 2022
Merged
@kofemann
Copy link
Member Author

@icewind1991 any plans to include this into NC24?

@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
This was referenced Aug 12, 2022
Merged
Merged
@blizzz blizzz mentioned this pull request Aug 24, 2022
Merged
@kofemann
files: make OC\Files\Storage\Local WORM friendly
b6065a2 
Some filesystems run as a Write-Once-Read-Many storages. This
makes them impossible to use with NexeCloud, as the file system
layers uses `truncate` syscall (through file_put_contents function).

As Nextcloud is never updates existing files, removing the old entry
and creatint a new one on update will allow NextCoud to update on such
file systems.

Update Local#fopen and Local#file_put_contents to remote existing
file before truncating.

Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
@kofemann kofemann force-pushed the dcache-worm branch 3 times, most recently from 677382f to 9bf9c94 Compare August 25, 2022 09:37
@kofemann
Copy link
Member Author

rebased on top of current master

This was referenced Aug 30, 2022
Merged
Merged
@blizzz blizzz mentioned this pull request Sep 9, 2022
Merged
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
Merged
PVince81
PVince81 approved these changes Sep 16, 2022
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81
Copy link
Member

@kofemann the code looks fine. please sign off your commits, see https://github.com/nextcloud/server/pull/24596/checks?check_run_id=8012932504

I'll try and find a second reviewer to get this in. thanks!

CarlSchwan
CarlSchwan approved these changes Sep 16, 2022
View reviewed changes
lib/private/Files/Storage/Local.php Outdated Show resolved Hide resolved
@kofemann
files: remove destination file before copying new content (WORM)
8fc4cf6 
Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
@kofemann
config: add a switch to control truncate before update
f41209a 
To avoid extra truncate on non WORM file systems, add a new config
option `localstorage.unlink_on_truncate`, which defaults to false.

The OC\Files\Storage\Local is update to respect that option.

Signed-off-by: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Sep 16, 2022
@PVince81 PVince81 merged commit 1025d04 into nextcloud:master Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarlSchwan CarlSchwan CarlSchwan approved these changes

@PVince81 PVince81 PVince81 approved these changes

@artonge artonge Awaiting requested review from artonge

@icewind1991 icewind1991 Awaiting requested review from icewind1991

Assignees

@PVince81 PVince81

Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Milestone
Nextcloud 25
Development

Successfully merging this pull request may close these issues.
7 participants
@kofemann @rullzer @PVince81 @icewind1991 @CarlSchwan @blizzz @szaimen