Repository navigation
feat(server): disk usage accounting - #4488
andreinnanu wants to merge 13 commits into
Conversation
|
Thanks for the PR. It is labeled Slash commands (own line, regular comment) move it around the queue:
See CONTRIBUTING.md for details. |
|
Thanks @andreinnanu for this. @hubcio, can you answer the open question on freezing the sampler for the burst test? |
|
@andreinnanu right, the sampler interval is capped at 1 s whatever split the counter instead:
the burst test then compares the counter delta with the also run it with |
|
/skill team-review-slim |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary: disk_used_bytes overstates usage by the bytes written between samples when a topic sets preallocate_segments, because the segment reservation and every write into it both charge the same blocks; the review also flags an unused harness helper and a redundant length sum. A reported #[serial] isolation gap in the new tests was dropped as unproven.
Counts: critical 0, warning 1, nit 0, simplification 2
This review was generated by Claude Code 2.1.284 on deepseek-flash[1m]. Review the output before you act on it.
github-actions
Bot
added
S-waiting-on-author
|
Tested by manually freezing the sampler interval (adding a big extra sleep in the binary for testing purposes): Test scripts:
Results:
Because of different frees that are not taken into account, the overcount has the tendency to grow between samples, when it gets reseted. Is this something we should consider at this point? Also, the large overcounts are usually spikes created by the measuring script, the diff looks like this over time: |
|
@andreinnanu i did a quite a big change to server and all related crates in PR #4561. could you please rebase this PR? |
Yes, i can take care of that. Rebasing with current master should be good enough, right @hubcio? |
|
yes @andreinnanu |
61da1db to
f3d01a9
Compare
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (30.39%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #4488 +/- ##
============================================
+ Coverage 91.03% 91.04% +0.01%
+ Complexity 3877 3876 -1
============================================
Files 1561 1562 +1
Lines 403726 404216 +490
Branches 337601 338091 +490
============================================
+ Hits 367518 368022 +504
+ Misses 28849 28821 -28
- Partials 7359 7373 +14
🚀 New features to boost your workflow:
|
rebased |
|
/ready |
github-actions
Bot
added
S-waiting-on-review
|
could you please rebase this PR? |
Keep disk accounting compatible with the updated base. Preserve master's staging-file replacement safety and metric label escaping, alongside the disk usage collectors and test isolation. Forward allocation charges through master's WAL storage adapter so workspace and simulator-enabled builds keep charging disk writes.
hubcio
left a comment
hubcio
left a comment
There was a problem hiding this comment.
the main open points are the btrfs/openzfs statvfs lag after rebase, writes left uncharged when fallocate fails or does nothing, and the silent no-op charge_allocation default.
| pub fn rebase<T>(&self, measure: impl FnOnce() -> Option<(T, u64)>) -> Option<T> { | ||
| // Pairs with the release in `Charge::drop`: every write settled here | ||
| // finished before the volume is measured. | ||
| let settled = self.settled.load(Ordering::Acquire); |
There was a problem hiding this comment.
warning: btrfs and openzfs move f_bfree only at the next commit (30 s on btrfs), so after rebase drops settled charges the gauge reads low until then. keep settled charges past a commit on those filesystems, or document the ext4/xfs assumption.
There was a problem hiding this comment.
I did not consider multiple types of filesystems. Is there an official list that Iggy supports?
I tried to look in the docs, but no luck so far.
I would like to cover all the supported ones.
There was a problem hiding this comment.
just stick to popular ones - ext4/btrfx/xfs/zfs
There was a problem hiding this comment.
After working on it for a while, I chose to document the assumption and fix it in a future issue.
Should I write it?
| #[inline] | ||
| pub fn charge(&self, bytes: u64) -> Charge<'_> { | ||
| if bytes > 0 { | ||
| self.charged.fetch_add(bytes, Ordering::Relaxed); |
There was a problem hiding this comment.
nit: all shards bump the same two global atomics on every charge, so these cache lines bounce between cores. profile a many-shard small-batch run before deciding whether per-shard slots are needed.
There was a problem hiding this comment.
Yes, this is one of the things I was trying to avoid with the initial proposal.
I will do the profiling and come back with an answer.
There was a problem hiding this comment.
sure - thats why i treat it as a nit. i dont think there will be visible impact until we hit 10M+++ calls/s
Which issue does this PR address?
Relates to #4140
This PR alone doesn't solve the issue. A following one will.
Rationale
Enables accounting disk usage, preparing the ground for the configurable disk usage limit discussed within #4140.
As agreed there, this is the first of two PRs: accounting here, the limit and the refusal of new messages in the follow-up.
What changed?
Iggy had no view of how full its data volume is, so the disk usage limit planned in #4140 had nothing to check against.
A periodic
statvfssample alone is not enough: a burst can fill the remaining space between two samples.A process-wide estimate (
DISK_USAGE) of the bytes used on the data volume was added. The write sites charge what they allocate into a total the sampler never touches, and the existingiggy-system-statssampler re-bases the estimate onstatvfsevery second, so writes on the volume that Iggy does not control show up with the next sample. Used means total minus free blocks, asdfreports it (/statsfree isf_bavail, so the two differ by the root reserve).Preallocated segments charge only what the reservation newly allocates, and writes into a segment are charged only past what its reservation actually covers, measured by the file's
st_blocks. A reservation that fails, is unsupported, or succeeds without allocating (OpenZFS) covers nothing, so its writes are charged.The write sites charge what they allocate, and the existing
iggy-system-statssampler seeds the estimate at boot and re-bases it fromstatvfs, so writes on the data volume which are not under Iggy's control are seen with every sample.Two series are exported on
/metrics:disk_used_bytes(gauge): the estimated bytes used on the data volume, as total minus free blocks likedf. Not exported until the first sample.disk_allocated_bytes_total(counter): the bytes Iggy allocated on the data volume since the process started. Samples never move it, so its increase over an interval shows what Iggy wrote apart from everything else on the volume.The estimate errs towards a fuller disk: frees (deleted segments, truncations) are seen only at the next sample, and a failed write or reservation stays counted until then. It assumes
statvfsshows a finished write right away, as on ext4 and XFS. On btrfs and OpenZFS it can read low for up to one commit interval after a sample;Open question based on #4140 discussion
How do we want to approach freezing the sampler?
The goal was to add a burst test with the sampler frozen, but currently there is no mechanism for that. Reusing
sysinfo_print_intervaldoesn't work because its minimum value is 1s.Local Execution
disk_used_bytesgauge and its value and followsdfwhile a producer writes messagesAI Usage