NC | lifecycle | fix notifications #8913

nadavMiz · 2025-03-27T14:20:42Z

Describe the Problem

unlike the object server namespace_fs doesn't return object_info as part of the delete_multiple_objects result. object_info is needed to compose delete notification. change to return the entire object_info for the delete candidates object instead of just the key. then combine the object info with the result of delete_multiple_objects to create a new delete_object to pass to create_notification_delete_object. the implementation assumes that delete_multiple_objects return list has the same object order as the object list it got as an input.(same assumption is done in s3_post_bucket_delete)
also unlike the bucket object in containerized lifecycle, bucket_json does not contain the account name. added the account name from object sdk to the bucket_json before passing it to create_notification_delete_object.

Explain the Changes

return object_info instead of just key for expire delete candidates
create deleted_obj object that contain both object_info and info from delete_multiple_objects result and pass it to create_notification_delete_object
use bucket owner instead of root user as the object sdk account. (this is from NC | lifecycle | add expire non current and expire delete marker rules #8872 but its needed here as well)
add account name from object_sdk as bucket_owner in bucket_json before passing it to create_notification_delete_object

Issues

Fixes issues with #8883

####GAPS

lifecycle and nc lifecycle notification are similer, should have a joint function in notifations_util and call it from both
modifying bucket_json before passing it to create_notification_delete_object is a bit hacky. should probably modify create_notification_delete_object to account for different structure. should probably be done when combining the code

Testing Instructions:

sudo npx jest test_nc_lifecycle_cli

Doc added/updated
Tests added

romayalon · 2025-03-30T09:37:26Z

src/manage_nsfs/nc_lifecycle.js

@@ -161,7 +161,11 @@ async function process_buckets(config_fs, bucket_names, system_json) {
 */
 async function process_bucket(config_fs, bucket_name, system_json) {
    const bucket_json = await config_fs.get_bucket_by_name(bucket_name, config_fs_options);
-    const account = { email: '', nsfs_account_config: config_fs.fs_context, access_keys: [] };
+    const account = await config_fs.get_identity_by_id(bucket_json.owner_account, undefined, {silent_if_missing: true});


type should be account so it'll be backwards compatible

changed the type to account

romayalon · 2025-03-30T09:37:49Z

src/manage_nsfs/nc_lifecycle.js

@@ -210,23 +214,31 @@ async function process_rules(config_fs, bucket_json, object_sdk, should_notify)
    }
 }

-async function send_lifecycle_notifications(delete_res, bucket_json, object_sdk) {
+function create_notification_delete_object(delete_res, delete_obj_info) {


please move to notifications util section

please add jsdoc

do we need to pass the version id of the delete marker in the notification as well? what about deleted_delete_marker? will we need this info?

right its not part of the object_info because we get the object info of the object to delete. I can pass it from the res. there are many nuances I am adding tests now

src/manage_nsfs/nc_lifecycle.js

romayalon · 2025-03-30T09:47:36Z

src/manage_nsfs/nc_lifecycle.js

            })) {
+                const deleted_obj = create_notification_delete_object(delete_res[i], delete_candidates[i]);


are you sure that the delete_res[i] and delete_candidates[i] are the same object?

they should match. it is what we do in s3_post_bucket_delete. the alternative is to modify the return object of of delete_multiple_objects to be like in containerized. for this PR I wanted to match as much as possible to s3_post_bucket_delete. as I don't know why this way was decided. currently it shouldn't matter as we can't have both delete marker and regular delete at the same time. but I would still validate this

alphaprinz

Had some comments, otherwise LGTM

alphaprinz · 2025-03-31T20:01:09Z

src/manage_nsfs/nc_lifecycle.js

+ * @param {Object} bucket_json
+ * @param {Object} object_sdk
+ * @returns {Promise<Void>}
+ * NOTE implementation assumes delete_candidates and delete_res uses the same index. this assumtion is also made in


typo - assumption

alphaprinz · 2025-03-31T20:03:21Z

src/test/unit_tests/jest_tests/test_nc_lifecycle_cli.test.js

@@ -255,8 +270,8 @@ describe('noobaa cli - lifecycle', () => {
            }
        ];
        await bucketspace_fs.set_bucket_lifecycle_configuration_rules({ name: test_bucket, rules: lifecycle_rule });
-        create_object(test_bucket, test_key1, 100, true);
-        create_object(test_bucket, test_key2, 100, false);
+        create_object_lifecycle(test_bucket, test_key1, 100, true);


create_object_lifecycle is async. Don't you need an await when you call it?

src/manage_nsfs/nc_lifecycle.js

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

nadavMiz requested review from alphaprinz and romayalon March 27, 2025 14:20

pull-request-size bot added the size/S label Mar 27, 2025

nadavMiz force-pushed the nc-lifecycle-notifications-fix branch from 5c3f0e4 to d0a8d3d Compare March 27, 2025 14:21

romayalon reviewed Mar 30, 2025

View reviewed changes

nadavMiz force-pushed the nc-lifecycle-notifications-fix branch from d0a8d3d to 07dc57f Compare March 31, 2025 11:39

pull-request-size bot added size/L and removed size/S labels Mar 31, 2025

nadavMiz force-pushed the nc-lifecycle-notifications-fix branch from 07dc57f to 6ff5ce6 Compare March 31, 2025 11:47

nadavMiz requested a review from romayalon March 31, 2025 11:48

nadavMiz force-pushed the nc-lifecycle-notifications-fix branch from 6ff5ce6 to 9903b56 Compare March 31, 2025 11:59

alphaprinz reviewed Mar 31, 2025

View reviewed changes

nadavMiz self-assigned this Apr 1, 2025

nadavMiz force-pushed the nc-lifecycle-notifications-fix branch from 9903b56 to 8e6c4f2 Compare April 1, 2025 07:16

romayalon approved these changes Apr 1, 2025

View reviewed changes

src/manage_nsfs/nc_lifecycle.js Outdated Show resolved Hide resolved

nadavMiz force-pushed the nc-lifecycle-notifications-fix branch from 8e6c4f2 to 66f619b Compare April 1, 2025 09:11

NC | lifecycle | fix notifications

10e1156

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

nadavMiz force-pushed the nc-lifecycle-notifications-fix branch from 66f619b to 10e1156 Compare April 1, 2025 10:03

nadavMiz merged commit 04d2ead into noobaa:master Apr 1, 2025
11 checks passed

romayalon mentioned this pull request May 15, 2025

NC | 5.18.4 backports #9027

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

NC | lifecycle | fix notifications #8913

NC | lifecycle | fix notifications #8913

Uh oh!

nadavMiz commented Mar 27, 2025 •

edited

Loading

Uh oh!

romayalon Mar 30, 2025

Uh oh!

nadavMiz Mar 31, 2025

Uh oh!

romayalon Mar 30, 2025

Uh oh!

romayalon Mar 30, 2025

Uh oh!

nadavMiz Mar 31, 2025

Uh oh!

Uh oh!

romayalon Mar 30, 2025

Uh oh!

nadavMiz Mar 31, 2025

Uh oh!

alphaprinz left a comment

Uh oh!

alphaprinz Mar 31, 2025

Uh oh!

alphaprinz Mar 31, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		})) {
		const deleted_obj = create_notification_delete_object(delete_res[i], delete_candidates[i]);

NC | lifecycle | fix notifications #8913

NC | lifecycle | fix notifications #8913

Uh oh!

Conversation

nadavMiz commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Describe the Problem

Explain the Changes

Issues

Testing Instructions:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alphaprinz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nadavMiz commented Mar 27, 2025 •

edited

Loading