Skip to content

Commit ee14e85

Browse files
vakalapavakalapa
committed
Addressing some comments
1 parent 772b0f5 commit ee14e85

File tree

2 files changed

+64
-45
lines changed

2 files changed

+64
-45
lines changed

‎npm/nameSpaceController.go‎

Lines changed: 24 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func newNs(name string) (*Namespace, error) {
5050
return ns, nil
5151
}
5252

53-
func getNamespaceObjFromNsObj(nsObj *Namespace) *corev1.Namespace {
53+
func (nsObj *Namespace) getNamespaceObjFromNsObj() *corev1.Namespace {
5454
return &corev1.Namespace{
5555
ObjectMeta: metav1.ObjectMeta{
5656
Name: nsObj.name,
@@ -60,7 +60,7 @@ func getNamespaceObjFromNsObj(nsObj *Namespace) *corev1.Namespace {
6060
}
6161

6262
// setResourceVersion setter func for RV
63-
func setResourceVersion(nsObj *Namespace, rv string) {
63+
func (nsObj *Namespace) setResourceVersion(rv string) {
6464
nsObj.resourceVersion = util.ParseResourceVersion(rv)
6565
}
6666

@@ -103,15 +103,14 @@ func (nsc *nameSpaceController) needSync(obj interface{}, event string) (string,
103103

104104
nsObj, ok := obj.(*corev1.Namespace)
105105
if !ok {
106-
metrics.SendErrorLogAndMetric(util.NSID, "%s Pod: Received unexpected object type: %v", event, obj)
106+
metrics.SendErrorLogAndMetric(util.NSID, "[NAMESPACE %s EVENT] Received unexpected object type: %v", event, obj)
107107
return key, needSync
108108
}
109109

110110
var err error
111111
if key, err = cache.MetaNamespaceKeyFunc(obj); err != nil {
112112
utilruntime.HandleError(err)
113-
err = fmt.Errorf("[%sNameSpace] Error: nameSpaceKey is empty for %s namespace", event, util.GetNSNameWithPrefix(nsObj.Name))
114-
metrics.SendErrorLogAndMetric(util.NSID, err.Error())
113+
metrics.SendErrorLogAndMetric(util.NSID, "[NAMESPACE %s EVENT] Error: NameSpaceKey is empty for %s namespace", event, util.GetNSNameWithPrefix(nsObj.Name))
115114
return key, needSync
116115
}
117116

@@ -147,17 +146,15 @@ func (nsc *nameSpaceController) updateNamespace(old, new interface{}) {
147146
}
148147

149148
nsKey := util.GetNSNameWithPrefix(key)
149+
150150
nsc.npMgr.Lock()
151151
defer nsc.npMgr.Unlock()
152152
cachedNsObj, nsExists := nsc.npMgr.NsMap[nsKey]
153-
if !nsExists {
154-
nsc.workqueue.Add(key)
155-
return
156-
}
157-
158-
if reflect.DeepEqual(cachedNsObj.LabelsMap, nsObj.ObjectMeta.Labels) {
159-
log.Logf("[NAMESPACE UPDATE EVENT] Namespace [%s] labels did not change", key)
160-
return
153+
if nsExists {
154+
if reflect.DeepEqual(cachedNsObj.LabelsMap, nsObj.ObjectMeta.Labels) {
155+
log.Logf("[NAMESPACE UPDATE EVENT] Namespace [%s] labels did not change", key)
156+
return
157+
}
161158
}
162159

163160
nsc.workqueue.Add(key)
@@ -186,8 +183,7 @@ func (nsc *nameSpaceController) deleteNamespace(obj interface{}) {
186183
var key string
187184
if key, err = cache.MetaNamespaceKeyFunc(obj); err != nil {
188185
utilruntime.HandleError(err)
189-
err = fmt.Errorf("[NAMESPACE DELETE EVENT] Error: nameSpaceKey is empty for %s namespace", util.GetNSNameWithPrefix(nsObj.Name))
190-
metrics.SendErrorLogAndMetric(util.NSID, err.Error())
186+
metrics.SendErrorLogAndMetric(util.NSID, "[NAMESPACE DELETE EVENT] Error: nameSpaceKey is empty for %s namespace", util.GetNSNameWithPrefix(nsObj.Name))
191187
return
192188
}
193189

@@ -197,7 +193,7 @@ func (nsc *nameSpaceController) deleteNamespace(obj interface{}) {
197193
nsKey := util.GetNSNameWithPrefix(key)
198194
_, nsExists := nsc.npMgr.NsMap[nsKey]
199195
if !nsExists {
200-
log.Logf("[NAMESPACE Delete EVENT] Namespace [%s] does not exist in case, so returning", key)
196+
log.Logf("[NAMESPACE DELETE EVENT] Namespace [%s] does not exist in case, so returning", key)
201197
return
202198
}
203199

@@ -208,11 +204,9 @@ func (nsc *nameSpaceController) Run(threadiness int, stopCh <-chan struct{}) err
208204
defer utilruntime.HandleCrash()
209205
defer nsc.workqueue.ShutDown()
210206

211-
// Start the informer factories to begin populating the informer caches
212207
log.Logf("Starting Namespace controller\n")
213-
214208
log.Logf("Starting workers")
215-
// Launch two workers to process namespace resources
209+
// Launch workers to process namespace resources
216210
for i := 0; i < threadiness; i++ {
217211
go wait.Until(nsc.runWorker, time.Second, stopCh)
218212
}
@@ -248,7 +242,7 @@ func (nsc *nameSpaceController) processNextWorkItem() bool {
248242
utilruntime.HandleError(fmt.Errorf("expected string in workqueue but got %#v", obj))
249243
return nil
250244
}
251-
// Run the syncHandler, passing it the namespace string of the
245+
// Run the syncNameSpace, passing it the namespace string of the
252246
// resource to be synced.
253247
// TODO : may consider using "c.queue.AddAfter(key, *requeueAfter)" according to error type later
254248
if err := nsc.syncNameSpace(key); err != nil {
@@ -283,7 +277,7 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error {
283277
if err != nil {
284278
if errors.IsNotFound(err) {
285279
utilruntime.HandleError(fmt.Errorf("NameSpace '%s' in work queue no longer exists", key))
286-
// find the namespace object from a local cache and start cleaning up process (calling cleanUpDeletedPod function)
280+
// find the namespace object from a local cache and start cleaning up process (calling cleanDeletedNamespace function)
287281
nsKey := util.GetNSNameWithPrefix(key)
288282
cachedNs, found := nsc.npMgr.NsMap[nsKey]
289283
// if the namespace does not exists, we do not need to clean up process and retry it
@@ -303,17 +297,15 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error {
303297
}
304298

305299
if nsObj.DeletionTimestamp != nil || nsObj.DeletionGracePeriodSeconds != nil {
306-
return nsc.cleanDeletedNamespace(
307-
util.GetNSNameWithPrefix(nsObj.Name),
308-
nsObj.Labels,
309-
)
300+
return nsc.cleanDeletedNamespace(util.GetNSNameWithPrefix(nsObj.Name), nsObj.Labels)
310301

311302
}
312303

313304
err = nsc.syncUpdateNameSpace(nsObj)
314305
// 1. deal with error code and retry this
315306
if err != nil {
316-
return fmt.Errorf("failed to sync namespace due to %s", err.Error())
307+
metrics.SendErrorLogAndMetric(util.NSID, "[syncNameSpace] failed to sync namespace due to %s", err.Error())
308+
return err
317309
}
318310

319311
return nil
@@ -383,11 +375,8 @@ func (nsc *nameSpaceController) syncAddNameSpace(nsObj *corev1.Namespace) error
383375
}
384376
}
385377

386-
ns, err := newNs(nsName)
387-
if err != nil {
388-
metrics.SendErrorLogAndMetric(util.NSID, "[AddNamespace] Error: failed to create namespace %s with err: %v", nsName, err)
389-
}
390-
setResourceVersion(ns, nsObj.GetObjectMeta().GetResourceVersion())
378+
ns, _ := newNs(nsName)
379+
ns.setResourceVersion(nsObj.GetObjectMeta().GetResourceVersion())
391380

392381
// Append all labels to the cache NS obj
393382
ns.LabelsMap = util.AppendMap(ns.LabelsMap, nsLabel)
@@ -444,16 +433,14 @@ func (nsc *nameSpaceController) syncUpdateNameSpace(newNsObj *corev1.Namespace)
444433

445434
// Append all labels to the cache NS obj
446435
curNsObj.LabelsMap = util.ClearAndAppendMap(curNsObj.LabelsMap, newNsLabel)
447-
setResourceVersion(curNsObj, newNsObj.GetObjectMeta().GetResourceVersion())
436+
curNsObj.setResourceVersion(newNsObj.GetObjectMeta().GetResourceVersion())
448437
nsc.npMgr.NsMap[newNsNs] = curNsObj
449438

450439
return nil
451440
}
452441

453442
// cleanDeletedNamespace handles deleting namespace from ipset.
454443
func (nsc *nameSpaceController) cleanDeletedNamespace(nsName string, nsLabel map[string]string) error {
455-
var err error
456-
457444
log.Logf("NAMESPACE DELETING: [%s/%v]", nsName, nsLabel)
458445

459446
cachedNsObj, exists := nsc.npMgr.NsMap[nsName]
@@ -462,9 +449,11 @@ func (nsc *nameSpaceController) cleanDeletedNamespace(nsName string, nsLabel map
462449
}
463450

464451
log.Logf("NAMESPACE DELETING cached labels: [%s/%v]", nsName, cachedNsObj.LabelsMap)
465-
// Delete the namespace from its label's ipset list.
452+
453+
var err error
466454
ipsMgr := nsc.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr
467455
nsLabels := util.GetIPSetListFromLabels(cachedNsObj.LabelsMap)
456+
// Delete the namespace from its label's ipset list.
468457
for _, nsLabelKey := range nsLabels {
469458
labelKey := util.GetNSNameWithPrefix(nsLabelKey)
470459
log.Logf("Deleting namespace %s from ipset list %s", nsName, labelKey)

‎npm/nameSpaceController_test.go‎

Lines changed: 40 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ func newNsFixture(t *testing.T) *nameSpaceFixture {
5959
return f
6060
}
6161

62-
func (f *nameSpaceFixture) newNsController() {
62+
func (f *nameSpaceFixture) newNsController(stopCh chan struct{}) {
6363
f.kubeclient = k8sfake.NewSimpleClientset(f.kubeobjects...)
6464
f.kubeInformer = kubeinformers.NewSharedInformerFactory(f.kubeclient, noResyncPeriodFunc())
6565

@@ -69,6 +69,8 @@ func (f *nameSpaceFixture) newNsController() {
6969
for _, ns := range f.nsLister {
7070
f.kubeInformer.Core().V1().Namespaces().Informer().GetIndexer().Add(ns)
7171
}
72+
73+
f.kubeInformer.Start(stopCh)
7274
}
7375

7476
func (f *nameSpaceFixture) ipSetSave(ipsetConfigFile string) {
@@ -112,14 +114,6 @@ func newNameSpace(name, rv string, labels map[string]string) *corev1.Namespace {
112114
}
113115

114116
func addNamespace(t *testing.T, f *nameSpaceFixture, nsObj *corev1.Namespace) {
115-
f.nsLister = append(f.nsLister, nsObj)
116-
f.kubeobjects = append(f.kubeobjects, nsObj)
117-
118-
f.newNsController()
119-
stopCh := make(chan struct{})
120-
defer close(stopCh)
121-
f.kubeInformer.Start(stopCh)
122-
123117
t.Logf("Calling add namespace event")
124118
f.nsController.addNamespace(nsObj)
125119
if f.nsController.workqueue.Len() == 0 {
@@ -202,6 +196,12 @@ func TestAddNamespace(t *testing.T) {
202196
"app": "test-namespace",
203197
},
204198
)
199+
f.nsLister = append(f.nsLister, nsObj)
200+
f.kubeobjects = append(f.kubeobjects, nsObj)
201+
202+
stopCh := make(chan struct{})
203+
defer close(stopCh)
204+
f.newNsController(stopCh)
205205

206206
addNamespace(t, f, nsObj)
207207

@@ -235,6 +235,11 @@ func TestUpdateNamespace(t *testing.T) {
235235
"app": "new-test-namespace",
236236
},
237237
)
238+
f.nsLister = append(f.nsLister, oldNsObj)
239+
f.kubeobjects = append(f.kubeobjects, oldNsObj)
240+
241+
stopCh := make(chan struct{})
242+
defer close(stopCh)
238243
updateNamespace(t, f, oldNsObj, newNsObj)
239244

240245
testCases := []expectedNsValues{
@@ -274,6 +279,11 @@ func TestAddNamespaceLabel(t *testing.T) {
274279
"update": "true",
275280
},
276281
)
282+
f.nsLister = append(f.nsLister, oldNsObj)
283+
f.kubeobjects = append(f.kubeobjects, oldNsObj)
284+
285+
stopCh := make(chan struct{})
286+
defer close(stopCh)
277287
updateNamespace(t, f, oldNsObj, newNsObj)
278288

279289
testCases := []expectedNsValues{
@@ -314,6 +324,11 @@ func TestAddNamespaceLabelSameRv(t *testing.T) {
314324
"update": "true",
315325
},
316326
)
327+
f.nsLister = append(f.nsLister, oldNsObj)
328+
f.kubeobjects = append(f.kubeobjects, oldNsObj)
329+
330+
stopCh := make(chan struct{})
331+
defer close(stopCh)
317332
updateNamespace(t, f, oldNsObj, newNsObj)
318333

319334
testCases := []expectedNsValues{
@@ -356,6 +371,11 @@ func TestDeleteandUpdateNamespaceLabel(t *testing.T) {
356371
"update": "false",
357372
},
358373
)
374+
f.nsLister = append(f.nsLister, oldNsObj)
375+
f.kubeobjects = append(f.kubeobjects, oldNsObj)
376+
377+
stopCh := make(chan struct{})
378+
defer close(stopCh)
359379
updateNamespace(t, f, oldNsObj, newNsObj)
360380

361381
testCases := []expectedNsValues{
@@ -402,6 +422,11 @@ func TestNewNameSpaceUpdate(t *testing.T) {
402422
"update": "false",
403423
},
404424
)
425+
f.nsLister = append(f.nsLister, oldNsObj)
426+
f.kubeobjects = append(f.kubeobjects, oldNsObj)
427+
428+
stopCh := make(chan struct{})
429+
defer close(stopCh)
405430
newNsObj.SetUID("test2")
406431
updateNamespace(t, f, oldNsObj, newNsObj)
407432

@@ -434,6 +459,11 @@ func TestDeleteNamespace(t *testing.T) {
434459
"app": "test-namespace",
435460
},
436461
)
462+
f.nsLister = append(f.nsLister, nsObj)
463+
f.kubeobjects = append(f.kubeobjects, nsObj)
464+
465+
stopCh := make(chan struct{})
466+
defer close(stopCh)
437467
deleteNamespace(t, f, nsObj)
438468

439469
testCases := []expectedNsValues{
@@ -452,7 +482,7 @@ func TestGetNamespaceObjFromNsObj(t *testing.T) {
452482
"test": "new",
453483
}
454484

455-
nsObj := getNamespaceObjFromNsObj(ns)
485+
nsObj := ns.getNamespaceObjFromNsObj()
456486

457487
if !reflect.DeepEqual(ns.LabelsMap, nsObj.ObjectMeta.Labels) {
458488
t.Errorf("TestGetNamespaceObjFromNsObj failed @ nsObj labels check")

0 commit comments

Comments
 (0)