nsfs command: simple auth + fix forks flag + fix read_account perf + warmup before buffer pool reads #7307
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
guymguym
requested review from
jackyalbo,
tangledbytes,
dannyzaken,
liranmauda and
romayalon
liranmauda
approved these changes
Jun 11, 2023
guymguym
force-pushed
the
guy-p6
branch
4 times, most recently
from
1c41453
to
02043c6
Compare
guymguym
commented
Jun 15, 2023
| @@ -216,7 +217,7 @@ async function authorize_request_policy(req) { | |||
| return; | |||
| } | |||
|
|
|||
| const account = await req.object_sdk.rpc_client.account.read_account({}); | |||
| const account = req.object_sdk.requesting_account; | |||
There was a problem hiding this comment.
@jackyalbo @romayalon FYI - this is the fix we discussed, instead of read_account, it reuses the same account_cache that object_sdk uses to check the signatures.
There was a problem hiding this comment.
by default AFM will fetch the 4M block that includes this byte, so if you need 8M we need to set the chunck size for AFM to 12M, the other option is to set the offset to the first byte of the 4M block, in that case we can set the chunck size to 8M, this assume the block size is 4M and that NooBaa is checking for the block size.
guymguym
commented
Jun 15, 2023
src/sdk/namespace_fs.js
Outdated
Comment on lines
817
to
830
| // Our buffer pool keeps large buffers and we want to avoid spending | ||
| // all our large buffers and then have them waiting for high latency calls | ||
| // such as reading from archive/on-demand cache files. | ||
| // Instead, we detect the case where a file is "sparse", | ||
| // and then use just a small buffer to wait for a tiny read, | ||
| // which will recall the file from archive or load from remote into cache, | ||
| // and once it returns we can continue to the full fledged read. | ||
| if (is_sparse_file(stat)) { | ||
| dbg.log0('NamespaceFS: read_object_stream - warmup sparse file', { | ||
| file_path, pos, size: stat.size, blocks: stat.blocks, | ||
| }); | ||
| await file.read(fs_context, this.warmup_buffer, 0, 1, pos); | ||
| } | ||
|
|
There was a problem hiding this comment.
@eshelmarc This is the logic to improve buffer pool usage -
I only read 1 byte on the current pos, so it assumes that prefetching will get a whole 8MB.
…warmup before buffer pool reads 1. Add simple auth with single access_key secret_key to nsfs command. Previously the nsfs command had no authentication. This adds a single auth for single-tenancy which is the first step. Next the plan is to follow with multi-tenant configuration from a config file, and then integrate with user mapping DBs such as AD. 2. Add --forks flag option (only had an env ENDPOINTS_FORKS option so far), and also fix a bug that prevented from forks to work properly (primary had to return and not listen on same ports). 3. Fix performance regression for small requests caused by read_account() being called every request by authorize_request_policy. Instead, use the accounts_cache which we already maintained for the signature checking. (CC @jackyalbo @romayalon). 4. NSFS reads into warmup buffer to prefetch archived/cached files which are identified by being sparse (CC @eshelmarc) Signed-off-by: Guy Margalit <guymguym@gmail.com>
guymguym
changed the title
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explain the changes
Add simple auth with single access_key secret_key to nsfs command. Previously the nsfs command had no authentication. This adds a single auth for single-tenancy which is the first step. Next the plan is to follow with multi-tenant configuration from a config file, and then integrate with user mapping DBs such as AD.
Add --forks flag option (only had an env ENDPOINTS_FORKS option so far), and also fix a bug that prevented from forks to work properly (primary had to return and not listen on same ports).
Fix performance regression for small requests caused by read_account() being called every request by authorize_request_policy. Instead, use the accounts_cache which we already maintained for the signature checking. (CC @jackyalbo @romayalon).
NSFS reads into warmup buffer to prefetch archived/cached files which are identified by being sparse (CC @eshelmarc)
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
npm -- run nsfs .npm -- run nsfs --access_key 123 --secret_key XXX .npm -- run nsfs --forks 4 .