From 64feaef06ac95a606355d830a636e6d277134d97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kieszczy=C5=84ski?= Date: Tue, 25 Aug 2026 09:38:04 +0200 Subject: [PATCH 1/4] fix: compute allocatable from real reservations, not a flat 100Mi 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. --- ...enter.hetzner.cloud_hcloudnodeclasses.yaml | 55 +++++ cmd/controller/main.go | 2 +- pkg/apis/v1/hcloudnodeclass_types.go | 42 ++++ pkg/apis/v1/zz_generated.deepcopy.go | 41 ++++ pkg/cloudprovider/cloudprovider.go | 4 +- pkg/cloudprovider/cloudprovider_test.go | 14 +- pkg/operator/config.go | 48 +++- pkg/operator/config_test.go | 38 +++ pkg/providers/instancetype/allocatable.go | 126 ++++++++++ .../instancetype/allocatable_test.go | 142 +++++++++++ pkg/providers/instancetype/discovered.go | 96 ++++++++ pkg/providers/instancetype/discovered_test.go | 221 ++++++++++++++++++ pkg/providers/instancetype/instancetype.go | 95 +++++--- .../instancetype/instancetype_test.go | 20 +- 14 files changed, 890 insertions(+), 54 deletions(-) create mode 100644 pkg/providers/instancetype/allocatable.go create mode 100644 pkg/providers/instancetype/allocatable_test.go create mode 100644 pkg/providers/instancetype/discovered.go create mode 100644 pkg/providers/instancetype/discovered_test.go diff --git a/charts/karpenter-provider-hetzner/crds/karpenter.hetzner.cloud_hcloudnodeclasses.yaml b/charts/karpenter-provider-hetzner/crds/karpenter.hetzner.cloud_hcloudnodeclasses.yaml index 6cdb2e6..c4d64cd 100644 --- a/charts/karpenter-provider-hetzner/crds/karpenter.hetzner.cloud_hcloudnodeclasses.yaml +++ b/charts/karpenter-provider-hetzner/crds/karpenter.hetzner.cloud_hcloudnodeclasses.yaml @@ -83,6 +83,61 @@ spec: required: - family type: object + kubelet: + description: |- + Kubelet declares the reservations this node class's bootstrap applies to + the kubelet, so Karpenter can subtract them when computing how much of a + server a pod can actually use. + + This provider does not render the bootstrap: userData is supplied by the + operator, so these values describe what that userData does rather than + configure it. They must be kept in agreement with it. Declaring less than + the bootstrap reserves makes Karpenter believe a server is larger than it + is, which strands pods on nodes too small for them. + properties: + evictionHard: + additionalProperties: + type: string + description: |- + EvictionHard is the kubelet's hard eviction thresholds. Memory held back + for eviction is memory a pod cannot have, so it comes out of allocatable + the same way a reservation does. Values are either a quantity ("400Mi") or + a percentage of capacity ("10%"). + + Soft eviction is deliberately not modelled: it is a warning threshold that + does not reduce allocatable. + type: object + x-kubernetes-validations: + - message: valid keys for evictionHard are ['memory.available','nodefs.available','nodefs.inodesFree','imagefs.available','imagefs.inodesFree','pid.available'] + rule: self.all(x, x in ['memory.available','nodefs.available','nodefs.inodesFree','imagefs.available','imagefs.inodesFree','pid.available']) + kubeReserved: + additionalProperties: + type: string + description: |- + KubeReserved is resources reserved for Kubernetes system daemons, matching + the kubelet's --kube-reserved. + type: object + x-kubernetes-validations: + - message: valid keys for kubeReserved are ['cpu','memory','ephemeral-storage','pid'] + rule: self.all(x, x=='cpu' || x=='memory' || x=='ephemeral-storage' + || x=='pid') + - message: kubeReserved value cannot be a negative resource quantity + rule: self.all(x, !self[x].startsWith('-')) + systemReserved: + additionalProperties: + type: string + description: |- + SystemReserved is resources reserved for OS system daemons, matching the + kubelet's --system-reserved. + type: object + x-kubernetes-validations: + - message: valid keys for systemReserved are ['cpu','memory','ephemeral-storage','pid'] + rule: self.all(x, x=='cpu' || x=='memory' || x=='ephemeral-storage' + || x=='pid') + - message: systemReserved value cannot be a negative resource + quantity + rule: self.all(x, !self[x].startsWith('-')) + type: object labels: additionalProperties: type: string diff --git a/cmd/controller/main.go b/cmd/controller/main.go index e442fe8..36de910 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -40,7 +40,7 @@ func main() { // Create the three providers. instanceProvider := instance.NewProviderWithPlacementGroups(&hcloudClient.Server, &hcloudClient.PlacementGroup, cfg.ClusterName, &hcloudClient.Action) - typeProvider := instancetype.NewProvider(&hcloudClient.ServerType) + typeProvider := instancetype.NewProvider(&hcloudClient.ServerType, cfg.VMMemoryOverheadPercent) imageProvider := imagefamily.NewProvider(&hcloudClient.Image) // Create the cloud provider. diff --git a/pkg/apis/v1/hcloudnodeclass_types.go b/pkg/apis/v1/hcloudnodeclass_types.go index 6a68222..f5f92af 100644 --- a/pkg/apis/v1/hcloudnodeclass_types.go +++ b/pkg/apis/v1/hcloudnodeclass_types.go @@ -68,6 +68,48 @@ type HCloudNodeClassSpec struct { // +kubebuilder:default=true // +optional EnablePublicIPv6 *bool `json:"enablePublicIPv6,omitempty"` + + // Kubelet declares the reservations this node class's bootstrap applies to + // the kubelet, so Karpenter can subtract them when computing how much of a + // server a pod can actually use. + // + // This provider does not render the bootstrap: userData is supplied by the + // operator, so these values describe what that userData does rather than + // configure it. They must be kept in agreement with it. Declaring less than + // the bootstrap reserves makes Karpenter believe a server is larger than it + // is, which strands pods on nodes too small for them. + // +optional + Kubelet *KubeletConfiguration `json:"kubelet,omitempty"` +} + +// KubeletConfiguration mirrors the subset of kubelet settings that change how +// much of a node is schedulable. Only the reservation knobs are modelled; +// anything that does not move allocatable is deliberately absent. +type KubeletConfiguration struct { + // SystemReserved is resources reserved for OS system daemons, matching the + // kubelet's --system-reserved. + // +kubebuilder:validation:XValidation:message="valid keys for systemReserved are ['cpu','memory','ephemeral-storage','pid']",rule="self.all(x, x=='cpu' || x=='memory' || x=='ephemeral-storage' || x=='pid')" + // +kubebuilder:validation:XValidation:message="systemReserved value cannot be a negative resource quantity",rule="self.all(x, !self[x].startsWith('-'))" + // +optional + SystemReserved map[string]string `json:"systemReserved,omitempty"` + + // KubeReserved is resources reserved for Kubernetes system daemons, matching + // the kubelet's --kube-reserved. + // +kubebuilder:validation:XValidation:message="valid keys for kubeReserved are ['cpu','memory','ephemeral-storage','pid']",rule="self.all(x, x=='cpu' || x=='memory' || x=='ephemeral-storage' || x=='pid')" + // +kubebuilder:validation:XValidation:message="kubeReserved value cannot be a negative resource quantity",rule="self.all(x, !self[x].startsWith('-'))" + // +optional + KubeReserved map[string]string `json:"kubeReserved,omitempty"` + + // EvictionHard is the kubelet's hard eviction thresholds. Memory held back + // for eviction is memory a pod cannot have, so it comes out of allocatable + // the same way a reservation does. Values are either a quantity ("400Mi") or + // a percentage of capacity ("10%"). + // + // Soft eviction is deliberately not modelled: it is a warning threshold that + // does not reduce allocatable. + // +kubebuilder:validation:XValidation:message="valid keys for evictionHard are ['memory.available','nodefs.available','nodefs.inodesFree','imagefs.available','imagefs.inodesFree','pid.available']",rule="self.all(x, x in ['memory.available','nodefs.available','nodefs.inodesFree','imagefs.available','imagefs.inodesFree','pid.available'])" + // +optional + EvictionHard map[string]string `json:"evictionHard,omitempty"` } // UserDataSecretReference points at a Secret key holding the server userData. diff --git a/pkg/apis/v1/zz_generated.deepcopy.go b/pkg/apis/v1/zz_generated.deepcopy.go index 2a09032..ffefb43 100644 --- a/pkg/apis/v1/zz_generated.deepcopy.go +++ b/pkg/apis/v1/zz_generated.deepcopy.go @@ -109,6 +109,11 @@ func (in *HCloudNodeClassSpec) DeepCopyInto(out *HCloudNodeClassSpec) { *out = new(bool) **out = **in } + if in.Kubelet != nil { + in, out := &in.Kubelet, &out.Kubelet + *out = new(KubeletConfiguration) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HCloudNodeClassSpec. @@ -170,6 +175,42 @@ func (in *ImageSelector) DeepCopy() *ImageSelector { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *KubeletConfiguration) DeepCopyInto(out *KubeletConfiguration) { + *out = *in + if in.SystemReserved != nil { + in, out := &in.SystemReserved, &out.SystemReserved + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } + if in.KubeReserved != nil { + in, out := &in.KubeReserved, &out.KubeReserved + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } + if in.EvictionHard != nil { + in, out := &in.EvictionHard, &out.EvictionHard + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new KubeletConfiguration. +func (in *KubeletConfiguration) DeepCopy() *KubeletConfiguration { + if in == nil { + return nil + } + out := new(KubeletConfiguration) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ResolvedImage) DeepCopyInto(out *ResolvedImage) { *out = *in diff --git a/pkg/cloudprovider/cloudprovider.go b/pkg/cloudprovider/cloudprovider.go index c23cd67..419a61d 100644 --- a/pkg/cloudprovider/cloudprovider.go +++ b/pkg/cloudprovider/cloudprovider.go @@ -94,7 +94,7 @@ func (cp *CloudProvider) Create(ctx context.Context, nodeClaim *karpv1.NodeClaim } // Get instance types for the node class locations. - instanceTypes, err := cp.typeProvider.List(ctx, nodeClass.Spec.Locations) + instanceTypes, err := cp.typeProvider.List(ctx, nodeClass) if err != nil { return nil, fmt.Errorf("listing instance types: %w", err) } @@ -251,7 +251,7 @@ func (cp *CloudProvider) GetInstanceTypes(ctx context.Context, nodePool *karpv1. return nil, fmt.Errorf("resolving node class for node pool %s: %w", nodePool.Name, err) } - return cp.typeProvider.List(ctx, nodeClass.Spec.Locations) + return cp.typeProvider.List(ctx, nodeClass) } // IsDrifted determines whether the given NodeClaim has drifted from its desired state. diff --git a/pkg/cloudprovider/cloudprovider_test.go b/pkg/cloudprovider/cloudprovider_test.go index 0f94365..bcf6635 100644 --- a/pkg/cloudprovider/cloudprovider_test.go +++ b/pkg/cloudprovider/cloudprovider_test.go @@ -134,7 +134,7 @@ func buildCP(t *testing.T, nc *apiv1.HCloudNodeClass, server *hcloud.Server) (*c cp := cloudprovider.NewCloudProvider(kube, instance.NewProvider(fsc, "test-cluster"), - instancetype.NewProvider(stc), + instancetype.NewProvider(stc, 0), imagefamily.NewProvider(imgc)) nodeClaim := &karpv1.NodeClaim{ObjectMeta: metav1.ObjectMeta{Name: "claim"}} @@ -207,7 +207,7 @@ func buildCPWithTypes(t *testing.T, nc *apiv1.HCloudNodeClass, types []*hcloud.S fsc := &fakeServerClient{servers: map[int64]*hcloud.Server{}} stc := &fakeServerTypeClient{types: types} imgc := &fakeImageClient{images: []*hcloud.Image{{ID: 42, Description: "Ubuntu 24.04", Architecture: hcloud.ArchitectureX86}}} - typeProvider := instancetype.NewProvider(stc) + typeProvider := instancetype.NewProvider(stc, 0) cp := cloudprovider.NewCloudProvider(kube, instance.NewProvider(fsc, "test-cluster"), typeProvider, @@ -308,7 +308,7 @@ func TestCreate_InsufficientCapacityMarksUnavailable(t *testing.T) { t.Fatal("expected error on capacity failure") } // The offering for (cx22, nbg1) should now be marked unavailable. - its, lerr := typeProvider.List(context.Background(), []string{"nbg1"}) + its, lerr := typeProvider.List(context.Background(), &apiv1.HCloudNodeClass{Spec: apiv1.HCloudNodeClassSpec{Locations: []string{"nbg1"}}}) if lerr != nil { t.Fatal(lerr) } @@ -460,7 +460,7 @@ func TestCreate_UserDataFromSecret(t *testing.T) { imgc := &fakeImageClient{images: []*hcloud.Image{{ID: 42, Description: "Ubuntu 24.04", Architecture: hcloud.ArchitectureX86}}} cp := cloudprovider.NewCloudProvider(kube, instance.NewProvider(fsc, "test-cluster"), - instancetype.NewProvider(stc), + instancetype.NewProvider(stc, 0), imagefamily.NewProvider(imgc)) if _, err := cp.Create(context.Background(), createNodeClaim()); err != nil { @@ -485,7 +485,7 @@ func TestCreate_UserDataInlineWhenNoRef(t *testing.T) { imgc := &fakeImageClient{images: []*hcloud.Image{{ID: 42, Description: "Ubuntu 24.04", Architecture: hcloud.ArchitectureX86}}} cp := cloudprovider.NewCloudProvider(kube, instance.NewProvider(fsc, "test-cluster"), - instancetype.NewProvider(stc), + instancetype.NewProvider(stc, 0), imagefamily.NewProvider(imgc)) if _, err := cp.Create(context.Background(), createNodeClaim()); err != nil { @@ -519,7 +519,7 @@ func TestCreate_UserDataSecretKeyMissing(t *testing.T) { imgc := &fakeImageClient{images: []*hcloud.Image{{ID: 42, Description: "Ubuntu 24.04", Architecture: hcloud.ArchitectureX86}}} cp := cloudprovider.NewCloudProvider(kube, instance.NewProvider(fsc, "test-cluster"), - instancetype.NewProvider(stc), + instancetype.NewProvider(stc, 0), imagefamily.NewProvider(imgc)) _, err := cp.Create(context.Background(), createNodeClaim()) @@ -546,7 +546,7 @@ func TestCreate_UserDataSecretMissing(t *testing.T) { imgc := &fakeImageClient{images: []*hcloud.Image{{ID: 42, Description: "Ubuntu 24.04", Architecture: hcloud.ArchitectureX86}}} cp := cloudprovider.NewCloudProvider(kube, instance.NewProvider(fsc, "test-cluster"), - instancetype.NewProvider(stc), + instancetype.NewProvider(stc, 0), imagefamily.NewProvider(imgc)) _, err := cp.Create(context.Background(), createNodeClaim()) diff --git a/pkg/operator/config.go b/pkg/operator/config.go index 3590fdf..87a215c 100644 --- a/pkg/operator/config.go +++ b/pkg/operator/config.go @@ -4,16 +4,37 @@ import ( "fmt" "os" "regexp" + "strconv" "strings" ) var clusterNameRE = regexp.MustCompile(`^[a-zA-Z0-9._-]{1,63}$`) +// DefaultVMMemoryOverheadPercent is the fraction of a server type's advertised +// memory assumed to be unavailable to the guest. +// +// Hetzner advertises the RAM the VM is allocated, but the guest kernel never +// sees all of it: firmware, the kernel image and per-page structures take a cut +// that grows with the size of the machine. Measured across cx/cpx types the gap +// runs from roughly 4.4% on a 32Gi server to 6.9% on a 4Gi one, so a single +// fraction cannot be exact everywhere. 0.075 is deliberately on the safe side of +// every measurement: erring high costs a little schedulable memory, while erring +// low tells Karpenter a server is bigger than it is and strands pods on it. +// +// This is only the estimate used before a node of that type has been seen. Once +// one registers, its real capacity is recorded and used instead, so the constant +// matters for the first node of each server type and image, not the fleet. +const DefaultVMMemoryOverheadPercent = 0.075 + // Config holds provider configuration sourced from the environment. type Config struct { // ClusterName scopes all managed servers so multiple clusters can share // one Hetzner project without colliding. ClusterName string + + // VMMemoryOverheadPercent is subtracted from every server type's advertised + // memory. See DefaultVMMemoryOverheadPercent. + VMMemoryOverheadPercent float64 } // LoadConfig reads provider configuration from the environment. @@ -26,5 +47,30 @@ func LoadConfig() (*Config, error) { if !clusterNameRE.MatchString(name) { return nil, fmt.Errorf("CLUSTER_NAME %q is not a valid Hetzner label value (must match [a-zA-Z0-9._-], max 63 chars)", name) } - return &Config{ClusterName: name}, nil + + overhead, err := parseVMMemoryOverheadPercent(os.Getenv("VM_MEMORY_OVERHEAD_PERCENT")) + if err != nil { + return nil, err + } + + return &Config{ClusterName: name, VMMemoryOverheadPercent: overhead}, nil +} + +// parseVMMemoryOverheadPercent reads the overhead fraction, defaulting when +// unset. A value of 1 or more would leave a server with no memory at all, and a +// negative one would claim more memory than Hetzner sells; both are rejected at +// startup rather than silently producing unschedulable or oversized nodes. +func parseVMMemoryOverheadPercent(raw string) (float64, error) { + raw = strings.TrimSpace(raw) + if raw == "" { + return DefaultVMMemoryOverheadPercent, nil + } + v, err := strconv.ParseFloat(raw, 64) + if err != nil { + return 0, fmt.Errorf("VM_MEMORY_OVERHEAD_PERCENT %q is not a number (expected a fraction such as 0.075)", raw) + } + if v < 0 || v >= 1 { + return 0, fmt.Errorf("VM_MEMORY_OVERHEAD_PERCENT %v is out of range (must be at least 0 and less than 1)", v) + } + return v, nil } diff --git a/pkg/operator/config_test.go b/pkg/operator/config_test.go index 71a82c1..bdcbadf 100644 --- a/pkg/operator/config_test.go +++ b/pkg/operator/config_test.go @@ -28,3 +28,41 @@ func TestLoadConfig_RejectsInvalidClusterName(t *testing.T) { } } } + +func TestLoadConfig_VMMemoryOverheadPercent(t *testing.T) { + tests := []struct { + name string + env string + want float64 + wantErr bool + }{ + {name: "unset uses default", env: "", want: 0.075}, + {name: "explicit value", env: "0.1", want: 0.1}, + {name: "zero is allowed", env: "0", want: 0}, + {name: "negative rejected", env: "-0.1", wantErr: true}, + {name: "one rejected", env: "1", wantErr: true}, + {name: "above one rejected", env: "1.5", wantErr: true}, + {name: "non-numeric rejected", env: "7.5%", wantErr: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("CLUSTER_NAME", "test-cluster") + if tc.env != "" { + t.Setenv("VM_MEMORY_OVERHEAD_PERCENT", tc.env) + } + cfg, err := LoadConfig() + if tc.wantErr { + if err == nil { + t.Fatalf("expected error for %q, got config %+v", tc.env, cfg) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.VMMemoryOverheadPercent != tc.want { + t.Errorf("expected %v, got %v", tc.want, cfg.VMMemoryOverheadPercent) + } + }) + } +} diff --git a/pkg/providers/instancetype/allocatable.go b/pkg/providers/instancetype/allocatable.go new file mode 100644 index 0000000..56875d5 --- /dev/null +++ b/pkg/providers/instancetype/allocatable.go @@ -0,0 +1,126 @@ +package instancetype + +import ( + "strconv" + "strings" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + "sigs.k8s.io/karpenter/pkg/cloudprovider" + + apiv1 "github.com/paperclipinc/karpenter-provider-hetzner/pkg/apis/v1" +) + +// Kubelet's own defaults, applied when the node class declares no eviction +// thresholds. The kubelet always holds something back even when nothing is +// configured, so modelling zero would overstate what a pod can use. +const ( + defaultMemoryEvictionThreshold = "100Mi" + defaultNodefsEvictionThreshold = "10%" +) + +// overheadFor converts a node class's declared kubelet reservations into the +// overhead Karpenter subtracts from capacity. Karpenter's own formula is +// allocatable = capacity - (kubeReserved + systemReserved + evictionThreshold), +// so every one of those three has to be filled in for allocatable to match what +// the node will actually report. +// +// capacity is required because eviction thresholds may be expressed as a +// percentage of it. +func overheadFor(nodeClass *apiv1.HCloudNodeClass, capacity corev1.ResourceList) *cloudprovider.InstanceTypeOverhead { + var kubelet *apiv1.KubeletConfiguration + if nodeClass != nil { + kubelet = nodeClass.Spec.Kubelet + } + + overhead := &cloudprovider.InstanceTypeOverhead{ + EvictionThreshold: evictionThreshold(kubelet, capacity), + } + if kubelet != nil { + overhead.KubeReserved = parseResourceList(kubelet.KubeReserved) + overhead.SystemReserved = parseResourceList(kubelet.SystemReserved) + } + return overhead +} + +// evictionThreshold models the memory and disk the kubelet holds back to keep +// itself above its hard eviction signals. That headroom is unavailable to pods, +// so it reduces allocatable exactly as a reservation does. +// +// Only signals that move a resource Karpenter schedules on are translated: +// memory.available and nodefs.available. inodesFree and pid.available are +// accepted by the API but have no allocatable equivalent. +func evictionThreshold(kubelet *apiv1.KubeletConfiguration, capacity corev1.ResourceList) corev1.ResourceList { + var hard map[string]string + if kubelet != nil { + hard = kubelet.EvictionHard + } + + signal := func(name, fallback string) string { + if v, ok := hard[name]; ok && strings.TrimSpace(v) != "" { + return v + } + return fallback + } + + threshold := corev1.ResourceList{} + if q, ok := resolveThreshold(signal("memory.available", defaultMemoryEvictionThreshold), capacity[corev1.ResourceMemory]); ok { + threshold[corev1.ResourceMemory] = q + } + if q, ok := resolveThreshold(signal("nodefs.available", defaultNodefsEvictionThreshold), capacity[corev1.ResourceEphemeralStorage]); ok { + threshold[corev1.ResourceEphemeralStorage] = q + } + return threshold +} + +// resolveThreshold reads an eviction threshold, which the kubelet accepts either +// as a quantity ("400Mi") or as a percentage of the resource's capacity ("10%"). +// An unparseable value yields no threshold rather than a zero one, so a typo +// cannot quietly hand pods memory the kubelet is holding back. +func resolveThreshold(raw string, capacity resource.Quantity) (resource.Quantity, bool) { + raw = strings.TrimSpace(raw) + if pct, ok := strings.CutSuffix(raw, "%"); ok { + f, err := strconv.ParseFloat(strings.TrimSpace(pct), 64) + if err != nil || f < 0 || f > 100 { + return resource.Quantity{}, false + } + return *resource.NewQuantity(int64(float64(capacity.Value())*f/100), resource.BinarySI), true + } + q, err := resource.ParseQuantity(raw) + if err != nil || q.Sign() < 0 { + return resource.Quantity{}, false + } + return q, true +} + +// parseResourceList converts the node class's string-keyed reservations into a +// ResourceList. Values that do not parse are dropped: the CEL rules on the CRD +// reject them at admission, so anything reaching here came in some other way and +// is safer ignored than treated as zero. +func parseResourceList(in map[string]string) corev1.ResourceList { + if len(in) == 0 { + return nil + } + out := corev1.ResourceList{} + for k, v := range in { + q, err := resource.ParseQuantity(strings.TrimSpace(v)) + if err != nil || q.Sign() < 0 { + continue + } + out[corev1.ResourceName(k)] = q + } + if len(out) == 0 { + return nil + } + return out +} + +// memoryWithVMOverhead reduces a server type's advertised memory to what the +// guest is expected to actually see. See operator.DefaultVMMemoryOverheadPercent +// for why the gap exists and why it is a fraction rather than a constant. +func memoryWithVMOverhead(advertisedBytes int64, overheadPercent float64) int64 { + if overheadPercent <= 0 { + return advertisedBytes + } + return int64(float64(advertisedBytes) * (1 - overheadPercent)) +} diff --git a/pkg/providers/instancetype/allocatable_test.go b/pkg/providers/instancetype/allocatable_test.go new file mode 100644 index 0000000..50e57ee --- /dev/null +++ b/pkg/providers/instancetype/allocatable_test.go @@ -0,0 +1,142 @@ +package instancetype + +import ( + "context" + "testing" + + "github.com/hetznercloud/hcloud-go/v2/hcloud" + corev1 "k8s.io/api/core/v1" + + apiv1 "github.com/paperclipinc/karpenter-provider-hetzner/pkg/apis/v1" + "github.com/paperclipinc/karpenter-provider-hetzner/pkg/operator" +) + +// measuredNodes records what real Hetzner servers actually report once booted +// and registered, captured from running clusters. Hetzner's advertised size is +// not what the guest sees: the kernel and firmware reserve a slice that grows +// with RAM, so a cx53 advertised as 32Gi registers 31337Mi of capacity. On top +// of that the kubelet subtracts whatever the bootstrap reserved. +// +// These numbers are the reason this package cannot compute allocatable from the +// advertised size alone, and the reason the test below asserts an inequality +// rather than an exact figure: over-reporting allocatable makes Karpenter pick a +// node the pod cannot fit on, which strands the pod and churns the node forever. +// Under-reporting only costs money. +var measuredNodes = []struct { + name string + cores int + memGB float32 + realCapacityKi int64 + realAllocatableKi int64 +}{ + {"cpx22", 2, 4, 3905948, 2447772}, + {"cpx32", 4, 8, 7931612, 6473436}, + {"cx33", 4, 8, 7937224, 6479048}, + {"cx43", 8, 16, 15988560, 14530384}, + {"cx53", 16, 32, 32089152, 30630976}, +} + +// locationsNodeClass is a node class that only constrains locations, for tests +// about filtering rather than allocatable. +func locationsNodeClass(locations ...string) *apiv1.HCloudNodeClass { + return &apiv1.HCloudNodeClass{Spec: apiv1.HCloudNodeClassSpec{Locations: locations}} +} + +// k3sNodeClass mirrors the kubelet reservations these clusters' bootstrap sets: +// system-reserved and kube-reserved at 512Mi/200m each, plus a 400Mi hard +// eviction threshold. 1424Mi and 400m in total, on every server type. +func k3sNodeClass() *apiv1.HCloudNodeClass { + return &apiv1.HCloudNodeClass{ + Spec: apiv1.HCloudNodeClassSpec{ + Kubelet: &apiv1.KubeletConfiguration{ + SystemReserved: map[string]string{"cpu": "200m", "memory": "512Mi"}, + KubeReserved: map[string]string{"cpu": "200m", "memory": "512Mi"}, + EvictionHard: map[string]string{"memory.available": "400Mi"}, + }, + }, + } +} + +// TestAllocatable_NeverExceedsRegisteredNode is the regression for the churn +// loop: Karpenter sized a node from an allocatable figure larger than the +// machine's real capacity, the pod stayed Pending, the empty node was reclaimed, +// and provisioning repeated indefinitely. +func TestAllocatable_NeverExceedsRegisteredNode(t *testing.T) { + for _, tc := range measuredNodes { + t.Run(tc.name, func(t *testing.T) { + st := makeServerType(tc.name, hcloud.ArchitectureX86, hcloud.CPUTypeShared, tc.cores, tc.memGB, 80, testPricings) + client := &mockServerTypeClient{types: []*hcloud.ServerType{st}} + p := NewProvider(client, operator.DefaultVMMemoryOverheadPercent) + + types, err := p.List(context.Background(), k3sNodeClass()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(types) != 1 { + t.Fatalf("expected 1 instance type, got %d", len(types)) + } + + alloc := types[0].Allocatable() + gotMem := alloc[corev1.ResourceMemory] + realMem := tc.realAllocatableKi * 1024 + if gotMem.Value() > realMem { + t.Errorf("memory allocatable %dMi exceeds what the node registers (%dMi): overestimates by %dMi", + gotMem.Value()/1024/1024, realMem/1024/1024, (gotMem.Value()-realMem)/1024/1024) + } + + gotCPU := alloc[corev1.ResourceCPU] + realCPU := int64(tc.cores)*1000 - 400 + if gotCPU.MilliValue() > realCPU { + t.Errorf("cpu allocatable %dm exceeds what the node registers (%dm)", gotCPU.MilliValue(), realCPU) + } + }) + } +} + +// TestAllocatable_WithinBudgetOfRegisteredNode guards the other direction. The +// estimate must be conservative, but an estimate far below reality silently +// buys larger servers than the workload needs, so hold it to a bounded shortfall. +func TestAllocatable_WithinBudgetOfRegisteredNode(t *testing.T) { + const maxShortfallMi = 1200 + + for _, tc := range measuredNodes { + t.Run(tc.name, func(t *testing.T) { + st := makeServerType(tc.name, hcloud.ArchitectureX86, hcloud.CPUTypeShared, tc.cores, tc.memGB, 80, testPricings) + client := &mockServerTypeClient{types: []*hcloud.ServerType{st}} + p := NewProvider(client, operator.DefaultVMMemoryOverheadPercent) + + types, err := p.List(context.Background(), k3sNodeClass()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + gotMem := types[0].Allocatable()[corev1.ResourceMemory] + shortfallMi := (tc.realAllocatableKi*1024 - gotMem.Value()) / 1024 / 1024 + if shortfallMi > maxShortfallMi { + t.Errorf("memory allocatable %dMi understates the node by %dMi (budget %dMi)", + gotMem.Value()/1024/1024, shortfallMi, maxShortfallMi) + } + }) + } +} + +// TestAllocatable_NoKubeletConfig falls back to advertised-minus-VM-overhead +// when the NodeClass declares no reservations. The result must still not exceed +// the machine's real capacity, since the VM overhead applies regardless. +func TestAllocatable_NoKubeletConfig(t *testing.T) { + tc := measuredNodes[4] // cx53 + st := makeServerType(tc.name, hcloud.ArchitectureX86, hcloud.CPUTypeShared, tc.cores, tc.memGB, 80, testPricings) + client := &mockServerTypeClient{types: []*hcloud.ServerType{st}} + p := NewProvider(client, operator.DefaultVMMemoryOverheadPercent) + + types, err := p.List(context.Background(), &apiv1.HCloudNodeClass{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + gotMem := types[0].Allocatable()[corev1.ResourceMemory] + if gotMem.Value() > tc.realCapacityKi*1024 { + t.Errorf("memory allocatable %dMi exceeds real capacity %dMi with no kubelet config", + gotMem.Value()/1024/1024, tc.realCapacityKi/1024) + } +} diff --git a/pkg/providers/instancetype/discovered.go b/pkg/providers/instancetype/discovered.go new file mode 100644 index 0000000..b14795f --- /dev/null +++ b/pkg/providers/instancetype/discovered.go @@ -0,0 +1,96 @@ +package instancetype + +import ( + "fmt" + "sort" + "strings" + "sync" + + "k8s.io/apimachinery/pkg/api/resource" + + apiv1 "github.com/paperclipinc/karpenter-provider-hetzner/pkg/apis/v1" +) + +// discoveredCapacityCache records the memory capacity real servers report once +// they boot, keyed by server type and image. +// +// The estimate in operator.DefaultVMMemoryOverheadPercent is a single fraction +// standing in for a gap that varies by machine size, so it is wrong everywhere +// by a little. A booted node is not an estimate, and one observation is good for +// every future node of that type on that image. This is the same approach the +// AWS provider takes with its own overhead percent, for the same reason. +// +// State is process-local and deliberately not persisted. Karpenter must be able +// to size a node before any node exists, so a cold cache has to be survivable +// anyway; that being true, a durable store would add a failure mode without +// removing one. A restart simply falls back to the estimate until the next node +// registers. +type discoveredCapacityCache struct { + mu sync.RWMutex + byType map[string]resource.Quantity +} + +func newDiscoveredCapacityCache() *discoveredCapacityCache { + return &discoveredCapacityCache{byType: map[string]resource.Quantity{}} +} + +// discoveredKey scopes an observation to the image it was taken on. The gap +// between advertised and visible memory is set by the guest kernel, so a +// different image can produce a different figure for the same server type, and +// carrying a measurement across an image change would apply a number that was +// never true of the new one. +// +// A node class with no resolved images yet keys on the server type alone. That +// is only reachable before the image controller has run, and it is the same +// bucket such a node class would use consistently. +func discoveredKey(serverType string, nodeClass *apiv1.HCloudNodeClass) string { + if nodeClass == nil || len(nodeClass.Status.ResolvedImages) == 0 { + return serverType + } + ids := make([]string, 0, len(nodeClass.Status.ResolvedImages)) + for _, img := range nodeClass.Status.ResolvedImages { + ids = append(ids, fmt.Sprintf("%s/%d", img.Architecture, img.ImageID)) + } + // Sorted so that a reordering of the same images is the same key. + sort.Strings(ids) + return serverType + "|" + strings.Join(ids, ",") +} + +// get returns the measured capacity for a server type under a node class's +// image, if one has been recorded. +func (c *discoveredCapacityCache) get(serverType string, nodeClass *apiv1.HCloudNodeClass) (resource.Quantity, bool) { + c.mu.RLock() + defer c.mu.RUnlock() + q, ok := c.byType[discoveredKey(serverType, nodeClass)] + return q, ok +} + +// record stores a capacity observed on a registered node, keeping the smallest +// value seen. +// +// Monotonically downward on purpose. Nodes of one type do vary slightly, and the +// consequence of the two directions is not symmetric: too low costs a little +// schedulable memory, too high strands a pod on a node that cannot hold it. A +// genuine increase -- a new image, say -- arrives under a different key, so +// holding the minimum here never pins the cache to a stale low value. +func (c *discoveredCapacityCache) record(serverType string, nodeClass *apiv1.HCloudNodeClass, observed resource.Quantity) bool { + if observed.Sign() <= 0 { + return false + } + key := discoveredKey(serverType, nodeClass) + + c.mu.Lock() + defer c.mu.Unlock() + if existing, ok := c.byType[key]; ok && existing.Cmp(observed) <= 0 { + return false + } + c.byType[key] = observed + return true +} + +// Record stores a capacity measured on a registered node. It reports whether +// this changed what the provider will use, so callers can log first discoveries +// and genuine drops without narrating every node that agrees with the cache. +func (p *Provider) Record(serverType string, nodeClass *apiv1.HCloudNodeClass, observed resource.Quantity) bool { + return p.discovered.record(serverType, nodeClass, observed) +} diff --git a/pkg/providers/instancetype/discovered_test.go b/pkg/providers/instancetype/discovered_test.go new file mode 100644 index 0000000..9ab008d --- /dev/null +++ b/pkg/providers/instancetype/discovered_test.go @@ -0,0 +1,221 @@ +package instancetype + +import ( + "context" + "testing" + + "github.com/hetznercloud/hcloud-go/v2/hcloud" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + + apiv1 "github.com/paperclipinc/karpenter-provider-hetzner/pkg/apis/v1" + "github.com/paperclipinc/karpenter-provider-hetzner/pkg/operator" +) + +func imageNodeClass(images ...apiv1.ResolvedImage) *apiv1.HCloudNodeClass { + return &apiv1.HCloudNodeClass{Status: apiv1.HCloudNodeClassStatus{ResolvedImages: images}} +} + +func TestDiscovered_RecordThenGet(t *testing.T) { + c := newDiscoveredCapacityCache() + nc := imageNodeClass(apiv1.ResolvedImage{Architecture: "x86", ImageID: 42}) + + if _, ok := c.get("cx53", nc); ok { + t.Fatal("expected no entry before recording") + } + if changed := c.record("cx53", nc, resource.MustParse("31337Mi")); !changed { + t.Error("first observation should report a change") + } + got, ok := c.get("cx53", nc) + if !ok { + t.Fatal("expected an entry after recording") + } + if got.String() != "31337Mi" { + t.Errorf("expected 31337Mi, got %s", got.String()) + } +} + +// A larger observation must not raise the cached figure. Nodes of one type vary +// slightly, and raising it would hand pods memory some nodes of that type do not +// have. +func TestDiscovered_KeepsSmallest(t *testing.T) { + c := newDiscoveredCapacityCache() + nc := imageNodeClass(apiv1.ResolvedImage{Architecture: "x86", ImageID: 42}) + + c.record("cx53", nc, resource.MustParse("31337Mi")) + + if changed := c.record("cx53", nc, resource.MustParse("31400Mi")); changed { + t.Error("a larger observation should not change the cache") + } + got, _ := c.get("cx53", nc) + if got.String() != "31337Mi" { + t.Errorf("larger observation overwrote the cache: got %s", got.String()) + } + + if changed := c.record("cx53", nc, resource.MustParse("31300Mi")); !changed { + t.Error("a smaller observation should change the cache") + } + got, _ = c.get("cx53", nc) + if got.String() != "31300Mi" { + t.Errorf("expected the smaller value to win, got %s", got.String()) + } +} + +// The advertised-to-visible gap is set by the guest kernel, so a measurement +// taken on one image says nothing about another. +func TestDiscovered_ScopedToImage(t *testing.T) { + c := newDiscoveredCapacityCache() + oldImage := imageNodeClass(apiv1.ResolvedImage{Architecture: "x86", ImageID: 42}) + newImage := imageNodeClass(apiv1.ResolvedImage{Architecture: "x86", ImageID: 99}) + + c.record("cx53", oldImage, resource.MustParse("31337Mi")) + + if _, ok := c.get("cx53", newImage); ok { + t.Error("a measurement from one image must not apply to another") + } + if _, ok := c.get("cx53", oldImage); !ok { + t.Error("the original image should still resolve") + } +} + +// Resolved images are a set, not a sequence: the same images listed in a +// different order are the same image. +func TestDiscovered_ImageOrderIrrelevant(t *testing.T) { + c := newDiscoveredCapacityCache() + a := imageNodeClass( + apiv1.ResolvedImage{Architecture: "x86", ImageID: 42}, + apiv1.ResolvedImage{Architecture: "arm", ImageID: 43}, + ) + b := imageNodeClass( + apiv1.ResolvedImage{Architecture: "arm", ImageID: 43}, + apiv1.ResolvedImage{Architecture: "x86", ImageID: 42}, + ) + + c.record("cx53", a, resource.MustParse("31337Mi")) + if _, ok := c.get("cx53", b); !ok { + t.Error("reordering the same images produced a different key") + } +} + +func TestDiscovered_RejectsNonPositive(t *testing.T) { + c := newDiscoveredCapacityCache() + nc := imageNodeClass(apiv1.ResolvedImage{Architecture: "x86", ImageID: 42}) + + for _, bad := range []string{"0", "-1Mi"} { + if changed := c.record("cx53", nc, resource.MustParse(bad)); changed { + t.Errorf("observation %q should be rejected", bad) + } + if _, ok := c.get("cx53", nc); ok { + t.Errorf("observation %q was stored", bad) + } + } +} + +// The point of the whole mechanism: a measured node overrides the estimate. +func TestDiscovered_OverridesEstimateInList(t *testing.T) { + st := makeServerType("cx53", hcloud.ArchitectureX86, hcloud.CPUTypeShared, 16, 32, 320, testPricings) + client := &mockServerTypeClient{types: []*hcloud.ServerType{st}} + p := NewProvider(client, operator.DefaultVMMemoryOverheadPercent) + nc := imageNodeClass(apiv1.ResolvedImage{Architecture: "x86", ImageID: 42}) + + before, err := p.List(context.Background(), nc) + if err != nil { + t.Fatal(err) + } + estimated := before[0].Capacity[corev1.ResourceMemory] + // 32Gi less 7.5% is ~30310Mi, short of the 31337Mi a real cx53 reports. + if estimated.Value() >= 31337*1024*1024 { + t.Fatalf("expected the estimate to undershoot reality, got %s", estimated.String()) + } + + p.Record("cx53", nc, resource.MustParse("31337Mi")) + + after, err := p.List(context.Background(), nc) + if err != nil { + t.Fatal(err) + } + measured := after[0].Capacity[corev1.ResourceMemory] + if measured.Value() != 31337*1024*1024 { + t.Errorf("expected the measured capacity to be used, got %s", measured.String()) + } +} + +// A measurement recorded under one node class must not leak into another whose +// images differ, even though both share the catalogue cache. +func TestDiscovered_DoesNotLeakAcrossNodeClasses(t *testing.T) { + st := makeServerType("cx53", hcloud.ArchitectureX86, hcloud.CPUTypeShared, 16, 32, 320, testPricings) + client := &mockServerTypeClient{types: []*hcloud.ServerType{st}} + p := NewProvider(client, operator.DefaultVMMemoryOverheadPercent) + + measured := imageNodeClass(apiv1.ResolvedImage{Architecture: "x86", ImageID: 42}) + other := imageNodeClass(apiv1.ResolvedImage{Architecture: "x86", ImageID: 99}) + + p.Record("cx53", measured, resource.MustParse("31337Mi")) + + types, err := p.List(context.Background(), other) + if err != nil { + t.Fatal(err) + } + got := types[0].Capacity[corev1.ResourceMemory] + if got.Value() == 31337*1024*1024 { + t.Error("a measurement leaked into a node class with a different image") + } +} + +func TestEvictionThreshold(t *testing.T) { + capacity := corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("32Gi"), + corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"), + } + + tests := []struct { + name string + hard map[string]string + wantMemory string + }{ + {name: "absent uses kubelet default", hard: nil, wantMemory: "100Mi"}, + {name: "explicit quantity", hard: map[string]string{"memory.available": "400Mi"}, wantMemory: "400Mi"}, + {name: "percentage of capacity", hard: map[string]string{"memory.available": "10%"}, wantMemory: "3435973836"}, + {name: "unparseable yields nothing", hard: map[string]string{"memory.available": "banana"}, wantMemory: ""}, + {name: "empty falls back to default", hard: map[string]string{"memory.available": ""}, wantMemory: "100Mi"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := evictionThreshold(&apiv1.KubeletConfiguration{EvictionHard: tc.hard}, capacity) + q, ok := got[corev1.ResourceMemory] + if tc.wantMemory == "" { + if ok { + t.Errorf("expected no memory threshold, got %s", q.String()) + } + return + } + if !ok { + t.Fatal("expected a memory threshold") + } + if q.String() != tc.wantMemory { + t.Errorf("expected %s, got %s", tc.wantMemory, q.String()) + } + }) + } +} + +// A negative or malformed reservation must be dropped rather than treated as +// zero, so a typo cannot silently remove a reservation the node really applies. +func TestParseResourceList_DropsInvalid(t *testing.T) { + got := parseResourceList(map[string]string{ + "cpu": "200m", + "memory": "-512Mi", + "pid": "not-a-quantity", + }) + if _, ok := got[corev1.ResourceMemory]; ok { + t.Error("negative memory reservation was kept") + } + if _, ok := got["pid"]; ok { + t.Error("unparseable reservation was kept") + } + cpu, ok := got[corev1.ResourceCPU] + if !ok || cpu.MilliValue() != 200 { + t.Errorf("valid cpu reservation was lost: %v", got) + } +} diff --git a/pkg/providers/instancetype/instancetype.go b/pkg/providers/instancetype/instancetype.go index 8d12747..6ff3eef 100644 --- a/pkg/providers/instancetype/instancetype.go +++ b/pkg/providers/instancetype/instancetype.go @@ -29,34 +29,53 @@ type ServerTypeClient interface { type Provider struct { client ServerTypeClient + // vmMemoryOverheadPercent estimates the gap between a server type's + // advertised memory and what the guest sees, for types no node has yet + // reported. Once one has, discovered holds the measured value instead. + vmMemoryOverheadPercent float64 + mu sync.RWMutex cachedTypes []*cloudprovider.InstanceType cacheExpiry time.Time unavailable *unavailableCache + discovered *discoveredCapacityCache } // NewProvider creates a new instance type provider. -func NewProvider(client ServerTypeClient) *Provider { +func NewProvider(client ServerTypeClient, vmMemoryOverheadPercent float64) *Provider { return &Provider{ - client: client, + client: client, + vmMemoryOverheadPercent: vmMemoryOverheadPercent, unavailable: newUnavailableCache( // 5m: long enough to route around a saturated location, short enough to // retry it soon. TODO: make configurable via operator config if needed. 5 * time.Minute, ), + discovered: newDiscoveredCapacityCache(), } } -// List returns all available InstanceTypes, filtered to those with offerings in the given locations. -// Results are cached for 6 hours. -func (p *Provider) List(ctx context.Context, locations []string) ([]*cloudprovider.InstanceType, error) { +// List returns all available InstanceTypes for a node class, filtered to those +// with offerings in its locations. A nil node class lists every type with no +// location filter and no declared kubelet reservations. +// +// The 6h cache holds only what depends on the hcloud catalogue. Anything that +// depends on the node class -- its kubelet reservations, and the capacity +// measured for its image -- is applied per call, so a node class edit takes +// effect immediately instead of at the next catalogue refresh. +func (p *Provider) List(ctx context.Context, nodeClass *apiv1.HCloudNodeClass) ([]*cloudprovider.InstanceType, error) { + var locations []string + if nodeClass != nil { + locations = nodeClass.Spec.Locations + } + p.mu.RLock() if p.cachedTypes != nil && time.Now().Before(p.cacheExpiry) { cached := p.cachedTypes p.mu.RUnlock() metrics.RecordCacheHit() - return p.applyAvailability(filterByLocations(cached, locations)), nil + return p.resolve(filterByLocations(cached, locations), nodeClass), nil } p.mu.RUnlock() @@ -66,7 +85,7 @@ func (p *Provider) List(ctx context.Context, locations []string) ([]*cloudprovid // Double-check after acquiring write lock. if p.cachedTypes != nil && time.Now().Before(p.cacheExpiry) { metrics.RecordCacheHit() - return p.applyAvailability(filterByLocations(p.cachedTypes, locations)), nil + return p.resolve(filterByLocations(p.cachedTypes, locations), nodeClass), nil } // Cache miss: fetch fresh data from the hcloud API. @@ -79,13 +98,13 @@ func (p *Provider) List(ctx context.Context, locations []string) ([]*cloudprovid types := make([]*cloudprovider.InstanceType, 0, len(serverTypes)) for _, st := range serverTypes { - types = append(types, toInstanceType(st)) + types = append(types, toInstanceType(st, p.vmMemoryOverheadPercent)) } p.cachedTypes = types p.cacheExpiry = time.Now().Add(cacheTTL) - return p.applyAvailability(filterByLocations(types, locations)), nil + return p.resolve(filterByLocations(types, locations), nodeClass), nil } // MarkUnavailable records that a (serverType, location) offering failed with a @@ -96,17 +115,20 @@ func (p *Provider) MarkUnavailable(serverType, location string) { p.unavailable.markUnavailable(serverType, location) } -// applyAvailability returns copies of the given instance types with each -// offering's Available flag computed live from the unavailable cache, so the -// 6h type-catalog cache never bakes in (and thus never staleness-traps) -// availability. +// resolve returns copies of the given instance types with everything that must +// not be baked into the 6h catalogue cache applied fresh: +// +// - each offering's Available flag, from the unavailable cache, so the +// catalogue never staleness-traps availability; +// - memory capacity measured from a registered node of that type and image, +// when one has been seen, in place of the estimate; +// - the overhead implied by this node class's declared kubelet reservations. // -// The returned InstanceType and Offering structs are fresh value-copies, so -// setting Available never mutates the cached entries. Note that nested -// reference fields (Requirements, Capacity, Overhead) are intentionally shared -// with the cache, not deep-copied: callers must treat returned types as -// read-only and must not mutate those maps. -func (p *Provider) applyAvailability(types []*cloudprovider.InstanceType) []*cloudprovider.InstanceType { +// The returned InstanceType and Offering structs are fresh value-copies, and +// Capacity is rebuilt rather than shared because the discovered value differs +// per node class. Requirements stays shared read-only with the cached entry, so +// callers must not mutate it. +func (p *Provider) resolve(types []*cloudprovider.InstanceType, nodeClass *apiv1.HCloudNodeClass) []*cloudprovider.InstanceType { out := make([]*cloudprovider.InstanceType, len(types)) for i, it := range types { offerings := make(cloudprovider.Offerings, len(it.Offerings)) @@ -116,22 +138,33 @@ func (p *Provider) applyAvailability(types []*cloudprovider.InstanceType) []*clo cp.Available = !p.unavailable.isUnavailable(it.Name, zone) offerings[j] = &cp } + + capacity := make(corev1.ResourceList, len(it.Capacity)) + for k, v := range it.Capacity { + capacity[k] = v + } + if measured, ok := p.discovered.get(it.Name, nodeClass); ok { + capacity[corev1.ResourceMemory] = measured + } + // Construct a fresh InstanceType (rather than copying *it) to avoid - // copying the embedded sync.Once (govet copylocks); Requirements/Capacity/ - // Overhead are intentionally shared read-only with the cached entry. + // copying the embedded sync.Once (govet copylocks), which also matters + // because Allocatable() memoises on first call. out[i] = &cloudprovider.InstanceType{ Name: it.Name, Offerings: offerings, Requirements: it.Requirements, - Capacity: it.Capacity, - Overhead: it.Overhead, + Capacity: capacity, + Overhead: overheadFor(nodeClass, capacity), } } return out } -// toInstanceType maps a Hetzner ServerType to a Karpenter InstanceType. -func toInstanceType(st *hcloud.ServerType) *cloudprovider.InstanceType { +// toInstanceType maps a Hetzner ServerType to a Karpenter InstanceType. Only +// node-class-independent facts belong here, since the result is cached across +// every node class; Overhead is left nil and filled in by resolve. +func toInstanceType(st *hcloud.ServerType, vmMemoryOverheadPercent float64) *cloudprovider.InstanceType { arch := "amd64" if st.Architecture == hcloud.ArchitectureARM { arch = "arm64" @@ -156,8 +189,10 @@ func toInstanceType(st *hcloud.ServerType) *cloudprovider.InstanceType { }) } - // Memory: ServerType.Memory is float32 in GB. - memBytes := int64(float64(st.Memory) * 1024 * 1024 * 1024) + // Memory: ServerType.Memory is float32 in GB. Hetzner's figure is what the + // VM is allocated, not what the guest kernel ends up seeing, so hold back an + // estimate of the difference until a real node tells us the true number. + memBytes := memoryWithVMOverhead(int64(float64(st.Memory)*1024*1024*1024), vmMemoryOverheadPercent) // Disk: ServerType.Disk is int in GB. diskBytes := int64(st.Disk) * 1024 * 1024 * 1024 @@ -177,12 +212,6 @@ func toInstanceType(st *hcloud.ServerType) *cloudprovider.InstanceType { corev1.ResourceEphemeralStorage: *resource.NewQuantity(diskBytes, resource.BinarySI), corev1.ResourcePods: *resource.NewQuantity(110, resource.DecimalSI), }, - Overhead: &cloudprovider.InstanceTypeOverhead{ - KubeReserved: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("100m"), - corev1.ResourceMemory: resource.MustParse("100Mi"), - }, - }, } } diff --git a/pkg/providers/instancetype/instancetype_test.go b/pkg/providers/instancetype/instancetype_test.go index 38c3b17..c370c51 100644 --- a/pkg/providers/instancetype/instancetype_test.go +++ b/pkg/providers/instancetype/instancetype_test.go @@ -46,7 +46,7 @@ var testPricings = []hcloud.ServerTypeLocationPricing{ func TestList_NoLocationFilter(t *testing.T) { st := makeServerType("cx11", hcloud.ArchitectureX86, hcloud.CPUTypeShared, 1, 2, 20, testPricings) client := &mockServerTypeClient{types: []*hcloud.ServerType{st}} - p := NewProvider(client) + p := NewProvider(client, 0) types, err := p.List(context.Background(), nil) if err != nil { @@ -63,10 +63,10 @@ func TestList_NoLocationFilter(t *testing.T) { func TestList_LocationFilter(t *testing.T) { st := makeServerType("cx11", hcloud.ArchitectureX86, hcloud.CPUTypeShared, 1, 2, 20, testPricings) client := &mockServerTypeClient{types: []*hcloud.ServerType{st}} - p := NewProvider(client) + p := NewProvider(client, 0) // Only request nbg1; fsn1 offering should be filtered out but type still returned. - types, err := p.List(context.Background(), []string{"nbg1"}) + types, err := p.List(context.Background(), locationsNodeClass("nbg1")) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -81,9 +81,9 @@ func TestList_LocationFilter(t *testing.T) { func TestList_LocationFilterExcludesAll(t *testing.T) { st := makeServerType("cx11", hcloud.ArchitectureX86, hcloud.CPUTypeShared, 1, 2, 20, testPricings) client := &mockServerTypeClient{types: []*hcloud.ServerType{st}} - p := NewProvider(client) + p := NewProvider(client, 0) - types, err := p.List(context.Background(), []string{"hel1"}) // not in pricings + types, err := p.List(context.Background(), locationsNodeClass("hel1")) // not in pricings if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -94,7 +94,7 @@ func TestList_LocationFilterExcludesAll(t *testing.T) { func TestInstanceType_Capacity(t *testing.T) { st := makeServerType("cx21", hcloud.ArchitectureX86, hcloud.CPUTypeShared, 2, 4, 40, testPricings) - it := toInstanceType(st) + it := toInstanceType(st, 0) cpu := it.Capacity[corev1.ResourceCPU] if cpu.Value() != 2 { @@ -121,7 +121,7 @@ func TestInstanceType_Capacity(t *testing.T) { func TestInstanceType_ArchARM(t *testing.T) { st := makeServerType("cax11", hcloud.ArchitectureARM, hcloud.CPUTypeShared, 2, 4, 40, testPricings) - it := toInstanceType(st) + it := toInstanceType(st, 0) archReq := it.Requirements.Get("kubernetes.io/arch") if archReq.Any() != "arm64" { @@ -131,7 +131,7 @@ func TestInstanceType_ArchARM(t *testing.T) { func TestInstanceType_ArchX86(t *testing.T) { st := makeServerType("cx11", hcloud.ArchitectureX86, hcloud.CPUTypeShared, 1, 2, 20, testPricings) - it := toInstanceType(st) + it := toInstanceType(st, 0) archReq := it.Requirements.Get("kubernetes.io/arch") if archReq.Any() != "amd64" { @@ -176,7 +176,7 @@ func TestHourlyNetPrice(t *testing.T) { func TestList_CacheHit(t *testing.T) { st := makeServerType("cx11", hcloud.ArchitectureX86, hcloud.CPUTypeShared, 1, 2, 20, testPricings) client := &mockServerTypeClient{types: []*hcloud.ServerType{st}} - p := NewProvider(client) + p := NewProvider(client, 0) _, _ = p.List(context.Background(), nil) _, _ = p.List(context.Background(), nil) @@ -189,7 +189,7 @@ func TestList_CacheHit(t *testing.T) { func TestList_ReflectsUnavailable(t *testing.T) { st := makeServerType("cx11", hcloud.ArchitectureX86, hcloud.CPUTypeShared, 1, 2, 20, testPricings) client := &mockServerTypeClient{types: []*hcloud.ServerType{st}} - p := NewProvider(client) + p := NewProvider(client, 0) // Before marking: both offerings (nbg1, fsn1) must be available. before, err := p.List(context.Background(), nil) From a4c2ee94f97b57848a9889bc444122cfefb6dd0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kieszczy=C5=84ski?= Date: Tue, 25 Aug 2026 09:42:47 +0200 Subject: [PATCH 2/4] feat: measure server capacity from registered nodes 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. --- .../templates/deployment.yaml | 6 + charts/karpenter-provider-hetzner/values.yaml | 10 + cmd/controller/main.go | 2 + .../instancetype/capacity/controller.go | 170 +++++++++++++++ .../instancetype/capacity/controller_test.go | 195 ++++++++++++++++++ 5 files changed, 383 insertions(+) create mode 100644 pkg/controllers/instancetype/capacity/controller.go create mode 100644 pkg/controllers/instancetype/capacity/controller_test.go diff --git a/charts/karpenter-provider-hetzner/templates/deployment.yaml b/charts/karpenter-provider-hetzner/templates/deployment.yaml index 146d2a5..b63f241 100644 --- a/charts/karpenter-provider-hetzner/templates/deployment.yaml +++ b/charts/karpenter-provider-hetzner/templates/deployment.yaml @@ -68,6 +68,12 @@ spec: key: {{ .Values.auth.secretRef.key }} - name: CLUSTER_NAME value: {{ .Values.clusterName | quote }} + # Emitted unconditionally rather than through `with`, which skips + # falsy values: 0 is a legitimate setting (trust Hetzner's advertised + # memory exactly) and must reach the operator, not fall back to the + # default. + - name: VM_MEMORY_OVERHEAD_PERCENT + value: {{ .Values.vmMemoryOverheadPercent | quote }} - name: METRICS_PORT value: {{ .Values.metrics.port | quote }} - name: HEALTH_PROBE_PORT diff --git a/charts/karpenter-provider-hetzner/values.yaml b/charts/karpenter-provider-hetzner/values.yaml index 3e5cecf..9267fba 100644 --- a/charts/karpenter-provider-hetzner/values.yaml +++ b/charts/karpenter-provider-hetzner/values.yaml @@ -52,6 +52,16 @@ args: [] # Required: scopes managed servers so multiple clusters can share one Hetzner project. clusterName: "" +# Fraction of a server type's advertised memory assumed to be invisible to the +# guest. Hetzner advertises what the VM is allocated; the kernel and firmware +# take a cut that varies with machine size, so this is an estimate, used only +# until a node of that type registers and reports its real capacity. +# +# Raise it if nodes still register smaller than Karpenter expects. Lower it only +# with measurements in hand: too low tells Karpenter a server is bigger than it +# is, which strands pods on nodes that cannot hold them. +vmMemoryOverheadPercent: 0.075 + metrics: port: 8080 healthProbe: diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 36de910..e491737 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -15,6 +15,7 @@ import ( _ "github.com/paperclipinc/karpenter-provider-hetzner/pkg/apis/v1" hetznercp "github.com/paperclipinc/karpenter-provider-hetzner/pkg/cloudprovider" + instancetypecapacity "github.com/paperclipinc/karpenter-provider-hetzner/pkg/controllers/instancetype/capacity" "github.com/paperclipinc/karpenter-provider-hetzner/pkg/controllers/nodeclass" hetznerop "github.com/paperclipinc/karpenter-provider-hetzner/pkg/operator" "github.com/paperclipinc/karpenter-provider-hetzner/pkg/providers/imagefamily" @@ -74,5 +75,6 @@ func main() { op.InstanceTypeStore, ), nodeClassController, + instancetypecapacity.NewController(op.GetClient(), typeProvider), )...).Start(ctx) } diff --git a/pkg/controllers/instancetype/capacity/controller.go b/pkg/controllers/instancetype/capacity/controller.go new file mode 100644 index 0000000..9a242c6 --- /dev/null +++ b/pkg/controllers/instancetype/capacity/controller.go @@ -0,0 +1,170 @@ +// Package capacity records the memory capacity Hetzner servers actually report +// once they boot, so Karpenter can size nodes from measurements instead of an +// estimate. +// +// Hetzner advertises the memory a VM is allocated, not what the guest kernel +// ends up seeing. The difference is a few percent, it varies with the size of +// the machine, and it is invisible from the API -- so the instance type provider +// falls back to a single conservative fraction until a real node of that type +// reports its own figure. This controller supplies that figure. +// +// The AWS provider solves the same problem the same way, for the same reason. +package capacity + +import ( + "context" + "fmt" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/resource" + controllerruntime "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + karpv1 "sigs.k8s.io/karpenter/pkg/apis/v1" + + apiv1 "github.com/paperclipinc/karpenter-provider-hetzner/pkg/apis/v1" +) + +// CapacityRecorder is the instance type provider's seam for accepting a +// measurement. It reports whether the observation changed what the provider will +// use, so that only first discoveries and genuine drops are logged. +type CapacityRecorder interface { + Record(serverType string, nodeClass *apiv1.HCloudNodeClass, observed resource.Quantity) bool +} + +// Controller records capacity from registered nodes. +type Controller struct { + kubeClient client.Client + recorder CapacityRecorder +} + +func NewController(kubeClient client.Client, recorder CapacityRecorder) *Controller { + return &Controller{kubeClient: kubeClient, recorder: recorder} +} + +func (c *Controller) Name() string { + return "instancetype.capacity" +} + +// Reconcile reads one registered node's reported memory capacity and hands it to +// the instance type provider. +// +// Every path that cannot produce a trustworthy measurement returns without +// recording rather than returning an error. A node whose node pool or node class +// has been deleted is a race with teardown, not a fault, and requeueing it would +// spin against objects that are never coming back. +func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) { + node := &corev1.Node{} + if err := c.kubeClient.Get(ctx, req.NamespacedName, node); err != nil { + if apierrors.IsNotFound(err) { + return reconcile.Result{}, nil + } + return reconcile.Result{}, fmt.Errorf("getting node %s: %w", req.Name, err) + } + + // Only a registered node has finished joining and settled on the capacity it + // will report for the rest of its life. + if node.Labels[karpv1.NodeRegisteredLabelKey] != "true" { + return reconcile.Result{}, nil + } + + serverType := node.Labels[corev1.LabelInstanceTypeStable] + if serverType == "" { + return reconcile.Result{}, nil + } + + observed := node.Status.Capacity[corev1.ResourceMemory] + // Zero is the absence of a reading, not a very small machine. Recording it + // would make Karpenter believe the type holds nothing and strand every pod + // scheduled onto it. + if observed.Sign() <= 0 { + return reconcile.Result{}, nil + } + + nodeClass, err := c.nodeClassFor(ctx, node) + if err != nil || nodeClass == nil { + return reconcile.Result{}, err + } + + if c.recorder.Record(serverType, nodeClass, observed) { + log.FromContext(ctx).WithValues( + "serverType", serverType, + "nodeClass", nodeClass.Name, + "node", node.Name, + "capacity", observed.String(), + ).Info("recorded server type memory capacity measured from a registered node") + } + return reconcile.Result{}, nil +} + +// nodeClassFor resolves the node's node pool and, through it, the node class the +// server was built from. It returns nil without error when the node is not ours +// or when either object has already been deleted. +func (c *Controller) nodeClassFor(ctx context.Context, node *corev1.Node) (*apiv1.HCloudNodeClass, error) { + poolName := node.Labels[karpv1.NodePoolLabelKey] + if poolName == "" { + return nil, nil + } + + nodePool := &karpv1.NodePool{} + if err := c.kubeClient.Get(ctx, client.ObjectKey{Name: poolName}, nodePool); err != nil { + if apierrors.IsNotFound(err) { + return nil, nil + } + return nil, fmt.Errorf("getting nodepool %s: %w", poolName, err) + } + + ref := nodePool.Spec.Template.Spec.NodeClassRef + // A node pool belonging to another provider says nothing about how Hetzner + // sizes a server, even if the label happens to name a type we know. + if ref == nil || ref.Group != apiv1.Group || ref.Kind != "HCloudNodeClass" { + return nil, nil + } + + nodeClass := &apiv1.HCloudNodeClass{} + if err := c.kubeClient.Get(ctx, client.ObjectKey{Name: ref.Name}, nodeClass); err != nil { + if apierrors.IsNotFound(err) { + return nil, nil + } + return nil, fmt.Errorf("getting hcloudnodeclass %s: %w", ref.Name, err) + } + return nodeClass, nil +} + +// Register wires the controller to nodes becoming registered. +// +// A node's reported capacity does not change after it joins, so there is nothing +// to learn from watching it afterwards. The predicates narrow the watch to the +// transition into registration, plus already-registered nodes at startup so a +// fresh process rebuilds its cache from the running fleet rather than waiting for +// the next launch. +func (c *Controller) Register(_ context.Context, m manager.Manager) error { + return controllerruntime.NewControllerManagedBy(m). + For(&corev1.Node{}, builder.WithPredicates(predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + return e.Object.GetLabels()[karpv1.NodeRegisteredLabelKey] == "true" + }, + UpdateFunc: func(e event.UpdateEvent) bool { + // Only the moment registration is gained; a node that was already + // registered has nothing new to say. + if e.ObjectOld.GetLabels()[karpv1.NodeRegisteredLabelKey] != "" { + return false + } + return e.ObjectNew.GetLabels()[karpv1.NodeRegisteredLabelKey] == "true" + }, + DeleteFunc: func(event.DeleteEvent) bool { return false }, + GenericFunc: func(event.GenericEvent) bool { return false }, + })). + Named(c.Name()). + // One worker: the work is a map write behind a mutex, and serialising it + // keeps contention off the provider's read path. + WithOptions(controller.Options{MaxConcurrentReconciles: 1}). + Complete(c) +} diff --git a/pkg/controllers/instancetype/capacity/controller_test.go b/pkg/controllers/instancetype/capacity/controller_test.go new file mode 100644 index 0000000..50f5049 --- /dev/null +++ b/pkg/controllers/instancetype/capacity/controller_test.go @@ -0,0 +1,195 @@ +package capacity + +import ( + "context" + "testing" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + karpv1 "sigs.k8s.io/karpenter/pkg/apis/v1" + + apiv1 "github.com/paperclipinc/karpenter-provider-hetzner/pkg/apis/v1" +) + +// recordedCall captures what the controller handed the provider. +type recordedCall struct { + serverType string + nodeClass string + observed resource.Quantity +} + +type fakeRecorder struct{ calls []recordedCall } + +func (f *fakeRecorder) Record(serverType string, nodeClass *apiv1.HCloudNodeClass, observed resource.Quantity) bool { + name := "" + if nodeClass != nil { + name = nodeClass.Name + } + f.calls = append(f.calls, recordedCall{serverType: serverType, nodeClass: name, observed: observed}) + return true +} + +func testNodeClass() *apiv1.HCloudNodeClass { + return &apiv1.HCloudNodeClass{ + ObjectMeta: metav1.ObjectMeta{Name: "default"}, + Status: apiv1.HCloudNodeClassStatus{ + ResolvedImages: []apiv1.ResolvedImage{{Architecture: "x86", ImageID: 42}}, + }, + } +} + +func testNodePool(group, kind string) *karpv1.NodePool { + return &karpv1.NodePool{ + ObjectMeta: metav1.ObjectMeta{Name: "workers"}, + Spec: karpv1.NodePoolSpec{ + Template: karpv1.NodeClaimTemplate{ + Spec: karpv1.NodeClaimTemplateSpec{ + NodeClassRef: &karpv1.NodeClassReference{Name: "default", Group: group, Kind: kind}, + }, + }, + }, + } +} + +// testNode is a registered Karpenter-owned node reporting a real cx53's capacity. +func testNode() *corev1.Node { + return &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "worker-1", + Labels: map[string]string{ + corev1.LabelInstanceTypeStable: "cx53", + karpv1.NodePoolLabelKey: "workers", + karpv1.NodeRegisteredLabelKey: "true", + }, + }, + Status: corev1.NodeStatus{ + Capacity: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("32089152Ki"), + }, + }, + } +} + +func reconcileNode(t *testing.T, node *corev1.Node, objs ...client.Object) *fakeRecorder { + t.Helper() + _ = apiv1.SchemeBuilder.AddToScheme(scheme.Scheme) + + all := append([]client.Object{node}, objs...) + kube := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(all...).Build() + + rec := &fakeRecorder{} + c := NewController(kube, rec) + if _, err := c.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{Name: node.Name}, + }); err != nil { + t.Fatalf("unexpected error: %v", err) + } + return rec +} + +func TestReconcile_RecordsRegisteredNodeCapacity(t *testing.T) { + rec := reconcileNode(t, testNode(), testNodePool(apiv1.Group, "HCloudNodeClass"), testNodeClass()) + + if len(rec.calls) != 1 { + t.Fatalf("expected 1 recorded observation, got %d", len(rec.calls)) + } + got := rec.calls[0] + if got.serverType != "cx53" { + t.Errorf("expected server type cx53, got %q", got.serverType) + } + if got.nodeClass != "default" { + t.Errorf("expected node class default, got %q", got.nodeClass) + } + if got.observed.String() != "32089152Ki" { + t.Errorf("expected the node's reported capacity, got %s", got.observed.String()) + } +} + +// An unregistered node has not finished joining, so its reported capacity is not +// yet trustworthy. +func TestReconcile_IgnoresUnregisteredNode(t *testing.T) { + node := testNode() + delete(node.Labels, karpv1.NodeRegisteredLabelKey) + + rec := reconcileNode(t, node, testNodePool(apiv1.Group, "HCloudNodeClass"), testNodeClass()) + if len(rec.calls) != 0 { + t.Errorf("expected no observation from an unregistered node, got %d", len(rec.calls)) + } +} + +// Nodes Karpenter does not own tell us nothing about a Hetzner server type as +// this provider builds it. +func TestReconcile_IgnoresNodeWithoutNodePool(t *testing.T) { + node := testNode() + delete(node.Labels, karpv1.NodePoolLabelKey) + + rec := reconcileNode(t, node) + if len(rec.calls) != 0 { + t.Errorf("expected no observation from a node with no node pool, got %d", len(rec.calls)) + } +} + +// A node pool pointing at another provider's node class is not ours to measure. +// +// The HCloudNodeClass is seeded deliberately: without it the lookup would 404 +// and the test would pass whether or not the group and kind are checked at all. +func TestReconcile_IgnoresForeignNodeClass(t *testing.T) { + rec := reconcileNode(t, testNode(), testNodePool("karpenter.k8s.aws", "EC2NodeClass"), testNodeClass()) + if len(rec.calls) != 0 { + t.Errorf("expected no observation for a foreign node class, got %d", len(rec.calls)) + } +} + +func TestReconcile_IgnoresNodeWithoutInstanceType(t *testing.T) { + node := testNode() + delete(node.Labels, corev1.LabelInstanceTypeStable) + + rec := reconcileNode(t, node, testNodePool(apiv1.Group, "HCloudNodeClass"), testNodeClass()) + if len(rec.calls) != 0 { + t.Errorf("expected no observation without an instance type, got %d", len(rec.calls)) + } +} + +// A node reporting no memory is reporting an absence of data, not a very small +// machine; recording it would strand every pod on that type. +func TestReconcile_IgnoresZeroCapacity(t *testing.T) { + node := testNode() + node.Status.Capacity = corev1.ResourceList{corev1.ResourceMemory: resource.MustParse("0")} + + rec := reconcileNode(t, node, testNodePool(apiv1.Group, "HCloudNodeClass"), testNodeClass()) + if len(rec.calls) != 0 { + t.Errorf("expected no observation for zero capacity, got %d", len(rec.calls)) + } +} + +// A missing node pool or node class is a race with deletion, not an error worth +// retrying: the node is on its way out. +func TestReconcile_ToleratesMissingNodePool(t *testing.T) { + rec := reconcileNode(t, testNode()) + if len(rec.calls) != 0 { + t.Errorf("expected no observation when the node pool is gone, got %d", len(rec.calls)) + } +} + +// A deleted node must not error the reconcile loop. +func TestReconcile_ToleratesMissingNode(t *testing.T) { + _ = apiv1.SchemeBuilder.AddToScheme(scheme.Scheme) + kube := fake.NewClientBuilder().WithScheme(scheme.Scheme).Build() + rec := &fakeRecorder{} + c := NewController(kube, rec) + + if _, err := c.Reconcile(context.Background(), reconcile.Request{ + NamespacedName: types.NamespacedName{Name: "gone"}, + }); err != nil { + t.Fatalf("expected no error for a missing node, got %v", err) + } + if len(rec.calls) != 0 { + t.Errorf("expected no observation, got %d", len(rec.calls)) + } +} From 16bd48582611a2bfeb732586eadec96a8156a890 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kieszczy=C5=84ski?= Date: Tue, 25 Aug 2026 09:43:28 +0200 Subject: [PATCH 3/4] docs: document kubelet reservations and VM memory overhead --- README.md | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/README.md b/README.md index 4d33277..08ebd79 100644 --- a/README.md +++ b/README.md @@ -149,9 +149,56 @@ Provisioned nodes carry, in addition to the well-known Karpenter labels: | `labels` | `map[string]string` | no | — | Extra hcloud labels on the Hetzner server (useful for cost attribution or firewall label-selectors) | | `userData` | `string` | no | — | Inline cloud-init / Talos machine config. Overridden by `userDataSecretRef` when both are set. | | `userDataSecretRef` | `object {namespace, name, key}` | no | — | Source `userData` from a Secret instead of inline. The Secret is read at server-create time; its value never appears in the NodeClass spec or git. Takes precedence over `userData`. | +| `kubelet.systemReserved` | `map[string]string` | no | — | What your bootstrap passes to the kubelet as `--system-reserved`. Keys: `cpu`, `memory`, `ephemeral-storage`, `pid`. | +| `kubelet.kubeReserved` | `map[string]string` | no | — | What your bootstrap passes as `--kube-reserved`. Same keys. | +| `kubelet.evictionHard` | `map[string]string` | no | `memory.available: 100Mi`, `nodefs.available: 10%` | What your bootstrap passes as `--eviction-hard`. Values are a quantity (`400Mi`) or a percentage (`10%`). | Status exposes `conditions` (`ImagesReady`, `NetworkReady`, `ResourcesReady`, `UserDataReady`, aggregated into `Ready`) and `resolvedImages` (image ID per architecture). +### Declaring kubelet reservations + +Karpenter decides which server type a pod fits on by subtracting reserved +resources from a type's capacity. It cannot see your bootstrap, so anything your +userData reserves has to be declared here as well: + +```yaml +spec: + kubelet: + systemReserved: {cpu: 200m, memory: 512Mi} + kubeReserved: {cpu: 200m, memory: 512Mi} + evictionHard: {memory.available: 400Mi} +``` + +**These values describe your userData; they do not configure it.** Setting them +here does not change what the node reserves, and the two must be kept in +agreement. Declaring less than the bootstrap actually reserves is the failure +worth knowing about: Karpenter then believes a server is larger than it is, +picks one too small for the pod, and the pod stays `Pending` while the empty node +is consolidated away and replaced — indefinitely, without an error anywhere. + +Karpenter's own `nodeclaim/consistency` check will not catch this. It compares +capacity rather than allocatable, and only reports a shortfall beyond 10%. + +If your bootstrap sets no reservations, omit the block: the kubelet's own +defaults are modelled already. + +### Advertised vs. usable memory + +Hetzner advertises the memory a VM is allocated, but the guest kernel never sees +all of it — firmware, the kernel image and per-page structures take a cut that +grows with the size of the machine. A server advertised as 32Gi reports about +31.3Gi; one advertised as 4Gi reports about 3.7Gi. + +The provider holds back `VM_MEMORY_OVERHEAD_PERCENT` (default `0.075`) to cover +this. It is only an estimate, and it is only used until a node of that server +type and image registers: from then on the capacity that node reported is used +instead, keyed by server type and resolved image. Look for +`recorded server type memory capacity measured from a registered node` in the +logs. + +If nodes still register smaller than Karpenter expected, raise the percentage. +Lower it only with measurements in hand. + ## Examples Ready-to-apply examples live in the [`examples/`](examples/) directory. Each @@ -177,6 +224,7 @@ comments explaining every field. |---------|----------|-------------| | `HCLOUD_TOKEN` | yes | Hetzner Cloud API token | | `CLUSTER_NAME` | yes | Cluster identifier; scopes managed servers | +| `VM_MEMORY_OVERHEAD_PERCENT` | no (0.075) | Fraction of advertised memory assumed invisible to the guest, used until a node of that type reports its real capacity. Must be `>= 0` and `< 1`. | | `METRICS_PORT` | no (8080) | Prometheus metrics port | | `HEALTH_PROBE_PORT` | no (8081) | Health/readiness probe port | From d6e1b3c11e3211e716151c41c8822fcbd7c8eb4f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Kieszczy=C5=84ski?= Date: Tue, 25 Aug 2026 11:47:40 +0200 Subject: [PATCH 4/4] fix: keep the previous reserve when no kubelet block is declared 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. --- README.md | 7 ++- pkg/providers/instancetype/allocatable.go | 24 ++++++++-- .../instancetype/allocatable_test.go | 48 +++++++++++++++++++ 3 files changed, 74 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 08ebd79..4cb77c2 100644 --- a/README.md +++ b/README.md @@ -179,8 +179,11 @@ is consolidated away and replaced — indefinitely, without an error anywhere. Karpenter's own `nodeclaim/consistency` check will not catch this. It compares capacity rather than allocatable, and only reports a shortfall beyond 10%. -If your bootstrap sets no reservations, omit the block: the kubelet's own -defaults are modelled already. +Omitting the block leaves the previous behaviour in place — a flat `100m`/`100Mi` +reserve plus the kubelet's own default eviction thresholds — so upgrading cannot +silently raise a node's advertised capacity. Declaring a block replaces that +default with exactly what you state, so declare everything your bootstrap +reserves, not just the parts that differ. ### Advertised vs. usable memory diff --git a/pkg/providers/instancetype/allocatable.go b/pkg/providers/instancetype/allocatable.go index 56875d5..ff48a56 100644 --- a/pkg/providers/instancetype/allocatable.go +++ b/pkg/providers/instancetype/allocatable.go @@ -36,13 +36,31 @@ func overheadFor(nodeClass *apiv1.HCloudNodeClass, capacity corev1.ResourceList) overhead := &cloudprovider.InstanceTypeOverhead{ EvictionThreshold: evictionThreshold(kubelet, capacity), } - if kubelet != nil { - overhead.KubeReserved = parseResourceList(kubelet.KubeReserved) - overhead.SystemReserved = parseResourceList(kubelet.SystemReserved) + if kubelet == nil { + // A node class that declares nothing has said nothing about its bootstrap, + // which is not the same as saying it reserves nothing. Before this package + // read reservations from the node class it subtracted a flat 100m/100Mi + // from every type; keeping that as the undeclared default means upgrading + // cannot silently raise a node's advertised capacity, which would push pods + // onto machines that never had room for them. + overhead.KubeReserved = legacyDefaultKubeReserved() + return overhead } + overhead.KubeReserved = parseResourceList(kubelet.KubeReserved) + overhead.SystemReserved = parseResourceList(kubelet.SystemReserved) return overhead } +// legacyDefaultKubeReserved is what this provider reserved on every server type +// before reservations became declarable. It applies only when a node class omits +// the kubelet block entirely; one that declares a block is taken at its word. +func legacyDefaultKubeReserved() corev1.ResourceList { + return corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("100Mi"), + } +} + // evictionThreshold models the memory and disk the kubelet holds back to keep // itself above its hard eviction signals. That headroom is unavailable to pods, // so it reduces allocatable exactly as a reservation does. diff --git a/pkg/providers/instancetype/allocatable_test.go b/pkg/providers/instancetype/allocatable_test.go index 50e57ee..7124962 100644 --- a/pkg/providers/instancetype/allocatable_test.go +++ b/pkg/providers/instancetype/allocatable_test.go @@ -120,6 +120,54 @@ func TestAllocatable_WithinBudgetOfRegisteredNode(t *testing.T) { } } +// TestAllocatable_NoKubeletConfigKeepsLegacyReservation pins the upgrade path. +// +// Before this package read reservations from the node class it subtracted a flat +// 100m/100Mi from every type. A node class that declares no kubelet block has +// said nothing about its bootstrap, so silently dropping that to zero would raise +// advertised CPU on upgrade -- the wrong direction, and invisible. Absent means +// "unchanged", not "nothing reserved". +func TestAllocatable_NoKubeletConfigKeepsLegacyReservation(t *testing.T) { + st := makeServerType("cx23", hcloud.ArchitectureX86, hcloud.CPUTypeShared, 2, 4, 40, testPricings) + client := &mockServerTypeClient{types: []*hcloud.ServerType{st}} + p := NewProvider(client, operator.DefaultVMMemoryOverheadPercent) + + types, err := p.List(context.Background(), &apiv1.HCloudNodeClass{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + cpu := types[0].Allocatable()[corev1.ResourceCPU] + if cpu.MilliValue() != 1900 { + t.Errorf("expected 1900m (2 cores less the legacy 100m reserve), got %dm", cpu.MilliValue()) + } +} + +// A node class that does declare a kubelet block is taken at its word: the +// legacy default must not be added on top of what the operator stated. +func TestAllocatable_DeclaredKubeletOverridesLegacyDefault(t *testing.T) { + st := makeServerType("cx23", hcloud.ArchitectureX86, hcloud.CPUTypeShared, 2, 4, 40, testPricings) + client := &mockServerTypeClient{types: []*hcloud.ServerType{st}} + p := NewProvider(client, operator.DefaultVMMemoryOverheadPercent) + + nc := &apiv1.HCloudNodeClass{ + Spec: apiv1.HCloudNodeClassSpec{ + Kubelet: &apiv1.KubeletConfiguration{ + KubeReserved: map[string]string{"cpu": "250m"}, + }, + }, + } + types, err := p.List(context.Background(), nc) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + cpu := types[0].Allocatable()[corev1.ResourceCPU] + if cpu.MilliValue() != 1750 { + t.Errorf("expected 1750m (2 cores less the declared 250m), got %dm", cpu.MilliValue()) + } +} + // TestAllocatable_NoKubeletConfig falls back to advertised-minus-VM-overhead // when the NodeClass declares no reservations. The result must still not exceed // the machine's real capacity, since the VM overhead applies regardless.