Skip to content

Commit c849725

Browse files
naveenpaul1naveenpaul1
committed
NSFS : S3 rm with --recursive option does not delete all the objects
Signed-off-by: naveenpaul1 <[email protected]>
1 parent 4379cbf commit c849725

File tree

4 files changed

+298
-33
lines changed

4 files changed

+298
-33
lines changed

‎src/sdk/namespace_fs.js

+34-30
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
'use strict';
55

66
const _ = require('lodash');
7-
const fs = require('fs');
87
const path = require('path');
98
const util = require('util');
109
const mime = require('mime');
@@ -14,6 +13,7 @@ const dbg = require('../util/debug_module')(__filename);
1413
const config = require('../../config');
1514
const crypto = require('crypto');
1615
const s3_utils = require('../endpoint/s3/s3_utils');
16+
const js_utils = require('../util/js_utils');
1717
const error_utils = require('../util/error_utils');
1818
const stream_utils = require('../util/stream_utils');
1919
const buffer_utils = require('../util/buffer_utils');
@@ -250,24 +250,6 @@ function is_sparse_file(stat) {
250250
return (stat.blocks * 512 < stat.size);
251251
}
252252

253-
/**
254-
* @param {fs.Dirent} e
255-
* @returns {string}
256-
*/
257-
function get_entry_name(e) {
258-
return e.name;
259-
}
260-
261-
/**
262-
* @param {string} name
263-
* @returns {fs.Dirent}
264-
*/
265-
function make_named_dirent(name) {
266-
const entry = new fs.Dirent();
267-
entry.name = name;
268-
return entry;
269-
}
270-
271253
function to_xattr(fs_xattr) {
272254
const xattr = _.mapKeys(fs_xattr, (val, key) => {
273255
// Prioritize ignore list
@@ -680,14 +662,30 @@ class NamespaceFS {
680662
const marker_dir = key_marker.slice(0, dir_key.length);
681663
const marker_ent = key_marker.slice(dir_key.length);
682664
// marker is after dir so no keys in this dir can apply
683-
if (dir_key < marker_dir) {
665+
if (is_first_dir_under_second_dir(dir_key, marker_dir)) {
684666
// dbg.log0(`marker is after dir so no keys in this dir can apply: dir_key=${dir_key} marker_dir=${marker_dir}`);
685667
return;
686668
}
687669
// when the dir portion of the marker is completely below the current dir
688670
// then every key in this dir satisfies the marker and marker_ent should not be used.
689-
const marker_curr = (marker_dir < dir_key) ? '' : marker_ent;
671+
const marker_curr = is_first_dir_under_second_dir(marker_dir, dir_key) ? '' : marker_ent;
690672
// dbg.log0(`process_dir: dir_key=${dir_key} prefix_ent=${prefix_ent} marker_curr=${marker_curr}`);
673+
674+
// compares dir path. returns true if dir1 is under second dir, false if otherwise
675+
// replasing slash with space to make sure secial charecters are not getting
676+
// more priory than backslash while comparing.
677+
/**
678+
* @param {string} first_dir
679+
* @param {string} second_dir
680+
*/
681+
function is_first_dir_under_second_dir(first_dir, second_dir) {
682+
// first_dir and second_dir comparison will fail when there are folder with structure slimier to
683+
// "dir_prfix.12345/" and "dir_prfix.12345.backup/" because "/" considered lesser than "." to fix this
684+
// all the backslash is replaced with space. This updated first_dir and second_dir used only for comparison.
685+
first_dir = first_dir.replaceAll('/', ' ');
686+
second_dir = second_dir.replaceAll('/', ' ');
687+
return first_dir < second_dir;
688+
}
691689
/**
692690
* @typedef {{
693691
* key: string,
@@ -696,12 +694,19 @@ class NamespaceFS {
696694
*/
697695
const insert_entry_to_results_arr = async r => {
698696
let pos;
697+
698+
const r_key_updated = r.key.replaceAll('/', ' ');
699699
// Since versions are arranged next to latest object in the latest first order,
700700
// no need to find the sorted last index. Push the ".versions/#VERSION_OBJECT" as
701701
// they are in order
702-
if (results.length && r.key < results[results.length - 1].key &&
702+
if (results.length && r_key_updated < results[results.length - 1].key &&
703703
!this._is_hidden_version_path(r.key)) {
704-
pos = _.sortedLastIndexBy(results, r, a => a.key);
704+
pos = js_utils.sortedLastIndexBy(results,
705+
curr => {
706+
const curr_key = r_key_updated.includes('/') ? curr.key : curr.key.replaceAll('/', ' ');
707+
return curr_key < r_key_updated;
708+
},
709+
);
705710
} else {
706711
pos = results.length;
707712
}
@@ -731,7 +736,7 @@ class NamespaceFS {
731736
const process_entry = async ent => {
732737
// dbg.log0('process_entry', dir_key, ent.name);
733738
if ((!ent.name.startsWith(prefix_ent) ||
734-
ent.name < marker_curr ||
739+
ent.name < marker_curr.split('/')[0] ||
735740
ent.name === this.get_bucket_tmpdir_name() ||
736741
ent.name === config.NSFS_FOLDER_OBJECT_NAME) ||
737742
this._is_hidden_version_path(ent.name)) {
@@ -774,7 +779,7 @@ class NamespaceFS {
774779
// insert dir object to objects list if its key is lexicographicly bigger than the key marker &&
775780
// no delimiter OR prefix is the current directory entry
776781
const is_dir_content = cached_dir.stat.xattr && cached_dir.stat.xattr[XATTR_DIR_CONTENT];
777-
if (is_dir_content && dir_key > key_marker && (!delimiter || dir_key === prefix)) {
782+
if (is_dir_content && is_first_dir_under_second_dir(marker_dir, dir_key) && (!delimiter || dir_key === prefix)) {
778783
const r = { key: dir_key, common_prefix: false };
779784
await insert_entry_to_results_arr(r);
780785
}
@@ -797,10 +802,9 @@ class NamespaceFS {
797802
{name: start_marker}
798803
) + 1;
799804
} else {
800-
marker_index = _.sortedLastIndexBy(
801-
sorted_entries,
802-
make_named_dirent(marker_curr),
803-
get_entry_name
805+
const marker_curr_updated = marker_curr.split('/')[0].replaceAll('/', ' ');
806+
marker_index = js_utils.sortedLastIndexBy(sorted_entries,
807+
curr => curr.name.replaceAll('/', ' ') <= marker_curr_updated,
804808
);
805809
}
806810

@@ -3360,6 +3364,7 @@ class NamespaceFS {
33603364
}
33613365
}
33623366

3367+
33633368
/** @type {PersistentLogger} */
33643369
NamespaceFS._migrate_wal = null;
33653370

@@ -3368,4 +3373,3 @@ NamespaceFS._restore_wal = null;
33683373

33693374
module.exports = NamespaceFS;
33703375
module.exports.multi_buffer_pool = multi_buffer_pool;
3371-

‎src/test/unit_tests/jest_tests/test_nsfs_list_object.test.js

+227
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,227 @@
1+
/* Copyright (C) 2016 NooBaa */
2+
/* eslint-disable no-undef */
3+
'use strict';
4+
const path = require('path');
5+
const fs_utils = require('../../../util/fs_utils');
6+
const { TMP_PATH } = require('../../system_tests/test_utils');
7+
const NamespaceFS = require('../../../sdk/namespace_fs');
8+
const buffer_utils = require('../../../util/buffer_utils');
9+
const tmp_ns_nsfs_path = path.join(TMP_PATH, 'test_nsfs_namespace_list');
10+
const nsfs_src_bkt = 'nsfs_src';
11+
const ns_nsfs_tmp_bucket_path = `${tmp_ns_nsfs_path}/${nsfs_src_bkt}`;
12+
const list_bkt = 'test_namespace_list_object';
13+
const timeout = 50000;
14+
const files_without_folders_to_upload = make_keys(264, i => `file_without_folder${i}`);
15+
const folders_to_upload = make_keys(264, i => `folder${i}/`);
16+
const files_in_folders_to_upload = make_keys(264, i => `folder1/file${i}`);
17+
const files_in_inner_folders_to_upload = make_keys(264, i => `folder1/inner_folder/file${i}`);
18+
const files_in_inner_backup_folders_to_upload = make_keys(264, i => `folder1/inner_folder.backup/file${i}`);
19+
const files_in_inner_all_backup_folders_to_upload = make_keys(264, i => `folder1/inner.folder.all.backup/file${i}`);
20+
const files_in_inner_all_folders_to_upload = make_keys(264, i => `folder1/inner_folder.all/file${i}`);
21+
const dummy_object_sdk = make_dummy_object_sdk();
22+
const ns_nsfs_tmp = new NamespaceFS({ bucket_path: ns_nsfs_tmp_bucket_path, bucket_id: '3', namespace_resource_id: undefined });
23+
let sorted_all_dir_entries;
24+
function make_dummy_object_sdk() {
25+
return {
26+
requesting_account: {
27+
force_md5_etag: false,
28+
nsfs_account_config: {
29+
uid: process.getuid(),
30+
gid: process.getgid(),
31+
}
32+
},
33+
abort_controller: new AbortController(),
34+
throw_if_aborted() {
35+
if (this.abort_controller.signal.aborted) throw new Error('request aborted signal');
36+
}
37+
};
38+
}
39+
function sort_entries_by_name(a, b) {
40+
if (a.replaceAll('/', ' ') < b.replaceAll('/', ' ')) return -1;
41+
if (a.replaceAll('/', ' ') > b.replaceAll('/', ' ')) return 1;
42+
return 0;
43+
}
44+
// eslint-disable-next-line max-lines-per-function
45+
describe('manage sorted list flow', () => {
46+
describe('list objects - pagination', () => {
47+
beforeAll(async () => {
48+
await fs_utils.create_fresh_path(ns_nsfs_tmp_bucket_path);
49+
await create_keys(list_bkt, ns_nsfs_tmp, [
50+
...files_without_folders_to_upload,
51+
...folders_to_upload,
52+
...files_in_folders_to_upload,
53+
...files_in_inner_folders_to_upload,
54+
...files_in_inner_backup_folders_to_upload,
55+
...files_in_inner_all_backup_folders_to_upload,
56+
...files_in_inner_all_folders_to_upload,
57+
]);
58+
sorted_all_dir_entries = [
59+
...files_without_folders_to_upload,
60+
...folders_to_upload,
61+
...files_in_folders_to_upload,
62+
...files_in_inner_folders_to_upload,
63+
...files_in_inner_backup_folders_to_upload,
64+
...files_in_inner_all_backup_folders_to_upload,
65+
...files_in_inner_all_folders_to_upload,
66+
];
67+
sorted_all_dir_entries.sort(sort_entries_by_name);
68+
}, timeout);
69+
afterAll(async function() {
70+
await delete_keys(list_bkt, ns_nsfs_tmp, [
71+
...folders_to_upload,
72+
...files_in_folders_to_upload,
73+
...files_without_folders_to_upload,
74+
...files_in_inner_folders_to_upload,
75+
...files_in_inner_backup_folders_to_upload,
76+
...files_in_inner_all_backup_folders_to_upload,
77+
...files_in_inner_all_folders_to_upload,
78+
]);
79+
await fs_utils.folder_delete(`${ns_nsfs_tmp_bucket_path}`);
80+
});
81+
it('page=1000', async () => {
82+
let r;
83+
let total_items = 0;
84+
let object_item_start_index = 0;
85+
for (;;) {
86+
r = await ns_nsfs_tmp.list_objects({
87+
bucket: list_bkt,
88+
key_marker: r ? r.next_marker : "",
89+
}, dummy_object_sdk);
90+
object_item_start_index = total_items;
91+
total_items += r.objects.length;
92+
await validat_pagination(r, total_items, object_item_start_index);
93+
if (!r.next_marker) {
94+
break;
95+
}
96+
}
97+
}, timeout);
98+
it('page=500', async () => {
99+
let r;
100+
let total_items = 0;
101+
let object_item_start_index = 0;
102+
for (;;) {
103+
r = await ns_nsfs_tmp.list_objects({
104+
bucket: list_bkt,
105+
limit: 500,
106+
key_marker: r ? r.next_marker : "",
107+
}, dummy_object_sdk);
108+
object_item_start_index = total_items;
109+
total_items += r.objects.length;
110+
await validat_pagination(r, total_items, object_item_start_index);
111+
if (!r.next_marker) {
112+
break;
113+
}
114+
}
115+
}, timeout);
116+
it('page=264', async () => {
117+
let r;
118+
let total_items = 0;
119+
let object_item_start_index = 0;
120+
for (;;) {
121+
r = await ns_nsfs_tmp.list_objects({
122+
bucket: list_bkt,
123+
limit: 264,
124+
key_marker: r ? r.next_marker : "",
125+
}, dummy_object_sdk);
126+
object_item_start_index = total_items;
127+
total_items += r.objects.length;
128+
await validat_pagination(r, total_items, object_item_start_index);
129+
if (!r.next_marker) {
130+
break;
131+
}
132+
}
133+
}, timeout);
134+
it('page=250', async () => {
135+
let r;
136+
let total_items = 0;
137+
let object_item_start_index = 0;
138+
for (;;) {
139+
r = await ns_nsfs_tmp.list_objects({
140+
bucket: list_bkt,
141+
limit: 250,
142+
key_marker: r ? r.next_marker : "",
143+
}, dummy_object_sdk);
144+
object_item_start_index = total_items;
145+
total_items += r.objects.length;
146+
await validat_pagination(r, total_items, object_item_start_index);
147+
if (!r.next_marker) {
148+
break;
149+
}
150+
}
151+
}, timeout);
152+
it('page=100', async () => {
153+
let r;
154+
let total_items = 0;
155+
let object_item_start_index = 0;
156+
for (;;) {
157+
r = await ns_nsfs_tmp.list_objects({
158+
bucket: list_bkt,
159+
limit: 100,
160+
key_marker: r ? r.next_marker : "",
161+
}, dummy_object_sdk);
162+
object_item_start_index = total_items;
163+
total_items += r.objects.length;
164+
await validat_pagination(r, total_items, object_item_start_index);
165+
if (!r.next_marker) {
166+
break;
167+
}
168+
}
169+
}, timeout);
170+
it('page=10', async () => {
171+
let r;
172+
let total_items = 0;
173+
let object_item_start_index = 0;
174+
for (;;) {
175+
r = await ns_nsfs_tmp.list_objects({
176+
bucket: list_bkt,
177+
limit: 10,
178+
key_marker: r ? r.next_marker : "",
179+
}, dummy_object_sdk);
180+
object_item_start_index = total_items;
181+
total_items += r.objects.length;
182+
await validat_pagination(r, total_items, object_item_start_index);
183+
if (!r.next_marker) {
184+
break;
185+
}
186+
}
187+
}, timeout);
188+
});
189+
});
190+
async function validat_pagination(r, total_items, object_item_start_index) {
191+
if (r.next_marker) {
192+
expect(r.is_truncated).toStrictEqual(true);
193+
expect(r.objects.map(it => it.key)).toEqual(sorted_all_dir_entries.slice(object_item_start_index, total_items));
194+
} else {
195+
expect(total_items).toEqual(1848);
196+
expect(r.is_truncated).toStrictEqual(false);
197+
}
198+
}
199+
/**
200+
* @param {number} count
201+
* @param {(i:number)=>string} gen
202+
* @returns {string[]}
203+
*/
204+
function make_keys(count, gen) {
205+
const arr = new Array(count);
206+
for (let i = 0; i < count; ++i) arr[i] = gen(i);
207+
arr.sort();
208+
Object.freeze(arr);
209+
return arr;
210+
}
211+
async function delete_keys(bkt, nsfs_delete_tmp, keys) {
212+
await nsfs_delete_tmp.delete_multiple_objects({
213+
bucket: bkt,
214+
objects: keys.map(key => ({ key })),
215+
}, dummy_object_sdk);
216+
}
217+
async function create_keys(bkt, nsfs_create_tmp, keys) {
218+
return Promise.all(keys.map(async key => {
219+
await nsfs_create_tmp.upload_object({
220+
bucket: bkt,
221+
key,
222+
content_type: 'application/octet-stream',
223+
source_stream: buffer_utils.buffer_to_read_stream(null),
224+
size: 0
225+
}, dummy_object_sdk);
226+
}));
227+
}

‎src/test/unit_tests/test_namespace_fs.js

+11-3
Original file line numberDiff line numberDiff line change
@@ -142,19 +142,27 @@ mocha.describe('namespace_fs', function() {
142142

143143
function assert_sorted_list(res) {
144144
let prev_key = '';
145+
const regex = /[^A-Za-z0-9]/;
145146
for (const { key } of res.objects) {
146147
if (res.next_marker) {
147148
assert(key <= res.next_marker, 'bad next_marker at key ' + key);
148149
}
149-
assert(prev_key <= key, 'objects not sorted at key ' + key);
150+
// String comparison will fail when with special character and backslash cases,
151+
// Where special characters such as dot, hyphen are listed before backslash in sorted list.
152+
// compare string only if key contains no special characters
153+
if (!regex.test(key)) {
154+
assert(prev_key <= key, 'objects not sorted at key ' + key + " prev_key: " + prev_key);
155+
}
150156
prev_key = key;
151157
}
152-
prev_key = '';
153158
for (const key of res.common_prefixes) {
154159
if (res.next_marker) {
155160
assert(key <= res.next_marker, 'next_marker at key ' + key);
156161
}
157-
assert(prev_key <= key, 'prefixes not sorted at key ' + key);
162+
163+
if (!regex.test(key)) {
164+
assert(prev_key <= key, 'prefixes not sorted at key ' + key + " prev_key: " + prev_key);
165+
}
158166
prev_key = key;
159167
}
160168
}

0 commit comments

Comments
 (0)