address review comments

Signed-off-by: Mladjan Gadzic <[email protected]>

Author: mladjan-gadzic

pkg/daemon/daemon.go

@@ -250,6 +250,8 @@ func getPodNetworkInfo(netName string, pod *kapi.Pod, netMap networksMap) (*podN
 func (d *daemon) allocatePodNetworkGUID(allocatedGUID, podNetworkID string, podUID types.UID, targetPkey string) error {
 	existingPkey, _ := d.guidPool.Get(allocatedGUID)
 	if existingPkey != "" {
+		// This happens when a GUID is being reallocated to a different PKey
+		// (e.g., pod was rescheduled or network configuration changed)
 		parsedPkey, err := utils.ParsePKey(existingPkey)
 		if err != nil {
 			log.Error().Msgf("failed to parse PKey %s with error: %v", existingPkey, err)

pkg/watcher/handler/pod.go

@@ -60,6 +60,11 @@ func (p *podEventHandler) OnAdd(obj interface{}, _ bool) {
 		return
 	}
 
+	if utils.PodIsRunning(pod) {
+		log.Debug().Msg("pod is already in running state")
+		return
+	}
+
 	if utils.PodIsFinished(pod) {
 		log.Debug().Msg("pod is already in finished state")
 		return
@@ -93,6 +98,12 @@ func (p *podEventHandler) OnUpdate(oldObj, newObj interface{}) {
 		return
 	}
 
+	if utils.PodIsRunning(pod) {
+		log.Debug().Msg("pod is already in running state")
+		p.retryPods.Delete(pod.UID)
+		return
+	}
+
 	if utils.PodIsFinished(pod) {
 		log.Debug().Msg("pod is finished")
 		p.OnDelete(newObj)

pkg/watcher/handler/pod_test.go

@@ -65,13 +65,17 @@ var _ = Describe("Pod Event Handler", func() {
 			pod1 := &kapi.Pod{Spec: kapi.PodSpec{HostNetwork: true}}
 			// Running pod
 			pod2 := &kapi.Pod{Status: kapi.PodStatus{Phase: kapi.PodRunning}}
+			// Finished pod (succeeded)
+			pod3 := &kapi.Pod{Status: kapi.PodStatus{Phase: kapi.PodSucceeded}}
+			// Finished pod (failed)
+			pod4 := &kapi.Pod{Status: kapi.PodStatus{Phase: kapi.PodFailed}}
 			// No network attachment annotation
-			pod3 := &kapi.Pod{}
+			pod5 := &kapi.Pod{}
 			// Not scheduled
-			pod4 := &kapi.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
+			pod6 := &kapi.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
 				v1.NetworkAttachmentAnnot: `[{"name":"test"}]`}}}
 			// Invalid network attachment annotation
-			pod5 := &kapi.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
+			pod7 := &kapi.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
 				v1.NetworkAttachmentAnnot: `[invalid]`}},
 				Spec: kapi.PodSpec{NodeName: "test"}}
 
@@ -81,6 +85,8 @@ var _ = Describe("Pod Event Handler", func() {
 			podEventHandler.OnAdd(pod3, true)
 			podEventHandler.OnAdd(pod4, true)
 			podEventHandler.OnAdd(pod5, true)
+			podEventHandler.OnAdd(pod6, true)
+			podEventHandler.OnAdd(pod7, true)
 
 			addMap, _ := podEventHandler.GetResults()
 			Expect(len(addMap.Items)).To(Equal(0))
@@ -129,6 +135,20 @@ var _ = Describe("Pod Event Handler", func() {
 			addMap, _ := podEventHandler.GetResults()
 			Expect(len(addMap.Items)).To(Equal(0))
 		})
+		It("On update finished pod should trigger delete", func() {
+			// Finished pod that should trigger OnDelete
+			pod := &kapi.Pod{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
+				v1.NetworkAttachmentAnnot: `[{"name":"test", "namespace":"default", "cni-args":{"guid":"02:00:00:00:02:00:00:00", "mellanox.infiniband.app":"configured"}}]`}},
+				Status: kapi.PodStatus{Phase: kapi.PodSucceeded}}
+
+			podEventHandler := NewPodEventHandler()
+			podEventHandler.OnUpdate(nil, pod)
+
+			// Should be added to delete map
+			_, delMap := podEventHandler.GetResults()
+			Expect(len(delMap.Items)).To(Equal(1))
+			Expect(len(delMap.Items["default_test"].([]*kapi.Pod))).To(Equal(1))
+		})
 	})
 	Context("OnDelete", func() {
 		It("On delete pod event", func() {