fix: compute node allocatable from real reservations and usable memory - #55
Draft
pkieszcz wants to merge 4 commits into
Draft
fix: compute node allocatable from real reservations and usable memory#55pkieszcz wants to merge 4 commits into
pkieszcz wants to merge 4 commits into
Conversation
Karpenter sizes a node by subtracting overhead from an instance type's capacity. This provider declared a flat 100m/100Mi of kubeReserved and left systemReserved and evictionThreshold empty, so the figure it advertised bore no relation to what a node would report once it booted. Two independent errors stacked up. RESERVATIONS. Karpenter computes allocatable as capacity minus kubeReserved plus systemReserved plus evictionThreshold. Two of those three were nil and the third was a guess, so whatever the bootstrap actually reserved went unmodelled. This provider does not render userData, so it cannot know those values: they are now declared on the node class as spec.kubelet, matching the shape the AWS and Azure providers use for the same purpose. Karpenter core deliberately dropped its own kubelet type in v1 and left this to providers. The declaration is descriptive, not prescriptive -- it states what the operator's userData already does, and has to be kept in agreement with it. Declaring less than the bootstrap reserves is the failure this commit fixes, so the field is documented as such rather than left to be inferred. CAPACITY. Memory was taken as the advertised size times 1024^3. Hetzner advertises what the VM is allocated, but the guest kernel never sees all of it; firmware, the kernel image and per-page structures take a cut. Measured across cx and cpx types the gap runs from about 4.4% on a 32Gi server to 6.9% on a 4Gi one, growing in absolute terms and shrinking as a fraction, so no constant is right everywhere. VM_MEMORY_OVERHEAD_PERCENT holds it back, defaulting to 0.075 -- the same default and the same env var the AWS and Azure providers use. A value of 1 or more would leave a server with no memory at all, so the operator refuses to start rather than producing unschedulable nodes. Both errors pushed the same way. Advertising more allocatable than a node has makes Karpenter pick a server the pod cannot fit on: the pod stays Pending, the node is Empty, consolidation reclaims it, and provisioning repeats. Advertising less only costs money. Every default here is therefore chosen to undershoot. Nothing caught this because core's own guard cannot. nodeclaim/consistency's NodeShape compares capacity, never allocatable, and only reports below 90%; the worst capacity gap measured here is 6.87%, just inside its tolerance. List now takes the node class rather than a list of locations, because the overhead depends on it. The 6h catalogue cache keeps only what the hcloud API decides; anything node-class-specific is applied per call, so editing a node class takes effect immediately instead of at the next refresh. A discovered-capacity cache is added as the seam for the next commit, which measures capacity from registered nodes and uses it in place of the estimate. The regression test is a table of what real servers report, captured from running clusters, asserting an inequality rather than an exact figure: the estimate must never exceed reality, and must stay within a bounded shortfall of it. Both halves of the fix were mutation-tested -- disabling either makes it fail on every type, by the measured amount.
The VM memory overhead percent is a single fraction standing in for a gap that is not constant: measured across cx and cpx types it runs from about 4.4% on a 32Gi server to 6.9% on a 4Gi one. Any value is therefore wrong nearly everywhere, and the only safe direction to be wrong in is downward, which leaves schedulable memory on the table on exactly the large nodes where it is worth most. A booted node is not an estimate. This watches nodes as they become registered, reads the memory capacity the kubelet reports, and hands it to the instance type provider, which uses it in place of the estimate for every later node of that server type and image. The estimate then only has to be good enough for the first node of each type, rather than permanently right. This is the approach the AWS provider takes with its own overhead percent, whose flag help calls the percentage the value used "when cached information is unavailable". Azure instead models its bootstrap's published reservation formula exactly; that option is not open here, because k3s publishes no such formula and the reservations are whatever the operator's userData sets. Measurements are keyed by server type and resolved image. The gap is produced by the guest kernel, so a different image can produce a different figure, and carrying a measurement across an image change would apply a number that was never true of the new one. Resolved images are treated as a set, so reordering them is not a different key. The cache keeps the smallest value seen for a key. Nodes of one type do vary by a few hundred KiB, and the two directions are not symmetric: too low costs a little schedulable memory, too high strands a pod on a node that cannot hold it. A genuine increase arrives under a different key, so holding the minimum never pins the cache to a stale low value. State is process-local and deliberately not persisted. Karpenter has to be able to size a node before any node of that type exists, so a cold cache must be survivable regardless; given that, a durable store would add a failure mode without removing one. A restart falls back to the estimate until the fleet is observed again, which the startup predicate does immediately rather than waiting for the next launch. Guards, each mutation-tested: only registered nodes, since an unregistered one has not settled on what it will report; only nodes whose node pool resolves to an HCloudNodeClass, since another provider's node says nothing about how Hetzner sizes a server; and never a zero reading, which is the absence of data rather than a very small machine. A deleted node pool or node class returns without recording instead of erroring, because that is a race with teardown and requeueing would spin against objects that are not coming back. One of those tests initially passed for the wrong reason -- it asserted a foreign node class was ignored, but never created the HCloudNodeClass, so the lookup 404'd whether or not the group and kind were checked. Mutation testing caught it; the test now seeds the node class so the guard is what makes it pass. No RBAC change: nodes, nodepools and hcloudnodeclasses were already granted get/list/watch. VM_MEMORY_OVERHEAD_PERCENT is emitted unconditionally rather than through a `with` block, so that 0 -- trust Hetzner's figure exactly -- reaches the operator instead of being skipped as falsy and silently defaulting.
An absent kubelet block means the node class has said nothing about its bootstrap, which is not the same as saying it reserves nothing. Dropping the flat 100m/100Mi this provider used to subtract would raise a node's advertised CPU on upgrade -- the wrong direction, and silent: pods get placed on machines that never had room for them, which is the failure this branch exists to fix. Undeclared now means unchanged. A node class that does declare a block is taken at its word, so the default is replaced rather than added to.
Contributor
|
On the open question at the end: yes to the runtime check comparing predicted allocatable against what nodes actually register — but as a follow-up PR, not here. This one is already large and self-contained; the drift check is additive and read-only, so it can land on its own. (govulncheck failure is unrelated — fixed on main by #57, rebase to clear.) |
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.
Karpenter decides which server type a pod fits on by subtracting reserved resources from the type's capacity. This provider declared a flat
100m/100MiofkubeReservedand leftsystemReservedandevictionThresholdnil, so the figure it advertised bore no relation to what a node reports once it boots.Where that lands: a pod is scheduled onto a server too small to hold it, stays
Pending, leaves the nodeEmpty, consolidation reclaims the node, and provisioning does it again. WithconsolidateAfter: 0sthat loop runs at the speed of the API. Nothing logs an error — the pod is simply Pending and servers churn.Two errors, stacking
Reservations. Core computes
allocatable = capacity − (kubeReserved + systemReserved + evictionThreshold). Two of those three were nil and the third was a guess, so whatever the bootstrap reserved went unmodelled — on a k3s bootstrap reserving 512Mi + 512Mi + a 400Mi eviction threshold, that is 1424Mi and 400m per node, on every type.Capacity. Memory was
advertised_GB × 1024³. Hetzner advertises what the VM is allocated; the guest kernel never sees all of it. Measured on running clusters:Growing in absolute terms, shrinking as a fraction — a fixed firmware and kernel cost plus per-page structures. No constant is right everywhere.
Combined, the old code overstated a cx53's allocatable by 2755Mi. Both errors push the same way, and the two directions are not symmetric: too high strands pods forever, too low only costs money. Every default here is chosen to undershoot.
Why nothing caught it
Core's
nodeclaim/consistencyNodeShapeis the obvious guard, and it is structurally blind to this on both counts: it compares capacity, never allocatable, and only reports below 90%. The worst capacity gap measured here is 6.87% — inside its tolerance.The fix
spec.kubeleton HCloudNodeClass (systemReserved,kubeReserved,evictionHard), matching the shape AWS and Azure use — core deliberately dropped its own kubelet type in v1 and left this to providers.The field is descriptive, not prescriptive: this provider does not render userData, so it states what the operator's bootstrap already does and must be kept in agreement with it. That is called out in the field doc and the README rather than left to be discovered.
VM_MEMORY_OVERHEAD_PERCENT, default0.075— the same env var and the same default as the AWS and Azure providers. Values>= 1would leave a server with no memory, so the operator refuses to start rather than producing unschedulable nodes.A capacity controller that watches nodes becoming registered, reads the memory the kubelet reports, and uses it in place of the estimate for later nodes of that server type and image.
That last piece is why 7.5% is defensible. It is a single fraction standing in for a gap that varies from 4.4% to 6.9%, so it is wrong nearly everywhere; the AWS flag's own help text calls the percentage the value used "when cached information is unavailable". Azure instead models its bootstrap's published reservation formula exactly — not an option here, since k3s publishes no such formula and the reservations are whatever the operator's userData sets. So: estimate first, measure as soon as there is something to measure.
Measurements are keyed by server type and resolved image, because the gap is produced by the guest kernel and carrying a figure across an image change would apply a number that was never true of the new image. The cache keeps the smallest value per key — nodes of one type vary slightly, and only one direction of error is safe. State is process-local and not persisted: Karpenter must be able to size a node before any node of that type exists, so a cold cache has to be survivable regardless, and given that, a durable store adds a failure mode without removing one.
Verification
The regression test is a table of what real servers report, captured from running clusters across five server types, asserting an inequality rather than an exact figure: never above what the node registers, and within a bounded shortfall of it.
Every guard is mutation-tested — reverting it makes exactly its test fail, by the measured amount. That process earned its keep here: the test asserting a foreign node class is ignored initially passed for the wrong reason, because it never created the
HCloudNodeClassand the lookup 404'd whether or not the group and kind were checked. It now seeds the node class so the guard is what makes it pass.Compatibility
spec.kubeletis optional and additive; existing node classes are unaffected except that their allocatable becomes correct. A cluster whose bootstrap sets no reservations needs no change — the kubelet's own defaults are modelled.Provider.Listnow takes the node class rather than a list of locations, since the overhead depends on it. The 6h catalogue cache keeps only what the hcloud API decides; anything node-class-specific is applied per call, so editing a node class takes effect immediately rather than at the next refresh.No RBAC change: nodes, nodepools and hcloudnodeclasses already carry
get/list/watch.One caveat worth stating plainly: because the declaration and the bootstrap are separate, they can drift. A runtime check that compares predicted allocatable against what nodes actually register would close that, and would have turned this bug into a number instead of a churn loop. Happy to add it here or as a follow-up.