From 13d465a76b389f41e1c7f31d415faa61ea2fa8dd Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 20 Sep 2024 10:28:19 +0200 Subject: [PATCH 1/6] test: re-add object store primary storage phpunit tests Signed-off-by: Robin Appelman --- .../phpunit-object-store-primary.yml | 121 ++++++++++++++++++ tests/preseed-config.php | 15 +++ 2 files changed, 136 insertions(+) create mode 100644 .github/workflows/phpunit-object-store-primary.yml diff --git a/.github/workflows/phpunit-object-store-primary.yml b/.github/workflows/phpunit-object-store-primary.yml new file mode 100644 index 0000000000000..0c8140a96ce2e --- /dev/null +++ b/.github/workflows/phpunit-object-store-primary.yml @@ -0,0 +1,121 @@ +# SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors +# SPDX-License-Identifier: MIT +name: PHPUnit primary object store +on: + pull_request: + schedule: + - cron: "15 2 * * *" + +concurrency: + group: phpunit-object-store-primary-${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +jobs: + changes: + runs-on: ubuntu-latest-low + + outputs: + src: ${{ steps.changes.outputs.src}} + + steps: + - uses: dorny/paths-filter@de90cc6fb38fc0963ad72b210f1f284cd68cea36 # v3.0.2 + id: changes + continue-on-error: true + with: + filters: | + src: + - '.github/workflows/**' + - '3rdparty/**' + - '**/appinfo/**' + - '**/lib/**' + - '**/templates/**' + - '**/tests/**' + - 'vendor/**' + - 'vendor-bin/**' + - '.php-cs-fixer.dist.php' + - 'composer.json' + - 'composer.lock' + - '**.php' + + object-store-primary-tests-minio: + runs-on: ubuntu-latest + needs: changes + + if: ${{ github.repository_owner != 'nextcloud-gmbh' && needs.changes.outputs.src != 'false' }} + + strategy: + # do not stop on another job's failure + fail-fast: false + matrix: + php-versions: ['8.1'] + key: ['s3', 's3-multibucket'] + + name: php${{ matrix.php-versions }}-${{ matrix.key }}-minio + + services: + cache: + image: ghcr.io/nextcloud/continuous-integration-redis:latest + ports: + - 6379:6379/tcp + options: --health-cmd="redis-cli ping" --health-interval=10s --health-timeout=5s --health-retries=3 + + minio: + image: bitnami/minio + env: + MINIO_ROOT_USER: nextcloud + MINIO_ROOT_PASSWORD: bWluaW8tc2VjcmV0LWtleS1uZXh0Y2xvdWQ= + MINIO_DEFAULT_BUCKETS: nextcloud + ports: + - "9000:9000" + + steps: + - name: Checkout server + uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 + with: + submodules: true + + - name: Set up php ${{ matrix.php-versions }} + uses: shivammathur/setup-php@c5fc0d8281aba02c7fda07d3a70cc5371548067d #v2.25.2 + with: + php-version: ${{ matrix.php-versions }} + extensions: mbstring, fileinfo, intl, sqlite, pdo_sqlite, zip, gd + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + + - name: Set up Nextcloud + env: + OBJECT_STORE: ${{ matrix.key }} + OBJECT_STORE_KEY: nextcloud + OBJECT_STORE_SECRET: bWluaW8tc2VjcmV0LWtleS1uZXh0Y2xvdWQ= + run: | + composer install + cp tests/redis.config.php config/ + cp tests/preseed-config.php config/config.php + ./occ maintenance:install --verbose --database=sqlite --database-name=nextcloud --database-host=127.0.0.1 --database-user=root --database-pass=rootpassword --admin-user admin --admin-pass password + php -f tests/enable_all.php | grep -i -C9999 error && echo "Error during app setup" && exit 1 || exit 0 + + - name: Wait for S3 + run: | + sleep 10 + curl -f -m 1 --retry-connrefused --retry 10 --retry-delay 10 http://localhost:9000/minio/health/ready + + - name: PHPUnit + run: composer run test:db + + - name: S3 logs + if: always() + run: | + cat data/nextcloud.log + docker ps -a + docker ps -aq | while read container ; do IMAGE=$(docker inspect --format='{{.Config.Image}}' $container); echo $IMAGE; docker logs $container; echo "\n\n" ; done + + + object-store-primary-summary: + runs-on: ubuntu-latest-low + needs: [changes,object-store-primary-tests-minio] + + if: always() + + steps: + - name: Summary status + run: if ${{ needs.changes.outputs.src != 'false' && needs.object-store-primary-tests-minio.result != 'success' }}; then exit 1; fi diff --git a/tests/preseed-config.php b/tests/preseed-config.php index c62b447128040..8a56e43c7a0f3 100644 --- a/tests/preseed-config.php +++ b/tests/preseed-config.php @@ -34,6 +34,21 @@ 'use_path_style' => true ] ]; +} elseif (getenv('OBJECT_STORE') === 's3-multibucket') { + $CONFIG['objectstore_multibucket'] = [ + 'class' => 'OC\\Files\\ObjectStore\\S3', + 'arguments' => [ + 'bucket' => 'nextcloud', + 'autocreate' => true, + 'key' => getenv('OBJECT_STORE_KEY') ?: 'nextcloud', + 'secret' => getenv('OBJECT_STORE_SECRET') ?: 'nextcloud', + 'hostname' => getenv('OBJECT_STORE_HOST') ?: 'localhost', + 'port' => 9000, + 'use_ssl' => false, + // required for some non amazon s3 implementations + 'use_path_style' => true + ] + ]; } elseif (getenv('OBJECT_STORE') === 'azure') { $CONFIG['objectstore'] = [ 'class' => 'OC\\Files\\ObjectStore\\Azure', From e05a474aa4ea9ffe5106d7f1788a200b0b9429e4 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 20 Sep 2024 11:19:14 +0200 Subject: [PATCH 2/6] fix: ensure source folder is removed from cache when moving to objectstore otherwise this causes confusion down the line as it's contents will be moved to the new cache Signed-off-by: Robin Appelman --- lib/private/Files/ObjectStore/ObjectStoreStorage.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index 423255e2c9ad9..408a7e33cbe8f 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -629,6 +629,7 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t $this->moveFromStorage($sourceStorage, $child->getPath(), $targetInternalPath . '/' . $child->getName()); } $sourceStorage->rmdir($sourceInternalPath); + $sourceStorage->getCache()->remove($sourceInternalPath); } else { $sourceStream = $sourceStorage->fopen($sourceInternalPath, 'r'); if (!$sourceStream) { From 354f452f81d5444ffa0a00d39feb5a3cb563c338 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 25 Sep 2024 17:52:42 +0200 Subject: [PATCH 3/6] fix: preserve fileid when moving from objectstore to non-objectstore Signed-off-by: Robin Appelman --- apps/files_trashbin/tests/StorageTest.php | 6 +----- .../Files/ObjectStore/ObjectStoreStorage.php | 13 ++++++++++-- lib/private/Files/Storage/Common.php | 20 +++++++++++++++---- 3 files changed, 28 insertions(+), 11 deletions(-) diff --git a/apps/files_trashbin/tests/StorageTest.php b/apps/files_trashbin/tests/StorageTest.php index d6e911e3dea6f..bcd4124b8be16 100644 --- a/apps/files_trashbin/tests/StorageTest.php +++ b/apps/files_trashbin/tests/StorageTest.php @@ -33,7 +33,6 @@ use OC\Files\Filesystem; use OC\Files\Storage\Common; -use OC\Files\Storage\Local; use OC\Files\Storage\Temporary; use OCA\Files_Trashbin\AppInfo\Application; use OCA\Files_Trashbin\Events\MoveToTrashEvent; @@ -697,10 +696,7 @@ public function testTrashbinCollision() { $this->assertEquals('bar', $this->rootView->file_get_contents($this->user . '/files_trashbin/files/test.txt.d1001')); } - public function testMoveFromStoragePreserveFileId() { - if (!$this->userView->getMount('')->getStorage()->instanceOfStorage(Local::class)) { - $this->markTestSkipped("Skipping on non-local users storage"); - } + public function testMoveFromStoragePreserveFileId(): void { $this->userView->file_put_contents('test.txt', 'foo'); $fileId = $this->userView->getFileInfo('test.txt')->getId(); diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index 408a7e33cbe8f..1330b9a40fe6c 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -60,6 +60,7 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil private bool $handleCopiesAsOwned; protected bool $validateWrites = true; + private bool $preserveCacheItemsOnDelete = false; /** * @param array $params @@ -196,7 +197,9 @@ private function rmObjects(ICacheEntry $entry): bool { } } - $this->getCache()->remove($entry->getPath()); + if (!$this->preserveCacheItemsOnDelete) { + $this->getCache()->remove($entry->getPath()); + } return true; } @@ -231,7 +234,9 @@ public function rmObject(ICacheEntry $entry): bool { } //removing from cache is ok as it does not exist in the objectstore anyway } - $this->getCache()->remove($entry->getPath()); + if (!$this->preserveCacheItemsOnDelete) { + $this->getCache()->remove($entry->getPath()); + } return true; } @@ -783,4 +788,8 @@ public function cancelChunkedWrite(string $targetPath, string $writeToken): void $urn = $this->getURN($cacheEntry->getId()); $this->objectStore->abortMultipartUpload($urn, $writeToken); } + + public function setPreserveCacheOnDelete(bool $preserve) { + $this->preserveCacheItemsOnDelete = $preserve; + } } diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php index fb4aa0a7c3c58..ab9cd5c33261b 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -49,6 +49,7 @@ use OC\Files\Cache\Updater; use OC\Files\Cache\Watcher; use OC\Files\Filesystem; +use OC\Files\ObjectStore\ObjectStoreStorage; use OC\Files\Storage\Wrapper\Jail; use OC\Files\Storage\Wrapper\Wrapper; use OCP\Files\EmptyFileNameException; @@ -704,10 +705,21 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t $result = $this->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, true); if ($result) { - if ($sourceStorage->is_dir($sourceInternalPath)) { - $result = $sourceStorage->rmdir($sourceInternalPath); - } else { - $result = $sourceStorage->unlink($sourceInternalPath); + if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) { + /** @var ObjectStoreStorage $sourceStorage */ + $sourceStorage->setPreserveCacheOnDelete(true); + } + try { + if ($sourceStorage->is_dir($sourceInternalPath)) { + $result = $sourceStorage->rmdir($sourceInternalPath); + } else { + $result = $sourceStorage->unlink($sourceInternalPath); + } + } finally { + if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) { + /** @var ObjectStoreStorage $sourceStorage */ + $sourceStorage->setPreserveCacheOnDelete(false); + } } } return $result; From ff2c9e8c714a5303e572d94cf770f45cfd5a0c98 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 25 Sep 2024 18:01:53 +0200 Subject: [PATCH 4/6] test: fix share storage move test with object store Signed-off-by: Robin Appelman --- apps/files_sharing/tests/SharedStorageTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/files_sharing/tests/SharedStorageTest.php b/apps/files_sharing/tests/SharedStorageTest.php index 5209a30634a6d..14fdba0ade3cf 100644 --- a/apps/files_sharing/tests/SharedStorageTest.php +++ b/apps/files_sharing/tests/SharedStorageTest.php @@ -479,6 +479,7 @@ public function testMoveFromStorage() { $sourceStorage = new \OC\Files\Storage\Temporary([]); $sourceStorage->file_put_contents('foo.txt', 'asd'); + $sourceStorage->getScanner()->scan(''); $sharedStorage->moveFromStorage($sourceStorage, 'foo.txt', 'bar.txt'); $this->assertTrue($sharedStorage->file_exists('bar.txt')); From 99778e1eecd4cb46bc009c4da60883081f110173 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 26 Sep 2024 16:28:59 +0200 Subject: [PATCH 5/6] fix: rework move into object store to better preserve fileids Signed-off-by: Robin Appelman --- .../Files/ObjectStore/ObjectStoreStorage.php | 77 ++++++++++++++----- 1 file changed, 58 insertions(+), 19 deletions(-) diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index 1330b9a40fe6c..c49139424c66d 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -628,32 +628,71 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t if (!$sourceCacheEntry) { $sourceCacheEntry = $sourceCache->get($sourceInternalPath); } - if ($sourceCacheEntry->getMimeType() === FileInfo::MIMETYPE_FOLDER) { - $this->mkdir($targetInternalPath); - foreach ($sourceCache->getFolderContents($sourceInternalPath) as $child) { - $this->moveFromStorage($sourceStorage, $child->getPath(), $targetInternalPath . '/' . $child->getName()); - } + if (!$sourceCacheEntry) { + return false; + } + + $this->copyObjects($sourceStorage, $sourceCache, $sourceCacheEntry); + if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) { + /** @var ObjectStoreStorage $sourceStorage */ + $sourceStorage->setPreserveCacheOnDelete(true); + } + if ($sourceCacheEntry->getMimeType() === ICacheEntry::DIRECTORY_MIMETYPE) { $sourceStorage->rmdir($sourceInternalPath); - $sourceStorage->getCache()->remove($sourceInternalPath); } else { - $sourceStream = $sourceStorage->fopen($sourceInternalPath, 'r'); - if (!$sourceStream) { - return false; - } - // move the cache entry before the contents so that we have the correct fileid/urn for the target - $this->getCache()->moveFromCache($sourceCache, $sourceInternalPath, $targetInternalPath); - try { - $this->writeStream($targetInternalPath, $sourceStream, $sourceCacheEntry->getSize()); - } catch (\Exception $e) { - // restore the cache entry - $sourceCache->moveFromCache($this->getCache(), $targetInternalPath, $sourceInternalPath); - throw $e; - } $sourceStorage->unlink($sourceInternalPath); } + if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class)) { + /** @var ObjectStoreStorage $sourceStorage */ + $sourceStorage->setPreserveCacheOnDelete(false); + } + $this->getCache()->moveFromCache($sourceCache, $sourceInternalPath, $targetInternalPath); + return true; } + /** + * Copy the object(s) of a file or folder into this storage, without touching the cache + */ + private function copyObjects(IStorage $sourceStorage, ICache $sourceCache, ICacheEntry $sourceCacheEntry) { + $copiedFiles = []; + try { + foreach ($this->getAllChildObjects($sourceCache, $sourceCacheEntry) as $file) { + $sourceStream = $sourceStorage->fopen($file->getPath(), 'r'); + if (!$sourceStream) { + throw new \Exception("Failed to open source file {$file->getPath()} ({$file->getId()})"); + } + $this->objectStore->writeObject($this->getURN($file->getId()), $sourceStream, $file->getMimeType()); + if (is_resource($sourceStream)) { + fclose($sourceStream); + } + $copiedFiles[] = $file->getId(); + } + } catch (\Exception $e) { + foreach ($copiedFiles as $fileId) { + try { + $this->objectStore->deleteObject($this->getURN($fileId)); + } catch (\Exception $e) { + // ignore + } + } + throw $e; + } + } + + /** + * @return \Iterator + */ + private function getAllChildObjects(ICache $cache, ICacheEntry $entry): \Iterator { + if ($entry->getMimeType() === FileInfo::MIMETYPE_FOLDER) { + foreach ($cache->getFolderContentsById($entry->getId()) as $child) { + yield from $this->getAllChildObjects($cache, $child); + } + } else { + yield $entry; + } + } + public function copy($source, $target) { $source = $this->normalizePath($source); $target = $this->normalizePath($target); From e3aa73377141b158a775f58a3d038b3d0f4ee131 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 19 Sep 2024 19:33:34 +0200 Subject: [PATCH 6/6] fix: improve moving object store items to trashbin Signed-off-by: Robin Appelman --- apps/files_trashbin/lib/Trashbin.php | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php index ced40313d621a..575a0928c9002 100644 --- a/apps/files_trashbin/lib/Trashbin.php +++ b/apps/files_trashbin/lib/Trashbin.php @@ -53,7 +53,6 @@ use OC\Files\Node\Folder; use OC\Files\Node\NonExistingFile; use OC\Files\Node\NonExistingFolder; -use OC\Files\ObjectStore\ObjectStoreStorage; use OC\Files\View; use OC_User; use OCA\Files_Trashbin\AppInfo\Application; @@ -67,6 +66,7 @@ use OCP\Files\Node; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; +use OCP\Files\Storage\ILockingStorage; use OCP\FilesMetadata\IFilesMetadataManager; use OCP\IConfig; use OCP\Lock\ILockingProvider; @@ -290,11 +290,10 @@ public static function move2trash($file_path, $ownerOnly = false) { $trashPath = '/files_trashbin/files/' . static::getTrashFilename($filename, $timestamp); $gotLock = false; - while (!$gotLock) { + do { + /** @var ILockingStorage & Storage $trashStorage */ + [$trashStorage, $trashInternalPath] = $ownerView->resolvePath($trashPath); try { - /** @var \OC\Files\Storage\Storage $trashStorage */ - [$trashStorage, $trashInternalPath] = $ownerView->resolvePath($trashPath); - $trashStorage->acquireLock($trashInternalPath, ILockingProvider::LOCK_EXCLUSIVE, $lockingProvider); $gotLock = true; } catch (LockedException $e) { @@ -305,7 +304,7 @@ public static function move2trash($file_path, $ownerOnly = false) { $trashPath = '/files_trashbin/files/' . static::getTrashFilename($filename, $timestamp); } - } + } while (!$gotLock); $sourceStorage = $sourceInfo->getStorage(); $sourceInternalPath = $sourceInfo->getInternalPath(); @@ -319,14 +318,12 @@ public static function move2trash($file_path, $ownerOnly = false) { return false; } - $trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); - try { $moveSuccessful = true; - // when moving within the same object store, the cache update done above is enough to move the file - if (!($trashStorage->instanceOfStorage(ObjectStoreStorage::class) && $trashStorage->getId() === $sourceStorage->getId())) { - $trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); + $trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); + if ($sourceStorage->getCache()->inCache($sourceInternalPath)) { + $trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath); } } catch (\OCA\Files_Trashbin\Exceptions\CopyRecursiveException $e) { $moveSuccessful = false;