Store Fix: Removed dir on loadmeta failure #1807
Conversation
thorfour
force-pushed
the
fix/sync-blocks-cleanup
branch
from
0f26871 to
c53c6e3
Compare
If loadmeta fails during SyncBlocks, the block directory is left behind, resulting in subsequent failures to read the meta file. FailureToLoadMeta unit test Signed-off-by: Thor <[email protected]>
Signed-off-by: Thor <[email protected]>
thorfour
force-pushed
the
fix/sync-blocks-cleanup
branch
from
c53c6e3 to
dc20411
Compare
pkg/store/bucket.go
Outdated
| @@ -343,7 +343,7 @@ func (s *BucketStore) SyncBlocks(ctx context.Context) error { | |||
| bdir := path.Join(s.dir, id.String()) | |||
| meta, err := loadMeta(ctx, s.logger, s.bucket, bdir, id) | |||
| if err != nil { | |||
| return errors.Wrap(err, "load meta") | |||
| return errors.Wrap(os.RemoveAll(bdir), "load meta") | |||
There was a problem hiding this comment.
Hmm I don’t think this is quite right. This change will suppress the load meta error if the directory cleanup is successful. If we get an error loading metas, we want to return that error, not the error concerning cleanup. Cleanup should be a defer that logs any error.
There was a problem hiding this comment.
Do we want to return the error? Wouldn't that cause the Iter function to immediately abort the iteration?
There was a problem hiding this comment.
Returning the error is the current logic
There was a problem hiding this comment.
Sure.. but it would be aborting an Iter function due to a transient error (block is still being written), when it could continue on syncing the remaining blocks.
This change would bring this more in-line with the functions below it which also log the error and clean up the directory.
There was a problem hiding this comment.
I’m not necessarily against changing the logic, it simply isn’t what is done today and was not mentioned in your PR. Nevertheless, this change still suppresses the cause of the error without logging it. It also mixes up the causes of the errors. It would wrap a directory deletion error with load meta as though the deletion were the cause of failing to load the meta.
There was a problem hiding this comment.
I understand what you're saying. I've updated it to be more in-line with the functions below it. Log the error, and return the os.RemoveAll error.
Signed-off-by: Thor <[email protected]>
thorfour
force-pushed
the
fix/sync-blocks-cleanup
branch
from
3620a40 to
99bc680
Compare
There was a problem hiding this comment.
Thanks for this contribution!
resulting in subsequent failures to read the metafile since loadmeta just looks for the directory to exist.
That is not true as we check for file: if _, err := os.Stat(path.Join(dir, block.MetaFilename)); os.IsNotExist(err) { So what exactly are we fixing here?
I think this might work what you have, although blocking whole flow only if we cannot remove return os.RemoveAll(bdir) dir sounds wrong. We should handle leftover/partial directories well.
TBH I would love to work on so something mentioned here: #1335 so have a common library for fetching and maintaining metadata in both memory and disk. Probably something to do in the near future... still we can consider this PR.
|
@bwplotka you're right, just realized I had an older version of that file, that my IDE was presenting which had this instead of the line checking for the meta file. Egg on my face. Closing |
If loadmeta fails during SyncBlocks, the block directory is left behind,
resulting in subsequent failures to read the meta file since loadmeta just looks for the directory to exist.
FailureToLoadMeta unit test
Changes
Calls
os.RemoveAllon the blockdir when returning error from load metaVerification
Added unit test that checks if a sync against a block that's missing a meta.json gets removed.