Fixed calculation of reconcile event handling for PipePods (#1322)

* Fixed calculation of event handling for PipePods
* Added basic unit test for eventFilter delete methods

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

pkg/controller/instance/instance_controller.go

@@ -95,21 +95,6 @@ func (r *Reconciler) SetupWithManager(
 			return requests
 		})
 
-	// resPredicate ignores DeleteEvents for pipe-pods only (marked with task.PipePodAnnotation). This is due to an
-	// inherent race that was described in detail in #1116 (https://github.com/kudobuilder/kudo/issues/1116)
-	// tl;dr: pipe-task will delete the pipe pod at the end of the execution. this would normally trigger another
-	// Instance reconciliation which might end up copying pipe files twice. we avoid this by explicitly ignoring
-	// DeleteEvents for pipe-pods.
-	resPredicate := predicate.Funcs{
-		CreateFunc: func(event.CreateEvent) bool { return true },
-		DeleteFunc: func(e event.DeleteEvent) bool {
-			return e.Meta.GetAnnotations() != nil &&
-				funk.Contains(e.Meta.GetAnnotations(), task.PipePodAnnotation)
-		},
-		UpdateFunc:  func(event.UpdateEvent) bool { return true },
-		GenericFunc: func(event.GenericEvent) bool { return true },
-	}
-
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&kudov1beta1.Instance{}).
 		Owns(&kudov1beta1.Instance{}).
@@ -118,11 +103,31 @@ func (r *Reconciler) SetupWithManager(
 		Owns(&batchv1.Job{}).
 		Owns(&appsv1.StatefulSet{}).
 		Owns(&corev1.Pod{}).
-		WithEventFilter(resPredicate).
+		WithEventFilter(eventFilter()).
 		Watches(&source.Kind{Type: &kudov1beta1.OperatorVersion{}}, &handler.EnqueueRequestsFromMapFunc{ToRequests: addOvRelatedInstancesToReconcile}).
 		Complete(r)
 }
 
+// eventFilter ignores DeleteEvents for pipe-pods only (marked with task.PipePodAnnotation). This is due to an
+// inherent race that was described in detail in #1116 (https://github.com/kudobuilder/kudo/issues/1116)
+// tl;dr: pipe-task will delete the pipe pod at the end of the execution. this would normally trigger another
+// Instance reconciliation which might end up copying pipe files twice. we avoid this by explicitly ignoring
+// DeleteEvents for pipe-pods.
+func eventFilter() predicate.Funcs {
+	return predicate.Funcs{
+		CreateFunc: func(event.CreateEvent) bool { return true },
+		DeleteFunc: func(e event.DeleteEvent) bool {
+			return !isForPipePod(e)
+		},
+		UpdateFunc:  func(event.UpdateEvent) bool { return true },
+		GenericFunc: func(event.GenericEvent) bool { return true },
+	}
+}
+
+func isForPipePod(e event.DeleteEvent) bool {
+	return e.Meta.GetAnnotations() != nil && funk.Contains(e.Meta.GetAnnotations(), task.PipePodAnnotation)
+}
+
 // Reconcile is the main controller method that gets called every time something about the instance changes
 //
 //   +-------------------------------+

pkg/controller/instance/instance_controller_test.go

@@ -12,6 +12,9 @@ import (
 	"testing"
 	"time"
 
+	"github.com/kudobuilder/kudo/pkg/engine/task"
+	"sigs.k8s.io/controller-runtime/pkg/event"
+
 	"github.com/stretchr/testify/assert"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -348,3 +351,32 @@ func TestSpecParameterDifference(t *testing.T) {
 		assert.Equal(t, test.diff, diff)
 	}
 }
+
+func TestEventFilterForDelete(t *testing.T) {
+	var testParams = []struct {
+		name    string
+		allowed bool
+		e       event.DeleteEvent
+	}{
+		{"A Pod without annotations", true, event.DeleteEvent{
+			Meta:               &v1.Pod{},
+			Object:             nil,
+			DeleteStateUnknown: false,
+		}},
+		{"A Pod with pipePod annotation", false, event.DeleteEvent{
+			Meta: &v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{task.PipePodAnnotation: "true"},
+				},
+			},
+			Object:             nil,
+			DeleteStateUnknown: false,
+		}},
+	}
+
+	filter := eventFilter()
+	for _, test := range testParams {
+		diff := filter.Delete(test.e)
+		assert.Equal(t, test.allowed, diff, test.name)
+	}
+}