Merge pull request #752 from ffromani/nrt-hostlevel-redo

[noderesourcetopology] rewrite accounting of numa-affine resources with scope=container

Author: k8s-ci-robot

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"
@@ -55,32 +54,38 @@ func singleNUMAContainerLevelHandler(lh logr.Logger, pod *v1.Pod, zones topology
 	// https://kubernetes.io/docs/concepts/workloads/pods/init-containers/#understanding-init-containers
 	// therefore, we don't need to accumulate their resources together
 	for _, initContainer := range pod.Spec.InitContainers {
-		lh.V(6).Info("init container desired resources", stringify.ResourceListToLoggable(initContainer.Resources.Requests)...)
+		// TODO: handle sidecar explicitely (new kind)
+		clh := lh.WithValues(logging.KeyContainer, initContainer.Name, logging.KeyContainerKind, logging.KindContainerInit)
+		clh.V(6).Info("desired resources", stringify.ResourceListToLoggable(initContainer.Resources.Requests)...)
 
-		_, match := resourcesAvailableInAnyNUMANodes(lh, nodes, initContainer.Resources.Requests, qos, nodeInfo)
+		_, match := resourcesAvailableInAnyNUMANodes(clh, nodes, initContainer.Resources.Requests, qos, nodeInfo)
 		if !match {
 			// we can't align init container, so definitely we can't align a pod
-			lh.V(2).Info("cannot align container", "name", initContainer.Name, "kind", "init")
+			clh.V(2).Info("cannot align container")
 			return framework.NewStatus(framework.Unschedulable, "cannot align init container")
 		}
 	}
 
 	for _, container := range pod.Spec.Containers {
-		// TODO: add containerName
-		lh.V(6).Info("app container resources", stringify.ResourceListToLoggable(container.Resources.Requests)...)
+		clh := lh.WithValues(logging.KeyContainer, container.Name, logging.KeyContainerKind, logging.KindContainerApp)
+		clh.V(6).Info("container requests", stringify.ResourceListToLoggable(container.Resources.Requests)...)
 
-		numaID, match := resourcesAvailableInAnyNUMANodes(lh, nodes, container.Resources.Requests, qos, nodeInfo)
+		numaID, match := resourcesAvailableInAnyNUMANodes(clh, nodes, container.Resources.Requests, qos, nodeInfo)
 		if !match {
 			// we can't align container, so definitely we can't align a pod
-			lh.V(2).Info("cannot align container", "name", container.Name, "kind", "app")
+			clh.V(2).Info("cannot align container")
 			return framework.NewStatus(framework.Unschedulable, "cannot align container")
 		}
 
 		// 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(lh, nodes, numaID, container)
+		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)
 	}
-	lh.V(2).Info("can align all containers")
 	return nil
 }
 
@@ -150,17 +155,10 @@ func resourcesAvailableInAnyNUMANodes(lh logr.Logger, numaNodes NUMANodeList, re
 
 	// at least one NUMA node is available
 	ret := !bitmask.IsEmpty()
-	lh.V(2).Info("final verdict", "suitable", ret)
+	lh.V(2).Info("final verdict", "suitable", ret, "numaCell", numaID)
 	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")
 
@@ -172,11 +170,12 @@ func singleNUMAPodLevelHandler(lh logr.Logger, pod *v1.Pod, zones topologyv1alph
 	logNumaNodes(lh, "pod handler NUMA resources", nodeInfo.Node().Name, nodes)
 	lh.V(6).Info("pod desired resources", stringify.ResourceListToLoggable(resources)...)
 
-	if _, match := resourcesAvailableInAnyNUMANodes(lh, createNUMANodeList(lh, zones), resources, v1qos.GetPodQOS(pod), nodeInfo); !match {
+	numaID, match := resourcesAvailableInAnyNUMANodes(lh, createNUMANodeList(lh, zones), resources, v1qos.GetPodQOS(pod), nodeInfo)
+	if !match {
 		lh.V(2).Info("cannot align pod", "name", pod.Name)
 		return framework.NewStatus(framework.Unschedulable, "cannot align pod")
 	}
-	lh.V(2).Info("can align pod")
+	lh.V(4).Info("all container placed", "numaCell", numaID)
 	return nil
 }
 
@@ -218,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/logging/logging.go

@@ -26,11 +26,13 @@ import (
 
 // well-known structured log keys
 const (
-	KeyLogID  string = "logID"
-	KeyPod    string = "pod"
-	KeyPodUID string = "podUID"
-	KeyNode   string = "node"
-	KeyFlow   string = "flow"
+	KeyLogID         string = "logID"
+	KeyPod           string = "pod"
+	KeyPodUID        string = "podUID"
+	KeyNode          string = "node"
+	KeyFlow          string = "flow"
+	KeyContainer     string = "container"
+	KeyContainerKind string = "kind"
 )
 
 const (
@@ -42,6 +44,11 @@ const (
 	FlowCacheSync string = "resync"
 )
 
+const (
+	KindContainerInit string = "init"
+	KindContainerApp  string = "app"
+)
+
 const (
 	SubsystemForeignPods string = "foreignpods"
 	SubsystemNRTCache    string = "nrtcache"

pkg/noderesourcetopology/numaresources.go

@@ -17,10 +17,91 @@ 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 +133,83 @@ 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
+}
+
+func subtractFromNUMAs(resources corev1.ResourceList, numaNodes NUMANodeList, nodes ...int) {
+	for resName, quantity := range resources {
+		for _, node := range nodes {
+			// quantity is zero no need to iterate through another NUMA node, go to another resource
+			if quantity.IsZero() {
+				break
+			}
+
+			nRes := numaNodes[node].Resources
+			if available, ok := nRes[resName]; ok {
+				switch quantity.Cmp(available) {
+				case 0: // the same
+					// basically zero container resources
+					quantity.Sub(available)
+					// zero NUMA quantity
+					nRes[resName] = resource.Quantity{}
+				case 1: // container wants more resources than available in this NUMA zone
+					// substract NUMA resources from container request, to calculate how much is missing
+					quantity.Sub(available)
+					// zero NUMA quantity
+					nRes[resName] = resource.Quantity{}
+				case -1: // there are more resources available in this NUMA zone than container requests
+					// substract container resources from resources available in this NUMA node
+					available.Sub(quantity)
+					// zero container quantity
+					quantity = resource.Quantity{}
+					nRes[resName] = available
+				}
+			}
+		}
+	}
+}