From fd948ef32e68c42fe372a7aea44befc5443aa991 Mon Sep 17 00:00:00 2001 From: Junguk Cho Date: Tue, 4 May 2021 12:06:19 -0700 Subject: [PATCH 1/3] fix incorrect NsMap local cache management between nameSpaceController and podcontroller --- npm/ipsm/ipsm.go | 3 +++ npm/nameSpaceController.go | 1 + npm/podController.go | 30 +++++++++--------------------- 3 files changed, 13 insertions(+), 21 deletions(-) diff --git a/npm/ipsm/ipsm.go b/npm/ipsm/ipsm.go index da7412fee5..f2aeba6032 100644 --- a/npm/ipsm/ipsm.go +++ b/npm/ipsm/ipsm.go @@ -256,6 +256,9 @@ func (ipsMgr *IpsetManager) CreateSet(setName string, spec []string) error { spec: spec, } log.Logf("Creating Set: %+v", entry) + // (TODO): need to differentiate errCode handler + // since errCode can be one in case of "set with the same name already exists" and "maximal number of sets reached, cannot create more." + // It may have more situations with errCode==1. if errCode, err := ipsMgr.Run(entry); err != nil && errCode != 1 { metrics.SendErrorLogAndMetric(util.IpsmID, "Error: failed to create ipset.") return err diff --git a/npm/nameSpaceController.go b/npm/nameSpaceController.go index dfb3c10848..2e1eeafe02 100644 --- a/npm/nameSpaceController.go +++ b/npm/nameSpaceController.go @@ -41,6 +41,7 @@ type Namespace struct { } // newNS constructs a new namespace object. +// (TODO): need to change newNS function. It always returns "nil" func newNs(name string) (*Namespace, error) { ns := &Namespace{ name: name, diff --git a/npm/podController.go b/npm/podController.go index f931c0ac8f..cf935c2b60 100644 --- a/npm/podController.go +++ b/npm/podController.go @@ -384,18 +384,9 @@ func (c *podController) syncAddedPod(podObj *corev1.Pod) error { ipsMgr := c.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr klog.Infof("POD CREATING: [%s%s/%s/%s%+v%s]", string(podObj.GetUID()), podNs, podObj.Name, podObj.Spec.NodeName, podObj.Labels, podObj.Status.PodIP) - // Add pod namespace if it doesn't exist var err error - if _, exists := c.npMgr.NsMap[podNs]; !exists { - // (TODO): need to change newNS function. It always returns "nil" - c.npMgr.NsMap[podNs], _ = newNs(podNs) - klog.Infof("Creating set: %v, hashedSet: %v", podNs, util.GetHashedName(podNs)) - if err = ipsMgr.CreateSet(podNs, append([]string{util.IpsetNetHashFlag})); err != nil { - return fmt.Errorf("[syncAddedPod] Error: creating ipset %s with err: %v", podNs, err) - } - } - npmPodObj := newNpmPod(podObj) + // Add the pod to its namespace's ipset. klog.Infof("Adding pod %s to ipset %s", npmPodObj.PodIP, podNs) if err = ipsMgr.AddToSet(podNs, npmPodObj.PodIP, util.IpsetNetHashFlag, podKey); err != nil { @@ -433,24 +424,21 @@ func (c *podController) syncAddedPod(podObj *corev1.Pod) error { // syncAddAndUpdatePod handles updating pod ip in its label's ipset. func (c *podController) syncAddAndUpdatePod(newPodObj *corev1.Pod) error { - podKey, _ := cache.MetaNamespaceKeyFunc(newPodObj) - - klog.Infof("[syncAddAndUpdatePod] updating Pod with key %s", podKey) - newPodObjNs := util.GetNSNameWithPrefix(newPodObj.ObjectMeta.Namespace) + var err error ipsMgr := c.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr - // Add pod namespace if it doesn't exist - var err error + // Create ipset related to namespace which this pod belong to if they do not exist. + // Make the shared NsMap structure read-only in podController + newPodObjNs := util.GetNSNameWithPrefix(newPodObj.Namespace) if _, exists := c.npMgr.NsMap[newPodObjNs]; !exists { - // (TODO): need to change newNS function. It always returns "nil" - c.npMgr.NsMap[newPodObjNs], _ = newNs(newPodObjNs) - klog.Infof("Creating set: %v, hashedSet: %v", newPodObjNs, util.GetHashedName(newPodObjNs)) if err = ipsMgr.CreateSet(newPodObjNs, []string{util.IpsetNetHashFlag}); err != nil { - return fmt.Errorf("[syncAddAndUpdatePod] Error: creating ipset %s with err: %v", newPodObjNs, err) + return fmt.Errorf("[syncAddAndUpdatePod] Error: failed to create ipset for namespace %s with err: %v", newPodObjNs, err) } } + podKey, _ := cache.MetaNamespaceKeyFunc(newPodObj) cachedNpmPodObj, exists := c.npMgr.PodMap[podKey] + klog.Infof("[syncAddAndUpdatePod] updating Pod with key %s", podKey) // No cached npmPod exists. start adding the pod in a cache if !exists { if err = c.syncAddedPod(newPodObj); err != nil { @@ -469,7 +457,7 @@ func (c *podController) syncAddAndUpdatePod(newPodObj *corev1.Pod) error { // then, re-add new pod obj. if cachedNpmPodObj.PodIP != newPodObj.Status.PodIP { metrics.SendErrorLogAndMetric(util.PodID, "[syncAddAndUpdatePod] Info: Unexpected state. Pod (Namespace:%s, Name:%s, newUid:%s) , has cachedPodIp:%s which is different from PodIp:%s", - newPodObjNs, newPodObj.Name, string(newPodObj.UID), cachedNpmPodObj.PodIP, newPodObj.Status.PodIP) + newPodObj.Namespace, newPodObj.Name, string(newPodObj.UID), cachedNpmPodObj.PodIP, newPodObj.Status.PodIP) klog.Infof("Deleting cached Pod with key:%s first due to IP Mistmatch", podKey) if err = c.cleanUpDeletedPod(podKey); err != nil { From 99c8f091bdf7569357e083f80476c9b87a40b15b Mon Sep 17 00:00:00 2001 From: Junguk Cho Date: Tue, 4 May 2021 12:14:19 -0700 Subject: [PATCH 2/3] Correct comment --- npm/podController.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/npm/podController.go b/npm/podController.go index cf935c2b60..a99f7c717b 100644 --- a/npm/podController.go +++ b/npm/podController.go @@ -427,7 +427,7 @@ func (c *podController) syncAddAndUpdatePod(newPodObj *corev1.Pod) error { var err error ipsMgr := c.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr - // Create ipset related to namespace which this pod belong to if they do not exist. + // Create ipset related to namespace which this pod belong to if it does not exist. // Make the shared NsMap structure read-only in podController newPodObjNs := util.GetNSNameWithPrefix(newPodObj.Namespace) if _, exists := c.npMgr.NsMap[newPodObjNs]; !exists { From f3e56bac4e387d45461091f4d251060333958f74 Mon Sep 17 00:00:00 2001 From: Junguk Cho Date: Tue, 4 May 2021 13:56:45 -0700 Subject: [PATCH 3/3] PodController adds list in ipset and our npm-namespace into NsMap local cache --- npm/podController.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/npm/podController.go b/npm/podController.go index a99f7c717b..61e9806064 100644 --- a/npm/podController.go +++ b/npm/podController.go @@ -428,12 +428,19 @@ func (c *podController) syncAddAndUpdatePod(newPodObj *corev1.Pod) error { ipsMgr := c.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr // Create ipset related to namespace which this pod belong to if it does not exist. - // Make the shared NsMap structure read-only in podController newPodObjNs := util.GetNSNameWithPrefix(newPodObj.Namespace) if _, exists := c.npMgr.NsMap[newPodObjNs]; !exists { if err = ipsMgr.CreateSet(newPodObjNs, []string{util.IpsetNetHashFlag}); err != nil { return fmt.Errorf("[syncAddAndUpdatePod] Error: failed to create ipset for namespace %s with err: %v", newPodObjNs, err) } + + if err = ipsMgr.AddToList(util.KubeAllNamespacesFlag, newPodObjNs); err != nil { + return fmt.Errorf("[syncAddAndUpdatePod] Error: failed to add %s to all-namespace ipset list with err: %v", newPodObjNs, err) + } + + // Add namespace object into NsMap cache only when two ipset operations are successful. + npmNs, _ := newNs(newPodObjNs) + c.npMgr.NsMap[newPodObjNs] = npmNs } podKey, _ := cache.MetaNamespaceKeyFunc(newPodObj)