From 6d8b41fac66ddfb902c08cec436af17a329f751b Mon Sep 17 00:00:00 2001 From: Gaurav Ghildiyal Date: Thu, 1 May 2025 20:49:28 -0700 Subject: [PATCH 1/2] Introduce special handling for updates involving a single resource slice. Typically, a single DRA driver uses only one ResourceSlice per resource pool. Currently, the ResourceSliceController updates this slice by deleting and recreating it when devices are added or removed. For this common single-slice-per-pool scenario, we can improve efficiency by directly updating the existing ResourceSlice instead. --- .../resourceslice/resourceslicecontroller.go | 70 ++++++++++--------- .../resourceslicecontroller_test.go | 46 ++++++++++-- 2 files changed, 80 insertions(+), 36 deletions(-) diff --git a/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/resourceslicecontroller.go b/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/resourceslicecontroller.go index 3e9c052ec40..ae62b16a5bd 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/resourceslicecontroller.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/resourceslicecontroller.go @@ -490,45 +490,51 @@ func (c *Controller) syncPool(ctx context.Context, poolName string) error { logger.V(5).Info("Existing slices", "obsolete", klog.KObjSlice(obsoleteSlices), "current", klog.KObjSlice(currentSlices)) if pool, ok := resources.Pools[poolName]; ok { - // Match each existing slice against the desired slices. - // Two slices match if they contain exactly the same - // device IDs, in an arbitrary order. Such a matched - // slice gets updated with the desired content if - // there is a difference. + // Match each existing slice against the desired slices. Two slices + // "match" if they contain exactly the same device IDs, in an arbitrary + // order. As a special case, slices are also considered "matched" in the + // scenario where there's a single existing slice and a single desired + // slice. Such a "matched" slice gets updated with the desired content + // if there is a difference. // - // This supports updating the definition of devices - // in a slice. Adding or removing devices is done - // by deleting the old slice and creating a new one. + // In the case where there is more than one existing or desired slices, + // adding or removing devices is done by deleting the old slice and + // creating a new one. This is primarily a simplification of the code: + // to support adding or removing devices from existing slices, we would + // have to identify "most similar" slices (= minimal editing distance). // - // This is primarily a simplification of the code: - // to support adding or removing devices from - // existing slices, we would have to identify "most - // similar" slices (= minimal editing distance). + // In currentSliceForDesiredSlice we keep track of which desired slice + // has a matched slice. // - // In currentSliceForDesiredSlice we keep track of - // which desired slice has a matched slice. - // - // At the end of the loop, each current slice is either - // a match or obsolete. + // At the end of the loop, each current slice is either a match or + // obsolete. currentSliceForDesiredSlice := make(map[int]*resourceapi.ResourceSlice, len(pool.Slices)) - for _, currentSlice := range currentSlices { - matched := false - for i := range pool.Slices { - if _, ok := currentSliceForDesiredSlice[i]; ok { - // Already has a match. - continue + if len(currentSlices) == 1 && len(currentSlices) == len(pool.Slices) { + // If there's just one existing slice and one desired slice, assume + // they "matched" such that if required, it is the existing slice + // which gets updated and we avoid an unnecessary deletion and + // recreation of the slice. + currentSliceForDesiredSlice[0] = currentSlices[0] + } else { + for _, currentSlice := range currentSlices { + matched := false + for i := range pool.Slices { + if _, ok := currentSliceForDesiredSlice[i]; ok { + // Already has a match. + continue + } + if sameSlice(currentSlice, &pool.Slices[i]) { + currentSliceForDesiredSlice[i] = currentSlice + logger.V(5).Info("Matched existing slice", "slice", klog.KObj(currentSlice), "matchIndex", i) + matched = true + break + } } - if sameSlice(currentSlice, &pool.Slices[i]) { - currentSliceForDesiredSlice[i] = currentSlice - logger.V(5).Info("Matched existing slice", "slice", klog.KObj(currentSlice), "matchIndex", i) - matched = true - break + if !matched { + obsoleteSlices = append(obsoleteSlices, currentSlice) + logger.V(5).Info("Unmatched existing slice", "slice", klog.KObj(currentSlice)) } } - if !matched { - obsoleteSlices = append(obsoleteSlices, currentSlice) - logger.V(5).Info("Unmatched existing slice", "slice", klog.KObj(currentSlice)) - } } // Desired metadata which must be set in each slice. diff --git a/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/resourceslicecontroller_test.go b/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/resourceslicecontroller_test.go index 226b998071a..faa7ac18a98 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/resourceslicecontroller_test.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/resourceslicecontroller_test.go @@ -272,7 +272,7 @@ func TestControllerSyncPool(t *testing.T) { }, expectedResourceSlices: nil, }, - "delete-and-add-slice": { + "one-existing-and-one-desired-slice-should-be-updated-inplace": { nodeUID: nodeUID, initialObjects: []runtime.Object{ // no devices @@ -288,14 +288,52 @@ func TestControllerSyncPool(t *testing.T) { }, }, expectedStats: Stats{ - NumDeletes: 1, + NumUpdates: 1, + }, + expectedResourceSlices: []resourceapi.ResourceSlice{ + *MakeResourceSlice().Name(resourceSlice1).UID(resourceSlice1).ResourceVersion("1"). + NodeOwnerReferences(ownerName, string(nodeUID)).NodeName(ownerName). + Driver(driverName).Devices([]resourceapi.Device{{Name: deviceName}}). + Pool(resourceapi.ResourcePool{Name: poolName, Generation: 1, ResourceSliceCount: 1}).Obj(), + }, + }, + "delete-and-add-slice-when-more-than-one-existing-or-desired-slice": { + nodeUID: nodeUID, + initialObjects: []runtime.Object{ + // No devices in first ResourceSlice. + MakeResourceSlice().Name(resourceSlice1).UID(resourceSlice1). + NodeOwnerReferences(ownerName, string(nodeUID)).NodeName(ownerName). + Driver(driverName).Devices([]resourceapi.Device{}). + Pool(resourceapi.ResourcePool{Name: poolName, Generation: 1, ResourceSliceCount: 2}).Obj(), + MakeResourceSlice().Name(resourceSlice2).UID(resourceSlice2). + NodeOwnerReferences(ownerName, string(nodeUID)).NodeName(ownerName). + Driver(driverName).Devices([]resourceapi.Device{{Name: deviceName2}}). + Pool(resourceapi.ResourcePool{Name: poolName, Generation: 1, ResourceSliceCount: 2}).Obj(), + }, + inputDriverResources: &DriverResources{ + Pools: map[string]Pool{ + poolName: { + Slices: []Slice{ + {Devices: []resourceapi.Device{{Name: deviceName1}}}, + {Devices: []resourceapi.Device{{Name: deviceName2}}}, + }, + }, + }, + }, + expectedStats: Stats{ NumCreates: 1, + NumUpdates: 1, + NumDeletes: 1, }, expectedResourceSlices: []resourceapi.ResourceSlice{ *MakeResourceSlice().Name(generatedName1).GenerateName(generateName). NodeOwnerReferences(ownerName, string(nodeUID)).NodeName(ownerName). - Driver(driverName).Devices([]resourceapi.Device{{Name: deviceName}}). - Pool(resourceapi.ResourcePool{Name: poolName, Generation: 2, ResourceSliceCount: 1}).Obj(), + Driver(driverName).Devices([]resourceapi.Device{{Name: deviceName1}}). + Pool(resourceapi.ResourcePool{Name: poolName, Generation: 2, ResourceSliceCount: 2}).Obj(), + *MakeResourceSlice().Name(resourceSlice2).UID(resourceSlice2).ResourceVersion("1"). + NodeOwnerReferences(ownerName, string(nodeUID)).NodeName(ownerName). + Driver(driverName).Devices([]resourceapi.Device{{Name: deviceName2}}). + Pool(resourceapi.ResourcePool{Name: poolName, Generation: 2, ResourceSliceCount: 2}).Obj(), }, }, "delete-redundant-slice": { From edda4b02d613971272b804a943c9aa0022aee658 Mon Sep 17 00:00:00 2001 From: Gaurav Ghildiyal Date: Thu, 1 May 2025 23:17:48 -0700 Subject: [PATCH 2/2] fixup! Introduce special handling for updates involving a single resource slice. --- .../resourceslice/resourceslicecontroller.go | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/resourceslicecontroller.go b/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/resourceslicecontroller.go index ae62b16a5bd..7660d503a72 100644 --- a/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/resourceslicecontroller.go +++ b/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/resourceslicecontroller.go @@ -490,26 +490,31 @@ func (c *Controller) syncPool(ctx context.Context, poolName string) error { logger.V(5).Info("Existing slices", "obsolete", klog.KObjSlice(obsoleteSlices), "current", klog.KObjSlice(currentSlices)) if pool, ok := resources.Pools[poolName]; ok { - // Match each existing slice against the desired slices. Two slices - // "match" if they contain exactly the same device IDs, in an arbitrary - // order. As a special case, slices are also considered "matched" in the - // scenario where there's a single existing slice and a single desired - // slice. Such a "matched" slice gets updated with the desired content - // if there is a difference. + // Match each existing slice against the desired slices. + // Two slices "match" if they contain exactly the + // same device IDs, in an arbitrary order. As a + // special case, slices are also considered + // "matched" in the scenario where there's a single + // existing slice and a single desired slice. Such a + // matched slice gets updated with the desired + // content if there is a difference. // - // In the case where there is more than one existing or desired slices, - // adding or removing devices is done by deleting the old slice and - // creating a new one. This is primarily a simplification of the code: - // to support adding or removing devices from existing slices, we would - // have to identify "most similar" slices (= minimal editing distance). + // In the case where there is more than one existing + // or desired slices, adding or removing devices is + // done by deleting the old slice and creating a new one. // - // In currentSliceForDesiredSlice we keep track of which desired slice - // has a matched slice. + // This is primarily a simplification of the code: + // to support adding or removing devices from + // existing slices, we would have to identify "most + // similar" slices (= minimal editing distance). // - // At the end of the loop, each current slice is either a match or - // obsolete. + // In currentSliceForDesiredSlice we keep track of + // which desired slice has a matched slice. + // + // At the end of the loop, each current slice is either + // a match or obsolete. currentSliceForDesiredSlice := make(map[int]*resourceapi.ResourceSlice, len(pool.Slices)) - if len(currentSlices) == 1 && len(currentSlices) == len(pool.Slices) { + if len(currentSlices) == 1 && len(pool.Slices) == 1 { // If there's just one existing slice and one desired slice, assume // they "matched" such that if required, it is the existing slice // which gets updated and we avoid an unnecessary deletion and