NC | NSFS | Bucket Logging After Delete Bucket

[shirady]

Environment info

• NooBaa Version: 5.18, current master - After merging PR NC | NSFS | Add stat to bucket_namespace_cache #8527
• Platform: NC

Actual behavior

1. When trying to send_bucket_op_logs on delete bucket operation, we would not have the bucket_info anymore in the cache; therefore we would see the printing of "Could not log bucket operation".

Expected behavior

1. Needs to be discussed what is the optimal solution - but the basic expected is not to see those logs.

Steps to reproduce

1. Deployment in NC NSFS
2. Create an account and a bucket.
3. Delete the bucket, and look for the printing of "Could not log bucket operation".

We can see it in the Ceph tests
See this comment in the PR

More information - Screenshots / Logs / Other output

[shirady]

Hi,
The main question is to understand what should happen when a bucket is deleted -
Should we create the bucket logging? should we have the bucket notifications?

noobaa-core/src/endpoint/s3/s3_bucket_logging.js

Line 22 in f924d96

const bucket_info = await req.object_sdk.read_bucket_sdk_config_info(req.params.bucket);

we don't have the config file anymore, like trying to access to a bucket that don't exist.

@aspandey @alphaprinz
cc: @romayalon @jackyalbo

Note:
In case I can add 'delete_bucket' to outer if it is a simple solution and I can do it in my PR.

    if (req.params && req.params.bucket &&
        !(req.op_name === 'put_bucket' ||
+         req.op_name === 'bucket_delete' ||
          req.op_name === 'put_bucket_notification' ||
          req.op_name === 'get_bucket_notification'
        )) {

But I must understand what you think, and better to have the GAP open than to create a wrong flow.

[alphaprinz]

Regarding notifications, deleting a bucket is not an event that triggers a notification.
(You can see the list of events here).
So your suggested fix is ok for notifications.

[aspandey]

@shirady We can not do bucket logging if bucket does not exist..so in that case we should not even log this message if we do not want such logs.
However, I would not have any issue seeing such logs (may be added info) which could always help to debug.
May be log2 and not error..

[shirady]

@aspandey the bucket exists and then the client send a request to delete it (operation is delete bucket).
Do we need to log this information?

[aspandey]

@shirady If I remember correctly, bucket logging was introduced to log bucket operation only, means, get/put/delete objects on a bucket if it is configured to log.
Operation "Delete Bucket", where we want to remove a bucket completely, does not fall in that category and should not be logged.
This is what I remember from my patch. However, @jackyalbo had also sent a detailed PR for persistent bucket logging and should also give his thoughts in case things changed.

[nimrod-becker]

BucketLogging is set on a specific bucket, if the bucket is gone, then the logging config and the need to log.... no?

[shirady]

@nimrod-becker, this is what I'm trying to understand - if we need the last log of the operation of delete-bucket (until the cache changes we could have it).
@aspandey do you know why we didn't exclude it like put-bucket? (see comment)

[romayalon]

@shirady @aspandey
I think that in general we don't skip bucket operations here except put_bucket, is there a different place where we avoid logging on bucket operations?

[shirady]

I discussed with @aspandey and suggested the following minor changes:

1. Exclude the delete_bucket from the operations as I suggested here.
2. Wrap with try_catch when trying to read the bucket info - on a non existing bucket (before the change) we also throw an error there.
3. Add a check notification part (&& bucket_info) as it might be undefined - @alphaprinz .

If you have any comments regarding the code changes please write them in the PR #8527.

regarding @romayalon's question above - as I understand there many changes, and it is something that needs to be checked. we can open an issue about this gap - related to bucket logging (not related to the Add stat to bucket_namespace_cache).

[shirady]

I'm closing the issue since we added the changes in file s3_bucket_logging.js to avoid the issue of deleting an existing bucket (and also we had before the PR an error thrown on deleting a non-existing bucket).