-
Notifications
You must be signed in to change notification settings - Fork 76
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
Tiering with storage class support - TTL based worker + TMFS blocks migration #7313
Tiering with storage class support - TTL based worker + TMFS blocks migration #7313
Conversation
e9a0116
to
3d07817
Compare
be65336
to
8bf9d19
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Utkarsh-pro The structure looks good.
Notice the comment on adding a time condition to the evictor.
b225f63
to
2e513b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Utkarsh-pro
This PR is superb, but I raised several changes which are important (+ some cosmetic).
I'd love to discuss it further.
!!
@@ -222,6 +222,13 @@ export AWS_SECRET_ACCESS_KEY=$(npm run api -- account read_account '{}' --json | | |||
aws --endpoint http://localhost:6001 s3 mb s3://testbucket | |||
``` | |||
|
|||
Add additional second tier to the bucket so that TTL worker can do the cache eviction.Note that here: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ultimately we would want that tier 2 creation will be a system configuration and not require every bucket to be configured specifically. I would leave it like this for this PR, but this will follow soon.
@guymguym lemme try to summarize the proposed changes (specifically storage class) here:
Clarifications
|
@Utkarsh-pro I think you got it, just two notes:
Both TTL worker and TTF worker already call build_chunks and therefore invoke map_client, so there is no need to add such a call in the workers, only to make this call also handle the storage class move as I suggested in this comment - #7313 (comment)
Yes. For now we only handle the internal mappings. Exposing tiers to the clients is a whole new challenge. |
2e513b9
to
05db415
Compare
05db415
to
2e4f554
Compare
2e4f554
to
61264fb
Compare
61264fb
to
56fccd8
Compare
56fccd8
to
6fcaa6f
Compare
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> add evicter specific delays Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> restructure to use FSWrapper functions - ensure single fd Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> add support for last read based block eviction Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> replace naive evictor with TTL evictor Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> rename worker entities and add docs Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> add storage class construct Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix typos; add support for chunk-tier mapping in builds chunks; add tests for map_builder and map_client Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix subtle bug in compare_unordered Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> integrate storage classes completely Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix unit test Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix undefined key bug and allow running map_builder tests Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> rebase master + rename tier2 to tmfs Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
6fcaa6f
to
ec502f5
Compare
if (storage_class === 'GLACIER') { | ||
return this._move_blocks_to_glacier(block_ids); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to avoid confusion, I would change to not call these block_store_fs methods with *glacier in their name, because while the storage class is called glacier, we are translating it to TMFS. So I think I would so something like this here, and if TMFS is not enabled we should probably continue and throw the error (unsupported storage class ) because we don't have a glacier tier to move to. WDYT?
if (storage_class === 'GLACIER') { | |
return this._move_blocks_to_glacier(block_ids); | |
} | |
if (storage_class === 'GLACIER') { | |
if (config.BLOCK_STORE_FS_TMFS_ENABLED) { | |
return this._move_blocks_to_tmfs(block_ids); | |
} | |
} |
const current_attached_pools = tier_before_move.mirrors.map(mirror => mirror.spread_pools.map(pool => String(pool._id))).flat(); | ||
|
||
if ( | ||
!js_utils.compare_unordered(target_attached_pools, current_attached_pools, true) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about using _.xor() instead of creating a special function for this esoteric case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I .. did not know about xor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Utkarsh-pro All in all this is absolutely fantastic! I added couple of small comments, but it can also be addressed in next PRs. Godspeed.
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> add auto tier 2 creation & assignment to bucket if configured Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix issues Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> add auto tier 2 creation & assignment to bucket if configured Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix issues Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix docs Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> add auto tier 2 creation & assignment to bucket if configured Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix issues Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix docs Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix deepscan Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> add auto tier 2 creation & assignment to bucket if configured Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix issues Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix docs Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix deepscan Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> cleanup auto_setup_tier2 Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> add auto tier 2 creation & assignment to bucket if configured Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix issues Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix docs Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix deepscan Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> cleanup auto_setup_tier2 Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> add auto tier 2 creation & assignment to bucket if configured Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix issues Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix docs Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> fix deepscan Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> cleanup auto_setup_tier2 Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com> cleanup auto_setup_tier2 and reduce agent_blocks_reclaimer log verbosity Signed-off-by: Utkarsh Srivastava <srivastavautkarsh8097@gmail.com>
…ket-tmfs-autoconf [TMFS] Address minors from #7313 & support auto second tier creation & assignment if configured
Explain the changes
Context: NooBaa in its write flows marks an object for migration so that TMFS can migrate to another tier2 and NooBaa's read flow can recall the block from the other tier using TMFS calls but this would leave a bunch of objects on the disk (cache).
This PR adds a BG Worker which iterates over the chunks associated with a bucket (the bucket needs to have at least 2 tiers and for the case of TMFS, the second tier HAS to share the same pool) and select the chunks which haven't been read for longer than 30 mins (configurable) and move them to second tier and evict the associated the blocks.
Tested In the following way
find storage/backingstores -name '*.data' -type f | xargs -n1 -I{} sh -c "echo 'file: {} => '; xattr {}; echo"
which correctly showsuser._trigger.migrate
being set on the files.psql -d nbcore -U postgres -c "SELECT data FROM datachunks;"
shows the second tier as the tier selected fordatachunks
.