Skip to content

NC | lifecycle | add notifications for lifecycle expire #8883

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 20, 2025

Conversation

nadavMiz
Copy link
Contributor

@nadavMiz nadavMiz commented Mar 19, 2025

Describe the Problem

Explain the Changes

  1. add notifications handling for nc lifecycle based on containerized lifecycle notifications handling
  2. merged some of the notifications for lifecycle into notifications_util

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

manual testing:

  1. create new bucket: s3api create-bucket --bucket test_bucket
  2. create a new connection nb connection add --name conn --notification_protocol http
  3. create notifications json:
{
    "TopicConfigurations": [
        {
            "Id": "notifications",
            "TopicArn": "conn",
            "Events": [
                "s3:LifecycleExpiration:Delete",
                "s3:LifecycleExpiration:DeleteMarkerCreated"
            ]
        }
    ]
}
  1. add notifications configuration to the bucket: s3api put-bucket-notification-configuration --bucket test-bucket --notification-configuration file://notifications.json
  2. create lifecycle policy. for example this policy delete all objects:
{
    "Rules": [
        {
            "ID": "Transition and Expiration Rule",
            "Status": "Enabled",
            "Filter": {
                "Prefix": ""
            },
            "Expiration": {
                "Date": "2022-07-12"
            }
        }
    ]
}
  1. apply lifecycle policy to the bucket: s3api put-bucket-lifecycle-configuration --bucket test-bucket --lifecycle-configuration file://lifecycle.json
  2. add objects to the bucket
  3. run lifecycle cli: sudo node src/cmd/manage_nsfs lifecycle --disable_service_validation true --disable_runtime_validation true
  4. delete notifications should be in the permanent log file. can be configured as env variable NOTIFICATION_LOG_DIR or set NOTIFICATION_LOG_DIR in config.js
  5. to check notifications for delete marker, enable versioning on the bucket: s3api put-bucket-versioning --bucket test-bucket --versioning-configuration Status=Enabled, before running lifecycle
  • Doc added/updated
  • Tests added

Copy link
Contributor

@romayalon romayalon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
added comments about minor things we talked about

bucket: bucket_json.name,
objects: candidates.delete_candidates
})
op_func: async () =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't forget to move the _call_op_and_update_status() inside delete_objects_and_send_notifications()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead I created a function to send notifications and kept the delete function as it was before the change.

});

const writes = [];
for (const deleted_obj of res) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't forget merge logic with lifecycle.js

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alphaprinz is currently modifying the lifecycle.js code. will leave it separate for now. we can merge it in a different PR

@nadavMiz nadavMiz force-pushed the lifecycle-notifications branch from 281c22b to 75ce45e Compare March 20, 2025 11:35
@nadavMiz nadavMiz requested a review from alphaprinz March 20, 2025 15:04
@nadavMiz nadavMiz force-pushed the lifecycle-notifications branch from 75ce45e to 1b797af Compare March 20, 2025 16:45
@nadavMiz nadavMiz self-assigned this Mar 20, 2025
@nadavMiz nadavMiz merged commit 6f516e5 into noobaa:master Mar 20, 2025
11 of 12 checks passed
@nadavMiz nadavMiz deleted the lifecycle-notifications branch March 20, 2025 17:19
@nadavMiz nadavMiz mentioned this pull request Mar 27, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants