NSFS | versioning | calculate multipart upload version by creation time #8644

nadavMiz · 2025-01-02T09:31:46Z

Explain the changes

according to aws specifications multipart upload version creation time rather than completion time. this effects version ordering as will no have the latest version if another object was uploaded after multipart upload creation but before completion

add fs_napi function utimensat to change access time of files. see https://man7.org/linux/man-pages/man3/futimens.3p.html
change the mtime of the upload file after mp completion to have the creation time, for correct version id time
on move_to_dest_versioning add case: when the version is not latest move directly to .version and break since we don't need to handle latest version (it remains the same in this case)
add tests for both new fs function and multipart upload order

Issues: Fixed #8411

Testing Instructions:

utimensat fs function - sudo node ./node_modules/mocha/bin/mocha src/test/unit_tests/test_nb_native_fs.js
multipart upload versioning - sudo NC_CORETEST=true node ./node_modules/mocha/bin/mocha src/test/unit_tests/test_bucketspace_versioning.js

Doc added/updated
Tests added

src/native/fs/fs_napi.cpp

src/test/unit_tests/test_nb_native_fs.js

src/sdk/namespace_fs.js

src/test/unit_tests/test_bucketspace_versioning.js

shirady · 2025-01-13T15:59:23Z

src/test/unit_tests/test_bucketspace_versioning.js

+                assert.ok(exist);
+                const comp_res = await compare_version_ids(full_path, mpu_key1, res_put_object.VersionId, res.VersionId);
+                assert.ok(comp_res);
+            });


Could you add an assert so we can see that res_put_object version_id is the latest explicitly?

compare_version_ids does exactly that

I agree with Shira it's better to call headObject and check that the latest version is the single put version and not the MPU

src/test/unit_tests/test_bucketspace_versioning.js

shirady · 2025-01-13T16:05:53Z

@nadavMiz I'm adding a TODO here so we will not forget - run the test on MAC as well.

Signed-off-by: nadav mizrahi <[email protected]>

src/sdk/namespace_fs.js

romayalon · 2025-01-14T13:13:18Z

src/sdk/namespace_fs.js

+            const create_object_upload_stat = await nb_native().fs.stat(fs_context, create_object_upload_path);
+            //according to aws, multipart upload version time should be calculated based on creation rather then completion time.
+            //change the files mtime to match the creation time
+            await nb_native().fs.utimensat(fs_context, upload_path, {modtime: create_object_upload_stat.mtimeNsBigint});


do you think we should fail the whole upload in case we were not able to set the mtime?

I guess we can use the existing time in case it fails

@guymguym WDYT? I'm not sure, since it might create a wrong .versions/ stack order, but on the other hand we probably don't want to fail on that

src/sdk/namespace_fs.js

romayalon · 2025-01-14T13:55:51Z

src/test/unit_tests/test_bucketspace_versioning.js

@@ -407,7 +407,54 @@ mocha.describe('bucketspace namespace_fs - versioning', function() {
                const exist = await version_file_exists(full_path, mpu_key1, '', prev_version_id);
                assert.ok(exist);
            });
+
+            mocha.it('mpu object - versioning enabled - put object before running mpu complete. mpu version should move to .versions dir and not latest', async function() {


worth adding a test for null version

romayalon · 2025-01-14T13:56:52Z

src/sdk/nb.d.ts

@@ -953,6 +953,10 @@ interface NativeFS {
    checkAccess(fs_context: NativeFSContext, path: string): Promise<void>;
    getsinglexattr(fs_context: NativeFSContext, path: string, key: string): Promise<string>;
    getpwname(fs_context: NativeFSContext, user: string): Promise<NativeFSUserObject>;
+    utimensat(fs_context: NativeFSContext, path: string, times: {
+        modtime?: bigint,


why not use the conventional mtime and atime?

modtime and actime are the names used by utime. https://man7.org/linux/man-pages/man2/utime.2.html. utimensat uses an array instead of a struct so it doesn't give them a name, but I decided to use the same naming. I can change them to fit with the names used in our code

so you can choose either, mtime and atime used in our code because of the node.js terms

romayalon · 2025-01-14T14:01:04Z

src/native/fs/fs_napi.cpp

+        SYSCALL_OR_RETURN(utimensat(AT_FDCWD, _path.c_str(), _times, 0));
+    }
+
+   void get_time_from_info(const Napi::Object& napi_times, const std::string& time_type, struct timespec& timeval, std::string& log_str) {


pls move helper functions to not be in the middle of the workers

get_time_from_info is only relevant to utimensat, so I added it as a member function to the class. where would you propose to move it?

I saw helper functions both at the top and at the bottom of this file, so check what you think is the best location

romayalon · 2025-01-14T14:07:23Z

src/native/fs/fs_napi.cpp

+            timeval.tv_nsec = time % int64_t(1e9);
+            log_str = std::to_string(time);
+        } else {
+            timeval.tv_nsec = UTIME_OMIT;


when will this happen? was this flow tested?

this will happen when one of the times is undefined. it means the time will not be changed. it is tested in Utimensat - change only mtime

but in namespaceFS we always take a defined mtime, I mean, I don't see how we can reach this code from our flow

this function is used both by mtime and atime. currently we always get here for atime (I can also see this in the logs). also I think its good to leave an option to just change the atime and not the mtime, in case we want to use this function elsewhere

got you, so for atime this flag will just keep it as it is since we pass only mtime

romayalon · 2025-01-14T14:18:05Z

src/native/fs/fs_napi.cpp

+    }
+    virtual void Work()
+    {
+        SYSCALL_OR_RETURN(utimensat(AT_FDCWD, _path.c_str(), _times, 0));


does mac have utimensat() function and same behavior?

its hard to know for sure as I don't have mac. but from what I found its available from osx 10.13. not sure what version of mac we start to support from.

please always make sure you are not breaking native build for the developers on mac, I sent you on slack the man utimensat

right, than it looks like it works on mac. also I should have mentioned it before: utimensat is defined in POSIX, so it should work on mac the same. even though you can never know. We will test this on mac regardless before pushing

nadavMiz requested review from guymguym and romayalon January 2, 2025 09:31

pull-request-size bot added the size/L label Jan 2, 2025

nadavMiz force-pushed the multipart-version-ordering branch 6 times, most recently from 12445b6 to 2c10bc0 Compare January 5, 2025 05:25

nadavMiz requested review from shirady and naveenpaul1 January 7, 2025 11:58

nadavMiz self-assigned this Jan 8, 2025

nadavMiz force-pushed the multipart-version-ordering branch from 2c10bc0 to b3b9a42 Compare January 8, 2025 12:29

shirady reviewed Jan 13, 2025

View reviewed changes

nadavMiz force-pushed the multipart-version-ordering branch from b3b9a42 to cc844df Compare January 14, 2025 09:20

NSFS | versioning | calculate multipart upload version by creation time

7d5bf69

Signed-off-by: nadav mizrahi <[email protected]>

nadavMiz force-pushed the multipart-version-ordering branch from cc844df to 7d5bf69 Compare January 14, 2025 09:33

romayalon reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NSFS | versioning | calculate multipart upload version by creation time #8644

NSFS | versioning | calculate multipart upload version by creation time #8644

nadavMiz commented Jan 2, 2025 •

edited

Loading

shirady Jan 13, 2025

nadavMiz Jan 14, 2025

romayalon Jan 14, 2025

shirady commented Jan 13, 2025

romayalon Jan 14, 2025

nadavMiz Jan 14, 2025

romayalon Jan 15, 2025

romayalon Jan 14, 2025

romayalon Jan 14, 2025

nadavMiz Jan 14, 2025

romayalon Jan 16, 2025

romayalon Jan 14, 2025

nadavMiz Jan 14, 2025 •

edited

Loading

romayalon Jan 16, 2025

romayalon Jan 14, 2025

nadavMiz Jan 14, 2025 •

edited

Loading

romayalon Jan 15, 2025

nadavMiz Jan 16, 2025

romayalon Jan 16, 2025

romayalon Jan 14, 2025

nadavMiz Jan 14, 2025 •

edited

Loading

romayalon Jan 15, 2025

nadavMiz Jan 16, 2025

NSFS | versioning | calculate multipart upload version by creation time #8644

Are you sure you want to change the base?

NSFS | versioning | calculate multipart upload version by creation time #8644

Conversation

nadavMiz commented Jan 2, 2025 • edited Loading

Explain the changes

Issues: Fixed #8411

Testing Instructions:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shirady commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadavMiz Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadavMiz Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadavMiz Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nadavMiz commented Jan 2, 2025 •

edited

Loading

nadavMiz Jan 14, 2025 •

edited

Loading

nadavMiz Jan 14, 2025 •

edited

Loading

nadavMiz Jan 14, 2025 •

edited

Loading