Skip to content

Commit

Permalink
fix: copy visited map argoproj#11699
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@ornew
ornew committed Jul 22, 2023
1 parent 5120026 commit cbd1a40
Show file tree
Hide file tree
Showing 2 changed files with 225 additions and 9 deletions.
21 changes: 13 additions & 8 deletions controller/cache/cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,8 @@ func isRootAppNode(r *clustercache.Resource) bool {
}

func getApp(r *clustercache.Resource, ns map[kube.ResourceKey]*clustercache.Resource) string {
return getAppRecursive(r, ns, map[kube.ResourceKey]bool{})
name, _ := getAppRecursive(r, ns, map[kube.ResourceKey]bool{})
return name
}

func ownerRefGV(ownerRef metav1.OwnerReference) schema.GroupVersion {
Expand All @@ -291,27 +292,31 @@ func ownerRefGV(ownerRef metav1.OwnerReference) schema.GroupVersion {
return gv
}

func getAppRecursive(r *clustercache.Resource, ns map[kube.ResourceKey]*clustercache.Resource, visited map[kube.ResourceKey]bool) string {
func getAppRecursive(r *clustercache.Resource, ns map[kube.ResourceKey]*clustercache.Resource, visited map[kube.ResourceKey]bool) (string, bool) {
if !visited[r.ResourceKey()] {
visited[r.ResourceKey()] = true
} else {
log.Warnf("Circular dependency detected: %v.", visited)
return resInfo(r).AppName
return resInfo(r).AppName, false
}

if resInfo(r).AppName != "" {
return resInfo(r).AppName
return resInfo(r).AppName, true
}
for _, ownerRef := range r.OwnerRefs {
gv := ownerRefGV(ownerRef)
if parent, ok := ns[kube.NewResourceKey(gv.Group, ownerRef.Kind, r.Ref.Namespace, ownerRef.Name)]; ok {
app := getAppRecursive(parent, ns, visited)
if app != "" {
return app
visited_branch := make(map[kube.ResourceKey]bool, len(visited))
for k, v := range visited {
visited_branch[k] = v
}
app, ok := getAppRecursive(parent, ns, visited_branch)
if app != "" || !ok {
return app, ok
}
}
}
return ""
return "", true
}

var (
Expand Down
213 changes: 212 additions & 1 deletion controller/cache/cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
apierr "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime/schema"

"github.com/argoproj/gitops-engine/pkg/cache"
"github.com/argoproj/gitops-engine/pkg/cache/mocks"
"github.com/argoproj/gitops-engine/pkg/health"
"github.com/argoproj/gitops-engine/pkg/utils/kube"
"github.com/stretchr/testify/mock"

appv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
Expand Down Expand Up @@ -204,6 +205,216 @@ func Test_asResourceNode_owner_refs(t *testing.T) {
assert.Equal(t, expected, resNode)
}

func Test_getAppRecursive(t *testing.T) {
for _, tt := range []struct {
name string
r *cache.Resource
ns map[kube.ResourceKey]*cache.Resource
wantName string
wantOK assert.BoolAssertionFunc
}{
{
name: "ok: cm1->app1",
r: &cache.Resource{
Ref: v1.ObjectReference{
Name: "cm1",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "app1"},
},
},
ns: map[kube.ResourceKey]*cache.Resource{
kube.NewResourceKey("", "", "", "app1"): {
Info: &ResourceInfo{
AppName: "app1",
},
},
},
wantName: "app1",
wantOK: assert.True,
},
{
name: "ok: cm1->cm2->app1",
r: &cache.Resource{
Ref: v1.ObjectReference{
Name: "cm1",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "cm2"},
},
},
ns: map[kube.ResourceKey]*cache.Resource{
kube.NewResourceKey("", "", "", "cm2"): {
Ref: v1.ObjectReference{
Name: "cm2",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "app1"},
},
},
kube.NewResourceKey("", "", "", "app1"): {
Info: &ResourceInfo{
AppName: "app1",
},
},
},
wantName: "app1",
wantOK: assert.True,
},
{
name: "cm1->cm2->app1 & cm1->cm3->app1",
r: &cache.Resource{
Ref: v1.ObjectReference{
Name: "cm1",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "cm2"},
{Name: "cm3"},
},
},
ns: map[kube.ResourceKey]*cache.Resource{
kube.NewResourceKey("", "", "", "cm2"): {
Ref: v1.ObjectReference{
Name: "cm2",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "app1"},
},
},
kube.NewResourceKey("", "", "", "cm3"): {
Ref: v1.ObjectReference{
Name: "cm3",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "app1"},
},
},
kube.NewResourceKey("", "", "", "app1"): {
Info: &ResourceInfo{
AppName: "app1",
},
},
},
wantName: "app1",
wantOK: assert.True,
},
{
// Nothing cycle.
// Issue #11699, fixed #12667.
name: "ok: cm1->cm2 & cm1->cm3->cm2 & cm1->cm3->app1",
r: &cache.Resource{
Ref: v1.ObjectReference{
Name: "cm1",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "cm2"},
{Name: "cm3"},
},
},
ns: map[kube.ResourceKey]*cache.Resource{
kube.NewResourceKey("", "", "", "cm2"): {
Ref: v1.ObjectReference{
Name: "cm2",
},
},
kube.NewResourceKey("", "", "", "cm3"): {
Ref: v1.ObjectReference{
Name: "cm3",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "cm2"},
{Name: "app1"},
},
},
kube.NewResourceKey("", "", "", "app1"): {
Info: &ResourceInfo{
AppName: "app1",
},
},
},
wantName: "app1",
wantOK: assert.True,
},
{
name: "cycle: cm1<->cm2",
r: &cache.Resource{
Ref: v1.ObjectReference{
Name: "cm1",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "cm2"},
},
},
ns: map[kube.ResourceKey]*cache.Resource{
kube.NewResourceKey("", "", "", "cm1"): {
Ref: v1.ObjectReference{
Name: "cm1",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "cm2"},
},
},
kube.NewResourceKey("", "", "", "cm2"): {
Ref: v1.ObjectReference{
Name: "cm2",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "cm1"},
},
},
},
wantName: "",
wantOK: assert.False,
},
{
name: "cycle: cm1->cm2->cm3->cm1",
r: &cache.Resource{
Ref: v1.ObjectReference{
Name: "cm1",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "cm2"},
},
},
ns: map[kube.ResourceKey]*cache.Resource{
kube.NewResourceKey("", "", "", "cm1"): {
Ref: v1.ObjectReference{
Name: "cm1",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "cm2"},
},
},
kube.NewResourceKey("", "", "", "cm2"): {
Ref: v1.ObjectReference{
Name: "cm2",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "cm3"},
},
},
kube.NewResourceKey("", "", "", "cm3"): {
Ref: v1.ObjectReference{
Name: "cm3",
},
OwnerRefs: []metav1.OwnerReference{
{Name: "cm1"},
},
},
},
wantName: "",
wantOK: assert.False,
},
} {
t.Run(tt.name, func(t *testing.T) {
visited := map[kube.ResourceKey]bool{}
got, ok := getAppRecursive(tt.r, tt.ns, visited)
assert.Equal(t, tt.wantName, got)
tt.wantOK(t, ok)
})
}
}

func TestSkipResourceUpdate(t *testing.T) {
var (
hash1_x string = "x"
Expand Down

0 comments on commit cbd1a40

Please sign in to comment.