WIP: kvs: call content.flush before checkpoint #6240
Conversation
chu11
force-pushed
the
issue6237_content_flush_before_checkpoint
branch
3 times, most recently
from
e1f7562
to
b019755
Compare
src/modules/kvs/kvs.c
Outdated
| if (!(f = kvs_checkpoint_commit (h, NULL, rootref, rootseq, 0, 0)) | ||
| || flux_rpc_get (f, NULL) < 0) | ||
| /* first must ensure all content is flushed */ | ||
| if (!(f1 = flux_rpc (h, "content.flush", NULL, 0, 0)) | ||
| || flux_rpc_get (f1, NULL) < 0) { | ||
| /* fallthrough to kvs_checkpoint_commit(), ENOSYS may be due | ||
| * to no backing store, but checkpoint can still be done to | ||
| * content cache. | ||
| */ | ||
| if (errno != ENOSYS) | ||
| goto error; | ||
| } | ||
|
|
||
| if (!(f2 = kvs_checkpoint_commit (h, NULL, rootref, rootseq, 0, 0)) | ||
| || flux_rpc_get (f2, NULL) < 0) | ||
| goto error; |
There was a problem hiding this comment.
Why fall through to the checkpoint commit? If there is no backing store then wouldn't it also fail?
There was a problem hiding this comment.
this admittedly surprised me, but you can checkpoint to the content cache without it going through to the backing store.
i.e. if there is no backing store, the checkpoint can succeed.
There was a problem hiding this comment.
ya know, thinking about this a bit .... do we want that behavior? Do we want checkpoints to work with the content cache and w/o a backing store. It seems like a nuanced corner case. I'm not entirely sure why it was initially supported that way. This would be for a different issue I think, as this PR solves a specific content.flush before "checkpoint" corner case.
There was a problem hiding this comment.
If there is a hook in the content cache for the checkpoint, why don't we do the cache flush there?
There was a problem hiding this comment.
Sorry I've forgotten so many details here!
There was a problem hiding this comment.
hmmm. supporting checkpoint w/o a backing module simply may have been a fallout from supporting the none backing module. #4492
perhaps checkpoint specifically should not be supported by the content cache.
There was a problem hiding this comment.
I guess it allows the kvs module to be reloaded and not lose all the data when there is no backing store?
There was a problem hiding this comment.
I guess it allows the kvs module to be reloaded and not lose all the data when there is no backing store?
I do think that's the point. But that is sort of a nuanced use case, possibly only useful in testing?
There was a problem hiding this comment.
If there is a hook in the content cache for the checkpoint, why don't we do the cache flush there?
That's not a bad idea. Lemme look into that, although it'd be for a different PR b/c FLUX_KVS_SYNC does two calls and that should probably be collapsed down into one as well.
chu11
force-pushed
the
issue6237_content_flush_before_checkpoint
branch
from
b019755
to
76e9d9f
Compare
|
making WIP, feel #6251 should come first now |
chu11
changed the title
Problem: An accidental 'd' was added to remove, making it "removed". Fix spelling.
Problem: A test in t0028-content-backing-none.t incorrectly calls checkpoint_put when it should call checkpoint_get. Fix invalid test.
Problem: The typical message unpack style is to place key names and storage pointers on the same line, but that is not done in several locations in the content and content backing modules. Correct code style to be more consistent to the rest of flux-core.
Problem: A backing store is required for content.flush but it is not required for content.checkpoint-put. This is inconsistent and can lead to checkpointing problems done the line. Require content.checkpoint-put to only work if there is a backing store available. As a consequence, remove code that handled "cached" checkpoints when a backing store is not available. Fixes flux-framework#6251
Problem: Now that the content backing store is required for checkpoints, many tests fail. Remove tests that previously assumed that checkpointing worked without a content backing store. Adjust some tests that now have an new error message.
Problem: There is no coverage to ensure that the "none" backing store works identically to when no backing store is never loaded. Add coverage in t0028-content-backing-none.t.
Problem: There is no coverage to ensure FLUX_KVS_SYNC fails when there is no longer space on disk. Add coverage to t0090-content-enospc.t.
Problem: There is no coverage to ensure FLUX_KVS_SYNC does not work if there is no backing store. Add coverage in t1010-kvs-commit-sync.t.
Problem: When the KVS module is unloaded, a checkpoint of the root reference is attempted. However, a content.flush is not done beforehand. This could result in an invalid checkpoint reference as data is not guaranteed to be flushed to the backing store. Solution: Call content.flush before checkpointing. Fixes flux-framework#6237
chu11
force-pushed
the
issue6237_content_flush_before_checkpoint
branch
from
76e9d9f
to
cb88b17
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6240 +/- ##
==========================================
- Coverage 83.36% 83.33% -0.03%
==========================================
Files 521 521
Lines 85693 85617 -76
==========================================
- Hits 71438 71351 -87
- Misses 14255 14266 +11
|
|
re-pushed, PR was built on top of #6255 and re-worked logically |
Problem: When the KVS module is unloaded, a checkpoint of the root
reference is attempted. However, a content.flush is not done
beforehand. This could result in an invalid checkpoint reference
as data is not guaranteed to be flushed to the backing store.
Solution: Call content.flush before checkpointing.
Fixes #6237
I threw in a few new tests for some extra coverage of FLUX_KVS_SYNC too.