From a5f8a2625f2fd681e6b3ffb45ab25f90e8583873 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 20 Sep 2024 10:28:19 +0200 Subject: [PATCH 1/5] 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 3095690e48369..1524861bf6c24 100644 --- a/tests/preseed-config.php +++ b/tests/preseed-config.php @@ -38,6 +38,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 da21acfb3ff5c6c7b8e1298cdefb5f17b10a6aab Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 20 Sep 2024 11:19:14 +0200 Subject: [PATCH 2/5] 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 c44b5b299eda9..883099931376a 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -602,6 +602,7 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t $this->moveFromStorage($sourceStorage, $child->getPath(), $targetInternalPath . '/' . $child->getName(), $child); } $sourceStorage->rmdir($sourceInternalPath); + $sourceStorage->getCache()->remove($sourceInternalPath); } else { $sourceStream = $sourceStorage->fopen($sourceInternalPath, 'r'); if (!$sourceStream) { From 888d06dff99457218a095ccc4afd98630e321ac9 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 25 Sep 2024 17:52:42 +0200 Subject: [PATCH 3/5] fix: preserve fileid when moving from objectstore to non-objectstore Signed-off-by: Robin Appelman --- apps/files_trashbin/tests/StorageTest.php | 4 ---- .../Files/ObjectStore/ObjectStoreStorage.php | 13 ++++++++++-- lib/private/Files/Storage/Common.php | 20 +++++++++++++++---- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/apps/files_trashbin/tests/StorageTest.php b/apps/files_trashbin/tests/StorageTest.php index 9de4bf0cc13fb..65e4d9a9bca0c 100644 --- a/apps/files_trashbin/tests/StorageTest.php +++ b/apps/files_trashbin/tests/StorageTest.php @@ -8,7 +8,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; @@ -673,9 +672,6 @@ public function testTrashbinCollision(): void { } public function testMoveFromStoragePreserveFileId(): void { - if (!$this->userView->getMount('')->getStorage()->instanceOfStorage(Local::class)) { - $this->markTestSkipped('Skipping on non-local users storage'); - } $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 883099931376a..ab4852f9c8526 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -38,6 +38,7 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common implements IChunkedFil private bool $handleCopiesAsOwned; protected bool $validateWrites = true; + private bool $preserveCacheItemsOnDelete = false; /** * @param array $params @@ -171,7 +172,9 @@ private function rmObjects(ICacheEntry $entry): bool { } } - $this->getCache()->remove($entry->getPath()); + if (!$this->preserveCacheItemsOnDelete) { + $this->getCache()->remove($entry->getPath()); + } return true; } @@ -206,7 +209,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; } @@ -755,4 +760,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 d78a6ca1ab35e..6fe647a6addd0 100644 --- a/lib/private/Files/Storage/Common.php +++ b/lib/private/Files/Storage/Common.php @@ -15,6 +15,7 @@ use OC\Files\Cache\Watcher; use OC\Files\FilenameValidator; use OC\Files\Filesystem; +use OC\Files\ObjectStore\ObjectStoreStorage; use OC\Files\Storage\Wrapper\Jail; use OC\Files\Storage\Wrapper\Wrapper; use OCP\Files\Cache\ICache; @@ -586,10 +587,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 f8a59b56b4791413b5ac36459e8b92ed6017b19b Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 25 Sep 2024 18:01:53 +0200 Subject: [PATCH 4/5] 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 49ff97c053a92..430dd158bc394 100644 --- a/apps/files_sharing/tests/SharedStorageTest.php +++ b/apps/files_sharing/tests/SharedStorageTest.php @@ -451,6 +451,7 @@ public function testMoveFromStorage(): void { $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 3e12e1e7898903f6ef26f21db8bc21e7b29f0868 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 26 Sep 2024 16:28:59 +0200 Subject: [PATCH 5/5] fix: rework move into object store to better preserve fileids Signed-off-by: Robin Appelman --- .../Files/ObjectStore/ObjectStoreStorage.php | 74 ++++++++++++++----- 1 file changed, 55 insertions(+), 19 deletions(-) diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index ab4852f9c8526..5f568a3aba341 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -601,32 +601,68 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t if (!$sourceCacheEntry) { $sourceCacheEntry = $sourceCache->get($sourceInternalPath); } - if ($sourceCacheEntry->getMimeType() === FileInfo::MIMETYPE_FOLDER) { - $this->mkdir($targetInternalPath); - foreach ($sourceCache->getFolderContentsById($sourceCacheEntry->getId()) as $child) { - $this->moveFromStorage($sourceStorage, $child->getPath(), $targetInternalPath . '/' . $child->getName(), $child); - } + + $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): bool { $source = $this->normalizePath($source); $target = $this->normalizePath($target);