Allow configuring the location of the `appdata_[instanceid]` folder #36337

summersab · 2023-01-24T21:20:51Z

Allow configuring the location of the appdata_[instanceid] folder

Resolves: Allow configuration of appdata directory location #12873

Summary

This allows administrators to change the location of the appdata_[instanceid] folder by specifying an appdataroot value in config.php. This is especially useful when using shared storage, object storage as primary storage (storing appdata_[instanceid] in an object store is extremely slow), etc.

TODO

Resolved with commit 783ad06

Regardless of appdataroot being set, the function should really return $rootFolder and not $rootFolder->get($folderName). The current code will store files in /appdata_[instanceid]/appdata_[instanceid] when an appdataroot is provided and /appdata_[instanceid] in normal operation (i.e. it will create a /appdata_[instanceid] folder on the mount point instead of writing appdata folders to the mount point itself). Some apps such as OnlyOffice expect the appdata_[instanceid] folder to be a direct child of system root:
https://github.com/ONLYOFFICE/onlyoffice-nextcloud/blob/504884f16ce2b66e45a66579aa3cdf0fd0197330/lib/templatemanager.php#L53

However, I am having permission issues. The system is able to create files and folders at the mount point, but when it tries to read the folders, they don't appear to exist, and errors are thrown. I think it has something to do with the OC\FilesView attached to the mount point/folder, but I'm not sure. It seems to occur at this function:

server/lib/private/Files/Node/Folder.php

Line 161 in 75d7203

if ($this->checkPermissions(\OCP\Constants::PERMISSION_CREATE)) {

If that doesn't make sense, please feel free to ask me to explain in further detail.

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

lib/private/Files/AppData/AppData.php

kesselb · 2023-01-25T11:58:11Z

Thanks 👍

Can you have a look at tests/lib/Files/AppData/AppDataTest.php?

summersab · 2023-01-26T23:58:28Z

@kesselb - I hope I did that right? I've never made a commit to a project as big as this.

I see some of the other tests are failing. What else do I need to do?

nickvergessen · 2023-02-09T17:10:39Z

Something went wrong with your rebase.

Try:

git checkout master
# Replace nextcloud with the name of the remote, if you don't know, post the result of `git remote -v`
git pull nextcloud master
git checkout appdata_location
git rebase -i nextcloud/master
# Remove all commits which are not yours, then save and exit
git push --force origin appdata_location

summersab · 2023-02-09T17:25:45Z

Something went wrong with your rebase.

Yeah, I'm not the best with git, yet. I think I fixed it.

My latest commit also fixes the issue I was having trouble with as described in my initial comment (being unable to use the root of the new mount as appdata_[instanceid]. It was a issue with the caching and LocalRootScanner. In short, the PR now works the way it should have from the start.

lib/private/Files/Cache/LocalRootScanner.php

config/config.sample.php

Signed-off-by: summersab <arthur.summers@gmail.com> Signed-off-by: summersab <18727110+summersab@users.noreply.github.com> Set type of `$rootFolder` Signed-off-by: summersab <arthur.summers@gmail.com> Signed-off-by: summersab <18727110+summersab@users.noreply.github.com> implement test method Signed-off-by: summersab <18727110+summersab@users.noreply.github.com> update `config.sample.php` with `appdataroot` parameter Signed-off-by: summersab <18727110+summersab@users.noreply.github.com> fix caching issue preventing files from being written directly to mount point update method to check mount points and determine if cache scanner should run simplify method to check mount points fix unit test add missing line; cleanup lint perform lint cleanup run cs-fixer replace deprecated methods; fix invalid value for `$mountPoint` in `LocalRootScanner:shouldScanPath` rename appdataroot to appdatadirectory remove inaccurate language from `config.sample.php` Signed-off-by: Andrew Summers <18727110+summersab@users.noreply.github.com>

icewind1991 · 2023-09-22T09:38:52Z

lib/private/Files/AppData/AppData.php

+				$arguments = [
+					'datadir' => $appdatadirectory,
+				];
+				$storage = new LocalRootStorage($arguments);


This should just be Local not LocalRootStorage

icewind1991 · 2023-09-22T09:39:36Z

lib/private/Files/AppData/AppData.php

 		$this->config = $systemConfig;
 		$this->appId = $appId;
 		$this->folders = new CappedMemoryCache();
+
+		$this->rootFolder = new LazyRoot(function () use ($rootFolder, $systemConfig) {


Why are you creating a new LazyRoot?

icewind1991 · 2023-09-22T09:41:13Z

lib/private/Files/Cache/LocalRootScanner.php

+		$storageId = $this->storage->getId();
+		$mount = Filesystem::getMountManager()->findByStorageId($storageId);
+		$mountPoint = sizeof($mount) == 1 ? $mount[0]->getMountPoint() : "null";
+
 		$path = trim($path, '/');
-		return $path === '' || str_starts_with($path, 'appdata_') || str_starts_with($path, '__groupfolders');
+		return $path === '' || str_starts_with($path, 'appdata_') || str_starts_with($path, '__groupfolders') || str_starts_with($mountPoint, '/appdata_');


If you want to stop the scanner from touching the new appdata mountpoint, you can instead create a storage with a "noop" scanner.

imchasingshadows · 2024-02-15T13:57:29Z

Is there still work that needs to be done here? Seems like this has been idle for quite some time.

susnux · 2024-02-15T17:58:07Z

Is there still work that needs to be done here? Seems like this has been idle for quite some time.

See the unresolved comments above, so yes there is work to be done.
But feel free to fork and contribute on the feature :)

skjnldsv · 2024-05-02T14:40:45Z

Closing this pull request due to lack of recent activity and updates. We appreciate your contribution and encourage you to reopen or provide further updates if necessary. ☺️
Our aim is to keep the project moving forward with active collaboration. Thank you for your understanding and continued support! 🙏

luxzg · 2024-05-02T14:58:27Z

Wait, you closed an issue due to inactivity, issue that had already started development and people are actively waiting for it? If you want activity I will repost comment here every week so issue is "active". I don't think that's what is expected, and is usually considered spam on GitHub, but after this closing it seems this project has different code of conduct - "spam us if you want the feature".

Please be reasonable and keep the issue open until it is finished, reviewed and accepted.

It was planned for NC 26, then 27, 28, 29... I have no words...

ChristophWurst · 2024-05-02T15:15:24Z

It was planned for NC 26, then 27, 28, 29...

For your information: assigning to a milestone is not a sign of planning. All PRs are moved to the next milestone after the branch-off.
This PR is waiting for the author or someone else to finish the work, then it can be merged. The requested changes had been pending for half a year.

luxzg · 2024-05-02T15:24:52Z

Ok, sorry for misinterpreting the closing comment, it was just for this commit which indeed got stale, not the original issue. Hopefully someone will step up to finish the needed work.

szaimen added this to the Nextcloud 26 milestone Jan 24, 2023

szaimen added enhancement 3. to review Waiting for reviews labels Jan 24, 2023

szaimen requested review from a team, ArtificialOwl, icewind1991 and blizzz and removed request for a team January 24, 2023 21:23

tcitworld reviewed Jan 25, 2023

View reviewed changes

lib/private/Files/AppData/AppData.php Outdated Show resolved Hide resolved

summersab force-pushed the appdata_location branch 2 times, most recently from 2ce53a5 to 9cdfd1e Compare January 27, 2023 00:27

This comment was marked as resolved.

Sign in to view

summersab force-pushed the appdata_location branch 3 times, most recently from cea86b9 to 145000e Compare January 28, 2023 15:49

blizzz mentioned this pull request Feb 1, 2023

26.0.0 beta 2 #36484

Merged

This comment was marked as resolved.

Sign in to view

summersab force-pushed the appdata_location branch from e4e1ca8 to ce10d86 Compare February 9, 2023 17:07

summersab requested review from ChristophWurst, miaulalala and nickvergessen as code owners February 9, 2023 17:07

summersab force-pushed the appdata_location branch 2 times, most recently from 33d6508 to 783ad06 Compare February 9, 2023 17:20

github-advanced-security bot found potential problems Feb 9, 2023

View reviewed changes

lib/private/Files/Cache/LocalRootScanner.php Fixed Show resolved Hide resolved

summersab force-pushed the appdata_location branch from 783ad06 to cb5b45e Compare February 9, 2023 17:55

come-nc force-pushed the appdata_location branch from 1280c4d to 28c874e Compare August 28, 2023 13:09

nickvergessen reviewed Aug 28, 2023

View reviewed changes

config/config.sample.php Outdated Show resolved Hide resolved

summersab force-pushed the appdata_location branch from 47f154e to 96681f1 Compare September 7, 2023 21:57

juliusknorr removed their request for review September 9, 2023 07:50

icewind1991 requested changes Sep 22, 2023

View reviewed changes

skjnldsv mentioned this pull request Nov 1, 2023

chore: 28.0.0 beta 1 #41228

Merged

This was referenced Nov 6, 2023

28.0.0 beta 2 #41314

Merged

28.0.0 beta 3 #41379

Merged

This was referenced Nov 14, 2023

28.0.0 beta 4 #41477

Merged

28.0.0 RC1 #41621

Merged

blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023

This was referenced Mar 12, 2024

29.0.0 beta 2 #44152

Merged

29.0.0 beta 3 #44186

Merged

skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Mar 15, 2024

skjnldsv removed this from the Nextcloud 29 milestone Mar 15, 2024

skjnldsv marked this pull request as draft March 15, 2024 15:14

susnux added this to the Nextcloud 30 milestone Apr 18, 2024

skjnldsv closed this May 2, 2024

luxzg mentioned this pull request May 2, 2024

Allow configuration of appdata directory location #12873

Open

skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow configuring the location of the `appdata_[instanceid]` folder #36337

Allow configuring the location of the `appdata_[instanceid]` folder #36337

summersab commented Jan 24, 2023 •

edited

Loading

kesselb commented Jan 25, 2023

summersab commented Jan 26, 2023 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

nickvergessen commented Feb 9, 2023

summersab commented Feb 9, 2023 •

edited

Loading

icewind1991 Sep 22, 2023

icewind1991 Sep 22, 2023

icewind1991 Sep 22, 2023

imchasingshadows commented Feb 15, 2024

susnux commented Feb 15, 2024

skjnldsv commented May 2, 2024

luxzg commented May 2, 2024

ChristophWurst commented May 2, 2024

luxzg commented May 2, 2024

Allow configuring the location of the appdata_[instanceid] folder #36337

Allow configuring the location of the appdata_[instanceid] folder #36337

Conversation

summersab commented Jan 24, 2023 • edited Loading

Summary

TODO

Checklist

kesselb commented Jan 25, 2023

summersab commented Jan 26, 2023 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

nickvergessen commented Feb 9, 2023

summersab commented Feb 9, 2023 • edited Loading

icewind1991 Sep 22, 2023

Choose a reason for hiding this comment

icewind1991 Sep 22, 2023

Choose a reason for hiding this comment

icewind1991 Sep 22, 2023

Choose a reason for hiding this comment

imchasingshadows commented Feb 15, 2024

susnux commented Feb 15, 2024

skjnldsv commented May 2, 2024

luxzg commented May 2, 2024

ChristophWurst commented May 2, 2024

luxzg commented May 2, 2024

Allow configuring the location of the `appdata_[instanceid]` folder #36337

Allow configuring the location of the `appdata_[instanceid]` folder #36337

summersab commented Jan 24, 2023 •

edited

Loading

summersab commented Jan 26, 2023 •

edited

Loading

summersab commented Feb 9, 2023 •

edited

Loading