Merge pull request #134340 from pohly/dra-resourceslice-cleanup

DRA: ResourceSlice tracker cleanup

staging/src/k8s.io/dynamic-resource-allocation/api/uniquestring.go

@@ -17,6 +17,7 @@ limitations under the License.
 package api
 
 import (
+	"encoding/json"
 	"unique"
 )
 
@@ -35,6 +36,11 @@ func (us UniqueString) String() string {
 	return unique.Handle[string](us).Value()
 }
 
+// MarshalJSON is primarily useful for pretty-printing as JSON or YAML.
+func (us UniqueString) MarshalJSON() ([]byte, error) {
+	return json.Marshal(us.String())
+}
+
 // MakeUniqueString constructs a new unique string.
 func MakeUniqueString(str string) UniqueString {
 	return UniqueString(unique.Make(str))

staging/src/k8s.io/dynamic-resource-allocation/resourceslice/tracker/tracker.go

@@ -387,7 +387,11 @@ func (t *Tracker) resourceSliceAdd(ctx context.Context) func(obj any) {
 		if !ok {
 			return
 		}
-		logger.V(5).Info("ResourceSlice add", "slice", klog.KObj(slice))
+		if loggerV := logger.V(6); loggerV.Enabled() {
+			loggerV.Info("ResourceSlice added", "slice", klog.Format(slice))
+		} else {
+			logger.V(5).Info("ResourceSlice added", "slice", klog.KObj(slice))
+		}
 		t.syncSlice(ctx, slice.Name, true)
 	}
 }
@@ -406,9 +410,9 @@ func (t *Tracker) resourceSliceUpdate(ctx context.Context) func(oldObj, newObj a
 		if loggerV := logger.V(6); loggerV.Enabled() {
 			// While debugging, one needs a full dump of the objects for context *and*
 			// a diff because otherwise small changes would be hard to spot.
-			loggerV.Info("ResourceSlice update", "slice", klog.Format(oldSlice), "oldSlice", klog.Format(newSlice), "diff", diff.Diff(oldSlice, newSlice))
+			loggerV.Info("ResourceSlice updated", "diff", diff.Diff(oldSlice, newSlice), "slice", klog.Format(newSlice))
 		} else {
-			logger.V(5).Info("ResourceSlice update", "slice", klog.KObj(newSlice))
+			logger.V(5).Info("ResourceSlice updated", "slice", klog.KObj(newSlice))
 		}
 		t.syncSlice(ctx, newSlice.Name, true)
 	}
@@ -424,7 +428,7 @@ func (t *Tracker) resourceSliceDelete(ctx context.Context) func(obj any) {
 		if !ok {
 			return
 		}
-		logger.V(5).Info("ResourceSlice delete", "slice", klog.KObj(slice))
+		logger.V(5).Info("ResourceSlice deleted", "slice", klog.KObj(slice))
 		t.syncSlice(ctx, slice.Name, true)
 	}
 }
@@ -432,12 +436,16 @@ func (t *Tracker) resourceSliceDelete(ctx context.Context) func(obj any) {
 func (t *Tracker) deviceTaintAdd(ctx context.Context) func(obj any) {
 	logger := klog.FromContext(ctx)
 	return func(obj any) {
-		patch, ok := obj.(*resourcealphaapi.DeviceTaintRule)
+		rule, ok := obj.(*resourcealphaapi.DeviceTaintRule)
 		if !ok {
 			return
 		}
-		logger.V(5).Info("DeviceTaintRule add", "patch", klog.KObj(patch))
-		for _, sliceName := range t.sliceNamesForPatch(ctx, patch) {
+		if loggerV := logger.V(6); loggerV.Enabled() {
+			loggerV.Info("DeviceTaintRule added", "deviceTaintRule", klog.Format(rule))
+		} else {
+			logger.V(5).Info("DeviceTaintRule added", "deviceTaintRule", klog.KObj(rule))
+		}
+		for _, sliceName := range t.sliceNamesForPatch(ctx, rule) {
 			t.syncSlice(ctx, sliceName, false)
 		}
 	}
@@ -446,26 +454,26 @@ func (t *Tracker) deviceTaintAdd(ctx context.Context) func(obj any) {
 func (t *Tracker) deviceTaintUpdate(ctx context.Context) func(oldObj, newObj any) {
 	logger := klog.FromContext(ctx)
 	return func(oldObj, newObj any) {
-		oldPatch, ok := oldObj.(*resourcealphaapi.DeviceTaintRule)
+		oldRule, ok := oldObj.(*resourcealphaapi.DeviceTaintRule)
 		if !ok {
 			return
 		}
-		newPatch, ok := newObj.(*resourcealphaapi.DeviceTaintRule)
+		newRule, ok := newObj.(*resourcealphaapi.DeviceTaintRule)
 		if !ok {
 			return
 		}
 		if loggerV := logger.V(6); loggerV.Enabled() {
-			loggerV.Info("DeviceTaintRule update", "patch", klog.KObj(newPatch), "diff", diff.Diff(oldPatch, newPatch))
+			loggerV.Info("DeviceTaintRule updated", "diff", diff.Diff(oldRule, newRule), "deviceTaintRule", klog.KObj(newRule))
 		} else {
-			logger.V(5).Info("DeviceTaintRule update", "patch", klog.KObj(newPatch))
+			logger.V(5).Info("DeviceTaintRule updated", "deviceTaintRule", klog.KObj(newRule))
 		}
 
 		// Slices that matched the old patch may need to be updated, in
 		// case they no longer match the new patch and need to have the
 		// patch's changes reverted.
 		slicesToSync := sets.New[string]()
-		slicesToSync.Insert(t.sliceNamesForPatch(ctx, oldPatch)...)
-		slicesToSync.Insert(t.sliceNamesForPatch(ctx, newPatch)...)
+		slicesToSync.Insert(t.sliceNamesForPatch(ctx, oldRule)...)
+		slicesToSync.Insert(t.sliceNamesForPatch(ctx, newRule)...)
 		for _, sliceName := range slicesToSync.UnsortedList() {
 			t.syncSlice(ctx, sliceName, false)
 		}
@@ -482,7 +490,7 @@ func (t *Tracker) deviceTaintDelete(ctx context.Context) func(obj any) {
 		if !ok {
 			return
 		}
-		logger.V(5).Info("DeviceTaintRule delete", "patch", klog.KObj(patch))
+		logger.V(5).Info("DeviceTaintRule deleted", "patch", klog.KObj(patch))
 		for _, sliceName := range t.sliceNamesForPatch(ctx, patch) {
 			t.syncSlice(ctx, sliceName, false)
 		}

staging/src/k8s.io/dynamic-resource-allocation/resourceslice/tracker/tracker_test.go

@@ -67,17 +67,35 @@ func update[T any](oldObj, newObj *T) [2]*T {
 	return [2]*T{oldObj, newObj}
 }
 
-func runInputEvents(tCtx *testContext, events []any) {
-	for _, event := range events {
-		switch event := event.(type) {
-		case []any:
-			for _, event := range event {
-				applyEventPair(tCtx, event)
+func runInputEvents(tCtx *testContext, events []any, permutation []int) {
+	for _, i := range permutation {
+		event, name := lookupEvent(events, i)
+		stepCtx := tCtx.withLoggerName(fmt.Sprintf("event #%s", name))
+		applyEventPair(stepCtx, event)
+	}
+}
+
+// lookupEvent is the opposite of flatten: it takes an index
+// after flattening and maps it back to the event in the original
+// event hierarchy. To do so, it descends into the second level where necessary.
+func lookupEvent(events []any, index int) (any, string) {
+	numEvents := 0
+	for i := range events {
+		if e, ok := events[i].([]any); ok {
+			for j := range e {
+				if numEvents == index {
+					return e[j], fmt.Sprintf("%d/%d", i, j)
+				}
+				numEvents++
 			}
-		default:
-			applyEventPair(tCtx, event)
+		} else {
+			if numEvents == index {
+				return events[i], fmt.Sprintf("%d", i)
+			}
+			numEvents++
 		}
 	}
+	panic(fmt.Sprintf("invalid event index #%d", index))
 }
 
 func applyEventPair(tCtx *testContext, event any) {
@@ -140,6 +158,18 @@ type testContext struct {
 	*fake.Clientset
 }
 
+func (t *testContext) withLoggerName(name string) *testContext {
+	logger := klog.FromContext(t.Context)
+	logger = klog.LoggerWithName(logger, name)
+	t = &testContext{
+		T:         t.T,
+		Context:   klog.NewContext(t.Context, logger),
+		Tracker:   t.Tracker,
+		Clientset: t.Clientset,
+	}
+	return t
+}
+
 var (
 	now, _      = time.Parse(time.RFC3339, "2006-01-02T15:04:05Z")
 	driver1     = "driver1.example.com"
@@ -335,6 +365,11 @@ func TestListPatchedResourceSlices(t *testing.T) {
 		// the order in those nested lists is preserved.
 		events                []any
 		expectedPatchedSlices []*resourceapi.ResourceSlice
+		// The exact events that are emitted for a sequence of events is
+		// highly dependent on the order in which those events are received.
+		// We punt on determining a set of validation criteria for every
+		// possible sequence and only check them against the first
+		// permutation: the order in which the events are defined.
 		expectedHandlerEvents []handlerEvent
 		expectEvents          func(t *assert.CollectT, events *v1.EventList)
 		expectUnhandledErrors func(t *testing.T, errs []error)
@@ -643,7 +678,15 @@ func TestListPatchedResourceSlices(t *testing.T) {
 		}
 	}
 
-	testHandlers := func(tCtx *testContext, test test, testExpectedEmittedEvents bool) {
+	testHandlers := func(tCtx *testContext, test test, permutation []int) {
+		isPermutated := false
+		for i, j := range permutation {
+			if i != j {
+				isPermutated = true
+				break
+			}
+		}
+
 		var handlerEvents []handlerEvent
 		handler := cache.ResourceEventHandlerFuncs{
 			AddFunc: func(obj interface{}) {
@@ -663,9 +706,9 @@ func TestListPatchedResourceSlices(t *testing.T) {
 			unhandledErrors = append(unhandledErrors, err)
 		}
 
-		runInputEvents(tCtx, test.events)
+		runInputEvents(tCtx, test.events, permutation)
 
-		if testExpectedEmittedEvents {
+		if !isPermutated {
 			assert.Equal(tCtx, test.expectedHandlerEvents, handlerEvents)
 		}
 
@@ -709,60 +752,47 @@ func TestListPatchedResourceSlices(t *testing.T) {
 
 	for name, tc := range tests {
 		t.Run(name, func(t *testing.T) {
-			// The exact events that are emitted for a sequence of events is
-			// highly dependent on the order in which those events are received.
-			// We punt on determining a set of validation criteria for every
-			// possible sequence and only check them against the first
-			// permutation: the order in which the events are defined.
-			testExpectedEmittedEvents := true
-
-			numEvents := len(tc.events)
-			if numEvents <= 1 {
-				// No permutations.
-				tContext := setup(t)
-				testHandlers(tContext, tc, testExpectedEmittedEvents)
-				return
-			}
-
-			// flatten does one level of flattening of events. It also returns
-			// another slice of pairs of indices representing ranges which were
-			// flattened.
-			flatten := func(events []any) ([]any, [][2]int) {
-				var ret []any
+			// flatten does one level of flattening of events, counting all events.
+			// It also returns a slice of pairs of indices representing ranges which were
+			// flattened (= came from the second level) and which therefore must
+			// remain in that order.
+			flatten := func(events []any) (int, [][2]int) {
+				numEvents := 0
 				var ranges [][2]int
 				for _, e := range events {
 					switch e := e.(type) {
 					case []any:
-						ranges = append(ranges, [2]int{len(ret), len(ret) + len(e)})
-						ret = append(ret, e...)
+						ranges = append(ranges, [2]int{numEvents, numEvents + len(e)})
+						numEvents += len(e)
 					default:
-						ret = append(ret, e)
+						numEvents++
 					}
 				}
-				return ret, ranges
+				return numEvents, ranges
 			}
+			numEvents, constraints := flatten(tc.events)
 
-			var constraints [][2]int
-			tc.events, constraints = flatten(tc.events)
-			numEvents = len(tc.events)
+			if len(tc.events) <= 1 {
+				// No permutations possible.
+				var permutation []int
+				for i := 0; i < numEvents; i++ {
+					permutation = append(permutation, i)
+				}
+				tContext := setup(t)
+				testHandlers(tContext, tc, permutation)
+				return
+			}
 
 			permutation := make([]int, numEvents)
 			var permutate func(depth int)
 			permutate = func(depth int) {
 				if depth >= numEvents {
 					// Define a sub-test which runs the current permutation of events.
-					events := make([]any, numEvents)
-					for i := range numEvents {
-						events[i] = tc.events[permutation[i]]
-					}
-					tc := tc
-					tc.events = events
 					name := strings.Trim(fmt.Sprintf("%v", permutation), "[]")
 					t.Run(name, func(t *testing.T) {
 						tContext := setup(t)
-						testHandlers(tContext, tc, testExpectedEmittedEvents)
-
-						testExpectedEmittedEvents = false
+						// No need to clone the slice, we don't run in parallel.
+						testHandlers(tContext, tc, permutation)
 					})
 					return
 				}
@@ -778,10 +808,13 @@ func TestListPatchedResourceSlices(t *testing.T) {
 						}
 						for j := i + 1; j < constraint[1]; j++ {
 							if slices.Contains(permutation[0:depth], j) {
+								// Invalid permutation, would change order
+								// of sub-events.
 								continue nexti
 							}
 						}
 					}
+
 					// Pick it for the current position in permutation,
 					// continue with next position.
 					permutation[depth] = i