Merge pull request #504 from PiotrProkop/nrt-nodelocking

Add new NRT cache type DiscardReservedNodes

Author: k8s-ci-robot

apis/config/types.go

@@ -152,6 +152,10 @@ type NodeResourceTopologyMatchArgs struct {
 	ScoringStrategy ScoringStrategy
 	// If > 0, enables the caching facilities of the reserve plugin - which must be enabled
 	CacheResyncPeriodSeconds int64
+	// if set to true, exclude node from scheduling if there are any reserved pods for given node
+	// this option takes precedence over CacheResyncPeriodSeconds
+	// if DiscardReservedNodes is enabled, CacheResyncPeriodSeconds option is noop
+	DiscardReservedNodes bool
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

apis/config/v1/types.go

@@ -149,6 +149,10 @@ type NodeResourceTopologyMatchArgs struct {
 	ScoringStrategy *ScoringStrategy `json:"scoringStrategy,omitempty"`
 	// If > 0, enables the caching facilities of the reserve plugin - which must be enabled
 	CacheResyncPeriodSeconds *int64 `json:"cacheResyncPeriodSeconds,omitempty"`
+	// if set to true, exclude node from scheduling if there are any reserved pods for given node
+	// this option takes precedence over CacheResyncPeriodSeconds
+	// if DiscardReservedNodes is enabled, CacheResyncPeriodSeconds option is noop
+	DiscardReservedNodes bool `json:"discardReservedNodes,omitempty"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

apis/config/v1/zz_generated.conversion.go

@@ -148,6 +148,10 @@ type NodeResourceTopologyMatchArgs struct {
 	// implicitely enables the caching. If zero, disables the caching entirely.
 	// If the cache is enabled, the Reserve plugin must be enabled.
 	CacheResyncPeriodSeconds *int64 `json:"cacheResyncPeriodSeconds,omitempty"`
+	// if set to true, exclude node from scheduling if there are any reserved pods for given node
+	// this option takes precedence over CacheResyncPeriodSeconds
+	// if DiscardReservedNodes is enabled, CacheResyncPeriodSeconds option is noop
+	DiscardReservedNodes bool `json:"discardReservedNodes,omitempty"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

apis/config/v1beta2/types.go

@@ -152,6 +152,10 @@ type NodeResourceTopologyMatchArgs struct {
 	// implicitely enables the caching. If zero, disables the caching entirely.
 	// If the cache is enabled, the Reserve plugin must be enabled.
 	CacheResyncPeriodSeconds *int64 `json:"cacheResyncPeriodSeconds,omitempty"`
+	// if set to true, exclude node from scheduling if there are any reserved pods for given node
+	// this option takes precedence over CacheResyncPeriodSeconds
+	// if DiscardReservedNodes is enabled, CacheResyncPeriodSeconds option is noop
+	DiscardReservedNodes bool `json:"discardReservedNodes,omitempty"`
 }
 
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

apis/config/v1beta2/zz_generated.conversion.go

@@ -56,4 +56,10 @@ type Interface interface {
 
 	// UnreserveNodeResources decrement from the node assumed resources the resources required by the given pod.
 	UnreserveNodeResources(nodeName string, pod *corev1.Pod)
+
+	// PostBind is called after a pod is successfully bound. These plugins are
+	// informational. A common application of this extension point is for cleaning
+	// up. If a plugin needs to clean-up its state after a pod is scheduled and
+	// bound, PostBind is the extension point that it should register.
+	PostBind(nodeName string, pod *corev1.Pod)
 }

apis/config/v1beta3/types.go

@@ -0,0 +1,103 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cache
+
+import (
+	"sync"
+
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/klog/v2"
+
+	topologyv1alpha2 "github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/apis/topology/v1alpha2"
+	listerv1alpha2 "github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/generated/listers/topology/v1alpha2"
+)
+
+// DiscardReserved is intended to solve similiar problem as Overreserve Cache,
+// which is to minimize amount of incorrect scheduling decisions based on stale NRT data.
+// Unfortunately Overreserve cache only works for single-numa-node Topology Manager policy.
+// Dis tries to minimize amount of Admission Errors and non-optimal placement
+// when NodeResourceTopologyMatch plugin is used to schedule PODs requesting resources from multiple NUMA domains.
+// There are scenarios where using DiscardReserved won't mitigate drawbacks of using Passthrough cache.
+// NRT update is expected once PostBind triggers, but there's no guarantee about when this will happen.
+// In cases like:
+// - NFD(or any other component that advertises NRT) can be nonfunctional
+// - network can be slow
+// - Pod being scheduled after PostBind trigger and before NRT update
+// in those cases DiscardReserved cache will act same as Passthrough cache
+type DiscardReserved struct {
+	rMutex         sync.RWMutex
+	reservationMap map[string]map[types.UID]bool // Key is NodeName, value is Pod UID : reserved status
+	lister         listerv1alpha2.NodeResourceTopologyLister
+}
+
+func NewDiscardReserved(lister listerv1alpha2.NodeResourceTopologyLister) Interface {
+	return &DiscardReserved{
+		lister:         lister,
+		reservationMap: make(map[string]map[types.UID]bool),
+	}
+}
+
+func (pt *DiscardReserved) GetCachedNRTCopy(nodeName string, _ *corev1.Pod) (*topologyv1alpha2.NodeResourceTopology, bool) {
+	pt.rMutex.RLock()
+	defer pt.rMutex.RUnlock()
+	if t, ok := pt.reservationMap[nodeName]; ok {
+		if len(t) > 0 {
+			return nil, false
+		}
+	}
+
+	nrt, err := pt.lister.Get(nodeName)
+	if err != nil {
+		return nil, false
+	}
+	return nrt, true
+}
+
+func (pt *DiscardReserved) NodeMaybeOverReserved(nodeName string, pod *corev1.Pod) {}
+func (pt *DiscardReserved) NodeHasForeignPods(nodeName string, pod *corev1.Pod)    {}
+
+func (pt *DiscardReserved) ReserveNodeResources(nodeName string, pod *corev1.Pod) {
+	klog.V(5).InfoS("nrtcache NRT Reserve", "logID", klog.KObj(pod), "UID", pod.GetUID(), "node", nodeName)
+	pt.rMutex.Lock()
+	defer pt.rMutex.Unlock()
+
+	if pt.reservationMap[nodeName] == nil {
+		pt.reservationMap[nodeName] = make(map[types.UID]bool)
+	}
+	pt.reservationMap[nodeName][pod.GetUID()] = true
+}
+
+func (pt *DiscardReserved) UnreserveNodeResources(nodeName string, pod *corev1.Pod) {
+	klog.V(5).InfoS("nrtcache NRT Unreserve", "logID", klog.KObj(pod), "UID", pod.GetUID(), "node", nodeName)
+
+	pt.removeReservationForNode(nodeName, pod)
+}
+
+// PostBind is invoked to cleanup reservationMap
+func (pt *DiscardReserved) PostBind(nodeName string, pod *corev1.Pod) {
+	klog.V(5).InfoS("nrtcache NRT PostBind", "logID", klog.KObj(pod), "UID", pod.GetUID(), "node", nodeName)
+
+	pt.removeReservationForNode(nodeName, pod)
+}
+
+func (pt *DiscardReserved) removeReservationForNode(nodeName string, pod *corev1.Pod) {
+	pt.rMutex.Lock()
+	defer pt.rMutex.Unlock()
+
+	delete(pt.reservationMap[nodeName], pod.GetUID())
+}

apis/config/v1beta3/zz_generated.conversion.go

@@ -0,0 +1,172 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cache
+
+import (
+	"reflect"
+	"testing"
+
+	topologyv1alpha2 "github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/apis/topology/v1alpha2"
+	faketopologyv1alpha2 "github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/generated/clientset/versioned/fake"
+	topologyinformers "github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/generated/informers/externalversions"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+)
+
+func TestDiscardReservedNodesGetNRTCopy(t *testing.T) {
+	fakeClient := faketopologyv1alpha2.NewSimpleClientset()
+	fakeInformer := topologyinformers.NewSharedInformerFactory(fakeClient, 0).Topology().V1alpha2().NodeResourceTopologies()
+
+	nrtCache := NewDiscardReserved(fakeInformer.Lister())
+	var nrtObj *topologyv1alpha2.NodeResourceTopology
+	nrtObj, _ = nrtCache.GetCachedNRTCopy("node1", &corev1.Pod{})
+	if nrtObj != nil {
+		t.Fatalf("non-empty object from empty cache")
+	}
+
+	nodeTopologies := []*topologyv1alpha2.NodeResourceTopology{
+		{
+			ObjectMeta:       metav1.ObjectMeta{Name: "node1"},
+			TopologyPolicies: []string{string(topologyv1alpha2.SingleNUMANodeContainerLevel)},
+			Zones: topologyv1alpha2.ZoneList{
+				{
+					Name: "node-0",
+					Type: "Node",
+					Resources: topologyv1alpha2.ResourceInfoList{
+						MakeTopologyResInfo(cpu, "20", "4"),
+						MakeTopologyResInfo(memory, "8Gi", "8Gi"),
+						MakeTopologyResInfo(nicResourceName, "30", "10"),
+					},
+				},
+				{
+					Name: "node-1",
+					Type: "Node",
+					Resources: topologyv1alpha2.ResourceInfoList{
+						MakeTopologyResInfo(cpu, "30", "8"),
+						MakeTopologyResInfo(memory, "8Gi", "8Gi"),
+						MakeTopologyResInfo(nicResourceName, "30", "10"),
+					},
+				},
+			},
+		},
+	}
+	for _, obj := range nodeTopologies {
+		fakeInformer.Informer().GetStore().Update(obj)
+	}
+
+	nrtObj, ok := nrtCache.GetCachedNRTCopy("node1", &corev1.Pod{})
+	if !reflect.DeepEqual(nrtObj, nodeTopologies[0]) {
+		t.Fatalf("unexpected object from cache\ngot: %s\nexpected: %s\n", dumpNRT(nrtObj), dumpNRT(nodeTopologies[0]))
+	}
+
+	if !ok {
+		t.Fatalf("expecting GetCachedNRTCopy to return true not false")
+	}
+}
+
+func TestDiscardReservedNodesGetNRTCopyFails(t *testing.T) {
+	nrtCache := DiscardReserved{
+		reservationMap: map[string]map[types.UID]bool{
+			"node1": {
+				types.UID("some-uid"): true,
+			},
+		},
+	}
+
+	nrtObj, ok := nrtCache.GetCachedNRTCopy("node1", &corev1.Pod{})
+	if ok {
+		t.Fatal("expected false\ngot true\n")
+	}
+	if nrtObj != nil {
+		t.Fatalf("non-empty object")
+	}
+}
+
+func TestDiscardReservedNodesReserveNodeResources(t *testing.T) {
+	nrtCache := DiscardReserved{
+		reservationMap: make(map[string]map[types.UID]bool),
+	}
+
+	nrtCache.ReserveNodeResources("node1", &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "pod",
+			Namespace: "test",
+			UID:       "some-uid",
+		},
+	})
+	nodePods, ok := nrtCache.reservationMap["node1"]
+	if !ok {
+		t.Fatal("expected reservationMap to have entry for node1")
+	}
+
+	if len(nodePods) != 1 {
+		t.Fatalf("expected reservationMap entry for node1 to have len 1 not: %d", len(nodePods))
+	}
+
+	pod, ok := nodePods[types.UID("some-uid")]
+	if !ok {
+		t.Fatal("expected reservationMap for node1 to have some-uid")
+	}
+
+	if !pod {
+		t.Fatal("expected reservationMap entry node1 to have some-uid set to true not false")
+	}
+}
+
+func TestDiscardReservedNodesRemoveReservationForNode(t *testing.T) {
+	nrtCache := DiscardReserved{
+		reservationMap: make(map[string]map[types.UID]bool),
+	}
+
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "pod",
+			Namespace: "test",
+			UID:       "some-uid",
+		},
+	}
+
+	nrtCache.ReserveNodeResources("node1", pod)
+	nodePods, ok := nrtCache.reservationMap["node1"]
+	if !ok {
+		t.Fatal("expected reservationMap to have entry for node1")
+	}
+
+	if len(nodePods) != 1 {
+		t.Fatalf("expected reservationMap entry for node1 to have len 1 not: %d", len(nodePods))
+	}
+
+	podExists, ok := nodePods[types.UID("some-uid")]
+	if !ok {
+		t.Fatal("expected reservationMap for node1 to have some-uid")
+	}
+
+	if !podExists {
+		t.Fatal("expected reservationMap entry node1 to have some-uid set to true not false")
+	}
+
+	nrtCache.removeReservationForNode("node1", pod)
+	nodePods, ok = nrtCache.reservationMap["node1"]
+	if !ok {
+		t.Fatal("expected reservationMap to have entry for node1")
+	}
+
+	if len(nodePods) != 0 {
+		t.Fatalf("expected reservationMap entry for node1 to have len 0 not: %d", len(nodePods))
+	}
+}