diff --git a/resourcetopo/node_info.go b/resourcetopo/node_info.go index 8669f87c..4e40c438 100644 --- a/resourcetopo/node_info.go +++ b/resourcetopo/node_info.go @@ -58,8 +58,10 @@ type nodeInfo struct { // will be updated to true after the object added to cache or the manager is of virtual type. objectExisted bool - relationsLock sync.RWMutex - relations []ResourceRelation + // always call relationsLock.Lock() before relation change, so when locked, the relation is stable. + // kind like a lock.RLock() in relation change scenario. + relationsLock sync.RWMutex + labelRelations []ResourceRelation } func newNode(s *nodeStorage, cluster, namespace, name string) *nodeInfo { @@ -129,10 +131,10 @@ func (node *nodeInfo) checkLabelUpdateForPostNode(postNode *nodeInfo) { node.relationsLock.RLock() defer node.relationsLock.RUnlock() - if len(node.relations) == 0 { + if len(node.labelRelations) == 0 { return } - for _, relation := range node.relations { + for _, relation := range node.labelRelations { if !typeEqual(relation.PostMeta, postNode.storageRef.meta) { return } @@ -181,13 +183,10 @@ func (n *nodeInfo) updateNodeMeta(obj Object) { func (n *nodeInfo) objectDeleted() { n.metaLock.Lock() defer n.metaLock.Unlock() - n.relationsLock.Lock() - defer n.relationsLock.Unlock() n.objectExisted = false n.labels = nil n.ownerNodes = nil - n.relations = nil } func (n *nodeInfo) matched(selector labels.Selector) bool { diff --git a/resourcetopo/node_relation_utils.go b/resourcetopo/node_relation_utils.go index a239c2f0..75a1221e 100644 --- a/resourcetopo/node_relation_utils.go +++ b/resourcetopo/node_relation_utils.go @@ -99,6 +99,13 @@ func deleteAllRelation(node *nodeInfo) { // we always lock preOrder before postOrder, expect in this function. // but to keep transition atomic, and in object deletion scenario, // this is okay to hold the lock in whole process, and lock preOrderNode as needed. + // node.metaLock.Lock() + // defer node.metaLock.Unlock() + node.relationsLock.Lock() + node.objectDeleted() + node.labelRelations = nil + defer node.relationsLock.Unlock() + node.lock.Lock() defer node.lock.Unlock() diff --git a/resourcetopo/node_topology_updater.go b/resourcetopo/node_topology_updater.go index 7a871f99..d4a37f25 100644 --- a/resourcetopo/node_topology_updater.go +++ b/resourcetopo/node_topology_updater.go @@ -24,6 +24,11 @@ import ( "k8s.io/klog/v2" ) +type nodeMeta struct { + apiVersion, kind string + cluster, namespace, name string +} + var _ cache.ResourceEventHandler = &nodeStorage{} func (s *nodeStorage) OnAdd(obj interface{}) { @@ -63,23 +68,40 @@ func (s *nodeStorage) OnUpdate(oldObj, newObj interface{}) { return } - var resolvedRelations []ResourceRelation + var resolvedLabelRelations []ResourceRelation + resolvedDirectRelations := make(map[nodeMeta]interface{}) for _, resolver := range s.resolvers { relations := resolver.Resolve(newTopoObj) - resolvedRelations = append(resolvedRelations, relations...) + for _, relation := range relations { + if relation.LabelSelector != nil { + resolvedLabelRelations = append(resolvedLabelRelations, relation) + } else { + for _, directRef := range relation.DirectRefs { + resolvedDirectRelations[nodeMeta{ + apiVersion: relation.PostMeta.APIVersion, + kind: relation.PostMeta.Kind, + cluster: relation.Cluster, + namespace: directRef.Namespace, + name: directRef.Name, + }] = nil + } + } + } } - slices.SortFunc(resolvedRelations, compareResourceRelation) + slices.SortFunc(resolvedLabelRelations, compareLabelResourceRelation) node.relationsLock.Lock() - sortedSlicesCompare(node.relations, resolvedRelations, + sortedSlicesCompare(node.labelRelations, resolvedLabelRelations, func(relation ResourceRelation) { - s.removeResourceRelation(node, &relation) + s.removeLabelResourceRelation(node, &relation) }, func(relation ResourceRelation) { - s.addResourceRelation(node, &relation) + s.addLabelResourceRelation(node, &relation) }, - compareResourceRelation) - node.relations = resolvedRelations + compareLabelResourceRelation) + node.labelRelations = resolvedLabelRelations + + s.setDirectResourceRelation(node, resolvedDirectRelations) node.relationsLock.Unlock() if !node.labelEqualed(newTopoObj.GetLabels()) || @@ -132,7 +154,6 @@ func (s *nodeStorage) OnDelete(obj interface{}) { return } - node.objectDeleted() deleteAllRelation(node) node.checkGC() @@ -142,18 +163,24 @@ func (s *nodeStorage) OnDelete(obj interface{}) { func (s *nodeStorage) addNode(obj Object, node *nodeInfo) { node.updateNodeMeta(obj) - if len(node.relations) != 0 { - klog.Warningf("unexpected relations {%v}", node.relations) - node.relations = nil + if len(node.labelRelations) != 0 { + klog.Warningf("unexpected labelRelations {%v}", node.labelRelations) + node.labelRelations = nil } node.relationsLock.Lock() for _, resolver := range s.resolvers { relations := resolver.Resolve(obj) - node.relations = append(node.relations, relations...) + for _, relation := range relations { + if relation.LabelSelector != nil { + node.labelRelations = append(node.labelRelations, relation) + } else { + s.addDirectResourceRelation(node, &relation) + } + } } - slices.SortFunc(node.relations, compareResourceRelation) - for _, relation := range node.relations { - s.addResourceRelation(node, &relation) + slices.SortFunc(node.labelRelations, compareLabelResourceRelation) + for _, relation := range node.labelRelations { + s.addLabelResourceRelation(node, &relation) } node.relationsLock.Unlock() @@ -171,7 +198,7 @@ func (s *nodeStorage) addNode(obj Object, node *nodeInfo) { } } -func (s *nodeStorage) addResourceRelation(node *nodeInfo, relation *ResourceRelation) { +func (s *nodeStorage) addLabelResourceRelation(node *nodeInfo, relation *ResourceRelation) { postMeta := relation.PostMeta postMetaKey := generateMetaKey(postMeta) postStorage := s.manager.getStorage(postMeta) @@ -179,12 +206,6 @@ func (s *nodeStorage) addResourceRelation(node *nodeInfo, relation *ResourceRela klog.Errorf("Failed to get node storage by meta %s, ignore this relation", postMetaKey) return } - if len(relation.DirectRefs) > 0 { - for _, ref := range relation.DirectRefs { - postNode := postStorage.getOrCreateNode(relation.Cluster, ref.Namespace, ref.Name) - rangeAndSetDirectRefRelation(node, postNode, s.manager) - } - } if relation.LabelSelector != nil { var postNodes []*nodeInfo @@ -199,22 +220,65 @@ func (s *nodeStorage) addResourceRelation(node *nodeInfo, relation *ResourceRela } } -func (s *nodeStorage) removeResourceRelation(node *nodeInfo, relation *ResourceRelation) { - postStorage := s.manager.getStorage(relation.PostMeta) +func (s *nodeStorage) setDirectResourceRelation(node *nodeInfo, directRefs map[nodeMeta]interface{}) { + var toDel []*nodeInfo + rangeNodeList(node.directReferredPostOrders, func(postNode *nodeInfo) { + postNodeMeta := nodeMeta{ + apiVersion: postNode.storageRef.meta.APIVersion, + kind: postNode.storageRef.meta.Kind, + cluster: postNode.cluster, + namespace: postNode.namespace, + name: postNode.name, + } + if _, ok := directRefs[postNodeMeta]; !ok { + toDel = append(toDel, postNode) + } else { + delete(directRefs, postNodeMeta) + } + }) + + for _, postNode := range toDel { + if deleteDirectRelation(node, postNode) { + node.noticePostOrderRelationDeleted(postNode) + postNode.checkGC() + } + } + + for postNodeMeta := range directRefs { + postStorage := s.manager.getStorage(metav1.TypeMeta{APIVersion: postNodeMeta.apiVersion, Kind: postNodeMeta.kind}) + if postStorage == nil { + klog.Errorf("Failed to get node storage by meta %s, ignore this relation", generateKey(postNodeMeta.apiVersion, postNodeMeta.kind)) + continue + } + postNode := postStorage.getOrCreateNode(postNodeMeta.cluster, postNodeMeta.namespace, postNodeMeta.name) + rangeAndSetDirectRefRelation(node, postNode, s.manager) + } +} + +func (s *nodeStorage) addDirectResourceRelation(node *nodeInfo, relation *ResourceRelation) { + postMeta := relation.PostMeta + postMetaKey := generateMetaKey(postMeta) + postStorage := s.manager.getStorage(postMeta) if postStorage == nil { - klog.Error("Failed to get node Storage by %s, ignore this delete request", - generateMetaKey(relation.PostMeta)) + klog.Errorf("Failed to get node storage by meta %s, ignore this relation", postMetaKey) return } + if len(relation.DirectRefs) > 0 { for _, ref := range relation.DirectRefs { - postNode := postStorage.getNode(relation.Cluster, ref.Namespace, ref.Name) - if deleteDirectRelation(node, postNode) { - node.noticePostOrderRelationDeleted(postNode) - postNode.checkGC() - } + postNode := postStorage.getOrCreateNode(relation.Cluster, ref.Namespace, ref.Name) + rangeAndSetDirectRefRelation(node, postNode, s.manager) } } +} + +func (s *nodeStorage) removeLabelResourceRelation(node *nodeInfo, relation *ResourceRelation) { + postStorage := s.manager.getStorage(relation.PostMeta) + if postStorage == nil { + klog.Error("Failed to get node Storage by %s, ignore this delete request", + generateMetaKey(relation.PostMeta)) + return + } if relation.LabelSelector != nil { postNodes := postStorage.getMatchedNodeList(node.cluster, node.namespace, relation.LabelSelector) diff --git a/resourcetopo/resourcetopo_test.go b/resourcetopo/resourcetopo_test.go index d988f1c2..85f2d92f 100644 --- a/resourcetopo/resourcetopo_test.go +++ b/resourcetopo/resourcetopo_test.go @@ -72,7 +72,7 @@ var _ = Describe("test suite with ists config(label selector and virtual rersour Expect(podStorage).NotTo(BeNil()) Expect(istsStorage).NotTo(BeNil()) - ctx, cancel = context.WithCancel((context.Background())) + ctx, cancel = context.WithCancel(context.Background()) podHandler = &objecthandler{} Expect(manager.AddNodeHandler(PodMeta, podHandler)).Should(Succeed()) stsHandler = &objecthandler{} @@ -471,7 +471,7 @@ var _ = Describe("test suite with cluster role config(cluster role and direct re Expect(clusterroleStorage).NotTo(BeNil()) Expect(saStorage).NotTo(BeNil()) - ctx, cancel = context.WithCancel((context.Background())) + ctx, cancel = context.WithCancel(context.Background()) clusterRoleBindingHandler = &objecthandler{} Expect(manager.AddNodeHandler(ClusterRoleBindingMeta, clusterRoleBindingHandler)).NotTo(HaveOccurred()) clusterRoleHandler = &objecthandler{} @@ -804,7 +804,7 @@ var _ = Describe("test suite with svc and pod config(label selector and reverse svcStorage, _ = manager.GetTopoNodeStorage(ServiceMeta) Expect(svcStorage).NotTo(BeNil()) - ctx, cancel = context.WithCancel((context.Background())) + ctx, cancel = context.WithCancel(context.Background()) podHandler = &objecthandler{} Expect(manager.AddNodeHandler(PodMeta, podHandler)).To(Succeed()) svcHandler = &objecthandler{} @@ -1143,7 +1143,7 @@ var _ = Describe("test suite with deploy config(label selector and owner referen Expect(podStorage).NotTo(BeNil()) Expect(deployStorage).NotTo(BeNil()) - ctx, cancel = context.WithCancel((context.Background())) + ctx, cancel = context.WithCancel(context.Background()) podHandler = &objecthandler{} Expect(manager.AddNodeHandler(PodMeta, podHandler)).Should(Succeed()) replicasetHandler = &objecthandler{} @@ -1356,7 +1356,7 @@ var _ = Describe("test suite with deploy config(label selector and owner referen }) }) -var _ = Describe("test suite with relations update", func() { +var _ = Describe("test suite with labelRelations update", func() { var manager Manager var fakeClient *fake.Clientset var clusterRoleBindingHandler, clusterRoleHandler, saHandler *objecthandler @@ -1389,7 +1389,7 @@ var _ = Describe("test suite with relations update", func() { Expect(clusterroleStorage).NotTo(BeNil()) Expect(saStorage).NotTo(BeNil()) - ctx, cancel = context.WithCancel((context.Background())) + ctx, cancel = context.WithCancel(context.Background()) clusterRoleBindingHandler = &objecthandler{} Expect(manager.AddNodeHandler(ClusterRoleBindingMeta, clusterRoleBindingHandler)).NotTo(HaveOccurred()) clusterRoleHandler = &objecthandler{} @@ -1552,6 +1552,35 @@ var _ = Describe("test suite with relations update", func() { Expect(fakeClient.RbacV1().ClusterRoleBindings().Create(ctx, crb, metav1.CreateOptions{})).NotTo(BeNil()) syncStatus(checkAll) }) + It("create and update clusterRoleBinding with objects not existed", func() { + ns := "testclusterresource" + crbName := "crbtest" + crName := "crName" + saName := "saName" + saName2 := "saName2" + + clusterRoleBindingHandler.addCallExpected() + crb := newClusterRoleBinding(crbName, crName, + []types.NamespacedName{{Name: saName, Namespace: ns}}) + Expect(fakeClient.RbacV1().ClusterRoleBindings().Create(ctx, crb, metav1.CreateOptions{})).NotTo(BeNil()) + syncStatus(checkAll) + + clusterRoleBindingHandler.updateCallExpected() + crb = newClusterRoleBinding(crbName, crName, + []types.NamespacedName{{Name: saName, Namespace: ns}, {Name: saName2, Namespace: ns}}) + Expect(fakeClient.RbacV1().ClusterRoleBindings().Update(ctx, crb, metav1.UpdateOptions{})).NotTo(BeNil()) + syncStatus(checkAll) + + clusterRoleBindingHandler.updateCallExpected() + crb = newClusterRoleBinding(crbName, crName, []types.NamespacedName{{Name: saName2, Namespace: ns}}) + Expect(fakeClient.RbacV1().ClusterRoleBindings().Update(ctx, crb, metav1.UpdateOptions{})).NotTo(BeNil()) + syncStatus(checkAll) + + clusterRoleBindingHandler.updateCallExpected() + crb = newClusterRoleBinding(crbName, crName, []types.NamespacedName{}) + Expect(fakeClient.RbacV1().ClusterRoleBindings().Update(ctx, crb, metav1.UpdateOptions{})).NotTo(BeNil()) + syncStatus(checkAll) + }) }) var _ = Describe("test suite with mock relation for fed namespaces and local cluster pods", func() { @@ -1579,7 +1608,7 @@ var _ = Describe("test suite with mock relation for fed namespaces and local clu nsStorage, _ = manager.GetTopoNodeStorage(NamespaceMeta) Expect(nsStorage).NotTo(BeNil()) - ctx, cancel = context.WithCancel((context.Background())) + ctx, cancel = context.WithCancel(context.Background()) nsHandler = &objecthandler{} Expect(manager.AddNodeHandler(NamespaceMeta, nsHandler)).NotTo(HaveOccurred()) @@ -1780,7 +1809,7 @@ var _ = Describe("test suite for multi routine", func() { Expect(deployStorage).NotTo(BeNil()) Expect(inspectDeployStorage).NotTo(BeNil()) - ctx, cancel = context.WithCancel((context.Background())) + ctx, cancel = context.WithCancel(context.Background()) podHandler = &objecthandler{needRangePostOrder: true, needRangePreOrder: true} Expect(manager.AddNodeHandler(PodMeta, podHandler)).Should(Succeed()) replicasetHandler = &objecthandler{needRangePostOrder: true, needRangePreOrder: true} diff --git a/resourcetopo/types.go b/resourcetopo/types.go index 3fb8389c..af8741f5 100644 --- a/resourcetopo/types.go +++ b/resourcetopo/types.go @@ -159,6 +159,7 @@ type ResourceRelation struct { DirectRefs []types.NamespacedName // LabelSelector offers a kubernetes label-selector way for svc-pod type relation, could be nil if it's empty. + // If Set, will ignore DirectRefs. LabelSelector *metav1.LabelSelector } diff --git a/resourcetopo/utils.go b/resourcetopo/utils.go index 04db9fdb..9d06f823 100644 --- a/resourcetopo/utils.go +++ b/resourcetopo/utils.go @@ -21,7 +21,6 @@ import ( "fmt" "strings" - "golang.org/x/exp/slices" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -108,7 +107,7 @@ func existInList(l *list.List, value interface{}) bool { return false } -func compareResourceRelation(a, b ResourceRelation) int { +func compareLabelResourceRelation(a, b ResourceRelation) int { var res int if res = strings.Compare(a.PostMeta.APIVersion, b.PostMeta.APIVersion); res != 0 { return res @@ -117,20 +116,6 @@ func compareResourceRelation(a, b ResourceRelation) int { return res } - if res = len(a.DirectRefs) - len(b.DirectRefs); res != 0 { - return res - } - slices.SortFunc(a.DirectRefs, compareNodeName) - slices.SortFunc(b.DirectRefs, compareNodeName) - for i := 0; i < len(a.DirectRefs); i++ { - if res = strings.Compare(a.DirectRefs[i].Namespace, b.DirectRefs[i].Namespace); res != 0 { - return res - } - if res = strings.Compare(a.DirectRefs[i].Name, b.DirectRefs[i].Name); res != 0 { - return res - } - } - if res = strings.Compare(a.Cluster, b.Cluster); res != 0 { return res } diff --git a/resourcetopo/utils_test.go b/resourcetopo/utils_test.go index 372ba099..2c6e996c 100644 --- a/resourcetopo/utils_test.go +++ b/resourcetopo/utils_test.go @@ -119,8 +119,8 @@ func Test_compareResourceRelation(t *testing.T) { }, { - name: "direct ref size not equal", - want: -2, + name: "direct ref size not equal, but ignore diff", + want: 0, argsA: ResourceRelation{ PostMeta: PodMeta, DirectRefs: []types.NamespacedName{ @@ -149,8 +149,8 @@ func Test_compareResourceRelation(t *testing.T) { }, }, { - name: "direct ref not equal", - want: 1, + name: "direct ref size not equal, but ignore diff", + want: 0, argsA: ResourceRelation{ PostMeta: PodMeta, DirectRefs: []types.NamespacedName{ @@ -173,8 +173,8 @@ func Test_compareResourceRelation(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := compareResourceRelation(tt.argsA, tt.argsB); got != tt.want { - t.Errorf("compareResourceRelation() = %v, want %v", got, tt.want) + if got := compareLabelResourceRelation(tt.argsA, tt.argsB); got != tt.want { + t.Errorf("compareLabelResourceRelation() = %v, want %v", got, tt.want) } }) }