nrt: rewrite resource accounting for scope=container

Rewrite the accounting of NUMA-local resources when
using scope=container. The previous code was too lenient
and worked mostly by side effects when dealing with
non-NUMA affine resources.

A non-NUMA affine resource (aka a hostlevel resource)
is a resource which is not guaranteed to always have
a NUMA affinity. CPU and memory (incl. hugepages) always do,
but devices may or may not, both options are legal for
device plugins.

Similarly, ephemeral storage is a prominent example of resource
which should never have a NUMA affinity.
The accounting in this case was wrong because previously the
resource was considered NUMA affine.

Note: it's legal to configure topology updaters (e.g. NFD)
to not advertise CPU and memory in NRT objects.
Thus is best to treat lack of them as warnings, not
as blocking errors.

However if the per-NUMA affine counters go negative
this is definitely an error condition we need to detect
and be very loud about it.

Signed-off-by: Francesco Romani <[email protected]>

Author: ffromani

pkg/noderesourcetopology/filter.go

@@ -20,7 +20,6 @@ import (
 	"context"
 
 	v1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/resource"
 	"k8s.io/klog/v2"
 	v1qos "k8s.io/kubernetes/pkg/apis/core/v1/helper/qos"
 	kubeletconfig "k8s.io/kubernetes/pkg/kubelet/apis/config"
@@ -69,7 +68,7 @@ func singleNUMAContainerLevelHandler(lh logr.Logger, pod *v1.Pod, zones topology
 
 	for _, container := range pod.Spec.Containers {
 		clh := lh.WithValues(logging.KeyContainer, container.Name, logging.KeyContainerKind, logging.KindContainerApp)
-		clh.V(6).Info("app container resources", stringify.ResourceListToLoggable(container.Resources.Requests)...)
+		clh.V(6).Info("container requests", stringify.ResourceListToLoggable(container.Resources.Requests)...)
 
 		numaID, match := resourcesAvailableInAnyNUMANodes(clh, nodes, container.Resources.Requests, qos, nodeInfo)
 		if !match {
@@ -80,8 +79,12 @@ func singleNUMAContainerLevelHandler(lh logr.Logger, pod *v1.Pod, zones topology
 
 		// subtract the resources requested by the container from the given NUMA.
 		// this is necessary, so we won't allocate the same resources for the upcoming containers
-		subtractFromNUMA(clh, nodes, numaID, container)
-		clh.V(4).Info("app container placed", "numaCell", numaID)
+		err := subtractResourcesFromNUMANodeList(clh, nodes, numaID, qos, container.Resources.Requests)
+		if err != nil {
+			// this is an internal error which should never happen
+			return framework.NewStatus(framework.Error, "inconsistent resource accounting", err.Error())
+		}
+		clh.V(4).Info("container aligned", "numaCell", numaID)
 	}
 	return nil
 }
@@ -156,13 +159,6 @@ func resourcesAvailableInAnyNUMANodes(lh logr.Logger, numaNodes NUMANodeList, re
 	return numaID, ret
 }
 
-func isResourceSetSuitable(qos v1.PodQOSClass, resource v1.ResourceName, quantity, numaQuantity resource.Quantity) bool {
-	if qos != v1.PodQOSGuaranteed && isNUMAAffineResource(resource) {
-		return true
-	}
-	return numaQuantity.Cmp(quantity) >= 0
-}
-
 func singleNUMAPodLevelHandler(lh logr.Logger, pod *v1.Pod, zones topologyv1alpha2.ZoneList, nodeInfo *framework.NodeInfo) *framework.Status {
 	lh.V(5).Info("pod level single NUMA node handler")
 
@@ -221,28 +217,6 @@ func (tm *TopologyMatch) Filter(ctx context.Context, cycleState *framework.Cycle
 	return status
 }
 
-// subtractFromNUMA finds the correct NUMA ID's resources and subtract them from `nodes`.
-func subtractFromNUMA(lh logr.Logger, nodes NUMANodeList, numaID int, container v1.Container) {
-	for i := 0; i < len(nodes); i++ {
-		if nodes[i].NUMAID != numaID {
-			continue
-		}
-
-		nRes := nodes[i].Resources
-		for resName, quan := range container.Resources.Requests {
-			nodeResQuan := nRes[resName]
-			nodeResQuan.Sub(quan)
-			// we do not expect a negative value here, since this function only called
-			// when resourcesAvailableInAnyNUMANodes function is passed
-			// but let's log here if such unlikely case will occur
-			if nodeResQuan.Sign() == -1 {
-				lh.V(4).Info("resource quantity should not be a negative value", "resource", resName, "quantity", nodeResQuan.String())
-			}
-			nRes[resName] = nodeResQuan
-		}
-	}
-}
-
 func filterHandlerFromTopologyManagerConfig(conf TopologyManagerConfig) filterFn {
 	if conf.Policy != kubeletconfig.SingleNumaNodeTopologyManagerPolicy {
 		return nil

pkg/noderesourcetopology/numaresources.go

@@ -17,10 +17,89 @@ limitations under the License.
 package noderesourcetopology
 
 import (
+	"fmt"
+	"reflect"
+
+	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	v1helper "k8s.io/kubernetes/pkg/apis/core/v1/helper"
+	"sigs.k8s.io/scheduler-plugins/pkg/noderesourcetopology/stringify"
 )
 
+type NUMANode struct {
+	NUMAID    int
+	Resources corev1.ResourceList
+	Costs     map[int]int
+}
+
+func (n *NUMANode) WithCosts(costs map[int]int) *NUMANode {
+	n.Costs = costs
+	return n
+}
+
+func (n NUMANode) DeepCopy() NUMANode {
+	ret := NUMANode{
+		NUMAID:    n.NUMAID,
+		Resources: n.Resources.DeepCopy(),
+	}
+	if len(n.Costs) > 0 {
+		ret.Costs = make(map[int]int)
+		for key, val := range n.Costs {
+			ret.Costs[key] = val
+		}
+	}
+	return ret
+}
+
+func (n NUMANode) Equal(o NUMANode) bool {
+	if n.NUMAID != o.NUMAID {
+		return false
+	}
+	if !reflect.DeepEqual(n.Costs, o.Costs) {
+		return false
+	}
+	return equalResourceList(n.Resources, o.Resources)
+}
+
+func equalResourceList(ra, rb corev1.ResourceList) bool {
+	if len(ra) != len(rb) {
+		return false
+	}
+	for key, valA := range ra {
+		valB, ok := rb[key]
+		if !ok {
+			return false
+		}
+		if !valA.Equal(valB) {
+			return false
+		}
+	}
+	return true
+}
+
+type NUMANodeList []NUMANode
+
+func (nnl NUMANodeList) DeepCopy() NUMANodeList {
+	ret := make(NUMANodeList, 0, len(nnl))
+	for idx := 0; idx < len(nnl); idx++ {
+		ret = append(ret, nnl[idx].DeepCopy())
+	}
+	return ret
+}
+
+func (nnl NUMANodeList) Equal(oth NUMANodeList) bool {
+	if len(nnl) != len(oth) {
+		return false
+	}
+	for idx := 0; idx < len(nnl); idx++ {
+		if !nnl[idx].Equal(oth[idx]) {
+			return false
+		}
+	}
+	return true
+}
+
 func isHostLevelResource(resource corev1.ResourceName) bool {
 	// host-level resources are resources which *may* not be bound to NUMA nodes.
 	// A Key example is generic [ephemeral] storage which doesn't expose NUMA affinity.
@@ -52,3 +131,50 @@ func isNUMAAffineResource(resource corev1.ResourceName) bool {
 	// We can't tell for sure, so we default to "no".
 	return false
 }
+
+func isResourceSetSuitable(qos corev1.PodQOSClass, resource corev1.ResourceName, quantity, numaQuantity resource.Quantity) bool {
+	if qos != corev1.PodQOSGuaranteed && isNUMAAffineResource(resource) {
+		return true
+	}
+	return numaQuantity.Cmp(quantity) >= 0
+}
+
+// subtractResourcesFromNUMANodeList finds the correct NUMA ID's resources and always subtract them from `nodes` in-place.
+func subtractResourcesFromNUMANodeList(lh logr.Logger, nodes NUMANodeList, numaID int, qos corev1.PodQOSClass, containerRes corev1.ResourceList) error {
+	logEntries := []any{"numaCell", numaID}
+
+	for _, node := range nodes {
+		if node.NUMAID != numaID {
+			continue
+		}
+
+		lh.V(5).Info("NUMA resources before", append(logEntries, stringify.ResourceListToLoggable(node.Resources)...)...)
+
+		for resName, resQty := range containerRes {
+			isAffine := isNUMAAffineResource(resName)
+			if qos != corev1.PodQOSGuaranteed && isAffine {
+				lh.V(4).Info("ignoring QoS-depending exclusive request", "resource", resName, "QoS", qos)
+				continue
+			}
+			if resQty.IsZero() {
+				lh.V(4).Info("ignoring zero-valued request", "resource", resName)
+				continue
+			}
+			nResQ, ok := node.Resources[resName]
+			if !ok {
+				lh.V(4).Info("ignoring missing resource", "resource", resName, "affine", isAffine, "request", resQty.String())
+				continue
+			}
+			nodeResQty := nResQ.DeepCopy()
+			nodeResQty.Sub(resQty)
+			if nodeResQty.Sign() < 0 {
+				lh.V(1).Info("resource quantity should not be a negative value", "numaCell", numaID, "resource", resName, "quantity", nResQ.String(), "request", resQty.String())
+				return fmt.Errorf("resource %q request %s exceeds NUMA %d availability %s", string(resName), resQty.String(), numaID, nResQ.String())
+			}
+			node.Resources[resName] = nodeResQty
+		}
+
+		lh.V(5).Info("NUMA resources after", append(logEntries, stringify.ResourceListToLoggable(node.Resources)...)...)
+	}
+	return nil
+}