Skip to content

[NPM] fix management of DeletedFinalStateUnknown object on deletion event in each controller #856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 23, 2021
Merged

[NPM] fix management of DeletedFinalStateUnknown object on deletion event in each controller #856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions npm/nameSpaceController.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
69 changes: 65 additions & 4 deletions npm/nameSpaceController_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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) {
Copy link
Member

@matmerr matmerr Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential function name ambiguity, can clarify between this helper deleteNamespace and f.nsController.deleteNamespace()

Copy link
Contributor Author

@JungukCho JungukCho Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think last time Vamsi also mentioned this. We can discuss and refactor this point when we add more UTs with ipset and iptables.

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
Expand Down Expand Up @@ -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},
Expand All @@ -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{
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions npm/networkPolicyController.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
}
Expand Down
57 changes: 54 additions & 3 deletions npm/networkPolicyController_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Copy link
Contributor

@vakalapa vakalapa Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there is no major difference in functionality between TestDeleteNetworkPolicyWithTombstone and TestDeleteNetworkPolicyWithTombstoneAfterAddingNetworkPolicy as AddingNS does not care about tombstone and in deleting the way we contruct tombstone is same. So we can reduce the complexity these test files by having only one tescase and not send delefinalstate in deleteNetPol(t *testing.T, ...) func bu

Copy link
Contributor Author

@JungukCho JungukCho Apr 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are different.
First test case only check "delete" event function in each controller.
Second test case is to try to mimic the realistic situation which walks through "delete" event -> workqueue -> reconcile. Thus, second test case first adds resources and then we emulate the realistic story (i.e., missing event and relist happens). Then, it proceeds all required steps like cleaning up the applied states from "delete" event.
If we want to keep one test case, I would like to keep second one which has more comprehensive test.

I am not sure this increase complexity. It has one extra boolean variable for only delete event in UT and later we can do more sophisticated tests. But, if there is better way to deal with this, let me know.

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) {
Expand Down
18 changes: 18 additions & 0 deletions npm/npm_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion npm/podController.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading