fix management of deletefinalstateUnknown object on deletion event in each controller. Add Unit test for them. Correct log messages. (#856)

Author: JungukCho

npm/nameSpaceController.go

@@ -192,7 +192,7 @@ func (nsc *nameSpaceController) deleteNamespace(obj interface{}) {
 
 	var err error
 	var key string
-	if key, err = cache.MetaNamespaceKeyFunc(obj); err != nil {
+	if key, err = cache.MetaNamespaceKeyFunc(nsObj); err != nil {
 		utilruntime.HandleError(err)
 		metrics.SendErrorLogAndMetric(util.NSID, "[NAMESPACE DELETE EVENT] Error: nameSpaceKey is empty for %s namespace", util.GetNSNameWithPrefix(nsObj.Name))
 		return
@@ -287,7 +287,7 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error {
 	defer nsc.npMgr.Unlock()
 	if err != nil {
 		if errors.IsNotFound(err) {
-			utilruntime.HandleError(fmt.Errorf("NameSpace '%s' in work queue no longer exists", key))
+			klog.Infof("NameSpace %s not found, may be it is deleted", key)
 			// find the nsMap key and start cleaning up process (calling cleanDeletedNamespace function)
 			cachedNsKey := util.GetNSNameWithPrefix(key)
 			// cleanDeletedNamespace will check if the NS exists in cache, if it does, then proceeds with deletion

npm/nameSpaceController_test.go

@@ -16,6 +16,7 @@ import (
 	kubeinformers "k8s.io/client-go/informers"
 	k8sfake "k8s.io/client-go/kubernetes/fake"
 	core "k8s.io/client-go/testing"
+	"k8s.io/client-go/tools/cache"
 )
 
 var (
@@ -124,15 +125,24 @@ func updateNamespace(t *testing.T, f *nameSpaceFixture, oldNsObj, newNsObj *core
 	f.nsController.processNextWorkItem()
 }
 
-func deleteNamespace(t *testing.T, f *nameSpaceFixture, nsObj *corev1.Namespace) {
+func deleteNamespace(t *testing.T, f *nameSpaceFixture, nsObj *corev1.Namespace, isDeletedFinalStateUnknownObject IsDeletedFinalStateUnknownObject) {
 	addNamespace(t, f, nsObj)
 	t.Logf("Complete add namespace event")
 
 	t.Logf("Updating kubeinformer namespace object")
 	f.kubeInformer.Core().V1().Namespaces().Informer().GetIndexer().Delete(nsObj)
 
 	t.Logf("Calling delete namespace event")
-	f.nsController.deleteNamespace(nsObj)
+	if isDeletedFinalStateUnknownObject {
+		tombstone := cache.DeletedFinalStateUnknown{
+			Key: nsObj.Name,
+			Obj: nsObj,
+		}
+		f.nsController.deleteNamespace(tombstone)
+	} else {
+		f.nsController.deleteNamespace(nsObj)
+	}
+
 	if f.nsController.workqueue.Len() == 0 {
 		t.Logf("Delete Namespace: worker queue length is 0 ")
 		return
@@ -432,7 +442,7 @@ func TestDeleteNamespace(t *testing.T) {
 	stopCh := make(chan struct{})
 	defer close(stopCh)
 	f.newNsController(stopCh)
-	deleteNamespace(t, f, nsObj)
+	deleteNamespace(t, f, nsObj, DeletedFinalStateknownObject)
 
 	testCases := []expectedNsValues{
 		{0, 1, 0},
@@ -444,6 +454,57 @@ func TestDeleteNamespace(t *testing.T) {
 	}
 }
 
+func TestDeleteNamespaceWithTombstone(t *testing.T) {
+	f := newNsFixture(t)
+	f.ipSetSave(util.IpsetTestConfigFile)
+	defer f.ipSetRestore(util.IpsetTestConfigFile)
+	stopCh := make(chan struct{})
+	defer close(stopCh)
+	f.newNsController(stopCh)
+
+	nsObj := newNameSpace(
+		"test-namespace",
+		"0",
+		map[string]string{
+			"app": "test-namespace",
+		},
+	)
+	tombstone := cache.DeletedFinalStateUnknown{
+		Key: nsObj.Name,
+		Obj: nsObj,
+	}
+
+	f.nsController.deleteNamespace(tombstone)
+
+	testCases := []expectedNsValues{
+		{0, 1, 0},
+	}
+	checkNsTestResult("TestDeleteNamespaceWithTombstone", f, testCases)
+}
+
+func TestDeleteNamespaceWithTombstoneAfterAddingNameSpace(t *testing.T) {
+	nsObj := newNameSpace(
+		"test-namespace",
+		"0",
+		map[string]string{
+			"app": "test-namespace",
+		},
+	)
+
+	f := newNsFixture(t)
+	f.nsLister = append(f.nsLister, nsObj)
+	f.kubeobjects = append(f.kubeobjects, nsObj)
+	stopCh := make(chan struct{})
+	defer close(stopCh)
+	f.newNsController(stopCh)
+
+	deleteNamespace(t, f, nsObj, DeletedFinalStateUnknownObject)
+	testCases := []expectedNsValues{
+		{0, 1, 0},
+	}
+	checkNsTestResult("TestDeleteNamespaceWithTombstoneAfterAddingNameSpace", f, testCases)
+}
+
 func TestGetNamespaceObjFromNsObj(t *testing.T) {
 	ns, _ := newNs("test-ns")
 	ns.LabelsMap = map[string]string{
@@ -471,7 +532,7 @@ func checkNsTestResult(testName string, f *nameSpaceFixture, testCases []expecte
 			f.t.Errorf("PodMap length = %d, want %d. Map: %+v", got, test.expectedLenOfPodMap, f.npMgr.PodMap)
 		}
 		if got := len(f.npMgr.NsMap); got != test.expectedLenOfNsMap {
-			f.t.Errorf("npMgr length = %d, want %d. Map: %+v", got, test.expectedLenOfNsMap, f.npMgr.NsMap)
+			f.t.Errorf("NsMap length = %d, want %d. Map: %+v", got, test.expectedLenOfNsMap, f.npMgr.NsMap)
 		}
 		if got := f.nsController.workqueue.Len(); got != test.expectedLenOfWorkQueue {
 			f.t.Errorf("Workqueue length = %d, want %d", got, test.expectedLenOfWorkQueue)

npm/networkPolicyController.go

@@ -122,7 +122,7 @@ func (c *networkPolicyController) updateNetworkPolicy(old, new interface{}) {
 }
 
 func (c *networkPolicyController) deleteNetworkPolicy(obj interface{}) {
-	_, ok := obj.(*networkingv1.NetworkPolicy)
+	netPolObj, ok := obj.(*networkingv1.NetworkPolicy)
 	// DeleteFunc gets the final state of the resource (if it is known).
 	// Otherwise, it gets an object of type DeletedFinalStateUnknown.
 	// This can happen if the watch is closed and misses the delete event and
@@ -135,7 +135,7 @@ func (c *networkPolicyController) deleteNetworkPolicy(obj interface{}) {
 			return
 		}
 
-		if _, ok = tombstone.Obj.(*networkingv1.NetworkPolicy); !ok {
+		if netPolObj, ok = tombstone.Obj.(*networkingv1.NetworkPolicy); !ok {
 			metrics.SendErrorLogAndMetric(util.NetpolID, "[NETPOL DELETE EVENT] Received unexpected object type (error decoding object tombstone, invalid type): %v", obj)
 			utilruntime.HandleError(fmt.Errorf("error decoding object tombstone, invalid type"))
 			return
@@ -144,7 +144,7 @@ func (c *networkPolicyController) deleteNetworkPolicy(obj interface{}) {
 
 	var netPolkey string
 	var err error
-	if netPolkey, err = cache.MetaNamespaceKeyFunc(obj); err != nil {
+	if netPolkey, err = cache.MetaNamespaceKeyFunc(netPolObj); err != nil {
 		utilruntime.HandleError(err)
 		return
 	}

npm/networkPolicyController_test.go

@@ -20,6 +20,7 @@ import (
 	kubeinformers "k8s.io/client-go/informers"
 	k8sfake "k8s.io/client-go/kubernetes/fake"
 	core "k8s.io/client-go/testing"
+	"k8s.io/client-go/tools/cache"
 )
 
 type netPolFixture struct {
@@ -165,13 +166,22 @@ func addNetPol(t *testing.T, f *netPolFixture, netPolObj *networkingv1.NetworkPo
 	f.netPolController.processNextWorkItem()
 }
 
-func deleteNetPol(t *testing.T, f *netPolFixture, netPolObj *networkingv1.NetworkPolicy) {
+func deleteNetPol(t *testing.T, f *netPolFixture, netPolObj *networkingv1.NetworkPolicy, isDeletedFinalStateUnknownObject IsDeletedFinalStateUnknownObject) {
 	addNetPol(t, f, netPolObj)
 	t.Logf("Complete adding network policy event")
 
 	// simulate network policy deletion event and delete network policy object from sharedInformer cache
 	f.kubeInformer.Networking().V1().NetworkPolicies().Informer().GetIndexer().Delete(netPolObj)
-	f.netPolController.deleteNetworkPolicy(netPolObj)
+	if isDeletedFinalStateUnknownObject {
+		netPolKey := getKey(netPolObj, t)
+		tombstone := cache.DeletedFinalStateUnknown{
+			Key: netPolKey,
+			Obj: netPolObj,
+		}
+		f.netPolController.deleteNetworkPolicy(tombstone)
+	} else {
+		f.netPolController.deleteNetworkPolicy(netPolObj)
+	}
 
 	if f.netPolController.workqueue.Len() == 0 {
 		f.isEnqueueEventIntoWorkQueue = false
@@ -305,13 +315,54 @@ func TestDeleteNetworkPolicy(t *testing.T) {
 	defer close(stopCh)
 	f.newNetPolController(stopCh)
 
-	deleteNetPol(t, f, netPolObj)
+	deleteNetPol(t, f, netPolObj, DeletedFinalStateknownObject)
 	testCases := []expectedNetPolValues{
 		{1, 0, 0, false, true, 0, nil, 1, nil},
 	}
 	checkNetPolTestResult("TestDelNetPol", f, testCases)
 }
 
+func TestDeleteNetworkPolicyWithTombstone(t *testing.T) {
+	netPolObj := createNetPol()
+
+	f := newNetPolFixture(t)
+	f.isEnqueueEventIntoWorkQueue = false
+	f.netPolLister = append(f.netPolLister, netPolObj)
+	f.kubeobjects = append(f.kubeobjects, netPolObj)
+	stopCh := make(chan struct{})
+	defer close(stopCh)
+	f.newNetPolController(stopCh)
+
+	netPolKey := getKey(netPolObj, t)
+	tombstone := cache.DeletedFinalStateUnknown{
+		Key: netPolKey,
+		Obj: netPolObj,
+	}
+
+	f.netPolController.deleteNetworkPolicy(tombstone)
+	testCases := []expectedNetPolValues{
+		{1, 0, 0, false, false, 0, nil, 0, nil},
+	}
+	checkNetPolTestResult("TestDeleteNetworkPolicyWithTombstone", f, testCases)
+}
+
+func TestDeleteNetworkPolicyWithTombstoneAfterAddingNetworkPolicy(t *testing.T) {
+	netPolObj := createNetPol()
+
+	f := newNetPolFixture(t)
+	f.netPolLister = append(f.netPolLister, netPolObj)
+	f.kubeobjects = append(f.kubeobjects, netPolObj)
+	stopCh := make(chan struct{})
+	defer close(stopCh)
+	f.newNetPolController(stopCh)
+
+	deleteNetPol(t, f, netPolObj, DeletedFinalStateUnknownObject)
+	testCases := []expectedNetPolValues{
+		{1, 0, 0, false, true, 0, nil, 1, nil},
+	}
+	checkNetPolTestResult("TestDeleteNetworkPolicyWithTombstoneAfterAddingNetworkPolicy", f, testCases)
+}
+
 // this unit test is for the case where states of network policy are changed, but network policy controller does not need to reconcile.
 // Check it with expectedEnqueueEventIntoWorkQueue variable.
 func TestUpdateNetworkPolicy(t *testing.T) {

npm/npm_test.go

@@ -8,8 +8,26 @@ import (
 	"github.com/Azure/azure-container-networking/npm/iptm"
 	"github.com/Azure/azure-container-networking/npm/metrics"
 	"github.com/Azure/azure-container-networking/npm/util"
+	"k8s.io/client-go/tools/cache"
 )
 
+// To indicate the object is needed to be DeletedFinalStateUnknown Object
+type IsDeletedFinalStateUnknownObject bool
+
+const (
+	DeletedFinalStateUnknownObject IsDeletedFinalStateUnknownObject = true
+	DeletedFinalStateknownObject   IsDeletedFinalStateUnknownObject = false
+)
+
+func getKey(obj interface{}, t *testing.T) string {
+	key, err := cache.DeletionHandlingMetaNamespaceKeyFunc(obj)
+	if err != nil {
+		t.Errorf("Unexpected error getting key for obj %v: %v", obj, err)
+		return ""
+	}
+	return key
+}
+
 func newNPMgr(t *testing.T) *NetworkPolicyManager {
 	npMgr := &NetworkPolicyManager{
 		NsMap:            make(map[string]*Namespace),

npm/podController.go

@@ -245,7 +245,7 @@ func (c *podController) deletePod(obj interface{}) {
 
 	var err error
 	var key string
-	if key, err = cache.MetaNamespaceKeyFunc(obj); err != nil {
+	if key, err = cache.MetaNamespaceKeyFunc(podObj); err != nil {
 		utilruntime.HandleError(err)
 		metrics.SendErrorLogAndMetric(util.PodID, "[POD DELETE EVENT] Error: podKey is empty for %s pod in %s with UID %s",
 			podObj.ObjectMeta.Name, util.GetNSNameWithPrefix(podObj.Namespace), podObj.UID)