atelet: measure what a snapshot image costs, not how long it is - #931
Open
Chenyi Wang (chw120) wants to merge 2 commits into
Open
atelet: measure what a snapshot image costs, not how long it is#931Chenyi Wang (chw120) wants to merge 2 commits into
Chenyi Wang (chw120) wants to merge 2 commits into
Conversation
atelet_snapshot_size_bytes read fi.Size() for every image it recorded. For the gVisor images that is right, but the micro-VM memory image is sparse: cloud-hypervisor creates memory-ranges at the guest's full RAM size and writes only the pages the guest ever touched. Its length is therefore a constant — the VM's configured memory — so the metric reported the same number for a guest that had just booted and one that had filled its RAM. On a live counter actor it read 2147483648 for a file that occupies 163577856 bytes: the configured 2048 MiB, every time, on every actor. Record st_blocks*512 instead, which is what the filesystem actually gave the file. Sparse or not, that is the answer to "what did this checkpoint cost", and it is the figure the snapshot's transfer time and storage bill follow. The count rounds up to a block, so the small JSON images beside the memory one now over-report by under 4 KiB. That is noise on a histogram whose first bucket boundary is 1 MB, and the gVisor images are dense, so for them the two readings agree to within that rounding. docs/observability.md described this metric as the uncompressed size of a gVisor image, which it was back when gVisor was the only sandbox. It now says what is recorded, and why the two sandboxes read differently. The existing test helper of the same name is now diskUsage, so the hole-preservation tests keep statting by path rather than going through the code they would otherwise be validating.
Collaborator
Author
|
The previous snapshot size metric did not consider the micro-VM case, so this update ensures both scenarios are supported. Please let me know if this approach is correct. Thanks. cc: Benjamin Elder (@BenTheElder) Max Smythe (@maxsmythe) Haven Xia (@HavenXia) |
Krisztian F (krisztianfekete)
left a comment
Krisztian F (krisztianfekete)
left a comment
Collaborator
There was a problem hiding this comment.
Thanks, added a few comments, but looks good overall!
We should probably also wire atelet.snapshot.size to PlatformMetricPrefixes e2e tests, it can also be a follow-up, though.
Collaborator
There was a problem hiding this comment.
Can we test with something less compressible?
| "atelet.snapshot.size", | ||
| metric.WithUnit("By"), | ||
| metric.WithDescription("Uncompressed size in bytes of each gVisor snapshot image written during checkpoint."), | ||
| metric.WithDescription("On-disk size in bytes of each snapshot image written during checkpoint: the blocks the filesystem allocated, not the length the file reports. The two differ for the micro-VM memory image, which is sparse — see allocatedBytes."), |
Collaborator
There was a problem hiding this comment.
This string here will become the HELP text of the metric and will show up at lots of places, where users cannot look up the Go function. Can you please shorten it to one sentence and move the explanation to docs/observability.md?
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
2 participants
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.
atelet_snapshot_size_bytes read fi.Size() for every image it recorded. For the gVisor images that is right, but the micro-VM memory image is sparse: cloud-hypervisor creates memory-ranges at the guest's full RAM size and writes only the pages the guest ever touched. Its length is therefore a constant — the VM's configured memory — so the metric reported the same number for a guest that had just booted and one that had filled its RAM. On a live counter actor it read 2147483648 for a file that occupies 163577856 bytes: the configured 2048 MiB, every time, on every actor.
Record st_blocks*512 instead, which is what the filesystem actually gave the file. Sparse or not, that is the answer to "what did this checkpoint cost", and it is the figure the snapshot's transfer time and storage bill follow.
The count rounds up to a block, so the small JSON images beside the memory one now over-report by under 4 KiB. That is noise on a histogram whose first bucket boundary is 1 MB, and the gVisor images are dense, so for them the two readings agree to within that rounding.
docs/observability.md described this metric as the uncompressed size of a gVisor image, which it was back when gVisor was the only sandbox. It now says what is recorded, and why the two sandboxes read differently.
The existing test helper of the same name is now diskUsage, so the hole-preservation tests keep statting by path rather than going through the code they would otherwise be validating.