Repository navigation
Add CPU placement policy, CPU topology and pool reporting - #155
Conversation
The device-side code in this branch needs API that has not landed upstream yet: the VmConfig CPU placement fields, the CPU topology and capability reporting on device info, ZInfoDevice.cpu_pools, API_CAPABILITY_CPU_PLACEMENT_POLICY and ErrorInfo.error_code. Without them nothing here compiles, so this commit points pillar and evetest at the fork carrying the proposed API (lf-edge/eve-api#155) through a replace directive, and vendors it. This commit exists only so the branch can be built, run and reviewed while the API is under discussion. It must be dropped and replaced by an ordinary `make bump-eve-api` once the API lands in lf-edge/eve-api: a replace directive pointing at a personal fork breaks dependency tracking and SBOM/licensing, and is never acceptable on master. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
de1d6f3 to
5304c9d
Compare
5304c9d to
bcbb2e3
Compare
bcbb2e3 to
8dd8bac
Compare
5bf89ef to
debcbb0
Compare
| API_CAPABILITY_REPORT_TPM_EVENTLOG = 21; // Support for reporting TPM Event Log "as is" without parsing and selectively reporting events | ||
| API_CAPABILITY_APP_INSTANCE_NET_INTERFACE_CHANGE = 22; // Support for adding and removing network interfaces without purge, but restart or the edge app | ||
| API_CAPABILITY_DEFERRED_QUEUE_METRICS = 23; // Reports deviceMetric.deferred_queue and the per-URL retriable, rejected and delivered counts | ||
| API_CAPABILITY_CPU_PLACEMENT_POLICY = 24; // VmConfig CPU placement (cpu_policy incl. CPU_POLICY_ISOLATED, pin_cpu as the pinning choice, full_pcpus_only, threads_per_core, numa_policy, io_placement): each field is applied, or the workload is refused with a structured error_code. Advertised only where EVE implements CPU placement |
There was a problem hiding this comment.
A device which supports this will also always(?) include a CPUPoolUtilization cpu_pools with at least the housekeeping pool, correct?
Thus one could have the controllers check against that. But perhaps it is still helpful to have an explicit capability to signal this explicitly?
There was a problem hiding this comment.
@eriknordmark yes, correct. current controller and UI do these checks. but since this feature will be evolving, does it make sense to add "sub capabilities" or we will introduce to many capabilities potentially.
There was a problem hiding this comment.
@rucoder Can OptionalCapabilities be used for as you said 'sub capabilities' ?
There was a problem hiding this comment.
@xyuria-zededa optional is really for things which might exist only in some images flavors (we use it for EVE-k).
@rucoder are we only adding this for amd64 images and not arm64? If so OptionalCapabilities makes a lot of sense.
There was a problem hiding this comment.
With an OptionalCapabilties field there is no need for an API capability. Remove it.
debcbb0 to
224b97d
Compare
The device-side code in this branch needs API that has not landed upstream yet: the VmConfig CPU placement fields, the CPU topology and capability reporting on device info, ZInfoDevice.cpu_pools, API_CAPABILITY_CPU_PLACEMENT_POLICY and ErrorInfo.error_code. Without them nothing here compiles, so this commit points pillar and evetest at the fork carrying the proposed API (lf-edge/eve-api#155) through a replace directive, and vendors it. This commit exists only so the branch can be built, run and reviewed while the API is under discussion. It must be dropped and replaced by an ordinary `make bump-eve-api` once the API lands in lf-edge/eve-api: a replace directive pointing at a personal fork breaks dependency tracking and SBOM/licensing, and is never acceptable on master. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
224b97d to
8cdf5ac
Compare
The device-side code in this branch needs API that has not landed upstream yet: the VmConfig CPU placement fields, the CPU topology and kernel isolation in the hardware inventory, ZInfoDevice.cpu_pools, ZInfoApp.cpu_placement_status and assigned_cpus, and the CPU placement capabilities. Without them nothing here compiles, so this commit points pillar and evetest at the fork carrying the proposed API (lf-edge/eve-api#155) through a replace directive, and vendors it. This commit exists only so the branch can be built, run and reviewed while the API is under discussion. It must be dropped and replaced by an ordinary `make bump-eve-api` once the API lands in lf-edge/eve-api: a replace directive pointing at a personal fork breaks dependency tracking and SBOM/licensing, and is never acceptable on master. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
The device-side code in this branch needs API that has not landed upstream yet: the VmConfig CPU placement fields, the CPU topology and kernel isolation in the hardware inventory, ZInfoDevice.cpu_pools, ZInfoApp.cpu_placement_status and assigned_cpus, and the CPU placement capabilities. Without them nothing here compiles, so this commit points pillar and evetest at the fork carrying the proposed API (lf-edge/eve-api#155) through a replace directive, and vendors it. This commit exists only so the branch can be built, run and reviewed while the API is under discussion. It must be dropped and replaced by an ordinary `make bump-eve-api` once the API lands in lf-edge/eve-api: a replace directive pointing at a personal fork breaks dependency tracking and SBOM/licensing, and is never acceptable on master. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
| enum CoreClass { | ||
| // EVE could not determine the class. | ||
| CORE_CLASS_UNSPECIFIED = 0; | ||
| CORE_CLASS_PERFORMANCE = 1; // e.g. Intel P-core, ARM "big" |
There was a problem hiding this comment.
Comment could follow the pattern and be above the definition....
There was a problem hiding this comment.
@rene which style do we follow? above the definition or on the right?
xyuria-zededa
left a comment
There was a problem hiding this comment.
Looks good from the api perspective
8cdf5ac to
0f6f3c2
Compare
The device-side code in this branch needs API that has not landed upstream yet: the VmConfig CPU placement fields, the CPU topology and kernel isolation in the hardware inventory, ZInfoDevice.cpu_pools, ZInfoApp.cpu_placement_status and assigned_cpus, and the CPU placement capabilities. Without them nothing here compiles, so this commit points pillar and evetest at the fork carrying the proposed API (lf-edge/eve-api#155) through a replace directive, and vendors it. This commit exists only so the branch can be built, run and reviewed while the API is under discussion. It must be dropped and replaced by an ordinary `make bump-eve-api` once the API lands in lf-edge/eve-api: a replace directive pointing at a personal fork breaks dependency tracking and SBOM/licensing, and is never acceptable on master. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
|
@eriknordmark could you re-review ? I think I addressed all the issues |
| | `hv_type_kubevirt` | Device runs the Kubevirt hypervisor | required before sending `EdgeNodeCluster`; the KVM flavor cannot join a cluster | | ||
| | `hw_inventory_support` | Device can produce `HardwareInventory` | distinguishes "found no hardware" from "cannot report" when `ZInfoHardware.inventory` is empty | | ||
| | `etcd_snapshot` | Device supports etcd snapshots | EVE-k cluster operations | | ||
| | `cpu_placement` | Device applies `VmConfig.cpu_policy`, `.pin_cpu` as the pinning choice, `.full_pcpus_only`, `.threads_per_core`, `.numa_policy`, `.io_placement`, each served or the workload refused, and reports `ZInfoApp.cpu_placement_status`, `.assigned_cpus` and `ZInfoDevice.cpu_pools`. Set only on KVM with a readable CPU topology | required before sending any of those fields: Xen, EVE-k or a node whose topology EVE cannot read does not place CPUs; only an EVE at `CPU_PLACEMENT_POLICY` sets it | |
There was a problem hiding this comment.
Xen is gone so don't list it here. What about ARM images?
| | `REPORT_TPM_EVENTLOG` = 21 | rpt | `ZAttestQuote.tpm_binary_event_log`, superseding the deprecated `ZAttestQuote.event_log` | | ||
| | `APP_INSTANCE_NET_INTERFACE_CHANGE` = 22 | cfg | adding or removing an `AppInstanceConfig.interfaces` entry needs only `restart`, not `purge`, so the app keeps its volumes; the `AppInstanceConfig` comment still describes the older purge requirement **(?)** | | ||
| | `DEFERRED_QUEUE_METRICS` = 23 | rpt | `deviceMetric.deferred_queue`, and `urlcloudMetric.retriableErrCount`, `.rejectedErrCount`, `.deliveredMsgCount` | | ||
| | `CPU_PLACEMENT_POLICY` = 24 | cfg | `VmConfig.cpu_policy`, `.pin_cpu` as the pinning choice, `.full_pcpus_only`, `.threads_per_core`, `.numa_policy`, `.io_placement` are known, legacy `pin_cpu` follows the same rules, and outcomes are reported in `ZInfoApp.cpu_placement_status` and `.assigned_cpus`. Every build reports it; whether a node places CPUs is `OptionalCapabilities.cpu_placement` | |
There was a problem hiding this comment.
No need for this given cpu_placement optional below.
| // stays until the config or the node changes, as for an unavailable | ||
| // adapter. A value keeps its meaning for good; a retired one is reserved, | ||
| // never reused. | ||
| enum CPUPlacementStatus { |
There was a problem hiding this comment.
Odd to have a combination of status codes (non, placed, pending, repack_advised) and error enums.
Better to split in a Status enum (with a failed value) and an Error enum (set if failed).
| // 1. "ZInfoDevice.optional_capabilities.cpu_placement" is set: the node | ||
| // places CPUs. Only an EVE at "API_CAPABILITY_CPU_PLACEMENT_POLICY" or | ||
| // later sets it; an older one ignores these fields; | ||
| // 2. for CPU_POLICY_DEDICATED, CPU_POLICY_ISOLATED or pin_cpu=true, the |
There was a problem hiding this comment.
Condition 2 names pin_cpu=true, so together with condition 1 this reads as "a controller MUST NOT send pin_cpu=true unless optional_capabilities.cpu_placement is set". That would rule out pin_cpu on every EVE predating this API and on EVE-k, which never sets cpu_placement but honors pin_cpu today (kubevirt maps it to DedicatedCPUPlacement).
I assume the MUST NOT is meant only for fields 28-32. Could you say so explicitly, and state what a controller may send in pin_cpu to a node without cpu_placement?
Topology-aware CPU placement for VM and container workloads. The controller picks the pool a workload's CPUs come from and whether its vCPUs are pinned; the device reports the topology, isolation and pool state the controller needs to decide what it may ask for. Every field has one meaning whatever the others say, and a request the device does not serve is refused, never reinterpreted. A controller learns two things from two places: what the node has (the hardware inventory) and whether the node applies the fields (OptionalCapabilities.cpu_placement). The inventory is never a gate; it sizes requests. - config/vm.proto: VmConfig.cpu_policy picks the pool: HOUSEKEEPING (the default pool for workloads as well as EVE's processes, holding the CPUs EVE reserves for its services), DEDICATED (CPUs of the workload's own) or ISOLATED (CPUs the kernel isolates, isolcpus). pin_cpu is the pinning choice on any pool: each vCPU on a host CPU of its own. full_pcpus_only, threads_per_core, numa_policy and io_placement refine a placement on the workload's own CPUs. With cpu_policy unset, pin_cpu=true means a dedicated workload, pinned on a VM, as for a controller that does not send cpu_policy. cpus is deprecated. The section comment lists what a controller checks before sending the new fields, and what pin_cpu means on a node that does not place CPUs. - info/hardware.proto: each logical CPU reports its socket, core, NUMA node, L2 and L3 cache domains, core class and base and maximum frequency, and CPUInfo lists the cache domains with the CPUs sharing each. The coordinates are optional: a CPU without socket_id and core_id means the topology could not be read. KernelCPUIsolation reports the CPUs the running kernel isolates (isolcpus) and runs tickless (nohz_full). A PCI device reports its local CPUs, whose NUMA node is the device's. - info/info.proto: ZInfoDevice.cpu_pools reports each pool's CPUs, free CPUs and thread and whole-core counts, so a controller can tell whether a workload fits before deploying it and explain a refusal after. A pool also carries warnings about how the node defines it (CPUPoolWarning: a CPUPoolIssue and its ErrorInfo), so far a mistake in isolcpus: covering the CPUs reserved for EVE, or isolating only part of a core. ZInfoApp.cpu_placement_status (CPUPlacementStatus) says where a workload stands with its CPU request: placed, waiting for CPUs a repack frees, part of a repack, or failed; for a failure ZInfoApp.cpu_placement_error (CPUPlacementError) says why. appErr keeps the text and retry_condition. The enums are targeted rather than a code in ErrorInfo, which is reported for many things besides apps; a value keeps its meaning for good, and a retired one is reserved. ZInfoApp.assigned_cpus reports the CPUs a workload of the dedicated or isolated pool holds, as assignedAdapters reports its PCI devices: its vCPUs' host CPUs in vCPU order, the idle siblings it parks and the shared CPUs its IO threads run on. - Capabilities: OptionalCapabilities.cpu_placement says this node places CPUs (KVM on any architecture, a CPU topology EVE could read); a controller sends the new fields only where it is set. An EVE that predates them does not set it, so no APICapability value is needed. CAPABILITIES.md lists it. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
0f6f3c2 to
c338d02
Compare
This EVE API update brings in the CPU placement fields, see lf-edge/eve-api#155: the CPU topology, cache domains and kernel CPU isolation in the hardware inventory, ZInfoDevice.cpu_pools, ZInfoApp.cpu_placement_status, cpu_placement_error and assigned_cpus, and OptionalCapabilities.cpu_placement. Adam stores device info re-encoded with protojson, which drops fields its eve-api does not know, so without this bump those fields are lost before evetest can read them. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
The controller re-serializes what a device sent using its own copy of the API, so fields newer than that copy never reach a test. 0.0.85 carries the eve-api with the CPU placement fields (lf-edge/eve-api#155, through lf-edge/adam#163), which the CPU placement tests assert on. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
lf-edge/eve-api#155 has merged: the CPU placement fields in VmConfig, the CPU topology, cache domains and kernel CPU isolation in the hardware inventory, the CPU pools on ZInfoDevice, the per-app placement status, error and assigned CPUs on ZInfoApp, and OptionalCapabilities.cpu_placement. Re-pin every subproject that imports eve-api/go with "make bump-eve-api", so the CPU placement code that follows builds against a real revision. That also picks up RuntimeStatus for runtime AppInstances, which landed since the previous pin. Nothing populates it yet, and the advertised ApiCapability is unchanged. pkg/pillar keeps the vendored pkg/kube/cnirpc that master carries: it is ahead of the version go.mod pins, and "go mod vendor" would revert it. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
Adds the API for topology-aware CPU placement of edge applications: the
controller states what a workload needs, the device picks the host CPUs, and
reports what it can do and what it did.
The device side is lf-edge/eve#6335. It builds against this branch (
c338d02)through a
replacedirective until this merges, then takes it withmake bump-eve-api. Both sides were tested together: EVE's evetest CPU placementtests with an Adam built on this API, and end to end against a controller and UI
built on it.
Two pieces: what the node has, and whether it applies the fields
KernelCPUIsolationOptionalCapabilitiescpu_placement: KVM on any architecture, with a CPU topology EVE could readThe controller gates the placement fields (28-32) on
optional_capabilities.cpu_placement; an EVE that predates them does not setit, so no
APICapabilityvalue is needed.pin_cpupredates them and may goto any node. The inventory is never a gate: reporting is not permission (cache
domains are reported on every node, but that does not mean EVE can allocate
cache). It sizes requests instead, e.g. two-thread cores before
threads_per_core=2, isolated CPUs beforeISOLATED. State sits beside thetwo: what is free now (
cpu_pools) and what each app got(
cpu_placement_status,cpu_placement_error,assigned_cpus).Config: what a workload asks for (
config/vm.proto)Two independent choices, each meaning the same whatever the other says:
cpu_policypicks the pool the workload's CPUs come from:HOUSEKEEPING(thedefault pool for workloads as well as EVE's processes, holding the CPUs EVE
reserves for its services),
DEDICATED(CPUs of its own, no other workload onthem) or
ISOLATED(CPUs of its own that the kernel isolates withisolcpus).pin_cpu(field 20) is the pinning choice on any pool: each vCPU of a VM on ahost CPU of its own. With
falseEVE pins nothing, and the workload's threadsrun on the pool's CPUs as the kernel or the workload places them; a real-time
container in the isolated pool sets its own affinity. A container has no vCPUs
to pin.
full_pcpus_only,threads_per_core,numa_policyandio_placementrefine aDEDICATEDorISOLATEDrequest: whole physical cores, how many SMT threads ofeach the workload uses, NUMA confinement, and where a VM's non-vCPU threads run.
The first three follow the Kubernetes CPU and Topology Managers.
A request the device cannot serve as sent, including a combination this EVE does
not implement, is refused, and
ZInfoApp.cpu_placement_errorsays why; nothingis reinterpreted or silently dropped. With
cpu_policyunset,pin_cpu=truemeans a dedicated workload, pinned on a VM, for a controller that does not send
cpu_policy; with both unset, the workload runs in the housekeeping pool.Info: what the node has, and what it did
info/hardware.proto: each logical CPU reports its socket, physical core,NUMA node, L2 and L3 cache domains, core class and base and maximum frequency;
CPUInfo.cacheslists the cache domains with the CPUs sharing each. Thecoordinates are
optional: a value the device cannot read is unset, neverzero.
CoreClassis reported for information; placement does not use it.KernelCPUIsolationreports the CPUs the running kernel isolates (isolcpus)and runs tickless (
nohz_full). PCI devices report their local CPUs, whoseNUMA node is the device's.
info/info.proto:ZInfoDevice.cpu_pools(62): each pool's CPUs, free CPUs, and thread andwhole-core counts, so a controller can tell whether a workload fits before
deploying it. A pool also carries
warnings(CPUPoolWarning: aCPUPoolIssueand itsErrorInfo) for mistakes inisolcpus.ZInfoApp.assigned_cpus(21): the CPUs a dedicated or isolated workloadholds, as
assignedAdaptersreports its PCI devices.ZInfoApp.cpu_placement_status(22): where a workload stands with its CPUrequest, and
ZInfoApp.cpu_placement_error(23): why it failed, set onlywith
FAILED.appErrkeeps the text andretry_condition. Targetedenums rather than a code in
ErrorInfo, which is reported for many thingsbesides apps; a value keeps its meaning for good, and a retired one is
reserved.
CPU_PLACEMENT_STATUS_PLACEDWAITINGREPACK_ADVISEDFAILEDcpu_placement_errorsays why (ERROR)CPU_PLACEMENT_ERROR_NEEDS_REPACKINSUFFICIENTPOLICY_INVALIDODD_VCPUthreads_per_core=2with an odd vCPU countTOPOLOGY_UNSUPPORTEDISOLATION_UNAVAILABLEISOLATEDon a node that isolates no CPUThe controller's one automatic action is the repack: it restarts every app
with
NEEDS_REPACKor inREPACK_ADVISEDtogether. The device retries norefusal; it stays until the config or the node changes, as for an
unavailable adapter.