feat: Add PVC evictor BlockRemoved events#605
Open
albertoperdomo2 wants to merge 7 commits into
Open
Conversation
Contributor
Author
|
cc: @kfirtoledo |
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
af821f3 to
d80e670
Compare
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
guygir
requested changes
May 28, 2026
Collaborator
guygir
left a comment
There was a problem hiding this comment.
Thanks @albertoperdomo2 , overall looks good - just a few small things that need addressing.
| batch_start_time = time.time() | ||
| deleted, freed = delete_batch(batch, dry_run, logger) | ||
|
|
||
| if deleted > 0 and event_publisher is not None and cache_path is not None: |
Collaborator
There was a problem hiding this comment.
When dry_run=True, delete_batch() still returns len(file_paths) as deleted, so this block can publish BlockRemoved even though no files were removed. could we skip event publishing when dry_run is true (and/or only publish after a real delete)?
Contributor
Author
There was a problem hiding this comment.
I do not think we should publish BlockRemoved events when dry_run=True. That can cause the Go indexer to drop valid checkpoints (in the upcoming storage indexer), which is data corruption from the indexer's perspective. The event consumer shouldn't need to know about dry runs.
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
Signed-off-by: Alberto Perdomo <aperdomo@redhat.com>
This file contains hidden or 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
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.
Summary
This PR adds
BlockRemovedevent emission from the PVC evictor's deleter process, complementing theBlockStoredevents added in #571. When the evictor deletes KV cache files from shared storage, it now publishesBlockRemovedevents so downstream consumers (e.g. the Go storage indexer) can remove stale checkpoints.The implementation extends
StorageEventPublisherwith apublish_blocks_removed()method that supports per-callmodel_nameoverrides, since the evictor handles files from multiple models on the same PVC. Themodel_nameconstructor parameter is optional to just support this multi-model use case.Block hashes are extracted by reversing the
FileMapperfilename convention (16-char hex basename), and the original model name is recovered fromconfig.jsonat the FileMapper base directory. Model name lookups are cached per base directory so only one filesystem read occurs per model per process lifetime. Events are grouped by model and published after each successful batch deletion.Testing
All existing tests pass. This PR adds the new deleter tests and extends storage event tests.
Validated with:
Related Issues
Note
Test layout depends on wether #578 is merged before this one or not.