From ca5478a458b1f671236d7e2a03333f6748dd02ba Mon Sep 17 00:00:00 2001 From: Nishant Dhungel Date: Mon, 27 Mar 2023 17:19:15 -0700 Subject: [PATCH 01/10] Added unit test for getting Secret Volume --- pkg/provider/aci_volumes_test.go | 113 +++++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/pkg/provider/aci_volumes_test.go b/pkg/provider/aci_volumes_test.go index d66259a8..a7182617 100644 --- a/pkg/provider/aci_volumes_test.go +++ b/pkg/provider/aci_volumes_test.go @@ -18,6 +18,7 @@ import ( "gotest.tools/assert" is "gotest.tools/assert/cmp" v1 "k8s.io/api/core/v1" + errors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -490,3 +491,115 @@ func TestCreatePodWithCSIVolume(t *testing.T) { }) } } + +func TestGetSecretVolume(t *testing.T) { + fakeVolumeSecret := "fake-volume-secret" + secretVolumeName := "SecretVolume" + secretName := "api-key" + + fakeSecret := v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: fakeVolumeSecret, + Namespace: podNamespace, + }, + Data: map[string][]byte{ + azureFileStorageAccountName: []byte("azureFileStorageAccountName"), + azureFileStorageAccountKey: []byte("azureFileStorageAccountKey")}, + } + + setOptional := new(bool) + *setOptional = false + + fakePodVolumes := []v1.Volume{ + { + Name: emptyVolumeName, + VolumeSource: v1.VolumeSource{ + EmptyDir: &v1.EmptyDirVolumeSource{}, + }, + }, + { + Name: secretVolumeName, + VolumeSource: v1.VolumeSource{ + Secret: &v1.SecretVolumeSource{ + SecretName: secretName, + Items: []v1.KeyToPath{ + { + Key: "azureFileStorageAccountName", + Path: "azureFileStorageAccountName", + }, + }, + Optional: setOptional, + }, + }, + }, + } + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + aciMocks := createNewACIMock() + + cases := []struct { + description string + callSecretMocks func(secretMock *MockSecretLister) + expectedError error + }{ + { + description: "Secret is nil and returns error while Optional is set to true", + callSecretMocks: func(secretMock *MockSecretLister) { + for _, volume := range fakePodVolumes { + if volume.Name == secretVolumeName { + mockSecretNamespaceLister := NewMockSecretNamespaceLister(mockCtrl) + secretMock.EXPECT().Secrets(podNamespace).Return(mockSecretNamespaceLister) + mockSecretNamespaceLister.EXPECT().Get(volume.Secret.SecretName).Return(nil, errors.NewNotFound(v1.Resource("secret"), secretName)) + } + } + }, + expectedError: fmt.Errorf("secret %s is required by Pod %s and does not exist", secretName, podName), + }, + { + description: "Secret returns a valid value", + callSecretMocks: func(secretMock *MockSecretLister) { + for _, volume := range fakePodVolumes { + if volume.Name == secretVolumeName { + mockSecretNamespaceLister := NewMockSecretNamespaceLister(mockCtrl) + secretMock.EXPECT().Secrets(podNamespace).Return(mockSecretNamespaceLister) + mockSecretNamespaceLister.EXPECT().Get(volume.Secret.SecretName).Return(&fakeSecret, nil) + } + } + }, + expectedError: nil, + }, + } + + for _, tc := range cases { + t.Run(tc.description, func(t *testing.T) { + mockSecretLister := NewMockSecretLister(mockCtrl) + + pod := testsutil.CreatePodObj(podName, podNamespace) + tc.callSecretMocks(mockSecretLister) + + pod.Spec.Volumes = fakePodVolumes + + provider, err := createTestProvider(aciMocks, NewMockConfigMapLister(mockCtrl), + mockSecretLister, NewMockPodLister(mockCtrl)) + if err != nil { + t.Fatal("Unable to create test provider", err) + } + + volumes,err := provider.getVolumes(context.Background(), pod) + + if tc.expectedError == nil { + azureStorageAccountName := base64.StdEncoding.EncodeToString([]byte("azureFileStorageAccountName")) + azureStorageAccountKey := base64.StdEncoding.EncodeToString([]byte("azureFileStorageAccountKey")) + assert.NilError(t, tc.expectedError, err) + assert.DeepEqual(t, *volumes[1].Secret[azureFileStorageAccountName], azureStorageAccountName) + assert.DeepEqual(t, *volumes[1].Secret[azureFileStorageAccountKey], azureStorageAccountKey) + } else { + assert.Equal(t, tc.expectedError.Error(), err.Error()) + } + + }) + } + +} \ No newline at end of file From 6c768fd13fcbaf559a9bcf0e21edcf1605564704 Mon Sep 17 00:00:00 2001 From: Nishant Dhungel Date: Mon, 27 Mar 2023 18:27:33 -0700 Subject: [PATCH 02/10] Fixed typo on description --- pkg/provider/aci_volumes_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/provider/aci_volumes_test.go b/pkg/provider/aci_volumes_test.go index a7182617..3786e047 100644 --- a/pkg/provider/aci_volumes_test.go +++ b/pkg/provider/aci_volumes_test.go @@ -545,7 +545,7 @@ func TestGetSecretVolume(t *testing.T) { expectedError error }{ { - description: "Secret is nil and returns error while Optional is set to true", + description: "Secret is nil and returns error while Optional is set to false", callSecretMocks: func(secretMock *MockSecretLister) { for _, volume := range fakePodVolumes { if volume.Name == secretVolumeName { From 37be2f860826188fdd8e0a6f14827c20d96612c4 Mon Sep 17 00:00:00 2001 From: Nishant Dhungel Date: Tue, 28 Mar 2023 09:37:37 -0700 Subject: [PATCH 03/10] Changed method name reflecting what's tested --- pkg/provider/aci_volumes_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/provider/aci_volumes_test.go b/pkg/provider/aci_volumes_test.go index 3786e047..966bee3a 100644 --- a/pkg/provider/aci_volumes_test.go +++ b/pkg/provider/aci_volumes_test.go @@ -492,7 +492,7 @@ func TestCreatePodWithCSIVolume(t *testing.T) { } } -func TestGetSecretVolume(t *testing.T) { +func TestGetVolumesForSecretVolume(t *testing.T) { fakeVolumeSecret := "fake-volume-secret" secretVolumeName := "SecretVolume" secretName := "api-key" From 56b2bf08e7e7d23de4d7c7348c7857fa50335187 Mon Sep 17 00:00:00 2001 From: Nishant Dhungel Date: Tue, 28 Mar 2023 09:41:08 -0700 Subject: [PATCH 04/10] Added new line in the end --- pkg/provider/aci_volumes_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/provider/aci_volumes_test.go b/pkg/provider/aci_volumes_test.go index 966bee3a..98eb912a 100644 --- a/pkg/provider/aci_volumes_test.go +++ b/pkg/provider/aci_volumes_test.go @@ -602,4 +602,4 @@ func TestGetVolumesForSecretVolume(t *testing.T) { }) } -} \ No newline at end of file +} From bf22fcde07c14f3ccdc56bc9f1c7ffc343db7d36 Mon Sep 17 00:00:00 2001 From: Nishant Dhungel Date: Mon, 27 Mar 2023 17:19:15 -0700 Subject: [PATCH 05/10] Added unit test for getting Secret Volume --- pkg/provider/aci_volumes_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/provider/aci_volumes_test.go b/pkg/provider/aci_volumes_test.go index 98eb912a..10c80aec 100644 --- a/pkg/provider/aci_volumes_test.go +++ b/pkg/provider/aci_volumes_test.go @@ -492,7 +492,7 @@ func TestCreatePodWithCSIVolume(t *testing.T) { } } -func TestGetVolumesForSecretVolume(t *testing.T) { +func TestGetSecretVolume(t *testing.T) { fakeVolumeSecret := "fake-volume-secret" secretVolumeName := "SecretVolume" secretName := "api-key" @@ -545,7 +545,7 @@ func TestGetVolumesForSecretVolume(t *testing.T) { expectedError error }{ { - description: "Secret is nil and returns error while Optional is set to false", + description: "Secret is nil and returns error while Optional is set to true", callSecretMocks: func(secretMock *MockSecretLister) { for _, volume := range fakePodVolumes { if volume.Name == secretVolumeName { From e39c86296060ad2c246d7b80a2502263cfc51d49 Mon Sep 17 00:00:00 2001 From: Nishant Dhungel Date: Mon, 27 Mar 2023 18:27:33 -0700 Subject: [PATCH 06/10] Fixed typo on description --- pkg/provider/aci_volumes_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/provider/aci_volumes_test.go b/pkg/provider/aci_volumes_test.go index 10c80aec..0be876e7 100644 --- a/pkg/provider/aci_volumes_test.go +++ b/pkg/provider/aci_volumes_test.go @@ -545,7 +545,7 @@ func TestGetSecretVolume(t *testing.T) { expectedError error }{ { - description: "Secret is nil and returns error while Optional is set to true", + description: "Secret is nil and returns error while Optional is set to false", callSecretMocks: func(secretMock *MockSecretLister) { for _, volume := range fakePodVolumes { if volume.Name == secretVolumeName { From ec18a72ffaeea7407b4760191a474f8e21e54043 Mon Sep 17 00:00:00 2001 From: Nishant Dhungel Date: Tue, 28 Mar 2023 09:37:37 -0700 Subject: [PATCH 07/10] Changed method name reflecting what's tested --- pkg/provider/aci_volumes_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/provider/aci_volumes_test.go b/pkg/provider/aci_volumes_test.go index 0be876e7..98eb912a 100644 --- a/pkg/provider/aci_volumes_test.go +++ b/pkg/provider/aci_volumes_test.go @@ -492,7 +492,7 @@ func TestCreatePodWithCSIVolume(t *testing.T) { } } -func TestGetSecretVolume(t *testing.T) { +func TestGetVolumesForSecretVolume(t *testing.T) { fakeVolumeSecret := "fake-volume-secret" secretVolumeName := "SecretVolume" secretName := "api-key" From b35dabe0de5fd4fe69acde401a0dec4e3bddf237 Mon Sep 17 00:00:00 2001 From: Nishant Dhungel Date: Tue, 4 Apr 2023 12:42:26 -0700 Subject: [PATCH 08/10] Added UT for real_time.go for CalculateUsageNanoCores --- pkg/metrics/real_time_test.go | 103 ++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) create mode 100644 pkg/metrics/real_time_test.go diff --git a/pkg/metrics/real_time_test.go b/pkg/metrics/real_time_test.go new file mode 100644 index 00000000..40ed00f4 --- /dev/null +++ b/pkg/metrics/real_time_test.go @@ -0,0 +1,103 @@ +package metrics + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCalculateUsageNanoCores(t *testing.T) { + + fake_container1 := "fake-container-name1" + fake_container2 := "fake-container-name2" + + newPodStatus_containers := []containerStats{ + { + Name: fake_container1, + CPU: cpuStats{ + UsageCoreNanoSeconds: 2345678900, + }, + }, + { + Name: fake_container2, + CPU: cpuStats{ + UsageCoreNanoSeconds: 2345678900, + }, + }, + } + + lastPodStatus_containers := []containerStats{ + { + Name: fake_container1, + CPU: cpuStats{ + UsageCoreNanoSeconds: 1234567800, + }, + }, + { + Name: fake_container2, + CPU: cpuStats{ + UsageCoreNanoSeconds: 1234567800, + }, + }, + } + + testCases := []struct { + desc string + containerName *string + lastPodStatus *realtimeMetricsExtensionPodStats + newPodStatus *realtimeMetricsExtensionPodStats + expectedUsage *uint64 + }{ + { + desc: "NewPodStatus timestamp is earlier than LastPodStatus", + containerName: nil, + lastPodStatus: &realtimeMetricsExtensionPodStats{ + Timestamp: 2345000000, + }, + newPodStatus: &realtimeMetricsExtensionPodStats{ + Timestamp: 1234000000, + }, + expectedUsage: newUInt64Pointer(0), + }, + { + desc: "Container Name is nil", + containerName: nil, + lastPodStatus: &realtimeMetricsExtensionPodStats{ + Timestamp: 1234500000, + CPU: cpuStats{ + UsageCoreNanoSeconds: 1234567000, + }, + }, + newPodStatus: &realtimeMetricsExtensionPodStats{ + Timestamp: 2345600000, + CPU: cpuStats{ + UsageCoreNanoSeconds: 2345678000, + }, + }, + //tc.newPodStatus.CPU.UsageCoreNanoSeconds-tc.lastPodStatus.CPU.UsageCoreNanoSeconds/((tc.newPodStatus.TimeStamp-tc.lastPodStatus.TimeStamp)/1000000000) + expectedUsage: newUInt64Pointer(1111111000 / (1111100000 / 1000000000)), + }, + { + desc: "Container Name is not nil", + containerName: &fake_container1, + lastPodStatus: &realtimeMetricsExtensionPodStats{ + Timestamp: 1234560000, + Containers: lastPodStatus_containers, + }, + newPodStatus: &realtimeMetricsExtensionPodStats{ + Timestamp: 2345670000, + Containers: newPodStatus_containers, + }, + //tc.newPodStatus.containers[i=0].CPU.UsageCoreNanoSeconds-tc.lastPodStatus.containers[i=0].CPU.UsageCoreNanoSeconds/((tc.newPodStatus.TimeStamp-tc.lastPodStatus.TimeStamp)/1000000000) + expectedUsage: newUInt64Pointer(1111111100 / (1111110000 / 1000000000)), + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + nanoCoreUsage := calculateUsageNanoCores(tc.containerName, tc.lastPodStatus, tc.newPodStatus) + assert.EqualValues(t, tc.expectedUsage, nanoCoreUsage, tc.desc) + }) + } + +} From 454c77451362ff3ecd003e6fd1cf2d292696a6ad Mon Sep 17 00:00:00 2001 From: Nishant Dhungel Date: Tue, 4 Apr 2023 15:49:06 -0700 Subject: [PATCH 09/10] Bugfix for using low and negative uint64 values in real_time.go. Added related test --- pkg/metrics/real_time.go | 18 ++++++++++++++++-- pkg/metrics/real_time_test.go | 17 +++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/pkg/metrics/real_time.go b/pkg/metrics/real_time.go index 0519cf69..4c9cadfd 100644 --- a/pkg/metrics/real_time.go +++ b/pkg/metrics/real_time.go @@ -220,9 +220,19 @@ func calculateUsageNanoCores(containerName *string, lastPodStatus *realtimeMetri return newUInt64Pointer(0) } var timeWindowsSeconds uint64 = timeWindowsNanoSeconds / 1000000000 + + if timeWindowsSeconds <= 0 { + return newUInt64Pointer(0) + } + if containerName == nil { // calculate for Pod - v := (newPodStatus.CPU.UsageCoreNanoSeconds - lastPodStatus.CPU.UsageCoreNanoSeconds) / timeWindowsSeconds + usageDiff := newPodStatus.CPU.UsageCoreNanoSeconds - lastPodStatus.CPU.UsageCoreNanoSeconds + if usageDiff <= 0 { + return newUInt64Pointer(0) + } + + v := usageDiff / timeWindowsSeconds return &v } else { // calcuate for specified container @@ -244,7 +254,11 @@ func calculateUsageNanoCores(containerName *string, lastPodStatus *realtimeMetri if newContainerUsageCoreNanoSeconds == nil { return newUInt64Pointer(0) } - v := (*newContainerUsageCoreNanoSeconds - *oldContainerUsageCoreNanoSeconds) / timeWindowsSeconds + usageDiff := *newContainerUsageCoreNanoSeconds - *oldContainerUsageCoreNanoSeconds + if usageDiff <= 0 { + return newUInt64Pointer(0) + } + v := usageDiff / timeWindowsSeconds return &v } } diff --git a/pkg/metrics/real_time_test.go b/pkg/metrics/real_time_test.go index 40ed00f4..e5e091dc 100644 --- a/pkg/metrics/real_time_test.go +++ b/pkg/metrics/real_time_test.go @@ -59,6 +59,23 @@ func TestCalculateUsageNanoCores(t *testing.T) { }, expectedUsage: newUInt64Pointer(0), }, + { + desc: "New and Last Pod Status Timestamp difference value is very low", + containerName: nil, + lastPodStatus: &realtimeMetricsExtensionPodStats{ + Timestamp: 1234, + CPU: cpuStats{ + UsageCoreNanoSeconds: 1234567, + }, + }, + newPodStatus: &realtimeMetricsExtensionPodStats{ + Timestamp: 2345, + CPU: cpuStats{ + UsageCoreNanoSeconds: 2345678, + }, + }, + expectedUsage: newUInt64Pointer(0), + }, { desc: "Container Name is nil", containerName: nil, From ce3708ab4a744ff7d1a89e4236ef8dd4a5d26083 Mon Sep 17 00:00:00 2001 From: Nishant Dhungel Date: Tue, 4 Apr 2023 18:54:37 -0700 Subject: [PATCH 10/10] Bugfix for shallow copy. Added more test cases. --- pkg/metrics/real_time.go | 14 +++++++------ pkg/metrics/real_time_test.go | 38 +++++++++++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/pkg/metrics/real_time.go b/pkg/metrics/real_time.go index 4c9cadfd..9d3ad13f 100644 --- a/pkg/metrics/real_time.go +++ b/pkg/metrics/real_time.go @@ -227,19 +227,20 @@ func calculateUsageNanoCores(containerName *string, lastPodStatus *realtimeMetri if containerName == nil { // calculate for Pod - usageDiff := newPodStatus.CPU.UsageCoreNanoSeconds - lastPodStatus.CPU.UsageCoreNanoSeconds + usageDiff := int64(newPodStatus.CPU.UsageCoreNanoSeconds - lastPodStatus.CPU.UsageCoreNanoSeconds) if usageDiff <= 0 { return newUInt64Pointer(0) } - v := usageDiff / timeWindowsSeconds + v := uint64(usageDiff) / timeWindowsSeconds return &v } else { // calcuate for specified container var oldContainerUsageCoreNanoSeconds *uint64 = nil for _, container := range lastPodStatus.Containers { if container.Name == *containerName { - oldContainerUsageCoreNanoSeconds = &container.CPU.UsageCoreNanoSeconds + containerUsageCoreNanoSeconds := container.CPU.UsageCoreNanoSeconds + oldContainerUsageCoreNanoSeconds = &containerUsageCoreNanoSeconds } } if oldContainerUsageCoreNanoSeconds == nil { @@ -248,17 +249,18 @@ func calculateUsageNanoCores(containerName *string, lastPodStatus *realtimeMetri var newContainerUsageCoreNanoSeconds *uint64 = nil for _, container := range newPodStatus.Containers { if container.Name == *containerName { - newContainerUsageCoreNanoSeconds = &container.CPU.UsageCoreNanoSeconds + containerUsageCoreNanoSeconds := container.CPU.UsageCoreNanoSeconds + newContainerUsageCoreNanoSeconds = &containerUsageCoreNanoSeconds } } if newContainerUsageCoreNanoSeconds == nil { return newUInt64Pointer(0) } - usageDiff := *newContainerUsageCoreNanoSeconds - *oldContainerUsageCoreNanoSeconds + usageDiff := int64(*newContainerUsageCoreNanoSeconds - *oldContainerUsageCoreNanoSeconds) if usageDiff <= 0 { return newUInt64Pointer(0) } - v := usageDiff / timeWindowsSeconds + v := uint64(usageDiff) / timeWindowsSeconds return &v } } diff --git a/pkg/metrics/real_time_test.go b/pkg/metrics/real_time_test.go index e5e091dc..b658b533 100644 --- a/pkg/metrics/real_time_test.go +++ b/pkg/metrics/real_time_test.go @@ -21,7 +21,7 @@ func TestCalculateUsageNanoCores(t *testing.T) { { Name: fake_container2, CPU: cpuStats{ - UsageCoreNanoSeconds: 2345678900, + UsageCoreNanoSeconds: 1234567800, }, }, } @@ -36,7 +36,7 @@ func TestCalculateUsageNanoCores(t *testing.T) { { Name: fake_container2, CPU: cpuStats{ - UsageCoreNanoSeconds: 1234567800, + UsageCoreNanoSeconds: 2345678900, }, }, } @@ -77,7 +77,24 @@ func TestCalculateUsageNanoCores(t *testing.T) { expectedUsage: newUInt64Pointer(0), }, { - desc: "Container Name is nil", + desc: "Container Name is nil and lastPodStatus.CPU.UsageCoreNanoSeconds is greater than newPodStatus.CPU.UsageCoreNanoSeconds", + containerName: nil, + lastPodStatus: &realtimeMetricsExtensionPodStats{ + Timestamp: 1234500000, + CPU: cpuStats{ + UsageCoreNanoSeconds: 2345678000, + }, + }, + newPodStatus: &realtimeMetricsExtensionPodStats{ + Timestamp: 2345600000, + CPU: cpuStats{ + UsageCoreNanoSeconds: 1234567000, + }, + }, + expectedUsage: newUInt64Pointer(0), + }, + { + desc: "Container Name is nil and newPodStatus.CPU.UsageCoreNanoSeconds is greater than lastPodStatus.CPU.UsageCoreNanoSeconds", containerName: nil, lastPodStatus: &realtimeMetricsExtensionPodStats{ Timestamp: 1234500000, @@ -95,7 +112,7 @@ func TestCalculateUsageNanoCores(t *testing.T) { expectedUsage: newUInt64Pointer(1111111000 / (1111100000 / 1000000000)), }, { - desc: "Container Name is not nil", + desc: "Container Name is not nil and newPodStatus.CPU.UsageCoreNanoSeconds is greater than lastPodStatus.CPU.UsageCoreNanoSeconds", containerName: &fake_container1, lastPodStatus: &realtimeMetricsExtensionPodStats{ Timestamp: 1234560000, @@ -108,6 +125,19 @@ func TestCalculateUsageNanoCores(t *testing.T) { //tc.newPodStatus.containers[i=0].CPU.UsageCoreNanoSeconds-tc.lastPodStatus.containers[i=0].CPU.UsageCoreNanoSeconds/((tc.newPodStatus.TimeStamp-tc.lastPodStatus.TimeStamp)/1000000000) expectedUsage: newUInt64Pointer(1111111100 / (1111110000 / 1000000000)), }, + { + desc: "Container Name is not nil and lastPodStatus.CPU.UsageCoreNanoSeconds is greater than newPodStatus.CPU.UsageCoreNanoSeconds", + containerName: &fake_container2, + lastPodStatus: &realtimeMetricsExtensionPodStats{ + Timestamp: 1234500000, + Containers: lastPodStatus_containers, + }, + newPodStatus: &realtimeMetricsExtensionPodStats{ + Timestamp: 2345600000, + Containers: lastPodStatus_containers, + }, + expectedUsage: newUInt64Pointer(0), + }, } for _, tc := range testCases {