diff --git a/pkg/kubelet/apis/podresources/server_v1_test.go b/pkg/kubelet/apis/podresources/server_v1_test.go index aaa560fc4f0..2bc69a00004 100644 --- a/pkg/kubelet/apis/podresources/server_v1_test.go +++ b/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 -} diff --git a/pkg/kubelet/checkpointmanager/checksum/checksum.go b/pkg/kubelet/checkpointmanager/checksum/checksum.go index d5892b7be65..5dcd4f24194 100644 --- a/pkg/kubelet/checkpointmanager/checksum/checksum.go +++ b/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 } diff --git a/pkg/kubelet/checkpointmanager/errors/errors.go b/pkg/kubelet/checkpointmanager/errors/errors.go index bb445f207ec..558f9c7e074 100644 --- a/pkg/kubelet/checkpointmanager/errors/errors.go +++ b/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") diff --git a/pkg/kubelet/cm/cpumanager/state/checkpoint.go b/pkg/kubelet/cm/cpumanager/state/checkpoint.go index eb2bfa27eaf..564c3482c04 100644 --- a/pkg/kubelet/cm/cpumanager/state/checkpoint.go +++ b/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 diff --git a/pkg/kubelet/cm/dra/state/checkpoint.go b/pkg/kubelet/cm/dra/state/checkpoint.go index 7c44f12eea9..6ea49c62e99 100644 --- a/pkg/kubelet/cm/dra/state/checkpoint.go +++ b/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 } diff --git a/pkg/kubelet/cm/dra/state/state_checkpoint.go b/pkg/kubelet/cm/dra/state/state_checkpoint.go index a82f6b11bb4..46e837b136c 100644 --- a/pkg/kubelet/cm/dra/state/state_checkpoint.go +++ b/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 diff --git a/pkg/kubelet/cm/dra/state/state_checkpoint_test.go b/pkg/kubelet/cm/dra/state/state_checkpoint_test.go index a5ff86eed17..cb930bdc8fe 100644 --- a/pkg/kubelet/cm/dra/state/state_checkpoint_test.go +++ b/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") + } +}