[NC | DBS3] Add support for reserved bucket tags #8967

tangledbytes · 2025-04-10T09:19:47Z

Explain the Changes

This PR adds the concept of "reserved" bucket tags which are bucket tags which can be set via noobaa cli or via put-bucket-tagging but they can come with further restrictions like a schema, immutability, etc.

DBS3 Example:

// In config-local.js or in config.json
config.NSFS_GLACIER_RESERVED_BUCKET_TAGS = {
    'deep-archive-copies': {
        schema: {
            $id: 'deep-archive-copies-schema-v0',
            enum: ['1', '2']
        }, // JSON Schema
        immutable: 'if-data',
        default: '1',
        event: true,
    }
}

Usage Example:

$ noobaa-cli bucket update --tag '[{ "key": "k1", "value": "v1" }]' ...
$ noobaa-cli bucket update --merge_tag '[{ "key": "k1", "value": "v1" }]' ...
$ aws s3api put-bucket-tagging TagSet=[{Key=deep-archive-copies,Value=2}]'

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

Doc added/updated
Tests added

config.js

src/cmd/manage_nsfs.js

shirady · 2025-04-21T12:58:47Z

src/cmd/manage_nsfs.js

@@ -256,25 +265,14 @@ async function update_bucket(data) {
 */
 async function delete_bucket(data, force) {
    try {
-        const temp_dir_name = native_fs_utils.get_bucket_tmpdir_name(data._id);
+        const bucket_empty = await _is_bucket_empty(data);


I think it is a bit confusing, suggesting to change it to:

Suggested change

const bucket_empty = await _is_bucket_empty(data);

const is_bucket_empty = await empty_bucket(data);

From what I understand the operation is to empty the bucket and return true when it ends.

src/manage_nsfs/manage_nsfs_constants.js

src/sdk/bucketspace_fs.js

shirady · 2025-04-21T13:04:11Z

src/sdk/bucketspace_fs.js

        try {
            const { name, tagging } = params;
            dbg.log0('BucketSpaceFS.put_bucket_tagging: Bucket name, tagging', name, tagging);
            const bucket = await this.config_fs.get_bucket_by_name(name);
-            bucket.tag = tagging;
+            const { ns } = await object_sdk.read_bucket_full_info(name);
+            const is_bucket_empty = await BucketSpaceFS.#_is_bucket_empty(name, null, ns, object_sdk);


Why do you pass null as the params?

Because I wanted to not provide and params in this case.

undefined vs null - this is quite opinionated. I prefer using null over undefined when I intentionally want to have a zero value. It helps me tell when the zero value was intentional and when the zero value is system generated.

I am aware that Guy disagrees with this opinion as null is an object.

src/sdk/bucketspace_fs.js

shirady · 2025-04-21T13:12:42Z

src/sdk/bucketspace_fs.js

+
+        let list;
+        try {
+            if (ns._is_versioning_disabled()) {


versioning is a property of the bucket, I think you can check if the bucket versioning is disabled in a simpler way.

noobaa-core/src/server/system_services/schemas/nsfs_bucket_schema.js

Lines 48 to 49 in 779c0bf

versioning: {

$ref: 'common_api#/definitions/versioning',

I wanted to keep the check consistent with the rest of bucketspace_fs.

shirady · 2025-04-21T13:13:54Z

src/sdk/bucketspace_fs.js

+                list = await ns.list_objects({ ...params, bucket: name, limit: 1 }, object_sdk);
+            } else {
+                list = await ns.list_object_versions({ ...params, bucket: name, limit: 1 }, object_sdk);


Is this code used more than once?
We can move it to a function and reuse it.

It is I think but don't think its worth moving these lines into a new function. This code is concise enough, wdyt?

shirady · 2025-04-21T13:18:00Z

@tangledbytes some general comments / questions:

Could you update the NC NSFS docs with the new change?
Could you add testing instructions and automated tests for the code changes? (especially on the reserved case)
Does it mean that now every bucket would have the property tag in it? Do we need to consider upgrade changes for older buckets?

guymguym · 2025-05-06T08:41:58Z

config.js

+    }
+ * }
+ */
+config.NSFS_GLACIER_RESERVED_BUCKET_TAGS = {};


IMO this structure is an overkill. Isn't a simple array of reserved tag names sufficient?

I wanted to keep it simple but at the same time I didn't want to write hard-coded things.

This structure just made sure that I don't have to hard-code few things like -

Validation for the tags - Different reserved tags will have different validation requirements.

Default values - Different tags would have different values, some might not even have one.

Immutability - Different tags would want to behave differently. Some tags might want to allow mutability, some might not and some might want to do conditionally.

Do you have a suggestion for a simpler structure but make sure that we don't have some specific code (because this generic code ain't too complicated)?

guymguym · 2025-05-06T08:45:03Z

src/sdk/bucketspace_fs.js

+     *
+     * @returns {Promise<boolean>}
+     */
+    static async #_is_bucket_empty(name, params, ns, object_sdk) {


IMO restricting visibility to private is something that should be used only for libraries and well defined API's between separate softwares, not as an internal encapsulation, which will just pollute our code with useless considerations of who should be able to see this function/variable.

guymguym · 2025-05-06T08:45:55Z

src/sdk/bucketspace_fs.js

+                continue;
+            }
+            if (!tag_info.immutable || (tag_info.immutable === 'if-data' && is_bucket_empty)) {
+                let validator = ajv.getSchema(tag_info.schema.$id);


using AJV for reserving tags is so much more complex that we should... I expected a much simpler implementation.

Signed-off-by: Utkarsh Srivastava <[email protected]> fix put_bucket_tagging test - pass dummy objectsdk Signed-off-by: Utkarsh Srivastava <[email protected]> add support for events, race safe tag manipulation and --merge_tag flag Signed-off-by: Utkarsh Srivastava <[email protected]> fix lint issues Signed-off-by: Utkarsh Srivastava <[email protected]> clarify the reserved tags config comment Signed-off-by: Utkarsh Srivastava <[email protected]> add docs for the CLI changes Signed-off-by: Utkarsh Srivastava <[email protected]> address PR comments Signed-off-by: Utkarsh Srivastava <[email protected]> add account name to the bucket create event Signed-off-by: Utkarsh Srivastava <[email protected]>

pull-request-size bot added the size/L label Apr 10, 2025

tangledbytes force-pushed the utkarsh/feat/nc-cli-bucket-tags branch 2 times, most recently from 6126797 to 779c0bf Compare April 11, 2025 12:30

tangledbytes requested review from romayalon and guymguym April 11, 2025 14:24

tangledbytes requested a review from shirady April 21, 2025 07:01

shirady reviewed Apr 21, 2025

View reviewed changes

tangledbytes force-pushed the utkarsh/feat/nc-cli-bucket-tags branch 3 times, most recently from 5cf067e to d52195c Compare April 28, 2025 08:16

guymguym reviewed May 6, 2025

View reviewed changes

tangledbytes force-pushed the utkarsh/feat/nc-cli-bucket-tags branch from d52195c to 0c76f7d Compare May 8, 2025 06:20

tangledbytes force-pushed the utkarsh/feat/nc-cli-bucket-tags branch from 0c76f7d to 4c43788 Compare May 8, 2025 06:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NC | DBS3] Add support for reserved bucket tags #8967

[NC | DBS3] Add support for reserved bucket tags #8967

tangledbytes commented Apr 10, 2025 •

edited

Loading

shirady Apr 21, 2025

shirady Apr 21, 2025

tangledbytes Apr 28, 2025

shirady Apr 21, 2025

tangledbytes Apr 28, 2025

shirady Apr 21, 2025

tangledbytes Apr 28, 2025

shirady commented Apr 21, 2025

guymguym May 6, 2025

tangledbytes May 7, 2025

guymguym May 6, 2025

guymguym May 6, 2025

	const bucket_empty = await _is_bucket_empty(data);
	const is_bucket_empty = await empty_bucket(data);

[NC | DBS3] Add support for reserved bucket tags #8967

Are you sure you want to change the base?

[NC | DBS3] Add support for reserved bucket tags #8967

Conversation

tangledbytes commented Apr 10, 2025 • edited Loading

Explain the Changes

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

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

shirady commented Apr 21, 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

tangledbytes commented Apr 10, 2025 •

edited

Loading