refactor: clean up hardware discovery - #781
Draft
klihub wants to merge 44 commits into
Draft
Conversation
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Ed Bartosh <eduard.bartosh@intel.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
klihub
force-pushed
the
refactor/hardware-discovery
branch
2 times, most recently
from
September 10, 2026 23:16
2b7acaf to
a929c26
Compare
The controller applied CPU frequency, uncore frequency and C-state settings to the CPUs assigned to each class in control.cpu.classes. It cannot have applied anything for some time: the assignments it reads are only ever written by its own Assign(), which is uncalled as the package is imported for its registration alone. So it read its configuration, found no assignments, and enforced nothing. pkg/resmgr/cpuclass does all of it now, in internal/cpufreq, internal/cpuidle and internal/uncorefreq, and balloons has driven that instead since cpuClasses were implemented. topology-aware has never referenced this controller at all. Leave the controllers configuration intact for now, simply giving a deprecation warning if we encounter one. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
NewCpuTreeFromSystem discovered a sysfs.System of its own, presumably only to explicitly force CPU and cache topology discovery. But DiscoverSystem ignores flags and discovers everything anyway, so that information was already present in the instance the policy has in its options. So take it as an argument instead and use the policy provided one. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Test packages which ship a recorded sysfs tree as a tarball unpack it into a local testdata directory when the tests run, leaving thousands of generated files behind for the next git add to potentially pick up. Ignore those directories. One rule matching testdata anywhere replaces the three which named a directory each, since every test package which does this wants the same thing. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
System.CPU, System.Node and System.Package returned a nil pointer inside a non-nil interface for an id the machine does not have. Such a value is never == nil, so callers' nil checks were dead code and the call after them would have panicked. Return an untyped nil instead, and stop dereferencing without checking in SingleThreadForCPUs and NodeDistance. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Define CPUSet and AnyCPUSet interfaces for storing sets of CPUs. Add a dense bitmask-based CpuMask implementation and a sparse CpuSet wrapping the k8s.io cpuset.CPUSet. Add units tests for each. Also, add a benchmark to check how the implementations perform in various operations as the number of maximum and stored CPUs grow. A nil set reads as the empty one. Every operation which does not modify a set takes a nil receiver, and a nil operand, as empty, which is Go's own rule for nil maps and slices and is what the k8s cpuset value type gave its callers for free. Set, Clear and Seal panic instead, saying what to do about it: no method can allocate a set and store it back into the caller's variable, so EmptyIfNil and Clone are how a caller gets one it can modify. Assisted-by: copilot-cli Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Add pkg/lib/hardware, a leaner replacement for pkg/sysfs. Discover reads a machine once and returns an immutable Machine which is safe to share. Reading goes through an io/fs.FS rooted at the host root, so WithRoot points discovery at a mounted host filesystem and WithFS substitutes a recorded or synthetic one. Handles are concrete, interned and nil-safe: a lookup for absent hardware answers Valid() == false rather than returning a typed nil whose methods then panic, as pkg/sysfs does. CPU sets are libcpu.CpuMask, sealed, so they are safe to share and panic if modified. A Zone is a named set of CPUs at one Level. Zones deliberately do not form a tree: whether a cluster sits inside a NUMA node or spans several is a property of the machine, and which levels are worth nesting differs per caller. Zones(level) is complete, and SameZones says when two levels cut the machine the same way, which is what the policies ask by hand today. TopologyIndex flattens a Machine into a coordinate lookup table for callers which consult it per allocation. The convenience layer holds the groupings several consumers had each grown their own version of: cache groups, logical clusters, thread rounding and closest-node queries. Unit tests cover the readers with fstest.MapFS, and discovery against the recorded sysfs trees plus synthetic ones, asserting invariants rather than fixed numbers. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Reimplement pkg/sysfs on top of pkg/lib/hardware, with the same names and the same signatures, so that a consumer moves over by changing one import line. Most already import pkg/sysfs aliased to "system", which is why the package is called that: for those files even the alias stays. This is a migration step and a proof. pkg/sysfs stays in the tree beside it, so equivalence_test.go can run every method of both against the same recorded topology and compare the answers. It compares everything, including the methods no caller uses, since those are the ones a reimplementation gets wrong unnoticed. test-setup.sh unpacks the six recorded machines the other packages already keep, rather than adding another copy of them here. internal/dropin holds one body of code compiled against both packages, so the compiler enforces that the surfaces match, and a test enforces that the two files are the same code. One pkg/sysfs quirk is copied deliberately: a NUMA node with no CPUs of its own reports package 0, which is the zero value of a field pkg/sysfs never assigns rather than a real package. hardware.MemoryNode says -1 instead. SST lives here rather than in hardware, and moves on to whoever still wants it. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Swap pkg/sysfs for pkg/lib/hardware/system everywhere but pkg/sysfs's own tests and the three places whose job is to compare against it. Nothing else changes: the drop-in presents the same names and signatures, so files which imported pkg/sysfs aliased to "system" keep even the alias, and the rest keep the name under an explicit one. This extra intermediate step lets us do an extra round of verification for the new implementation by running end-to-end tests and checking that everything passes. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The topology was discovered in policy.NewPolicy, as a sysfs.System. Discover it in NewResourceManager instead, as a hardware.Machine, and hand it down through policy.Options to the backends. For now we keep both sysfs.System and hardware.Machine in the policy backend options, but the former essentially come from the latter through the compatibility wrapper. Once we have converted everything to hardware.Machine, sysfs.System can be removed from the options. Note that the env overrides have to be asked for explicitly here, as the drop-in asked for them on the callers' behalf. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The policy takes its topology from the Machine the resource manager discovered, rather than from the pkg/sysfs interface wrapped around it. cpuallocator, cpuclass and libmem still want that interface, so they keep being handed the System from the backend options; the policy itself no longer reads it. toCpuSet and toCpuMask convert at the seam. The policy is written in cpuset.CPUSet and stays that way for now. Once we have removed all the remaining dependencies on sysfs.System, we can update the internals here to use libcpu.CpuMask, which should scale much better with the number of CPUs present. Building the L2 cache level no longer iterates a map, so a node's cache children come out in cache id order instead of a different order on every run. The set of nodes is unchanged; on all six recorded machines the tree matches what the previous code built. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
WithSystemNodes read the NUMA nodes through the pkg/sysfs interface. WithMachineNodes reads them from an already discovered hardware.Machine instead, so the package no longer depends on that interface at all. A node hardware could not classify maps to TypeDRAM rather than panicking as TypeForSysfs did on an unknown type. There is no type here for "do not know", and ordinary memory is what such a node arrived as before. Capacities are read during discovery rather than being re-read here. They cannot have changed, and a node whose meminfo cannot be read fails discovery, so the error this used to return now comes earlier. The public set types are unchanged: this is about where the topology comes from, not how sets are represented. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
NewCPUAllocator took a pkg/sysfs System. It takes an already discovered hardware.Machine now, and the topology cache is built from that and its TopologyIndex, so the package no longer depends on that interface. SST moves here with it. Topology discovery has nothing to do with SST -- it is a property of the running platform rather than of its shape -- and this is the only package which ever asked, so the probing is now done here, once per machine rather than once per package. Clusters come from hardware.LogicalClusters/MergeSingleCoreClusters call, which is equivalent to what the old sysfs interface reported. The public set types are unchanged. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
ParseFileEntries has nothing to do with CPU or memory topology. It parses a file of key and value lines, and lived in pkg/sysfs only because that is where it was written. Move ParseFileEntries to a pkg/utils/parse of its own, beside pkg/utils/cpuset and pkg/utils/topology, so that any further parsing helpers have somewhere to land without crowding the names in pkg/utils. It loses the prefix its package now carries and is parse.FileEntries. Its tests move with it. Fix an error with wrong formatting verbs vs. arguments. Also, remove the unused ParseEnabled in pkg/utils/parse.go instead of moving it here. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
GetMemoryCapacity reads MemTotal out of /proc/meminfo and has nothing to do with CPU or memory topology either. Move it from pkg/sysfs to pkg/utils. Not to pkg/utils/parse with the file entry parsing: it reads one fixed file rather than parsing what it is handed, so a parse.MemoryCapacity would promise the wrong thing. pkg/kubernetes takes it from there now, which is all it wanted from the topology packages. pkg/sysfs and the drop-in keep it, forwarding, as they do the parsing. With this the drop-in no longer imports pkg/sysfs for anything, which is what has to be true before pkg/sysfs can go. pkg/kubernetes has accessors of its own for the capacity the OOM adjustment estimates are calculated against, and neither needs to be exported. SetMemoryCapacity said why it was: so that the estimator tests could vary the capacity, which they could only do from outside because they were in kubernetes_test rather than in the package. Move the test into the package, as cpuset_test.go already is, and the setter can be unexported. Its GetMemoryCapacity goes altogether, never having been called by anything. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The plugin discovered a pkg/sysfs System of its own. It discovers a hardware.Machine instead and reads the topology from that, so it no longer depends on that interface. It never set a sysfs root, so the machine is discovered below "/" as before. This also fixes which nodes "cpu-packages" selects. It asks each memory node which package it is in, and a node with no CPUs of its own has no package to answer with: pkg/sysfs said 0 for those, not because they are in package 0 but because that is the zero value of a field it never assigned. On a machine with HBM, CXL or PMEM that meant every such node counted as package 0, so a container on package 0 got all of them, including the ones attached to another package, and a container on any other package got none. Such a node now belongs to the package of the nearest node which does have CPUs, which is what the kernel's distances say about where the memory is and the only thing there is to go on. On the topology the n6-hbm-cxl end-to-end suite describes, the two HBM and two CXL nodes are each unambiguously nearer one of the two packages, and that is what they are reported as now. Nothing exercised "cpu-packages": it appears in the sample and helm configurations as an example class and in no test, and the two lines of comment above it were its only specification. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The handler and its cpufreq, pct and uncorefreq internals read the topology through the pkg/sysfs interface. They take an already discovered hardware.Machine now, so none of them depends on that interface. Two of them get smaller rather than just different. pct declared a four-method subset of the interface for its Allocator to depend on, of which it called two, and those only to find one online CPU's frequency range. Its Sys is those two methods now, which a Machine satisfies as it is. uncorefreq's DiesForCpus walked every die of a CPU's package looking for the die which contained the CPU. A hardware.CPU answers with its own die, so it asks. The tests keep their shape. pct's fake was a Sys, a CPUPackage and a CPU built by embedding the interfaces and overriding a few methods; it is two methods returning nothing now, which is all the tests ever exercised through it. The handler's uncore tests described a cpu -> (pkg, die) layout to a fake System; they write the same layout out as sysfs and discover a machine from it, since a Machine cannot be faked. Those tests exercise real discovery now, and a new case covers DiesForCpus across several packages and dies, which none of them did. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The tests faked the pkg/sysfs interface: a mockSystem with all 35 of its methods, plus a mockCPUPackage, a mockCPU and a mockSystemNode, each built by embedding the interface and overriding the few methods a test needed. That works only for an interface, and the topology this policy is being moved onto is concrete types. Describe the machine instead and read it back through discovery. synthMachine writes a topology out as sysfs and discovers it, so a test says what nodes and CPUs it wants and gets the real thing. A node with memory and no CPUs of its own comes back as PMEM or HBM depending on its size, which is how the hardware package classifies one, so the memory kinds a test asks for are expressed as sizes rather than asserted into a fake. The mocks were worth less than their 328 lines suggest. The hint tests passed an entirely empty mockSystem, twice, to check that an absent socket and an absent NUMA node yield nothing; a machine with one CPU says the same. The coldstart test is the only other user and has been skipped for a while, for the very reason this addresses: it could not mock enough of the system. Its fixture is kept, as nodes to synthesize, and it stays skipped here. mockContainer, mockPod, mockCache and mockCPUAllocator stay. They have nothing to do with topology and five test files use them. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
NewHint took the pkg/sysfs interface to resolve a hint's NUMA nodes and CPUs against. It takes a hardware.Machine now, so the package no longer depends on that interface. Its one caller reaches the topology through the pool node it is scoring, so node gains a Machine alongside its System. The policy already has a machine in its backend options; the System goes when the policy itself stops reading it. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The policy took its topology from the pkg/sysfs interface. It reads the hardware.Machine the resource manager discovered instead, so it no longer depends on that interface at all. The questions it asks are all about one package or one die, and the hardware package addresses dies, clusters and caches by their full coordinates, since the kernel numbers them within their package. topology.go turns the former into the latter and keeps the conversions between the two set types in one place. The node filters the interface offered become predicates on a memory node there too. Two things which had been interface values are gone from the pool node. Its system.CPUPackage is the package zone, and what it was asked for -- the NUMA nodes of the package, and of one of its dies -- is a question about which nodes are local to a set of CPUs. Its system.Node is a hardware.MemoryNode. The tests which discovered a System beside a Machine now discover only the Machine, there being nothing left to read the former. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The hint tests could only check that a socket or NUMA hint naming something the machine does not have resolves to nothing. A TODO beside them asked for the other half and said why it was missing: the package could not be constructed, being a closed struct behind an interface. A machine can be described and discovered, so it can. Two sockets of two CPUs, one NUMA node each, and a hint naming either resolves to that socket's or that node's CPUs. This is the case which would have caught getting the package lookup wrong, as opposed to getting a miss wrong. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The metrics collector read the NUMA nodes and CPUs it reports through the pkg/sysfs interface. It reads them from the machine instead. It is the last thing in the tree which read that interface. Its memory figures come from one read of a node's meminfo now rather than from a MemoryInfo which returned both, and a node it cannot read reports zero capacity as well as zero usage. Reporting the capacity discovery recorded beside a usage which could not be read would be stating more than is known. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
BackendOptions carried the machine and, wrapped around the same machine, the pkg/sysfs interface, for backends which had not been moved over. All of them have, and the metrics collector was the last thing in the tree to read it, so the field goes, along with the policy's own copy and the sys root it needed. pkg/sysfs and the drop-in over it are now referenced by nothing but each other and their own tests. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Nothing in the repository uses it any more. Mark it deprecated so that tooling says so, and keep it for a release, since anything outside the repository which uses it deserves one in which to move over. The notice names the replacement and says how the two differ, which is more useful than pointing at the new package and leaving the reader to discover that a machine is discovered once, that lookups return handles which are never nil, that dies and cores are addressed by their coordinates, and that SST is not there at all. The drop-in stays too, and its own note is updated to say why: the migration step it provided is done, but it is what compares this package against the hardware package over the recorded machines, and that comparison cannot outlive its subject. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The test was disabled a year ago because the mocked system it ran against had no CPUs. It builds a machine of its own now, so that reason is gone and the skip goes with it. One thing was left over from the mocks. The memory type and cold start preferences reached the policy through GetResmgrAnnotation, which is not what it reads them with; they are annotations scoped to a container, resolved by GetEffectiveAnnotation. The mock pod's fields for the old form have no users left and go too. What the test asserts is what cold start promises: the container starts on the PMEM node alone, and the DRAM node joins it once the timer expires. Both halves fail if broken deliberately, so it is worth having back. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The allocator took and returned k8s cpuset sets, so a caller holding a mask converted on the way in and back on the way out, and the allocator converted the machine's own sets a third time to fill its topology cache. It takes masks now: the cache shares the machine's sealed sets instead of copying them, and the set algebra underneath every allocation is a good deal cheaper than the map-based sets it replaces. The from parameter loses its pointer. It was one because allocating takes the allocated CPUs out of the set, which a mask does natively, so the &-taking at the call sites goes with it. A CpuMask has no usable zero value where a cpuset.CPUSet has, so what used to read as an empty set now reads as a nil pointer: a map with no entry for a core kind the machine does not have, a struct field nobody assigned, an array of priorities with holes in it. Those read through EmptyIfNil. Nothing tested what allocating and releasing do to the set they are given, which is the whole way a caller learns what is left, so pin that. Releasing leaves the released CPUs in the set and returns the ones kept; its debug message said the opposite and now agrees with the code. The two policies still keep their own CPU sets as cpuset.CPUSet and convert where they call this. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The policy kept every CPU set as a k8s cpuset set and converted to a mask wherever it asked the hardware package or the CPU allocator something. Its own sets are masks now, so the CPU tree the allocator walks per admission compares masks rather than maps, and the conversions at the topology and allocator edges go away. What is left of the seam is where interfaces this policy calls still take cpuset sets: the CPU class controller, the IRQ affinity helpers, libmem's CPUSetAffinity, and the configuration which parses an operator's cpuset string. Fifteen call sites, all of them cold, against internals which no longer convert at all. Nothing here modifies a set in place, so nothing aliases: every operation builds a new mask, as it built a new cpuset before. The sets which used to fall out of a zero value are now created where they belong, in New and in the balloon constructors, since a mask has no usable zero value. The two map lookups which relied on that -- a CPU tree test asking about a balloon it has not created yet, and its accumulator of allocation rounds -- read through EmptyIfNil. Metric labels are unchanged: a mask spells itself the same way. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The policy kept its CPU sets as k8s cpuset sets and converted wherever it asked the hardware package or the CPU allocator something. Its own sets are masks now: the pool supplies, the grants, the hint scoring, the isolated and sliceable sets. Two things fall out of the type. takeCPUs took a destination set which all ten of its callers passed as nil, and a pointer to a source set so that it could write back what was left; the destination goes, and the allocator takes what it allocated out of the mask it is handed. A dry run, which works on a copy so that the supply is left alone, now says that rather than pointing at one. pkg/utils/topology comes along. It scores hints for this policy and has no other caller, so leaving it in cpuset sets would have meant converting into it and back three times per check. kubernetes.ShortCPUSet takes the libcpu interface, since all it ever wanted was to spell a set. Its test turned out to compare the unshortened form, so the shortening was never checked; with the comparison fixed the function needed fixing too, as it emitted segments out of order, and two of the expectations it had been measured against were wrong themselves. What is left of the seam is the CPU class controller, the IRQ affinity helpers, and the configuration which parses an operator's cpuset string. The sets which used to fall out of a zero value are created where they belong, in New. The tests which build a policy field by field, or leave a set out of a table, say so through EmptyIfNil. Saved state is unaffected: a grant's CPUs are persisted as the string they always were. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
An interrupt's affinity went in and out of this package as a k8s cpuset set, so both policies converted at the call and back at the return, having nothing but masks on either side. Nothing about what reaches procfs changes: the cache still writes the set's String() to smp_affinity_list and parses what it reads back, and a mask spells itself the same way. The affinity maps keep telling "no entry" from "empty set" with the two-value form, which is what they always did. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The CPU class controller took k8s cpuset sets throughout: the allowed set it is configured with, the CPUs a class is applied to, the allocation intent it scores hints against, and the candidate sets those hints hand back. Both policies had masks on their side of every one of those calls. Its own state and its inward edges go the same way. The sets pct keeps per package, the ones cpufreq keeps per domain, and the CPUs it reads out of goresctrl's SST types are all masks now, so nothing is converted anywhere inside either. That empties the seam. What is left of it in the policies is libmem's CPUSetAffinity, and the configuration, which parses an operator's cpuset string; balloons keeps one converter for each and topology-aware one. The k8s type is still used to parse a list of NUMA nodes in two places, which is what it is, and not a set of CPUs. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The DRA plugin, the CPU class controller's DRA paths and the policy-side adapter between them all took and returned k8s cpuset sets. Everything on either side of those calls is a mask now, so they take masks too. That is the plugin's interfaces in deps.go, its claim state and CDI writer, PickHpCpus, ReleaseHpCpus and AccountHpCpus down to the punit sets pct keeps for DRA holds, the adapter which routes between them, and the CPU sets the policy parses out of claim attributes and container cpusets. Nothing crosses the kubelet boundary as a set: a device is published by name and a claim identified by UID, so what the plugin says to the outside world does not change. This is here rather than in the DRA work itself because the type it converts to arrives with the commits below. Should the two be reordered, this is the piece which moves. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
The tests which needed a system faked one by embedding the sysfs.System interface. A hardware.Machine is a concrete type and cannot be faked, so they describe the machine they want and read it back through real discovery, which is what the rest of these suites already do. pct is the interesting one. Its Sys is two methods, both of which a Machine has, so the fake goes entirely and the tests hand it real machines with the package layouts the old fake described. That turned out to be worth less than it looks: the only thing pct reads out of a system is discoverTurboInfo, which no test entered, so nothing depended on the layout at all. TestDiscover- TurboInfo now does, which takes that function from no coverage to most of it and gives the machines something to be right about. The DRA tests take a machine each: the policy's own use oneCpuMachine, which is already there, and the CPU class ones are in an external test package and get a small machine of their own. Both are minimal on purpose, since those tests drive the PCT allocator through the Speed Select mock. The tests which built a policy backend hand it a machine now rather than a system, and the four which built one with neither get the smallest machine there is, since NewPolicy refuses to make a policy without one. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
klihub
force-pushed
the
refactor/hardware-discovery
branch
from
September 11, 2026 06:53
a929c26 to
1e75d57
Compare
libmem was the last consumer keeping a converter of its own. A memory node holds the set of CPUs closest to it, and that set is now a mask: WithMachineNodes hands the machine's own set straight to NewNode instead of listing it out and rebuilding it, and CPUSetAffinity takes anything which satisfies the libcpu interface, since all it does is ask what the set intersects. NewNode still takes its own copy of the CPUs, and now seals it. CloseCPUs hands that copy out, and a cpuset.CPUSet, which this used to be, could not be modified by the receiver either, so sealing is what keeps the node's set as immutable as it was. That retires balloons' toCpuSet. The only k8s set left in that policy comes in from the configuration, so toCpuMask stays. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
An Amount is a string, and the cpuset one is parsed on the way in, so nothing about this reaches the wire: the CRD schema for these fields is a plain string either way, and no type in the config API holds a set of CPUs. The return type of the parse method was the last seam, and both policies stitched it with a toCpuMask of their own right at the call. ParseCPUSet keeps its name, since it names what it parses, which is what the field is, and matches the AmountCPUSet and PrefixCPUSet around it. Only what it returns changes. Both toCpuMask helpers go with it. The k8s parser stays in both policies for the NUMA node lists, which are written in cpuset syntax but are not CPUs. The parsers agree on what they accept, empty string included, so only a malformed cpuset reads differently. libcpu names the offending list in its own error, and every caller here already says which amount it was reading, so ParseCPUSet no longer wraps what it gets: the message keeps the two parts it had rather than growing a third. Assisted-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
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.
Notes: So, this is a biggie... or rather a hugie. But I hope that it just looks scarier than it is in the end.
This is actually the really long head of a stream of changes I've been working on, the tail of which is an attempt to clean up some of the most offending/annoying bits in the topology-aware policy. That tail is still unfinished work, so it's not part of this commit set.
Anyway, as part of that work, I attempted a (maybe half-a{rs,ssist}ed) refactor of hardware discovery and abstraction we've accumulated over time into pkg/sysfs.System into a new pkg/lib/hardware.Machine abstraction, aiming for a smaller core API footprint + some convenience helpers for oft used functionality on top of it.
There is a lot of churn here in going through all the motions to
While going through all this some useful self-contained improvements fell out as side-effects, for instance
A big part of that churn is in intermediate steps most of which content-wise falls out in the end when all is said and done. The justification for the numerous intermediate steps is to keep them small and self-contained, touching one logical piece at a time, and the primary goal being to allow us to run end-to-end test after each change and verify that they all pass, giving much bigger confidence that the (assisted) conversions did not miss anything obvious, introducing bugs/regressions.
This is now at a point, where it makes sense to start filing it as a set of smaller (logically or gh-physically) stacked PRs and start merging it from the bottom of the stack... but only with one important precondition: we are happy and content enough with the replacement for sysfs.System and confident that anything remaining we'll be able to handle with much smaller further changes.
If we're not there wrt. the new abstraction, then we should focus on review-hammering exactly those bits until we're happy with it. So my most important ask and primary intention for filing this is to get that reviewed, so I can rework this with any necessary changes and then move on/switch back to my original work item of the topology-aware cleanups.
I will try to file a stacked version of this, splitting it into multiple smaller logical pieces, I just haven't gotten the strength/time to do it now/today.
Notes: This draft is rebased on the pending DRA PR#774 from @bart0sh (using its latest version at the time of the rebase), so I can readily resolve conflicts and make any necessary related changes for the bits it brings in. The introduced DRA end-to-end test still pass.