From 6673c0c1a48fcb449ff9ccf1430996572a1be4b8 Mon Sep 17 00:00:00 2001 From: Smriti Dahal Date: Wed, 5 Apr 2023 12:15:28 -0700 Subject: [PATCH 1/5] added test case for cleanupDanglingPods --- pkg/provider/podsTracker_test.go | 78 ++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/pkg/provider/podsTracker_test.go b/pkg/provider/podsTracker_test.go index 19e0c490..c0d83283 100644 --- a/pkg/provider/podsTracker_test.go +++ b/pkg/provider/podsTracker_test.go @@ -158,3 +158,81 @@ func TestProcessPodUpdates(t *testing.T) { }) } } + +func TestCleanupDanglingPods(t *testing.T) { + activePodName1 := "pod-" + uuid.New().String() + activePodName2 := "pod-" + uuid.New().String() + danglingPodName := "pod-" + uuid.New().String() + cgName := "cg-" + uuid.New().String() + podNamespace := "ns-" + uuid.New().String() + + activePododNames := []string{activePodName1, activePodName2} + activePods := testsutil.CreatePodsList(activePododNames, podNamespace) + + allPods := testsutil.CreatePodsList([]string{danglingPodName}, podNamespace) + allPods = append(allPods, activePods[0], activePods[1]) + + cg := testsutil.CreateContainerGroupObj(cgName, podNamespace, "Succeeded", + testsutil.CreateACIContainersListObj(runningState, "Initializing", + testsutil.CgCreationTime.Add(time.Second*2), + testsutil.CgCreationTime.Add(time.Second*3), + false, false, false), "Succeeded") + + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + aciMocks := createNewACIMock() + + aciMocks.MockGetContainerGroupList = func(ctx context.Context, resourceGroup string) ([]*azaciv2.ContainerGroup, error) { + var result []*azaciv2.ContainerGroup + result = append(result, cg) + return result, nil + } + + aciMocks.MockGetContainerGroup = func(ctx context.Context, resourceGroup, containerGroupName string) (*azaciv2.ContainerGroup, error) { + return cg, nil + } + + activePodsLister := NewMockPodLister(mockCtrl) + allPodsLister := NewMockPodLister(mockCtrl) + mockPodsNamespaceLister := NewMockPodNamespaceLister(mockCtrl) + + aciProvider, err := createTestProvider(aciMocks, NewMockConfigMapLister(mockCtrl), + NewMockSecretLister(mockCtrl), activePodsLister) + if err != nil { + t.Fatal("failed to create the test provider", err) + } + + podsTracker := &PodsTracker{ + pods: activePodsLister, + handler: aciProvider, + } + + podsTracker2 := &PodsTracker{ + pods: allPodsLister, + updateCb: func(updatedPod *v1.Pod) { + for index, pod := range allPods { + if updatedPod.Name == pod.Name && updatedPod.Namespace == pod.Namespace { + allPods[index] = updatedPod + break + } + } + }, + handler: aciProvider, + } + + activePodsLister.EXPECT().List(gomock.Any()).Return(activePods, nil) + allPodsLister.EXPECT().List(gomock.Any()).Return(allPods, nil) + + activePodsLister.EXPECT().Pods(podNamespace).Return(mockPodsNamespaceLister) + mockPodsNamespaceLister.EXPECT().Get(cgName).Return(allPods[0], nil) + + aciProvider.tracker = podsTracker2 + podsTracker.cleanupDanglingPods(context.Background()) + + assert.Check(t, allPods[0].Status.ContainerStatuses[0].State.Terminated != nil, "Container should be terminated because pod was deleted") + assert.Check(t, is.Nil((allPods[0].Status.ContainerStatuses[0].State.Running)), "Container should not be running becuase pod was deleted") + assert.Check(t, is.Equal((allPods[0].Status.ContainerStatuses[0].State.Terminated.ExitCode), containerExitCodePodDeleted), "Status exit code should be set to pod deleted") + assert.Check(t, is.Equal((allPods[0].Status.ContainerStatuses[0].State.Terminated.Reason), statusReasonPodDeleted), "Status reason should be set to pod deleted") + assert.Check(t, is.Equal((allPods[0].Status.ContainerStatuses[0].State.Terminated.Message), statusMessagePodDeleted), "Status message code should be set to pod deleted") +} From 8c409b473e2b74936080201d2604b8047f0849ea Mon Sep 17 00:00:00 2001 From: Smriti Dahal Date: Wed, 5 Apr 2023 12:26:11 -0700 Subject: [PATCH 2/5] typo fix --- pkg/provider/podsTracker_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/provider/podsTracker_test.go b/pkg/provider/podsTracker_test.go index c0d83283..9393d4d9 100644 --- a/pkg/provider/podsTracker_test.go +++ b/pkg/provider/podsTracker_test.go @@ -231,7 +231,7 @@ func TestCleanupDanglingPods(t *testing.T) { podsTracker.cleanupDanglingPods(context.Background()) assert.Check(t, allPods[0].Status.ContainerStatuses[0].State.Terminated != nil, "Container should be terminated because pod was deleted") - assert.Check(t, is.Nil((allPods[0].Status.ContainerStatuses[0].State.Running)), "Container should not be running becuase pod was deleted") + assert.Check(t, is.Nil((allPods[0].Status.ContainerStatuses[0].State.Running)), "Container should not be running because pod was deleted") assert.Check(t, is.Equal((allPods[0].Status.ContainerStatuses[0].State.Terminated.ExitCode), containerExitCodePodDeleted), "Status exit code should be set to pod deleted") assert.Check(t, is.Equal((allPods[0].Status.ContainerStatuses[0].State.Terminated.Reason), statusReasonPodDeleted), "Status reason should be set to pod deleted") assert.Check(t, is.Equal((allPods[0].Status.ContainerStatuses[0].State.Terminated.Message), statusMessagePodDeleted), "Status message code should be set to pod deleted") From 5b60025f76087b0a9f9db5200c27be3c94c7e2c1 Mon Sep 17 00:00:00 2001 From: Smriti Dahal Date: Wed, 5 Apr 2023 16:21:04 -0700 Subject: [PATCH 3/5] updated cleanupDanglingPods --- pkg/provider/podsTracker_test.go | 88 ++++++++++++++++++++------------ 1 file changed, 55 insertions(+), 33 deletions(-) diff --git a/pkg/provider/podsTracker_test.go b/pkg/provider/podsTracker_test.go index 9393d4d9..33eefcab 100644 --- a/pkg/provider/podsTracker_test.go +++ b/pkg/provider/podsTracker_test.go @@ -2,6 +2,7 @@ package provider import ( "context" + "fmt" "strings" "testing" "time" @@ -160,19 +161,30 @@ func TestProcessPodUpdates(t *testing.T) { } func TestCleanupDanglingPods(t *testing.T) { - activePodName1 := "pod-" + uuid.New().String() - activePodName2 := "pod-" + uuid.New().String() + podName1 := "pod-" + uuid.New().String() + podName2 := "pod-" + uuid.New().String() danglingPodName := "pod-" + uuid.New().String() - cgName := "cg-" + uuid.New().String() podNamespace := "ns-" + uuid.New().String() - activePododNames := []string{activePodName1, activePodName2} - activePods := testsutil.CreatePodsList(activePododNames, podNamespace) + podsNames := []string{podName1, podName2} + k8sPods := testsutil.CreatePodsList(podsNames, podNamespace) - allPods := testsutil.CreatePodsList([]string{danglingPodName}, podNamespace) - allPods = append(allPods, activePods[0], activePods[1]) + activePods := testsutil.CreatePodsList([]string{danglingPodName}, podNamespace) + activePods = append(activePods, k8sPods[0], k8sPods[1]) - cg := testsutil.CreateContainerGroupObj(cgName, podNamespace, "Succeeded", + cg1 := testsutil.CreateContainerGroupObj(podName1, podNamespace, "Succeeded", + testsutil.CreateACIContainersListObj(runningState, "Initializing", + testsutil.CgCreationTime.Add(time.Second*2), + testsutil.CgCreationTime.Add(time.Second*3), + false, false, false), "Succeeded") + + cg2 := testsutil.CreateContainerGroupObj(podName2, podNamespace, "Succeeded", + testsutil.CreateACIContainersListObj(runningState, "Initializing", + testsutil.CgCreationTime.Add(time.Second*2), + testsutil.CgCreationTime.Add(time.Second*3), + false, false, false), "Succeeded") + + cg3 := testsutil.CreateContainerGroupObj(danglingPodName, podNamespace, "Succeeded", testsutil.CreateACIContainersListObj(runningState, "Initializing", testsutil.CgCreationTime.Add(time.Second*2), testsutil.CgCreationTime.Add(time.Second*3), @@ -185,16 +197,39 @@ func TestCleanupDanglingPods(t *testing.T) { aciMocks.MockGetContainerGroupList = func(ctx context.Context, resourceGroup string) ([]*azaciv2.ContainerGroup, error) { var result []*azaciv2.ContainerGroup - result = append(result, cg) + result = append(result, cg1, cg2, cg3) return result, nil } aciMocks.MockGetContainerGroup = func(ctx context.Context, resourceGroup, containerGroupName string) (*azaciv2.ContainerGroup, error) { - return cg, nil + switch containerGroupName { + case podName1: + return cg1, nil + case podName2: + return cg2, nil + case danglingPodName: + return cg3, nil + default: + return nil, nil + } + } + + aciMocks.MockDeleteContainerGroup = func(ctx context.Context, resourceGroup, cgName string) error { + updatedActivePods := make([]*v1.Pod, 0) + + for _, pod := range activePods { + podCgName := fmt.Sprintf("%s-%s", pod.Namespace, pod.Name) + if podCgName != cgName { + updatedActivePods = append(updatedActivePods, pod) + } + } + + activePods = updatedActivePods + return nil } activePodsLister := NewMockPodLister(mockCtrl) - allPodsLister := NewMockPodLister(mockCtrl) + k8sPodsLister := NewMockPodLister(mockCtrl) mockPodsNamespaceLister := NewMockPodNamespaceLister(mockCtrl) aciProvider, err := createTestProvider(aciMocks, NewMockConfigMapLister(mockCtrl), @@ -204,35 +239,22 @@ func TestCleanupDanglingPods(t *testing.T) { } podsTracker := &PodsTracker{ - pods: activePodsLister, - handler: aciProvider, - } - - podsTracker2 := &PodsTracker{ - pods: allPodsLister, + pods: k8sPodsLister, updateCb: func(updatedPod *v1.Pod) { - for index, pod := range allPods { - if updatedPod.Name == pod.Name && updatedPod.Namespace == pod.Namespace { - allPods[index] = updatedPod - break - } - } }, handler: aciProvider, } - activePodsLister.EXPECT().List(gomock.Any()).Return(activePods, nil) - allPodsLister.EXPECT().List(gomock.Any()).Return(allPods, nil) + k8sPodsLister.EXPECT().List(gomock.Any()).Return(k8sPods, nil).AnyTimes() - activePodsLister.EXPECT().Pods(podNamespace).Return(mockPodsNamespaceLister) - mockPodsNamespaceLister.EXPECT().Get(cgName).Return(allPods[0], nil) + activePodsLister.EXPECT().Pods(podNamespace).Return(mockPodsNamespaceLister).AnyTimes() + mockPodsNamespaceLister.EXPECT().Get(danglingPodName).Return(activePods[0], nil) + mockPodsNamespaceLister.EXPECT().Get(podName1).Return(activePods[1], nil) + mockPodsNamespaceLister.EXPECT().Get(podName2).Return(activePods[2], nil) - aciProvider.tracker = podsTracker2 + aciProvider.tracker = podsTracker podsTracker.cleanupDanglingPods(context.Background()) - assert.Check(t, allPods[0].Status.ContainerStatuses[0].State.Terminated != nil, "Container should be terminated because pod was deleted") - assert.Check(t, is.Nil((allPods[0].Status.ContainerStatuses[0].State.Running)), "Container should not be running because pod was deleted") - assert.Check(t, is.Equal((allPods[0].Status.ContainerStatuses[0].State.Terminated.ExitCode), containerExitCodePodDeleted), "Status exit code should be set to pod deleted") - assert.Check(t, is.Equal((allPods[0].Status.ContainerStatuses[0].State.Terminated.Reason), statusReasonPodDeleted), "Status reason should be set to pod deleted") - assert.Check(t, is.Equal((allPods[0].Status.ContainerStatuses[0].State.Terminated.Message), statusMessagePodDeleted), "Status message code should be set to pod deleted") + assert.Equal(t, len(activePods), 2, "The dangling pod should be deleted from activePods") + assert.DeepEqual(t, activePods, k8sPods) } From 2889a1ed86a2080ef9aa212a856dc5a4a5ed45c0 Mon Sep 17 00:00:00 2001 From: Smriti Dahal Date: Wed, 5 Apr 2023 16:48:27 -0700 Subject: [PATCH 4/5] updating v12 -> corev1 in test/utils.go --- pkg/tests/utils.go | 102 ++++++++++++++++++++++----------------------- 1 file changed, 51 insertions(+), 51 deletions(-) diff --git a/pkg/tests/utils.go b/pkg/tests/utils.go index f9b99bb2..bb5c033e 100644 --- a/pkg/tests/utils.go +++ b/pkg/tests/utils.go @@ -10,7 +10,7 @@ import ( azaciv2 "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/containerinstance/armcontainerinstance/v2" "github.com/google/uuid" "github.com/virtual-kubelet/azure-aci/pkg/util" - v12 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -183,54 +183,54 @@ func CreateCGProbeObj(hasHTTPGet, hasExec bool) *azaciv2.ContainerProbe { } } -func GetPodConditions(creationTime, readyConditionTime v1.Time, readyConditionStatus v12.ConditionStatus) []v12.PodCondition { - return []v12.PodCondition{ +func GetPodConditions(creationTime, readyConditionTime v1.Time, readyConditionStatus corev1.ConditionStatus) []corev1.PodCondition { + return []corev1.PodCondition{ { - Type: v12.PodReady, + Type: corev1.PodReady, Status: readyConditionStatus, LastTransitionTime: readyConditionTime, }, { - Type: v12.PodInitialized, - Status: v12.ConditionTrue, + Type: corev1.PodInitialized, + Status: corev1.ConditionTrue, LastTransitionTime: creationTime, }, { - Type: v12.PodScheduled, - Status: v12.ConditionTrue, + Type: corev1.PodScheduled, + Status: corev1.ConditionTrue, LastTransitionTime: creationTime, }, } } -func CreatePodObj(podName, podNamespace string) *v12.Pod { - return &v12.Pod{ +func CreatePodObj(podName, podNamespace string) *corev1.Pod { + return &corev1.Pod{ ObjectMeta: v1.ObjectMeta{ Name: podName, Namespace: podNamespace, }, - Spec: v12.PodSpec{ - Containers: []v12.Container{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ { Name: "nginx", - Ports: []v12.ContainerPort{ + Ports: []corev1.ContainerPort{ { Name: "http", ContainerPort: 8080, }, }, - Resources: v12.ResourceRequirements{ - Requests: v12.ResourceList{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ "cpu": resource.MustParse("0.99"), "memory": resource.MustParse("1.5G"), }, - Limits: v12.ResourceList{ + Limits: corev1.ResourceList{ "cpu": resource.MustParse("3999m"), "memory": resource.MustParse("8010M"), }, }, - LivenessProbe: &v12.Probe{ - ProbeHandler: v12.ProbeHandler{ - HTTPGet: &v12.HTTPGetAction{ + LivenessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{ Port: intstr.FromString("http"), Path: "/", }, @@ -241,9 +241,9 @@ func CreatePodObj(podName, podNamespace string) *v12.Pod { SuccessThreshold: 3, FailureThreshold: 5, }, - ReadinessProbe: &v12.Probe{ - ProbeHandler: v12.ProbeHandler{ - HTTPGet: &v12.HTTPGetAction{ + ReadinessProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{ Port: intstr.FromInt(8080), Path: "/", }, @@ -260,19 +260,19 @@ func CreatePodObj(podName, podNamespace string) *v12.Pod { } } -func CreatePodProbeObj(hasHTTPGet, hasExec bool) *v12.Probe { - var httpGet *v12.HTTPGetAction - var exec *v12.ExecAction +func CreatePodProbeObj(hasHTTPGet, hasExec bool) *corev1.Probe { + var httpGet *corev1.HTTPGetAction + var exec *corev1.ExecAction if hasHTTPGet { - httpGet = &v12.HTTPGetAction{ + httpGet = &corev1.HTTPGetAction{ Port: intstr.FromString("http"), Path: "/", Scheme: "http", } } if hasExec { - exec = &v12.ExecAction{ + exec = &corev1.ExecAction{ Command: []string{ "/bin/sh", "-c", @@ -281,16 +281,16 @@ func CreatePodProbeObj(hasHTTPGet, hasExec bool) *v12.Probe { } } - return &v12.Probe{ - ProbeHandler: v12.ProbeHandler{ + return &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ HTTPGet: httpGet, Exec: exec, }, } } -func CreateContainerPortObj(portName string, containerPort int32) []v12.ContainerPort { - return []v12.ContainerPort{ +func CreateContainerPortObj(portName string, containerPort int32) []corev1.ContainerPort { + return []corev1.ContainerPort{ { Name: portName, ContainerPort: containerPort, @@ -298,21 +298,21 @@ func CreateContainerPortObj(portName string, containerPort int32) []v12.Containe } } -func CreatePodVolumeObj(azureFileVolumeName string, fakeSecretName string, projectedVolumeName string) []v12.Volume { +func CreatePodVolumeObj(azureFileVolumeName string, fakeSecretName string, projectedVolumeName string) []corev1.Volume { emptyVolumeName := "emptyVolumeName" fakeShareName1 := "aksshare1" - return []v12.Volume{ + return []corev1.Volume{ { Name: emptyVolumeName, - VolumeSource: v12.VolumeSource{ - EmptyDir: &v12.EmptyDirVolumeSource{}, + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{}, }, }, { Name: azureFileVolumeName, - VolumeSource: v12.VolumeSource{ - AzureFile: &v12.AzureFileVolumeSource{ + VolumeSource: corev1.VolumeSource{ + AzureFile: &corev1.AzureFileVolumeSource{ ShareName: fakeShareName1, SecretName: fakeSecretName, ReadOnly: true, @@ -320,15 +320,15 @@ func CreatePodVolumeObj(azureFileVolumeName string, fakeSecretName string, proje }, }, { Name: projectedVolumeName, - VolumeSource: v12.VolumeSource{ - Projected: &v12.ProjectedVolumeSource{ - Sources: []v12.VolumeProjection{ + VolumeSource: corev1.VolumeSource{ + Projected: &corev1.ProjectedVolumeSource{ + Sources: []corev1.VolumeProjection{ { - ConfigMap: &v12.ConfigMapProjection{ - LocalObjectReference: v12.LocalObjectReference{ + ConfigMap: &corev1.ConfigMapProjection{ + LocalObjectReference: corev1.LocalObjectReference{ Name: "kube-root-ca.crt", }, - Items: []v12.KeyToPath{ + Items: []corev1.KeyToPath{ { Key: "ca.crt", Path: "ca.crt", @@ -343,22 +343,22 @@ func CreatePodVolumeObj(azureFileVolumeName string, fakeSecretName string, proje } } -func CreatePodsList(podNames []string, podNameSpace string) []*v12.Pod { - result := make([]*v12.Pod, 0, len(podNames)) +func CreatePodsList(podNames []string, podNameSpace string) []*corev1.Pod { + result := make([]*corev1.Pod, 0, len(podNames)) for _, podName := range podNames { - pod := &v12.Pod{ + pod := &corev1.Pod{ ObjectMeta: v1.ObjectMeta{ Name: podName, Namespace: podNameSpace, CreationTimestamp: v1.NewTime(time.Now()), UID: types.UID(uuid.New().String()), }, - Status: v12.PodStatus{ - Phase: v12.PodRunning, - ContainerStatuses: []v12.ContainerStatus { + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{ { - State: v12.ContainerState { - Running: &v12.ContainerStateRunning{ + State: corev1.ContainerState{ + Running: &corev1.ContainerStateRunning{ StartedAt: v1.NewTime(time.Now()), }, }, From 0a98d8113dbf08e5dff13ba0e27b89cea23d2be9 Mon Sep 17 00:00:00 2001 From: Smriti Dahal Date: Thu, 6 Apr 2023 11:46:41 -0700 Subject: [PATCH 5/5] fixed for loop and updated assertion --- pkg/provider/podsTracker_test.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/provider/podsTracker_test.go b/pkg/provider/podsTracker_test.go index 33eefcab..4418fdb4 100644 --- a/pkg/provider/podsTracker_test.go +++ b/pkg/provider/podsTracker_test.go @@ -15,6 +15,7 @@ import ( "gotest.tools/assert" is "gotest.tools/assert/cmp" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" ) func TestUpdatePodStatus(t *testing.T) { @@ -217,10 +218,10 @@ func TestCleanupDanglingPods(t *testing.T) { aciMocks.MockDeleteContainerGroup = func(ctx context.Context, resourceGroup, cgName string) error { updatedActivePods := make([]*v1.Pod, 0) - for _, pod := range activePods { - podCgName := fmt.Sprintf("%s-%s", pod.Namespace, pod.Name) + for i := range activePods { + podCgName := fmt.Sprintf("%s-%s", activePods[i].Namespace, activePods[i].Name) if podCgName != cgName { - updatedActivePods = append(updatedActivePods, pod) + updatedActivePods = append(updatedActivePods, activePods[i]) } } @@ -256,5 +257,9 @@ func TestCleanupDanglingPods(t *testing.T) { podsTracker.cleanupDanglingPods(context.Background()) assert.Equal(t, len(activePods), 2, "The dangling pod should be deleted from activePods") - assert.DeepEqual(t, activePods, k8sPods) + for i := range activePods { + if !equality.Semantic.DeepEqual(activePods[i], k8sPods[i]) { + t.Errorf("activePods and k8sPods should be in sync. Expected %#v, got %#v", k8sPods[i], activePods[i]) + } + } }