Merge pull request #126156 from pohly/kubelet-test-enhancements

kubelet test enhancements

pkg/kubelet/apis/podresources/server_v1_test.go

@@ -19,10 +19,11 @@ package podresources
 import (
 	"context"
 	"fmt"
-	"reflect"
-	"sort"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -239,8 +240,8 @@ func TestListPodResourcesV1(t *testing.T) {
 			if err != nil {
 				t.Errorf("want err = %v, got %q", nil, err)
 			}
-			if !equalListResponse(tc.expectedResponse, resp) {
-				t.Errorf("want resp = %s, got %s", tc.expectedResponse.String(), resp.String())
+			if diff := cmp.Diff(tc.expectedResponse, resp, cmpopts.EquateEmpty()); diff != "" {
+				t.Fatal(diff)
 			}
 		})
 	}
@@ -541,8 +542,8 @@ func TestListPodResourcesWithInitContainersV1(t *testing.T) {
 			if err != nil {
 				t.Errorf("want err = %v, got %q", nil, err)
 			}
-			if !equalListResponse(tc.expectedResponse, resp) {
-				t.Errorf("want resp = %s, got %s", tc.expectedResponse.String(), resp.String())
+			if diff := cmp.Diff(tc.expectedResponse, resp, cmpopts.EquateEmpty()); diff != "" {
+				t.Fatal(diff)
 			}
 		})
 	}
@@ -678,6 +679,7 @@ func TestAllocatableResources(t *testing.T) {
 			desc:       "no devices, no CPUs, all memory",
 			allCPUs:    []int64{},
 			allDevices: []*podresourcesapi.ContainerDevices{},
+			allMemory:  allMemory,
 			expectedAllocatableResourcesResponse: &podresourcesapi.AllocatableResourcesResponse{
 				Memory: allMemory,
 			},
@@ -831,8 +833,8 @@ func TestAllocatableResources(t *testing.T) {
 				t.Errorf("want err = %v, got %q", nil, err)
 			}
 
-			if !equalAllocatableResourcesResponse(tc.expectedAllocatableResourcesResponse, resp) {
-				t.Errorf("want resp = %s, got %s", tc.expectedAllocatableResourcesResponse.String(), resp.String())
+			if diff := cmp.Diff(tc.expectedAllocatableResourcesResponse, resp, cmpopts.EquateEmpty()); diff != "" {
+				t.Fatal(diff)
 			}
 		})
 	}
@@ -1006,8 +1008,8 @@ func TestGetPodResourcesV1(t *testing.T) {
 				if err != tc.err {
 					t.Errorf("want exit = %v, got %v", tc.err, err)
 				} else {
-					if !equalGetResponse(tc.expectedResponse, resp) {
-						t.Errorf("want resp = %s, got %s", tc.expectedResponse.String(), resp.String())
+					if diff := cmp.Diff(tc.expectedResponse, resp, cmpopts.EquateEmpty()); diff != "" {
+						t.Fatal(diff)
 					}
 				}
 			}
@@ -1297,185 +1299,9 @@ func TestGetPodResourcesWithInitContainersV1(t *testing.T) {
 			if err != nil {
 				t.Errorf("want err = %v, got %q", nil, err)
 			}
-			if !equalGetResponse(tc.expectedResponse, resp) {
-				t.Errorf("want resp = %s, got %s", tc.expectedResponse.String(), resp.String())
+			if diff := cmp.Diff(tc.expectedResponse, resp, cmpopts.EquateEmpty()); diff != "" {
+				t.Fatal(diff)
 			}
 		})
 	}
 }
-
-func equalListResponse(respA, respB *podresourcesapi.ListPodResourcesResponse) bool {
-	if len(respA.PodResources) != len(respB.PodResources) {
-		return false
-	}
-	for idx := 0; idx < len(respA.PodResources); idx++ {
-		podResA := respA.PodResources[idx]
-		podResB := respB.PodResources[idx]
-		if podResA.Name != podResB.Name {
-			return false
-		}
-		if podResA.Namespace != podResB.Namespace {
-			return false
-		}
-		if len(podResA.Containers) != len(podResB.Containers) {
-			return false
-		}
-		for jdx := 0; jdx < len(podResA.Containers); jdx++ {
-			cntA := podResA.Containers[jdx]
-			cntB := podResB.Containers[jdx]
-
-			if cntA.Name != cntB.Name {
-				return false
-			}
-			if !equalInt64s(cntA.CpuIds, cntB.CpuIds) {
-				return false
-			}
-
-			if !equalContainerDevices(cntA.Devices, cntB.Devices) {
-				return false
-			}
-
-			if !equalDynamicResources(cntA.DynamicResources, cntB.DynamicResources) {
-				return false
-			}
-		}
-	}
-	return true
-}
-
-func equalDynamicResources(draResA, draResB []*podresourcesapi.DynamicResource) bool {
-	if len(draResA) != len(draResB) {
-		return false
-	}
-
-	for idx := 0; idx < len(draResA); idx++ {
-		cntDraResA := draResA[idx]
-		cntDraResB := draResB[idx]
-
-		if cntDraResA.ClassName != cntDraResB.ClassName {
-			return false
-		}
-		if cntDraResA.ClaimName != cntDraResB.ClaimName {
-			return false
-		}
-		if cntDraResA.ClaimNamespace != cntDraResB.ClaimNamespace {
-			return false
-		}
-		if len(cntDraResA.ClaimResources) != len(cntDraResB.ClaimResources) {
-			return false
-		}
-		for i := 0; i < len(cntDraResA.ClaimResources); i++ {
-			claimResA := cntDraResA.ClaimResources[i]
-			claimResB := cntDraResB.ClaimResources[i]
-			if len(claimResA.CDIDevices) != len(claimResB.CDIDevices) {
-				return false
-			}
-			for y := 0; y < len(claimResA.CDIDevices); y++ {
-				cdiDeviceA := claimResA.CDIDevices[y]
-				cdiDeviceB := claimResB.CDIDevices[y]
-				if cdiDeviceA.Name != cdiDeviceB.Name {
-					return false
-				}
-			}
-		}
-	}
-
-	return true
-}
-
-func equalContainerDevices(devA, devB []*podresourcesapi.ContainerDevices) bool {
-	if len(devA) != len(devB) {
-		return false
-	}
-
-	for idx := 0; idx < len(devA); idx++ {
-		cntDevA := devA[idx]
-		cntDevB := devB[idx]
-
-		if cntDevA.ResourceName != cntDevB.ResourceName {
-			return false
-		}
-		if !equalTopology(cntDevA.Topology, cntDevB.Topology) {
-			return false
-		}
-		if !equalStrings(cntDevA.DeviceIds, cntDevB.DeviceIds) {
-			return false
-		}
-	}
-
-	return true
-}
-
-func equalInt64s(a, b []int64) bool {
-	if len(a) != len(b) {
-		return false
-	}
-	aCopy := append([]int64{}, a...)
-	sort.Slice(aCopy, func(i, j int) bool { return aCopy[i] < aCopy[j] })
-	bCopy := append([]int64{}, b...)
-	sort.Slice(bCopy, func(i, j int) bool { return bCopy[i] < bCopy[j] })
-	return reflect.DeepEqual(aCopy, bCopy)
-}
-
-func equalStrings(a, b []string) bool {
-	if len(a) != len(b) {
-		return false
-	}
-	aCopy := append([]string{}, a...)
-	sort.Strings(aCopy)
-	bCopy := append([]string{}, b...)
-	sort.Strings(bCopy)
-	return reflect.DeepEqual(aCopy, bCopy)
-}
-
-func equalTopology(a, b *podresourcesapi.TopologyInfo) bool {
-	if a == nil && b != nil {
-		return false
-	}
-	if a != nil && b == nil {
-		return false
-	}
-	return reflect.DeepEqual(a, b)
-}
-
-func equalAllocatableResourcesResponse(respA, respB *podresourcesapi.AllocatableResourcesResponse) bool {
-	if !equalInt64s(respA.CpuIds, respB.CpuIds) {
-		return false
-	}
-	return equalContainerDevices(respA.Devices, respB.Devices)
-}
-
-func equalGetResponse(ResA, ResB *podresourcesapi.GetPodResourcesResponse) bool {
-	podResA := ResA.PodResources
-	podResB := ResB.PodResources
-	if podResA.Name != podResB.Name {
-		return false
-	}
-	if podResA.Namespace != podResB.Namespace {
-		return false
-	}
-	if len(podResA.Containers) != len(podResB.Containers) {
-		return false
-	}
-	for jdx := 0; jdx < len(podResA.Containers); jdx++ {
-		cntA := podResA.Containers[jdx]
-		cntB := podResB.Containers[jdx]
-
-		if cntA.Name != cntB.Name {
-			return false
-		}
-		if !equalInt64s(cntA.CpuIds, cntB.CpuIds) {
-			return false
-		}
-
-		if !equalContainerDevices(cntA.Devices, cntB.Devices) {
-			return false
-		}
-
-		if !equalDynamicResources(cntA.DynamicResources, cntB.DynamicResources) {
-			return false
-		}
-
-	}
-	return true
-}

pkg/kubelet/checkpointmanager/checksum/checksum.go

@@ -28,8 +28,9 @@ type Checksum uint64
 
 // Verify verifies that passed checksum is same as calculated checksum
 func (cs Checksum) Verify(data interface{}) error {
-	if cs != New(data) {
-		return errors.ErrCorruptCheckpoint
+	actualCS := New(data)
+	if cs != actualCS {
+		return &errors.CorruptCheckpointError{ActualCS: uint64(actualCS), ExpectedCS: uint64(cs)}
 	}
 	return nil
 }

pkg/kubelet/checkpointmanager/errors/errors.go

@@ -18,8 +18,28 @@ package errors
 
 import "fmt"
 
-// ErrCorruptCheckpoint error is reported when checksum does not match
-var ErrCorruptCheckpoint = fmt.Errorf("checkpoint is corrupted")
+// ErrCorruptCheckpoint error is reported when checksum does not match.
+// Check for it with:
+//
+//	var csErr *CorruptCheckpointError
+//	if errors.As(err, &csErr) { ... }
+//	if errors.Is(err, CorruptCheckpointError{}) { ... }
+type CorruptCheckpointError struct {
+	ActualCS, ExpectedCS uint64
+}
+
+func (err CorruptCheckpointError) Error() string {
+	return "checkpoint is corrupted"
+}
+
+func (err CorruptCheckpointError) Is(target error) bool {
+	switch target.(type) {
+	case *CorruptCheckpointError, CorruptCheckpointError:
+		return true
+	default:
+		return false
+	}
+}
 
 // ErrCheckpointNotFound is reported when checkpoint is not found for a given key
 var ErrCheckpointNotFound = fmt.Errorf("checkpoint is not found")

pkg/kubelet/cm/cpumanager/state/checkpoint.go

@@ -110,8 +110,12 @@ func (cp *CPUManagerCheckpointV1) VerifyChecksum() error {
 
 	hash := fnv.New32a()
 	fmt.Fprintf(hash, "%v", object)
-	if cp.Checksum != checksum.Checksum(hash.Sum32()) {
-		return errors.ErrCorruptCheckpoint
+	actualCS := checksum.Checksum(hash.Sum32())
+	if cp.Checksum != actualCS {
+		return &errors.CorruptCheckpointError{
+			ActualCS:   uint64(actualCS),
+			ExpectedCS: uint64(cp.Checksum),
+		}
 	}
 
 	return nil

pkg/kubelet/cm/dra/state/checkpoint.go

@@ -18,6 +18,7 @@ package state
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"hash/fnv"
 	"strings"
@@ -25,7 +26,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/dump"
 	"k8s.io/kubernetes/pkg/kubelet/checkpointmanager"
 	"k8s.io/kubernetes/pkg/kubelet/checkpointmanager/checksum"
-	"k8s.io/kubernetes/pkg/kubelet/checkpointmanager/errors"
+	cmerrors "k8s.io/kubernetes/pkg/kubelet/checkpointmanager/errors"
 )
 
 var _ checkpointmanager.Checkpoint = &DRAManagerCheckpoint{}
@@ -79,7 +80,7 @@ func (dc *DRAManagerCheckpoint) VerifyChecksum() error {
 	ck := dc.Checksum
 	dc.Checksum = 0
 	err := ck.Verify(dc)
-	if err == errors.ErrCorruptCheckpoint {
+	if errors.Is(err, cmerrors.CorruptCheckpointError{}) {
 		// Verify with old structs without ResourceHandles field
 		// TODO: remove in Beta
 		err = verifyChecksumWithoutResourceHandles(dc, ck)
@@ -115,8 +116,12 @@ func verifyChecksumWithoutResourceHandles(dc *DRAManagerCheckpoint, checkSum che
 	object = strings.Replace(object, "ClaimInfoStateListWithoutResourceHandles", "ClaimInfoStateList", 1)
 	hash := fnv.New32a()
 	fmt.Fprintf(hash, "%v", object)
-	if checkSum != checksum.Checksum(hash.Sum32()) {
-		return errors.ErrCorruptCheckpoint
+	actualCS := checksum.Checksum(hash.Sum32())
+	if checkSum != actualCS {
+		return &cmerrors.CorruptCheckpointError{
+			ActualCS:   uint64(actualCS),
+			ExpectedCS: uint64(checkSum),
+		}
 	}
 	return nil
 }

pkg/kubelet/cm/dra/state/state_checkpoint.go

@@ -126,7 +126,7 @@ func (sc *stateCheckpoint) GetOrCreate() (ClaimInfoStateList, error) {
 		return ClaimInfoStateList{}, nil
 	}
 	if err != nil {
-		return nil, fmt.Errorf("failed to get checkpoint %v: %v", sc.checkpointName, err)
+		return nil, fmt.Errorf("failed to get checkpoint %v: %w", sc.checkpointName, err)
 	}
 
 	return checkpoint.Entries, nil

pkg/kubelet/cm/dra/state/state_checkpoint_test.go

@@ -17,16 +17,19 @@ limitations under the License.
 package state
 
 import (
+	"errors"
 	"os"
 	"path"
 	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	resourcev1alpha2 "k8s.io/api/resource/v1alpha2"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/kubernetes/pkg/kubelet/checkpointmanager"
+	cmerrors "k8s.io/kubernetes/pkg/kubelet/checkpointmanager/errors"
 	testutil "k8s.io/kubernetes/pkg/kubelet/cm/cpumanager/state/testing"
 )
 
@@ -198,7 +201,7 @@ func TestCheckpointGetOrCreate(t *testing.T) {
 				assert.Error(t, err)
 				assert.Contains(t, err.Error(), tc.expectedError)
 			} else {
-				assert.NoError(t, err, "unexpected error while creating checkpointState")
+				requireNoCheckpointError(t, err)
 				// compare state after restoration with the one expected
 				assertStateEqual(t, state, tc.expectedState)
 			}
@@ -270,7 +273,7 @@ func TestOldCheckpointRestore(t *testing.T) {
 
 	checkpoint := NewDRAManagerCheckpoint()
 	err = cpm.GetCheckpoint(testingCheckpoint, checkpoint)
-	assert.NoError(t, err, "could not restore checkpoint")
+	requireNoCheckpointError(t, err)
 
 	checkpointData, err := checkpoint.MarshalCheckpoint()
 	assert.NoError(t, err, "could not Marshal Checkpoint")
@@ -278,3 +281,13 @@ func TestOldCheckpointRestore(t *testing.T) {
 	expectedData := `{"version":"v1","entries":[{"DriverName":"test-driver.cdi.k8s.io","ClassName":"class-name","ClaimUID":"067798be-454e-4be4-9047-1aa06aea63f7","ClaimName":"example","Namespace":"default","PodUIDs":{"139cdb46-f989-4f17-9561-ca10cfb509a6":{}},"ResourceHandles":null,"CDIDevices":{"test-driver.cdi.k8s.io":["example.com/example=cdi-example"]}}],"checksum":453625682}`
 	assert.Equal(t, expectedData, string(checkpointData), "expected ClaimInfoState does not equal to restored one")
 }
+
+func requireNoCheckpointError(t *testing.T, err error) {
+	t.Helper()
+	var cksumErr *cmerrors.CorruptCheckpointError
+	if errors.As(err, &cksumErr) {
+		t.Fatalf("unexpected corrupt checkpoint, expected checksum %d, got %d", cksumErr.ExpectedCS, cksumErr.ActualCS)
+	} else {
+		require.NoError(t, err, "could not restore checkpoint")
+	}
+}