Skip to content

Commit 36ff5cf

Browse files
ornewblakepettersson
ornew
and
blakepettersson
authored
fix: copy visited map argoproj#11699 (argoproj#12667)
This commit fixed an issue argoproj#11699 that caused a warning even if the cycle didn't exist. Fix false cycle discovery by copying the visited resource map before recursively calling of getAppRecursive. Fixes argoproj#11699 Signed-off-by: Arata Furukawa <[email protected]> Co-authored-by: Blake Pettersson <[email protected]>
1 parent 346ae95 commit 36ff5cf

File tree

2 files changed

+224
-8
lines changed

2 files changed

+224
-8
lines changed

‎controller/cache/cache.go

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,8 @@ func isRootAppNode(r *clustercache.Resource) bool {
290290
}
291291

292292
func getApp(r *clustercache.Resource, ns map[kube.ResourceKey]*clustercache.Resource) string {
293-
return getAppRecursive(r, ns, map[kube.ResourceKey]bool{})
293+
name, _ := getAppRecursive(r, ns, map[kube.ResourceKey]bool{})
294+
return name
294295
}
295296

296297
func ownerRefGV(ownerRef metav1.OwnerReference) schema.GroupVersion {
@@ -301,27 +302,31 @@ func ownerRefGV(ownerRef metav1.OwnerReference) schema.GroupVersion {
301302
return gv
302303
}
303304

304-
func getAppRecursive(r *clustercache.Resource, ns map[kube.ResourceKey]*clustercache.Resource, visited map[kube.ResourceKey]bool) string {
305+
func getAppRecursive(r *clustercache.Resource, ns map[kube.ResourceKey]*clustercache.Resource, visited map[kube.ResourceKey]bool) (string, bool) {
305306
if !visited[r.ResourceKey()] {
306307
visited[r.ResourceKey()] = true
307308
} else {
308309
log.Warnf("Circular dependency detected: %v.", visited)
309-
return resInfo(r).AppName
310+
return resInfo(r).AppName, false
310311
}
311312

312313
if resInfo(r).AppName != "" {
313-
return resInfo(r).AppName
314+
return resInfo(r).AppName, true
314315
}
315316
for _, ownerRef := range r.OwnerRefs {
316317
gv := ownerRefGV(ownerRef)
317318
if parent, ok := ns[kube.NewResourceKey(gv.Group, ownerRef.Kind, r.Ref.Namespace, ownerRef.Name)]; ok {
318-
app := getAppRecursive(parent, ns, visited)
319-
if app != "" {
320-
return app
319+
visited_branch := make(map[kube.ResourceKey]bool, len(visited))
320+
for k, v := range visited {
321+
visited_branch[k] = v
322+
}
323+
app, ok := getAppRecursive(parent, ns, visited_branch)
324+
if app != "" || !ok {
325+
return app, ok
321326
}
322327
}
323328
}
324-
return ""
329+
return "", true
325330
}
326331

327332
var (

‎controller/cache/cache_test.go

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"github.com/argoproj/gitops-engine/pkg/cache"
1919
"github.com/argoproj/gitops-engine/pkg/cache/mocks"
2020
"github.com/argoproj/gitops-engine/pkg/health"
21+
"github.com/argoproj/gitops-engine/pkg/utils/kube"
2122
"github.com/stretchr/testify/mock"
2223
"k8s.io/client-go/kubernetes/fake"
2324

@@ -319,6 +320,216 @@ func Test_asResourceNode_owner_refs(t *testing.T) {
319320
assert.Equal(t, expected, resNode)
320321
}
321322

323+
func Test_getAppRecursive(t *testing.T) {
324+
for _, tt := range []struct {
325+
name string
326+
r *cache.Resource
327+
ns map[kube.ResourceKey]*cache.Resource
328+
wantName string
329+
wantOK assert.BoolAssertionFunc
330+
}{
331+
{
332+
name: "ok: cm1->app1",
333+
r: &cache.Resource{
334+
Ref: v1.ObjectReference{
335+
Name: "cm1",
336+
},
337+
OwnerRefs: []metav1.OwnerReference{
338+
{Name: "app1"},
339+
},
340+
},
341+
ns: map[kube.ResourceKey]*cache.Resource{
342+
kube.NewResourceKey("", "", "", "app1"): {
343+
Info: &ResourceInfo{
344+
AppName: "app1",
345+
},
346+
},
347+
},
348+
wantName: "app1",
349+
wantOK: assert.True,
350+
},
351+
{
352+
name: "ok: cm1->cm2->app1",
353+
r: &cache.Resource{
354+
Ref: v1.ObjectReference{
355+
Name: "cm1",
356+
},
357+
OwnerRefs: []metav1.OwnerReference{
358+
{Name: "cm2"},
359+
},
360+
},
361+
ns: map[kube.ResourceKey]*cache.Resource{
362+
kube.NewResourceKey("", "", "", "cm2"): {
363+
Ref: v1.ObjectReference{
364+
Name: "cm2",
365+
},
366+
OwnerRefs: []metav1.OwnerReference{
367+
{Name: "app1"},
368+
},
369+
},
370+
kube.NewResourceKey("", "", "", "app1"): {
371+
Info: &ResourceInfo{
372+
AppName: "app1",
373+
},
374+
},
375+
},
376+
wantName: "app1",
377+
wantOK: assert.True,
378+
},
379+
{
380+
name: "cm1->cm2->app1 & cm1->cm3->app1",
381+
r: &cache.Resource{
382+
Ref: v1.ObjectReference{
383+
Name: "cm1",
384+
},
385+
OwnerRefs: []metav1.OwnerReference{
386+
{Name: "cm2"},
387+
{Name: "cm3"},
388+
},
389+
},
390+
ns: map[kube.ResourceKey]*cache.Resource{
391+
kube.NewResourceKey("", "", "", "cm2"): {
392+
Ref: v1.ObjectReference{
393+
Name: "cm2",
394+
},
395+
OwnerRefs: []metav1.OwnerReference{
396+
{Name: "app1"},
397+
},
398+
},
399+
kube.NewResourceKey("", "", "", "cm3"): {
400+
Ref: v1.ObjectReference{
401+
Name: "cm3",
402+
},
403+
OwnerRefs: []metav1.OwnerReference{
404+
{Name: "app1"},
405+
},
406+
},
407+
kube.NewResourceKey("", "", "", "app1"): {
408+
Info: &ResourceInfo{
409+
AppName: "app1",
410+
},
411+
},
412+
},
413+
wantName: "app1",
414+
wantOK: assert.True,
415+
},
416+
{
417+
// Nothing cycle.
418+
// Issue #11699, fixed #12667.
419+
name: "ok: cm1->cm2 & cm1->cm3->cm2 & cm1->cm3->app1",
420+
r: &cache.Resource{
421+
Ref: v1.ObjectReference{
422+
Name: "cm1",
423+
},
424+
OwnerRefs: []metav1.OwnerReference{
425+
{Name: "cm2"},
426+
{Name: "cm3"},
427+
},
428+
},
429+
ns: map[kube.ResourceKey]*cache.Resource{
430+
kube.NewResourceKey("", "", "", "cm2"): {
431+
Ref: v1.ObjectReference{
432+
Name: "cm2",
433+
},
434+
},
435+
kube.NewResourceKey("", "", "", "cm3"): {
436+
Ref: v1.ObjectReference{
437+
Name: "cm3",
438+
},
439+
OwnerRefs: []metav1.OwnerReference{
440+
{Name: "cm2"},
441+
{Name: "app1"},
442+
},
443+
},
444+
kube.NewResourceKey("", "", "", "app1"): {
445+
Info: &ResourceInfo{
446+
AppName: "app1",
447+
},
448+
},
449+
},
450+
wantName: "app1",
451+
wantOK: assert.True,
452+
},
453+
{
454+
name: "cycle: cm1<->cm2",
455+
r: &cache.Resource{
456+
Ref: v1.ObjectReference{
457+
Name: "cm1",
458+
},
459+
OwnerRefs: []metav1.OwnerReference{
460+
{Name: "cm2"},
461+
},
462+
},
463+
ns: map[kube.ResourceKey]*cache.Resource{
464+
kube.NewResourceKey("", "", "", "cm1"): {
465+
Ref: v1.ObjectReference{
466+
Name: "cm1",
467+
},
468+
OwnerRefs: []metav1.OwnerReference{
469+
{Name: "cm2"},
470+
},
471+
},
472+
kube.NewResourceKey("", "", "", "cm2"): {
473+
Ref: v1.ObjectReference{
474+
Name: "cm2",
475+
},
476+
OwnerRefs: []metav1.OwnerReference{
477+
{Name: "cm1"},
478+
},
479+
},
480+
},
481+
wantName: "",
482+
wantOK: assert.False,
483+
},
484+
{
485+
name: "cycle: cm1->cm2->cm3->cm1",
486+
r: &cache.Resource{
487+
Ref: v1.ObjectReference{
488+
Name: "cm1",
489+
},
490+
OwnerRefs: []metav1.OwnerReference{
491+
{Name: "cm2"},
492+
},
493+
},
494+
ns: map[kube.ResourceKey]*cache.Resource{
495+
kube.NewResourceKey("", "", "", "cm1"): {
496+
Ref: v1.ObjectReference{
497+
Name: "cm1",
498+
},
499+
OwnerRefs: []metav1.OwnerReference{
500+
{Name: "cm2"},
501+
},
502+
},
503+
kube.NewResourceKey("", "", "", "cm2"): {
504+
Ref: v1.ObjectReference{
505+
Name: "cm2",
506+
},
507+
OwnerRefs: []metav1.OwnerReference{
508+
{Name: "cm3"},
509+
},
510+
},
511+
kube.NewResourceKey("", "", "", "cm3"): {
512+
Ref: v1.ObjectReference{
513+
Name: "cm3",
514+
},
515+
OwnerRefs: []metav1.OwnerReference{
516+
{Name: "cm1"},
517+
},
518+
},
519+
},
520+
wantName: "",
521+
wantOK: assert.False,
522+
},
523+
} {
524+
t.Run(tt.name, func(t *testing.T) {
525+
visited := map[kube.ResourceKey]bool{}
526+
got, ok := getAppRecursive(tt.r, tt.ns, visited)
527+
assert.Equal(t, tt.wantName, got)
528+
tt.wantOK(t, ok)
529+
})
530+
}
531+
}
532+
322533
func TestSkipResourceUpdate(t *testing.T) {
323534
var (
324535
hash1_x string = "x"

0 commit comments

Comments
 (0)