the formatting is great for human. Would user ever need an actual bytes value to compare with some metrics?

Not sure if this question is for me or @kannon92 but I'll take a stab:

IIUC event messages are for humans to read and are normally batched by kubectl, so when the number varies over the different GC rounds every 5 minutes, it'll only show the latest one. But even if the user wants to know one of the following, there are better / more realistic ways for them to find it out, they both would be generated consistently regardless of whether the node is under disk pressure:

• how has my disk usage varied over time?
  • just check node-exporter metrics or similar
• how many images become unused each day?
  • check image GC text logs in the kubelet logs

I guess from the review comments from you and the other reviewer that this component suffers from Hyrum's Law, so if it's too dangerous to change an event description because someone intentionally keeps the node under disk pressure so they can parse this event and measure disk usage + GC history, then we should probably just close the linked issue as infeasible. I don't want to cause an outage for someone just to make one message more beginner-friendly...

I was mostly thinking of situation when user will notice a small increase in sizes from one log message to another. It will indicate something else is using memory. But yes, it feels like abusive use of a log message instead of finding the proper metric. This is why I think human-friendly value with the original value in brackets would be a good least-breaking alternative.

I am open to be convinced that this is not needed, reviewers, please comment

hat this component suffers from Hyrum's Law

I don't think this applies here but I am worried about any changes that are not directly related to making the logs easier to read.

Plus it is subjective.

This is why I think human-friendly value with the original value in brackets would be a good least-breaking alternative.

I've changed to raw bytes for the "freed" message to reduce the delta and because I think "freed 0 bytes" is more readable than "0 B freed". The delta in the latest commit:

• before: failed to garbage collect required amount of images. Wanted to free 473948979 bytes, but freed 0 bytes
• after: Insufficient free disk space on the node's image filesystem (95.0% of 10.0 GiB used). Failed to free sufficient space by deleting unused images (freed 0 bytes). Consider resizing the disk or deleting unused files.

ie the changes are:

• replacing "wanted to free 473948979 bytes" with 95.0% of 10.0 GiB used
  • IMO this is more readable because it maps to what the user knows from df -h and tells them "your disk is almost full!", so I'd lean towards excluding the raw bytes number here but can restore it if you prefer
• replacing but freed 0 bytes with (freed 0 bytes)
  • not much change in readability as this is normally 0, I'm hoping the parens and removing "but" draw less attention from inexperienced users

LMK if you prefer (95.0% of 10.0 GiB used), (95.0% of 10.0 GiB used, wanted to free 473948979 bytes), or something else.

I don't think this applies here but I am worried about any changes that are not directly related to making the logs easier to read.

Not sure I understand this. As I understand it, removing 9 digit numbers and the internals of the GC algorithm (wanted to free X, freed Y) are required to make the message more readable and convey the point "your disk is almost full!". I believe these details make the user think the unused images / GC algorithm are to blame, which is not the case (GC worked as intended in this codepath).

But maybe I've misunderstood and you're referring to something other than the replacements above?

I mean the full process of review. I think this is more focused now.

@SergeyKanzhelev Do you need any more info from me to choose between the options listed above? Or is the current (Insufficient free disk space on the node's image filesystem (95.0% of 10.0 GiB used). Failed to free sufficient space by deleting unused images (freed 0 bytes). Consider resizing the disk or deleting unused files.) fine?

let's keep the information on how much we believe we freed

I have added (XXX freed) to the error as requested - but can I ask why? Under disk pressure the event is generated every five minutes, so this will either be:

• the majority of the time[1]: the node is in steady state, it'll say (0 B freed) because the same images are in-use as the last time it ran GC
• rarely: it'll either print the total compressed or uncompressed size of the images (ie underestimating the amount of freed space) that became unused since the last GC attempt, so somewhere between 0 and 15% of the disk size

I can't think of a case where this is useful, except perhaps for an experienced operator of a super-high-churn cluster who knows that every five minutes, hundreds of MB of images should become unused, so if they see this event with less than that, they deduce that somehow the churn rate of their cluster has changed.

I hope it doesn't seem petty to argue about this. I'm trying to make the error easier for inexperienced users to understand, and IMO adding (0 B freed) or but freed 0 bytes might make them think something is wrong with the GC when it's not.

[1]: At least, every instance I found I Stack Overflow after checking the first 10 results had but freed 0 bytes or but only found 0 bytes eligible to free, which sounds very "broken" and I believe this is why people go down the "how can I debug the GC" rabbit hole

Amount freed is not super precise. So I agree with your reasoning. However I believe the ability to distinguish the difference of zero or non-zero sizes is important. It distinguish constant churn situation and kubelet "believes" it freed something (while runtime may be "broken") vs. there is nothing to free - every image is in-use.

Would it make sense to use imageSize here instead of its hardcoded value?

I prefer to keep the assertions as close to the behavior the user will observe as possible, otherwise the test code ends up just duplicating the original implementation (including its bugs). This way it's obvious that large sizes are meant to be printed as long numbers with no commas and if someone changes the formatting directive, they'll have to change the expected result too.