You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2021/03/22 02:48:34 UTC

[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #246: [YUNIKORN-584] App recovery is skipped when applicationID is not set in pods' label.

wilfred-s commented on a change in pull request #246:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/246#discussion_r598378697



##########
File path: pkg/appmgmt/general/general_test.go
##########
@@ -526,3 +527,269 @@ func TestGetOwnerReferences(t *testing.T) {
 	assert.Equal(t, returnedOwnerRefs[0].Kind, "Pod", "Unexpected owner reference Kind")
 	assert.Equal(t, returnedOwnerRefs[0].APIVersion, v1.SchemeGroupVersion.String(), "Unexpected owner reference Kind")
 }
+
+// nolint: funlen
+func TestListApplication(t *testing.T) {
+	var app01, app02, app03, app04, app05, app06 = "app00001",
+		"app00002", "app00003", "app00004", "app00005", "app00006"
+	var queue01, queue02 = "root.queue01", "root.queue02"
+	var ns01, ns02 = "namespace01", "namespace02"
+
+	// mock the pod lister for this test
+	mockedAPIProvider := client.NewMockedAPIProvider()
+	mockedPodLister := test.NewPodListerMock()
+	mockedAPIProvider.SetPodLister(mockedPodLister)
+
+	// app01 pods, running in namespace01 and queue01
+	// all pods are having applicationID and queue name specified
+	// 2 pods have assigned nodes, 1 pod is pending for scheduling
+	// allocated pod
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00001",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "allocated-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00002",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "allocated-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00003",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})
+
+	// app02 pods, running in queue02 and namespace02
+	// 2 pods are having applicationID and queue name specified
+	// both 2 pods are pending
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app02pod0001",
+			Namespace: ns02,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app02,
+				constants.LabelQueueName:     queue02,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app02pod0002",
+			Namespace: ns02,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app02,
+				constants.LabelQueueName:     queue02,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})
+
+	// app03 pods, running in queue02 and namespace02
+	// 2 pods do not have label applicationID specified, but have spark-app-selector
+	// both 2 pods are allocated
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app03pod0001",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.SparkLabelAppID: app03,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "some-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app03pod0002",
+			Namespace: ns02,
+			Labels: map[string]string{
+				constants.SparkLabelAppID: app03,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "some-node",
+		},
+	})
+
+	// app04 pods, running in queue01 and namespace01
+	// app04 has 2 pods which only has annotation yunikorn.apache.org/app-id specified
+	// both 2 pods are allocated
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app04pod0001",
+			Namespace: ns01,
+			Annotations: map[string]string{
+				constants.AnnotationApplicationID: app04,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "some-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app04pod0002",
+			Namespace: ns01,
+			Annotations: map[string]string{
+				constants.AnnotationApplicationID: app04,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "some-node",
+		},
+	})
+
+	// app05 pods, running in queue01 and namespace01
+	// app05 pod has no label or annotation specified
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app05pod0001",
+			Namespace: ns01,
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "some-node",
+		},
+	})
+
+	// app06 pods, running in queue01 and namespace01
+	// pod has spark-app-selector set and it is allocated but not scheduled by yunikorn
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app06pod0001",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.SparkLabelAppID: app06,
+			},
+		},
+		Spec: v1.PodSpec{
+			NodeName: "some-node",
+		},
+	})
+
+	// init the app manager and run listApp
+	am := NewManager(cache.NewMockedAMProtocol(), mockedAPIProvider)
+	apps, err := am.ListApplications()
+	assert.NilError(t, err)
+	assert.Equal(t, len(apps), 3)
+	_, exist := apps[app01]
+	assert.Equal(t, exist, true,
+		"app01 should be included in the list because "+
+			"it has applicationID and queue namespace specified in the"+
+			"queue and it has 2 pods allocated.")

Review comment:
       We do not have line length limits so do not split text over multiple lines there is no need.

##########
File path: pkg/appmgmt/general/general.go
##########
@@ -293,15 +292,8 @@ func (os *Manager) deletePod(obj interface{}) {
 }
 
 func (os *Manager) ListApplications() (map[string]interfaces.ApplicationMetadata, error) {
-	// selector: applicationID exist
-	slt := labels.NewSelector()
-	req, err := labels.NewRequirement(constants.LabelApplicationID, selection.Exists, nil)
-	if err != nil {
-		return nil, err
-	}
-	slt = slt.Add(*req)
-
 	// list all pods on this cluster
+	slt := labels.NewSelector()
 	appPods, err := os.apiProvider.GetAPIs().PodInformer.Lister().List(slt)

Review comment:
       With the new behaviour there is no need to store the selector.
   Can be simplified to `List(labels.NewSelector())` as it returns a typed nil.

##########
File path: pkg/appmgmt/general/general_test.go
##########
@@ -526,3 +527,269 @@ func TestGetOwnerReferences(t *testing.T) {
 	assert.Equal(t, returnedOwnerRefs[0].Kind, "Pod", "Unexpected owner reference Kind")
 	assert.Equal(t, returnedOwnerRefs[0].APIVersion, v1.SchemeGroupVersion.String(), "Unexpected owner reference Kind")
 }
+
+// nolint: funlen
+func TestListApplication(t *testing.T) {
+	var app01, app02, app03, app04, app05, app06 = "app00001",
+		"app00002", "app00003", "app00004", "app00005", "app00006"
+	var queue01, queue02 = "root.queue01", "root.queue02"
+	var ns01, ns02 = "namespace01", "namespace02"
+
+	// mock the pod lister for this test
+	mockedAPIProvider := client.NewMockedAPIProvider()
+	mockedPodLister := test.NewPodListerMock()
+	mockedAPIProvider.SetPodLister(mockedPodLister)
+
+	// app01 pods, running in namespace01 and queue01
+	// all pods are having applicationID and queue name specified
+	// 2 pods have assigned nodes, 1 pod is pending for scheduling
+	// allocated pod
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00001",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "allocated-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00002",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "allocated-node",
+		},
+	})

Review comment:
       This is a duplicate from app01pod00001 and does not test anything new, should be removed.

##########
File path: pkg/appmgmt/general/general_test.go
##########
@@ -526,3 +527,269 @@ func TestGetOwnerReferences(t *testing.T) {
 	assert.Equal(t, returnedOwnerRefs[0].Kind, "Pod", "Unexpected owner reference Kind")
 	assert.Equal(t, returnedOwnerRefs[0].APIVersion, v1.SchemeGroupVersion.String(), "Unexpected owner reference Kind")
 }
+
+// nolint: funlen
+func TestListApplication(t *testing.T) {
+	var app01, app02, app03, app04, app05, app06 = "app00001",
+		"app00002", "app00003", "app00004", "app00005", "app00006"
+	var queue01, queue02 = "root.queue01", "root.queue02"
+	var ns01, ns02 = "namespace01", "namespace02"
+
+	// mock the pod lister for this test
+	mockedAPIProvider := client.NewMockedAPIProvider()
+	mockedPodLister := test.NewPodListerMock()
+	mockedAPIProvider.SetPodLister(mockedPodLister)
+
+	// app01 pods, running in namespace01 and queue01
+	// all pods are having applicationID and queue name specified
+	// 2 pods have assigned nodes, 1 pod is pending for scheduling
+	// allocated pod
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00001",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "allocated-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00002",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "allocated-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00003",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})
+
+	// app02 pods, running in queue02 and namespace02
+	// 2 pods are having applicationID and queue name specified
+	// both 2 pods are pending
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app02pod0001",
+			Namespace: ns02,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app02,
+				constants.LabelQueueName:     queue02,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})

Review comment:
       This is a duplicate from app01pod00003 and does not test anything new, should be removed.

##########
File path: pkg/appmgmt/general/general_test.go
##########
@@ -526,3 +527,269 @@ func TestGetOwnerReferences(t *testing.T) {
 	assert.Equal(t, returnedOwnerRefs[0].Kind, "Pod", "Unexpected owner reference Kind")
 	assert.Equal(t, returnedOwnerRefs[0].APIVersion, v1.SchemeGroupVersion.String(), "Unexpected owner reference Kind")
 }
+
+// nolint: funlen
+func TestListApplication(t *testing.T) {
+	var app01, app02, app03, app04, app05, app06 = "app00001",
+		"app00002", "app00003", "app00004", "app00005", "app00006"
+	var queue01, queue02 = "root.queue01", "root.queue02"
+	var ns01, ns02 = "namespace01", "namespace02"
+
+	// mock the pod lister for this test
+	mockedAPIProvider := client.NewMockedAPIProvider()
+	mockedPodLister := test.NewPodListerMock()
+	mockedAPIProvider.SetPodLister(mockedPodLister)
+
+	// app01 pods, running in namespace01 and queue01
+	// all pods are having applicationID and queue name specified
+	// 2 pods have assigned nodes, 1 pod is pending for scheduling
+	// allocated pod
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00001",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "allocated-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00002",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "allocated-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00003",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})
+
+	// app02 pods, running in queue02 and namespace02
+	// 2 pods are having applicationID and queue name specified
+	// both 2 pods are pending
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app02pod0001",
+			Namespace: ns02,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app02,
+				constants.LabelQueueName:     queue02,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app02pod0002",
+			Namespace: ns02,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app02,
+				constants.LabelQueueName:     queue02,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})

Review comment:
       This is a duplicate from app01pod00003 and app02pod0001 and does not test anything new, should be removed.

##########
File path: pkg/appmgmt/general/general_test.go
##########
@@ -526,3 +527,269 @@ func TestGetOwnerReferences(t *testing.T) {
 	assert.Equal(t, returnedOwnerRefs[0].Kind, "Pod", "Unexpected owner reference Kind")
 	assert.Equal(t, returnedOwnerRefs[0].APIVersion, v1.SchemeGroupVersion.String(), "Unexpected owner reference Kind")
 }
+
+// nolint: funlen
+func TestListApplication(t *testing.T) {
+	var app01, app02, app03, app04, app05, app06 = "app00001",
+		"app00002", "app00003", "app00004", "app00005", "app00006"
+	var queue01, queue02 = "root.queue01", "root.queue02"
+	var ns01, ns02 = "namespace01", "namespace02"
+
+	// mock the pod lister for this test
+	mockedAPIProvider := client.NewMockedAPIProvider()
+	mockedPodLister := test.NewPodListerMock()
+	mockedAPIProvider.SetPodLister(mockedPodLister)
+
+	// app01 pods, running in namespace01 and queue01
+	// all pods are having applicationID and queue name specified
+	// 2 pods have assigned nodes, 1 pod is pending for scheduling
+	// allocated pod
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00001",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "allocated-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00002",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "allocated-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00003",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})
+
+	// app02 pods, running in queue02 and namespace02
+	// 2 pods are having applicationID and queue name specified
+	// both 2 pods are pending
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app02pod0001",
+			Namespace: ns02,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app02,
+				constants.LabelQueueName:     queue02,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app02pod0002",
+			Namespace: ns02,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app02,
+				constants.LabelQueueName:     queue02,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})
+
+	// app03 pods, running in queue02 and namespace02
+	// 2 pods do not have label applicationID specified, but have spark-app-selector
+	// both 2 pods are allocated
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app03pod0001",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.SparkLabelAppID: app03,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "some-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app03pod0002",
+			Namespace: ns02,
+			Labels: map[string]string{
+				constants.SparkLabelAppID: app03,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "some-node",
+		},
+	})

Review comment:
       This is a duplicate from app03pod0001 and does not test anything new, should be removed.

##########
File path: pkg/appmgmt/general/general_test.go
##########
@@ -526,3 +527,269 @@ func TestGetOwnerReferences(t *testing.T) {
 	assert.Equal(t, returnedOwnerRefs[0].Kind, "Pod", "Unexpected owner reference Kind")
 	assert.Equal(t, returnedOwnerRefs[0].APIVersion, v1.SchemeGroupVersion.String(), "Unexpected owner reference Kind")
 }
+
+// nolint: funlen
+func TestListApplication(t *testing.T) {
+	var app01, app02, app03, app04, app05, app06 = "app00001",
+		"app00002", "app00003", "app00004", "app00005", "app00006"
+	var queue01, queue02 = "root.queue01", "root.queue02"
+	var ns01, ns02 = "namespace01", "namespace02"
+
+	// mock the pod lister for this test
+	mockedAPIProvider := client.NewMockedAPIProvider()
+	mockedPodLister := test.NewPodListerMock()
+	mockedAPIProvider.SetPodLister(mockedPodLister)
+
+	// app01 pods, running in namespace01 and queue01
+	// all pods are having applicationID and queue name specified
+	// 2 pods have assigned nodes, 1 pod is pending for scheduling
+	// allocated pod
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00001",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "allocated-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00002",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "allocated-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app01pod00003",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app01,
+				constants.LabelQueueName:     queue01,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})
+
+	// app02 pods, running in queue02 and namespace02
+	// 2 pods are having applicationID and queue name specified
+	// both 2 pods are pending
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app02pod0001",
+			Namespace: ns02,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app02,
+				constants.LabelQueueName:     queue02,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app02pod0002",
+			Namespace: ns02,
+			Labels: map[string]string{
+				constants.LabelApplicationID: app02,
+				constants.LabelQueueName:     queue02,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+		},
+	})
+
+	// app03 pods, running in queue02 and namespace02
+	// 2 pods do not have label applicationID specified, but have spark-app-selector
+	// both 2 pods are allocated
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app03pod0001",
+			Namespace: ns01,
+			Labels: map[string]string{
+				constants.SparkLabelAppID: app03,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "some-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app03pod0002",
+			Namespace: ns02,
+			Labels: map[string]string{
+				constants.SparkLabelAppID: app03,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "some-node",
+		},
+	})
+
+	// app04 pods, running in queue01 and namespace01
+	// app04 has 2 pods which only has annotation yunikorn.apache.org/app-id specified
+	// both 2 pods are allocated
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app04pod0001",
+			Namespace: ns01,
+			Annotations: map[string]string{
+				constants.AnnotationApplicationID: app04,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "some-node",
+		},
+	})
+
+	mockedPodLister.AddPod(&v1.Pod{
+		TypeMeta: apis.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: apis.ObjectMeta{
+			Name:      "app04pod0002",
+			Namespace: ns01,
+			Annotations: map[string]string{
+				constants.AnnotationApplicationID: app04,
+			},
+		},
+		Spec: v1.PodSpec{
+			SchedulerName: constants.SchedulerName,
+			NodeName:      "some-node",
+		},
+	})

Review comment:
       This is a duplicate from app04pod0001 and does not test anything new, should be removed.

##########
File path: pkg/appmgmt/general/general_test.go
##########
@@ -526,3 +527,269 @@ func TestGetOwnerReferences(t *testing.T) {
 	assert.Equal(t, returnedOwnerRefs[0].Kind, "Pod", "Unexpected owner reference Kind")
 	assert.Equal(t, returnedOwnerRefs[0].APIVersion, v1.SchemeGroupVersion.String(), "Unexpected owner reference Kind")
 }
+
+// nolint: funlen
+func TestListApplication(t *testing.T) {

Review comment:
       This should be a table driven test.
   We have 4 things that can change: Labels, Annotations, SchedulerName and NodeName. The outcome is known: app exists or not. Setting up a table with those 4 inputs and the expected output. Create a new pod lister or add a reset function on the mocked pod lister and run the checks in a loop. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org