From bb61efb875b99fff61039e657279c39a32a9439f Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 20 Jun 2024 15:57:59 +0200 Subject: [PATCH 1/4] fix: write object to the correct urn when moving from another storage to object store Signed-off-by: Robin Appelman --- .../Files/ObjectStore/ObjectStoreStorage.php | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index d0e3562797309..b7697c76de475 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -581,6 +581,31 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t return parent::copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath); } + public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath, ?ICacheEntry $sourceCacheEntry = null): bool { + $sourceCache = $sourceStorage->getCache(); + if (!$sourceCacheEntry) { + $sourceCacheEntry = $sourceCache->get($sourceInternalPath); + } + if ($sourceCacheEntry->getMimeType() === FileInfo::MIMETYPE_FOLDER) { + foreach ($sourceCache->getFolderContents($sourceInternalPath) as $child) { + $this->moveFromStorage($sourceStorage, $child->getPath(), $targetInternalPath . '/' . $child->getName()); + } + $sourceStorage->rmdir($sourceInternalPath); + } else { + // 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, $sourceStorage->fopen($sourceInternalPath, 'r'), $sourceCacheEntry->getSize()); + } catch (\Exception $e) { + // restore the cache entry + $sourceCache->moveFromCache($this->getCache(), $targetInternalPath, $sourceInternalPath); + throw $e; + } + $sourceStorage->unlink($sourceInternalPath); + } + return true; + } + public function copy($source, $target) { $source = $this->normalizePath($source); $target = $this->normalizePath($target); From 37e84521059ab98e250398473567d1e8236b529e Mon Sep 17 00:00:00 2001 From: Christoph Fiehe Date: Sat, 14 Sep 2024 23:05:12 +0200 Subject: [PATCH 2/4] perf(ObjectStoreStorage): Improve (slow) move on same object bucket This commit fixes the issue #47856. When you upload a file into a group folder and when you use a single S3 bucket as primary storage, the final move operation hangs for a long time. In the background, Nextcloud initiates a copy-delete sequence from the bucket into the bucket, with causes a lot unnecessary overhead. Nextcloud thinks that the file must be imported to another storage and does not recognize that everything is done on the same object bucket. In that case, the import step can be completely skipped, which saves time, network bandwidth and reduces the load on the object storage. The behavior improves a lot with https://github.com/nextcloud/server/pull/46013. However, there are still some put messages that are being sent to the object storage when you use an object storage as primary storage and upload files into a group folder. Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com> Signed-off-by: Christoph Fiehe --- .../Files/ObjectStore/ObjectStoreStorage.php | 5 + ...ObjectStoreStoragesDifferentBucketTest.php | 39 +++++++ .../ObjectStoreStoragesSameBucketTest.php | 31 +++++ tests/lib/Files/Storage/StoragesTest.php | 108 ++++++++++++++++++ 4 files changed, 183 insertions(+) create mode 100644 tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php create mode 100644 tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php create mode 100644 tests/lib/Files/Storage/StoragesTest.php diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index b7697c76de475..cb7be3c031a04 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -583,6 +583,11 @@ public function copyFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $targetInternalPath, ?ICacheEntry $sourceCacheEntry = null): bool { $sourceCache = $sourceStorage->getCache(); + if ($sourceStorage->instanceOfStorage(ObjectStoreStorage::class) && $sourceStorage->getObjectStore()->getStorageId() === $this->getObjectStore()->getStorageId()) { + $this->getCache()->moveFromCache($sourceCache, $sourceInternalPath, $targetInternalPath); + // Do not import any data when source and target bucket are identical. + return true; + } if (!$sourceCacheEntry) { $sourceCacheEntry = $sourceCache->get($sourceInternalPath); } diff --git a/tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php b/tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php new file mode 100644 index 0000000000000..1915460777cf0 --- /dev/null +++ b/tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php @@ -0,0 +1,39 @@ +objectStore1 = new StorageObjectStore($baseStorage1); + $config['objectstore'] = $this->objectStore1; + $this->storage1 = new ObjectStoreStorageOverwrite($config); + + $baseStorage2 = new Temporary(); + $this->objectStore2 = new StorageObjectStore($baseStorage2); + $config['objectstore'] = $this->objectStore2; + $this->storage2 = new ObjectStoreStorageOverwrite($config); + } +} diff --git a/tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php b/tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php new file mode 100644 index 0000000000000..71011451a531c --- /dev/null +++ b/tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php @@ -0,0 +1,31 @@ +objectStore = new StorageObjectStore($baseStorage); + $config['objectstore'] = $this->objectStore; + // storage1 and storage2 share the same object store. + $this->storage1 = new ObjectStoreStorageOverwrite($config); + $this->storage2 = new ObjectStoreStorageOverwrite($config); + } +} diff --git a/tests/lib/Files/Storage/StoragesTest.php b/tests/lib/Files/Storage/StoragesTest.php new file mode 100644 index 0000000000000..a7578c24e3db7 --- /dev/null +++ b/tests/lib/Files/Storage/StoragesTest.php @@ -0,0 +1,108 @@ +storage1) && is_null($this->storage2)) { + return; + } + $this->storage1->getCache()->clear(); + $this->storage2->getCache()->clear(); + + parent::tearDown(); + } + + public function testMoveFileFromStorage() { + $source = 'source.txt'; + $target = 'target.txt'; + $storage2->file_put_contents($source, 'foo'); + + $storage1->moveFromStorage($storage2, $source, $target); + + $this->assertTrue($storage1->file_exists($target), $target.' was not created'); + $this->assertFalse($storage2->file_exists($source), $source.' still exists'); + $this->assertEquals('foo', $storage1->file_get_contents($target)); + } + + public function testMoveDirectoryFromStorage() { + $storage2->mkdir('source'); + $storage2->file_put_contents('source/test1.txt', 'foo'); + $storage2->file_put_contents('source/test2.txt', 'qwerty'); + $storage2->mkdir('source/subfolder'); + $storage2->file_put_contents('source/subfolder/test.txt', 'bar'); + + $storage1->moveFromStorage($storage2, 'source', 'target'); + + $this->assertTrue($storage1->file_exists('target')); + $this->assertTrue($storage1->file_exists('target/test1.txt')); + $this->assertTrue($storage1->file_exists('target/test2.txt')); + $this->assertTrue($storage1->file_exists('target/subfolder')); + $this->assertTrue($storage1->file_exists('target/subfolder/test.txt')); + + $this->assertFalse($storage2->file_exists('source')); + $this->assertFalse($storage2->file_exists('source/test1.txt')); + $this->assertFalse($storage2->file_exists('source/test2.txt')); + $this->assertFalse($storage2->file_exists('source/subfolder')); + $this->assertFalse($storage2->file_exists('source/subfolder/test.txt')); + + $this->assertEquals('foo', $storage1->file_get_contents('target/test1.txt')); + $this->assertEquals('qwerty', $storage1->file_get_contents('target/test2.txt')); + $this->assertEquals('bar', $storage1->file_get_contents('target/subfolder/test.txt')); + } + + public function testCopyFileFromStorage() { + $source = 'source.txt'; + $target = 'target.txt'; + $storage2->file_put_contents($source, 'foo'); + + $storage1->copyFromStorage($storage2, $source, $target); + + $this->assertTrue($storage1->file_exists($target), $target.' was not created'); + $this->assertTrue($storage2->file_exists($source), $source.' was deleted'); + $this->assertEquals('foo', $storage1->file_get_contents($target)); + } + + public function testCopyDirectoryFromStorage() { + $storage2->mkdir('source'); + $storage2->file_put_contents('source/test1.txt', 'foo'); + $storage2->file_put_contents('source/test2.txt', 'qwerty'); + $storage2->mkdir('source/subfolder'); + $storage2->file_put_contents('source/subfolder/test.txt', 'bar'); + + $storage1->copyFromStorage($storage2, 'source', 'target'); + + $this->assertTrue($storage1->file_exists('target')); + $this->assertTrue($storage1->file_exists('target/test1.txt')); + $this->assertTrue($storage1->file_exists('target/test2.txt')); + $this->assertTrue($storage1->file_exists('target/subfolder')); + $this->assertTrue($storage1->file_exists('target/subfolder/test.txt')); + + $this->assertTrue($storage2->file_exists('source')); + $this->assertTrue($storage2->file_exists('source/test1.txt')); + $this->assertTrue($storage2->file_exists('source/test2.txt')); + $this->assertTrue($storage2->file_exists('source/subfolder')); + $this->assertTrue($storage2->file_exists('source/subfolder/test.txt')); + + $this->assertEquals('foo', $storage1->file_get_contents('target/test1.txt')); + $this->assertEquals('qwerty', $storage1->file_get_contents('target/test2.txt')); + $this->assertEquals('bar', $storage1->file_get_contents('target/subfolder/test.txt')); + } +} From 71c3b504bd259e1c0491ba9bec751e2ecd920640 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 17 Sep 2024 19:20:13 +0200 Subject: [PATCH 3/4] fix(tests): Fix most obvious errors in ObjectStore tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some are still failing Signed-off-by: Côme Chilliet --- ...ObjectStoreStoragesDifferentBucketTest.php | 2 + .../ObjectStoreStoragesSameBucketTest.php | 2 + tests/lib/Files/Storage/StoragesTest.php | 112 +++++++++--------- 3 files changed, 60 insertions(+), 56 deletions(-) diff --git a/tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php b/tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php index 1915460777cf0..a0e18a5557b65 100644 --- a/tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php +++ b/tests/lib/Files/ObjectStore/ObjectStoreStoragesDifferentBucketTest.php @@ -7,6 +7,8 @@ namespace Test\Files\ObjectStore; +use OC\Files\ObjectStore\StorageObjectStore; +use OC\Files\Storage\Temporary; use Test\Files\Storage\StoragesTest; /** diff --git a/tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php b/tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php index 71011451a531c..19a1f4b7bc5ad 100644 --- a/tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php +++ b/tests/lib/Files/ObjectStore/ObjectStoreStoragesSameBucketTest.php @@ -7,6 +7,8 @@ namespace Test\Files\ObjectStore; +use OC\Files\ObjectStore\StorageObjectStore; +use OC\Files\Storage\Temporary; use Test\Files\Storage\StoragesTest; /** diff --git a/tests/lib/Files/Storage/StoragesTest.php b/tests/lib/Files/Storage/StoragesTest.php index a7578c24e3db7..d157d288f2c6e 100644 --- a/tests/lib/Files/Storage/StoragesTest.php +++ b/tests/lib/Files/Storage/StoragesTest.php @@ -33,76 +33,76 @@ protected function tearDown(): void { public function testMoveFileFromStorage() { $source = 'source.txt'; $target = 'target.txt'; - $storage2->file_put_contents($source, 'foo'); + $this->storage2->file_put_contents($source, 'foo'); - $storage1->moveFromStorage($storage2, $source, $target); + $this->storage1->moveFromStorage($this->storage2, $source, $target); - $this->assertTrue($storage1->file_exists($target), $target.' was not created'); - $this->assertFalse($storage2->file_exists($source), $source.' still exists'); - $this->assertEquals('foo', $storage1->file_get_contents($target)); + $this->assertTrue($this->storage1->file_exists($target), $target.' was not created'); + $this->assertFalse($this->storage2->file_exists($source), $source.' still exists'); + $this->assertEquals('foo', $this->storage1->file_get_contents($target)); } public function testMoveDirectoryFromStorage() { - $storage2->mkdir('source'); - $storage2->file_put_contents('source/test1.txt', 'foo'); - $storage2->file_put_contents('source/test2.txt', 'qwerty'); - $storage2->mkdir('source/subfolder'); - $storage2->file_put_contents('source/subfolder/test.txt', 'bar'); - - $storage1->moveFromStorage($storage2, 'source', 'target'); - - $this->assertTrue($storage1->file_exists('target')); - $this->assertTrue($storage1->file_exists('target/test1.txt')); - $this->assertTrue($storage1->file_exists('target/test2.txt')); - $this->assertTrue($storage1->file_exists('target/subfolder')); - $this->assertTrue($storage1->file_exists('target/subfolder/test.txt')); - - $this->assertFalse($storage2->file_exists('source')); - $this->assertFalse($storage2->file_exists('source/test1.txt')); - $this->assertFalse($storage2->file_exists('source/test2.txt')); - $this->assertFalse($storage2->file_exists('source/subfolder')); - $this->assertFalse($storage2->file_exists('source/subfolder/test.txt')); - - $this->assertEquals('foo', $storage1->file_get_contents('target/test1.txt')); - $this->assertEquals('qwerty', $storage1->file_get_contents('target/test2.txt')); - $this->assertEquals('bar', $storage1->file_get_contents('target/subfolder/test.txt')); + $this->storage2->mkdir('source'); + $this->storage2->file_put_contents('source/test1.txt', 'foo'); + $this->storage2->file_put_contents('source/test2.txt', 'qwerty'); + $this->storage2->mkdir('source/subfolder'); + $this->storage2->file_put_contents('source/subfolder/test.txt', 'bar'); + + $this->storage1->moveFromStorage($this->storage2, 'source', 'target'); + + $this->assertTrue($this->storage1->file_exists('target')); + $this->assertTrue($this->storage1->file_exists('target/test1.txt')); + $this->assertTrue($this->storage1->file_exists('target/test2.txt')); + $this->assertTrue($this->storage1->file_exists('target/subfolder')); + $this->assertTrue($this->storage1->file_exists('target/subfolder/test.txt')); + + $this->assertFalse($this->storage2->file_exists('source')); + $this->assertFalse($this->storage2->file_exists('source/test1.txt')); + $this->assertFalse($this->storage2->file_exists('source/test2.txt')); + $this->assertFalse($this->storage2->file_exists('source/subfolder')); + $this->assertFalse($this->storage2->file_exists('source/subfolder/test.txt')); + + $this->assertEquals('foo', $this->storage1->file_get_contents('target/test1.txt')); + $this->assertEquals('qwerty', $this->storage1->file_get_contents('target/test2.txt')); + $this->assertEquals('bar', $this->storage1->file_get_contents('target/subfolder/test.txt')); } public function testCopyFileFromStorage() { $source = 'source.txt'; $target = 'target.txt'; - $storage2->file_put_contents($source, 'foo'); + $this->storage2->file_put_contents($source, 'foo'); - $storage1->copyFromStorage($storage2, $source, $target); + $this->storage1->copyFromStorage($this->storage2, $source, $target); - $this->assertTrue($storage1->file_exists($target), $target.' was not created'); - $this->assertTrue($storage2->file_exists($source), $source.' was deleted'); - $this->assertEquals('foo', $storage1->file_get_contents($target)); + $this->assertTrue($this->storage1->file_exists($target), $target.' was not created'); + $this->assertTrue($this->storage2->file_exists($source), $source.' was deleted'); + $this->assertEquals('foo', $this->storage1->file_get_contents($target)); } public function testCopyDirectoryFromStorage() { - $storage2->mkdir('source'); - $storage2->file_put_contents('source/test1.txt', 'foo'); - $storage2->file_put_contents('source/test2.txt', 'qwerty'); - $storage2->mkdir('source/subfolder'); - $storage2->file_put_contents('source/subfolder/test.txt', 'bar'); - - $storage1->copyFromStorage($storage2, 'source', 'target'); - - $this->assertTrue($storage1->file_exists('target')); - $this->assertTrue($storage1->file_exists('target/test1.txt')); - $this->assertTrue($storage1->file_exists('target/test2.txt')); - $this->assertTrue($storage1->file_exists('target/subfolder')); - $this->assertTrue($storage1->file_exists('target/subfolder/test.txt')); - - $this->assertTrue($storage2->file_exists('source')); - $this->assertTrue($storage2->file_exists('source/test1.txt')); - $this->assertTrue($storage2->file_exists('source/test2.txt')); - $this->assertTrue($storage2->file_exists('source/subfolder')); - $this->assertTrue($storage2->file_exists('source/subfolder/test.txt')); - - $this->assertEquals('foo', $storage1->file_get_contents('target/test1.txt')); - $this->assertEquals('qwerty', $storage1->file_get_contents('target/test2.txt')); - $this->assertEquals('bar', $storage1->file_get_contents('target/subfolder/test.txt')); + $this->storage2->mkdir('source'); + $this->storage2->file_put_contents('source/test1.txt', 'foo'); + $this->storage2->file_put_contents('source/test2.txt', 'qwerty'); + $this->storage2->mkdir('source/subfolder'); + $this->storage2->file_put_contents('source/subfolder/test.txt', 'bar'); + + $this->storage1->copyFromStorage($this->storage2, 'source', 'target'); + + $this->assertTrue($this->storage1->file_exists('target')); + $this->assertTrue($this->storage1->file_exists('target/test1.txt')); + $this->assertTrue($this->storage1->file_exists('target/test2.txt')); + $this->assertTrue($this->storage1->file_exists('target/subfolder')); + $this->assertTrue($this->storage1->file_exists('target/subfolder/test.txt')); + + $this->assertTrue($this->storage2->file_exists('source')); + $this->assertTrue($this->storage2->file_exists('source/test1.txt')); + $this->assertTrue($this->storage2->file_exists('source/test2.txt')); + $this->assertTrue($this->storage2->file_exists('source/subfolder')); + $this->assertTrue($this->storage2->file_exists('source/subfolder/test.txt')); + + $this->assertEquals('foo', $this->storage1->file_get_contents('target/test1.txt')); + $this->assertEquals('qwerty', $this->storage1->file_get_contents('target/test2.txt')); + $this->assertEquals('bar', $this->storage1->file_get_contents('target/subfolder/test.txt')); } } From 4c95ba9f408275705a5403ac3c8105d72f03f137 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 26 Sep 2024 16:28:59 +0200 Subject: [PATCH 4/4] fix: rework move into object store to better preserve fileids Signed-off-by: Robin Appelman --- .../Files/ObjectStore/ObjectStoreStorage.php | 68 +++++++++++++++---- 1 file changed, 55 insertions(+), 13 deletions(-) diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index cb7be3c031a04..4699dc5d3922c 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/lib/private/Files/ObjectStore/ObjectStoreStorage.php @@ -591,26 +591,68 @@ public function moveFromStorage(IStorage $sourceStorage, $sourceInternalPath, $t if (!$sourceCacheEntry) { $sourceCacheEntry = $sourceCache->get($sourceInternalPath); } - if ($sourceCacheEntry->getMimeType() === FileInfo::MIMETYPE_FOLDER) { - foreach ($sourceCache->getFolderContents($sourceInternalPath) as $child) { - $this->moveFromStorage($sourceStorage, $child->getPath(), $targetInternalPath . '/' . $child->getName()); - } + + $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); } else { - // 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, $sourceStorage->fopen($sourceInternalPath, 'r'), $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);