Merge pull request #2166 from sthaha/refactor-pod-ctnr-lookup

vimalk78 · web-flow · commit bec7b86934a5 · 2025-06-18T12:13:51.000+05:30
refactor(pod): simplify Lookup returning Container Name
diff --git a/internal/k8s/pod/pod.go b/internal/k8s/pod/pod.go
@@ -5,7 +5,6 @@ package pod
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"log/slog"
 	"strings"
@@ -25,10 +24,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 )
 
-var (
-	ErrNoPod = errors.New("no pod found for container")
-)
-
 const (
 	indexContainerID = "containerID"
 )
@@ -37,30 +32,29 @@ type (
 	Informer interface {
 		service.Initializer
 		service.Runner
-		LookupByContainerID(containerID string) (*PodInfo, string, error)
+		LookupByContainerID(containerID string) (*ContainerInfo, bool, error)
 	}
 
-	PodInfo struct {
-		ID        string
-		Name      string
-		Namespace string
+	ContainerInfo struct {
+		PodID         string
+		PodName       string
+		Namespace     string
+		ContainerName string
 	}
-)
 
-type podInformer struct {
-	logger *slog.Logger
+	podInformer struct {
+		logger *slog.Logger
 
-	kubeConfigPath string
-	nodeName       string
+		kubeConfigPath string
+		nodeName       string
 
-	cfg     *rest.Config
-	manager manager.Manager
+		cfg     *rest.Config
+		manager manager.Manager
 
-	createRestConfigFunc func(kubeConfigPath string) (*rest.Config, error)
-	newManagerFunc       func(config *rest.Config, options ctrl.Options) (ctrl.Manager, error)
-}
+		createRestConfigFunc func(kubeConfigPath string) (*rest.Config, error)
+		newManagerFunc       func(config *rest.Config, options ctrl.Options) (ctrl.Manager, error)
+	}
 
-type (
 	Option struct {
 		logger         *slog.Logger
 		kubeConfigPath string
@@ -140,7 +134,6 @@ func (pi *podInformer) Init() error {
 }
 
 func (pi *podInformer) setupManager(scheme *k8sruntime.Scheme) (ctrl.Manager, error) {
-
 	cacheOp := cache.Options{}
 	cacheOp.ByObject = map[client.Object]cache.ByObject{
 		&corev1.Pod{}: {
@@ -213,7 +206,7 @@ func (pi *podInformer) Run(ctx context.Context) error {
 }
 
 // LookupByContainerID retrieves pod details and container name given a containerID
-func (pi *podInformer) LookupByContainerID(containerID string) (*PodInfo, string, error) {
+func (pi *podInformer) LookupByContainerID(containerID string) (*ContainerInfo, bool, error) {
 	var pods corev1.PodList
 
 	err := pi.manager.GetCache().List(
@@ -222,25 +215,27 @@ func (pi *podInformer) LookupByContainerID(containerID string) (*PodInfo, string
 		client.MatchingFields{indexContainerID: containerID},
 	)
 	if err != nil {
-		return nil, "", fmt.Errorf("error retrieving pod info from cache: %w", err)
+		return nil, false, fmt.Errorf("error retrieving pod info from cache: %w", err)
 	}
 
-	if len(pods.Items) == 0 {
-		return nil, "", ErrNoPod
+	switch count := len(pods.Items); {
+	case count == 0:
+		return nil, false, nil
+	case count > 1:
+		return nil, false, fmt.Errorf("multiple pods found for containerID: %s", containerID)
+
+	default: // case x == 1:
+		pod := pods.Items[0]
+		containerName := pi.findContainerName(&pod, containerID)
+		pi.logger.Debug("pod found for container", "container", containerID, "pod", pod.Name, "containerName", containerName)
+
+		return &ContainerInfo{
+			PodID:         string(pod.UID),
+			PodName:       pod.Name,
+			Namespace:     pod.Namespace,
+			ContainerName: containerName,
+		}, true, nil
 	}
-
-	if len(pods.Items) > 1 {
-		return nil, "", fmt.Errorf("multiple pods found for containerID: %s", containerID)
-	}
-
-	pod := pods.Items[0]
-	containerName := pi.findContainerName(&pod, containerID)
-	pi.logger.Debug("pod found for container", "container", containerID, "pod", pod.Name, "containerName", containerName)
-	return &PodInfo{
-		ID:        string(pod.UID),
-		Name:      pod.Name,
-		Namespace: pod.Namespace,
-	}, containerName, nil
 }
 
 func getConfig(kubeConfigPath string) (*rest.Config, error) {
diff --git a/internal/k8s/pod/pod_test.go b/internal/k8s/pod/pod_test.go
@@ -154,8 +154,10 @@ func TestPodInfo(t *testing.T) {
 			mock.Anything,
 			mock.Anything,
 		).Return(nil)
-		_, _, err := pi.LookupByContainerID("container1")
-		assert.ErrorIs(t, err, ErrNoPod, "unexpected error returned")
+		containerInfo, found, err := pi.LookupByContainerID("container1")
+		assert.NoError(t, err)
+		assert.False(t, found, "expected container not to be found")
+		assert.Nil(t, containerInfo, "expected nil container info")
 	})
 	t.Run("exactly one pod found", func(t *testing.T) {
 		pi := NewInformer()
@@ -179,12 +181,14 @@ func TestPodInfo(t *testing.T) {
 			pods := args.Get(1).(*corev1.PodList)
 			pods.Items = []corev1.Pod{pod1}
 		})
-		retPod, containerName, err := pi.LookupByContainerID("container1")
+		containerInfo, found, err := pi.LookupByContainerID("container1")
 		assert.NoError(t, err)
-		assert.Equal(t, string(pod1.UID), retPod.ID, "unexpected pod id")
-		assert.Equal(t, pod1.Name, retPod.Name, "unexpected pod name")
-		assert.Equal(t, pod1.Namespace, retPod.Namespace, "unexpected pod namespace")
-		assert.Equal(t, "", containerName, "expected empty container name")
+		assert.True(t, found, "expected container to be found")
+		assert.NotNil(t, containerInfo, "expected non-nil container info")
+		assert.Equal(t, string(pod1.UID), containerInfo.PodID, "unexpected pod id")
+		assert.Equal(t, pod1.Name, containerInfo.PodName, "unexpected pod name")
+		assert.Equal(t, pod1.Namespace, containerInfo.Namespace, "unexpected pod namespace")
+		assert.Equal(t, "", containerInfo.ContainerName, "expected empty container name")
 	})
 	t.Run("more than one pod found", func(t *testing.T) {
 		pi := NewInformer()
@@ -208,7 +212,8 @@ func TestPodInfo(t *testing.T) {
 			pods := args.Get(1).(*corev1.PodList)
 			pods.Items = []corev1.Pod{pod1, pod1}
 		})
-		_, _, err := pi.LookupByContainerID("container1")
+		_, found, err := pi.LookupByContainerID("container1")
+		assert.False(t, found, "expected container not to be found due to multiple pods")
 		assert.ErrorContains(t, err, "multiple pods found for containerID")
 	})
 	t.Run("cache error", func(t *testing.T) {
@@ -223,7 +228,8 @@ func TestPodInfo(t *testing.T) {
 			mock.Anything,
 			mock.Anything,
 		).Return(fmt.Errorf("!!you shall not pass!!"))
-		_, _, err := pi.LookupByContainerID("container1")
+		_, found, err := pi.LookupByContainerID("container1")
+		assert.False(t, found, "expected container not to be found due to cache error")
 		assert.ErrorContains(t, err, "error retrieving pod info from cache")
 	})
 }
@@ -280,14 +286,14 @@ func TestPodInformer_RunIntegration(t *testing.T) {
 
 		time.Sleep(50 * time.Millisecond)
 
-		podInfo, containerName, err := pi.LookupByContainerID("abc123")
+		containerInfo, found, err := pi.LookupByContainerID("abc123")
 		if err != nil {
 			t.Logf("LookupByContainerID lookup failed (expected in fake setup): %v", err)
-		} else {
-			assert.Equal(t, "test-pod", podInfo.Name)
-			assert.Equal(t, "default", podInfo.Namespace)
-			assert.Equal(t, "test-uid-123", podInfo.ID)
-			assert.Equal(t, "test-container", containerName)
+		} else if found {
+			assert.Equal(t, "test-pod", containerInfo.PodName)
+			assert.Equal(t, "default", containerInfo.Namespace)
+			assert.Equal(t, "test-uid-123", containerInfo.PodID)
+			assert.Equal(t, "test-container", containerInfo.ContainerName)
 		}
 
 		cancel()
diff --git a/internal/resource/informer.go b/internal/resource/informer.go
@@ -213,23 +213,26 @@ func (ri *resourceInformer) Refresh() error {
 	containersNoPod := []string{}
 	if ri.podInformer != nil {
 		for _, container := range containersRunning {
-			podInfo, containerName, err := ri.podInformer.LookupByContainerID(container.ID)
+			cntrInfo, found, err := ri.podInformer.LookupByContainerID(container.ID)
 			if err != nil {
-				if errors.Is(err, pod.ErrNoPod) {
-					containersNoPod = append(containersNoPod, container.ID)
-				} else {
-					ri.logger.Debug("Failed to get pod for container", "container", container.ID, "error", err)
-					refreshErrs = errors.Join(refreshErrs, fmt.Errorf("failed to get pod for container: %w", err))
-				}
+				ri.logger.Debug("Failed to get pod for container", "container", container.ID, "error", err)
+				refreshErrs = errors.Join(refreshErrs, fmt.Errorf("failed to get pod for container: %w", err))
 				continue
 			}
+
+			if !found {
+				containersNoPod = append(containersNoPod, container.ID)
+				continue
+			}
+
 			pod := &Pod{
-				ID:        podInfo.ID,
-				Name:      podInfo.Name,
-				Namespace: podInfo.Namespace,
+				ID:        cntrInfo.PodID,
+				Name:      cntrInfo.PodName,
+				Namespace: cntrInfo.Namespace,
 			}
 			container.Pod = pod
-			container.Name = containerName
+			container.Name = cntrInfo.ContainerName
+
 			_, seen := podsRunning[pod.ID]
 			// reset CPU Time of the pod if it is getting added to the running list for the first time
 			// in the subsequent iteration, the CPUTimeDelta should be incremented by container's CPUTimeDelta
diff --git a/internal/resource/mock_procfs_test.go b/internal/resource/mock_procfs_test.go
@@ -102,12 +102,12 @@ func (m *mockPodInformer) Run(ctx context.Context) error {
 	return args.Error(0)
 }
 
-func (m *mockPodInformer) LookupByContainerID(containerID string) (*pod.PodInfo, string, error) {
+func (m *mockPodInformer) LookupByContainerID(containerID string) (*pod.ContainerInfo, bool, error) {
 	args := m.Called(containerID)
-	if podInfo, ok := args.Get(0).(*pod.PodInfo); ok {
-		return podInfo, args.String(1), args.Error(2)
+	if podInfo, ok := args.Get(0).(*pod.ContainerInfo); ok {
+		return podInfo, args.Bool(1), args.Error(2)
 	}
-	return nil, args.String(1), args.Error(2)
+	return nil, args.Bool(1), args.Error(2)
 }
 
 func (m *mockPodInformer) Name() string {
diff --git a/internal/resource/procfs_reader_test.go b/internal/resource/procfs_reader_test.go
@@ -477,11 +477,12 @@ func TestRefresh_PodInformer(t *testing.T) {
 
 		mockPodInformer := new(mockPodInformer)
 		mockPodInformer.On("LookupByContainerID", containerID).Return(
-			&pod.PodInfo{
-				ID:        "pod123",
-				Name:      "mypod",
-				Namespace: "default",
-			}, "my-container", nil,
+			&pod.ContainerInfo{
+				PodID:         "pod123",
+				PodName:       "mypod",
+				Namespace:     "default",
+				ContainerName: "my-container",
+			}, true, nil,
 		)
 
 		informer, err := NewInformer(WithProcReader(mockProcFS), WithPodInformer(mockPodInformer))
@@ -516,7 +517,7 @@ func TestRefresh_PodInformer(t *testing.T) {
 		mockProcFS.On("CPUUsageRatio").Return(0.5, nil).Once()
 
 		mockPodInformer := new(mockPodInformer)
-		mockPodInformer.On("LookupByContainerID", containerID).Return(nil, "", pod.ErrNoPod)
+		mockPodInformer.On("LookupByContainerID", containerID).Return(nil, false, nil)
 
 		informer, err := NewInformer(
 			WithProcReader(mockProcFS),
@@ -556,7 +557,7 @@ func TestRefresh_PodInformer(t *testing.T) {
 
 		podError := errors.New("general error")
 		mockPodInformer := new(mockPodInformer)
-		mockPodInformer.On("LookupByContainerID", containerID).Return(nil, "", podError)
+		mockPodInformer.On("LookupByContainerID", containerID).Return(nil, false, podError)
 
 		informer, err := NewInformer(
 			WithProcReader(mockProcFS),
@@ -602,11 +603,12 @@ func TestLookupByContainerID_UpdatesContainerName(t *testing.T) {
 		// Mock pod informer that returns container name from pod info
 		mockPodInformer := new(mockPodInformer)
 		mockPodInformer.On("LookupByContainerID", containerID).Return(
-			&pod.PodInfo{
-				ID:        "pod-12345",
-				Name:      "test-app-pod",
-				Namespace: "production",
-			}, "app-container-from-pod", nil, // Container name comes from pod status
+			&pod.ContainerInfo{
+				PodID:         "pod-12345",
+				PodName:       "test-app-pod",
+				Namespace:     "production",
+				ContainerName: "app-container-from-pod", // Container name comes from pod status
+			}, true, nil,
 		)
 
 		informer, err := NewInformer(
@@ -671,11 +673,12 @@ func TestLookupByContainerID_UpdatesContainerName(t *testing.T) {
 		// Pod informer returns different name than environment
 		mockPodInformer := new(mockPodInformer)
 		mockPodInformer.On("LookupByContainerID", containerID).Return(
-			&pod.PodInfo{
-				ID:        "web-pod-67890",
-				Name:      "web-server",
-				Namespace: "default",
-			}, "nginx-from-pod", nil, // Different from environment name
+			&pod.ContainerInfo{
+				PodID:         "web-pod-67890",
+				PodName:       "web-server",
+				Namespace:     "default",
+				ContainerName: "nginx-from-pod", // Different from environment name
+			}, true, nil,
 		)
 
 		informer, err := NewInformer(

Original file line number	Diff line number	Diff line change
`@@ -102,12 +102,12 @@ func (m *mockPodInformer) Run(ctx context.Context) error {`
`102`	`102`	`return args.Error(0)`
`103`	`103`	`}`
`104`	`104`
`105`		`-func (m mockPodInformer) LookupByContainerID(containerID string) (pod.PodInfo, string, error) {`
	`105`	`+func (m mockPodInformer) LookupByContainerID(containerID string) (pod.ContainerInfo, bool, error) {`
`106`	`106`	`args := m.Called(containerID)`
`107`		`- if podInfo, ok := args.Get(0).(*pod.PodInfo); ok {`
`108`		`- return podInfo, args.String(1), args.Error(2)`
	`107`	`+ if podInfo, ok := args.Get(0).(*pod.ContainerInfo); ok {`
	`108`	`+ return podInfo, args.Bool(1), args.Error(2)`
`109`	`109`	`}`
`110`		`- return nil, args.String(1), args.Error(2)`
	`110`	`+ return nil, args.Bool(1), args.Error(2)`
`111`	`111`	`}`
`112`	`112`
`113`	`113`	`func (m *mockPodInformer) Name() string {`