diff --git a/pkg/models/resources/v1alpha2/persistentvolumeclaim/persistentvolumeclaims.go b/pkg/models/resources/v1alpha2/persistentvolumeclaim/persistentvolumeclaims.go index 53b51461a..210f5d962 100644 --- a/pkg/models/resources/v1alpha2/persistentvolumeclaim/persistentvolumeclaims.go +++ b/pkg/models/resources/v1alpha2/persistentvolumeclaim/persistentvolumeclaims.go @@ -123,6 +123,7 @@ func (s *persistentVolumeClaimSearcher) Search(namespace string, conditions *par r := make([]interface{}, 0) for _, i := range result { + i = i.DeepCopy() inUse := s.countPods(i.Name, i.Namespace) isSnapshotAllow := s.isSnapshotAllowed(i.GetAnnotations()["volume.beta.kubernetes.io/storage-provisioner"]) if i.Annotations == nil { diff --git a/pkg/models/resources/v1alpha2/storageclass/storageclasses.go b/pkg/models/resources/v1alpha2/storageclass/storageclasses.go index 6ef1bb720..357648f65 100644 --- a/pkg/models/resources/v1alpha2/storageclass/storageclasses.go +++ b/pkg/models/resources/v1alpha2/storageclass/storageclasses.go @@ -91,6 +91,7 @@ func (s *storageClassesSearcher) Search(namespace string, conditions *params.Con r := make([]interface{}, 0) for _, i := range result { + i = i.DeepCopy() count := s.countPersistentVolumeClaims(i.Name) if i.Annotations == nil { i.Annotations = make(map[string]string) diff --git a/pkg/models/resources/v1alpha2/storageclass/storageclasses_test.go b/pkg/models/resources/v1alpha2/storageclass/storageclasses_test.go index 5c85c2d1e..50295faad 100644 --- a/pkg/models/resources/v1alpha2/storageclass/storageclasses_test.go +++ b/pkg/models/resources/v1alpha2/storageclass/storageclasses_test.go @@ -22,8 +22,19 @@ var ( }, } + sc1Expected = &v1.StorageClass{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sc1", + Annotations: map[string]string{ + "kubesphere.io/pvc-count": "1", + }, + }, + } + scs = []interface{}{sc1} + scsExpected = []interface{}{sc1Expected} + pvc1 = &corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ Name: "pvc1", @@ -85,7 +96,7 @@ func TestSearch(t *testing.T) { conditions: ¶ms.Conditions{}, orderBy: "name", reverse: true, - expected: scs, + expected: scsExpected, expectedErr: nil, }, } diff --git a/pkg/models/resources/v1alpha3/node/nodes.go b/pkg/models/resources/v1alpha3/node/nodes.go index 7ca837ca0..6e9de6fdc 100644 --- a/pkg/models/resources/v1alpha3/node/nodes.go +++ b/pkg/models/resources/v1alpha3/node/nodes.go @@ -19,8 +19,6 @@ package node import ( "fmt" "sort" - "sync" - "time" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -36,7 +34,6 @@ import ( // Those annotations were added to node only for display purposes const ( - nodeAnnotatedAt = "node.kubesphere.io/last-annotated-at" nodeCPURequests = "node.kubesphere.io/cpu-requests" nodeMemoryRequests = "node.kubesphere.io/memory-requests" nodeCPULimits = "node.kubesphere.io/cpu-limits" @@ -54,13 +51,11 @@ const ( type nodesGetter struct { informers informers.SharedInformerFactory - mutex sync.Mutex } func New(informers informers.SharedInformerFactory) v1alpha3.Interface { return &nodesGetter{ informers: informers, - mutex: sync.Mutex{}, } } @@ -72,6 +67,20 @@ func (c *nodesGetter) Get(_, name string) (runtime.Object, error) { // ignore the error, skip annotating process if error happened pods, _ := c.informers.Core().V1().Pods().Lister().Pods("").List(labels.Everything()) + + // Never mutate original objects! + // Caches are shared across controllers, + // this means that if you mutate your "copy" (actually a reference or shallow copy) of an object, + // you'll mess up other controllers (not just your own). + // Also, if the mutated field is a map, + // a "concurrent map (read &) write" panic might occur, + // causing the ks-apiserver to crash. + // Refer: + // https://github.com/kubesphere/kubesphere/issues/4357 + // https://github.com/kubesphere/kubesphere/issues/3469 + // https://github.com/kubesphere/kubesphere/pull/4599 + // https://github.com/kubernetes/community/blob/master/contributors/devel/sig-api-machinery/controllers.md + node = node.DeepCopy() c.annotateNode(node, pods) return node, nil @@ -124,6 +133,7 @@ func (c *nodesGetter) List(_ string, q *query.Query) (*api.ListResult, error) { var result = make([]interface{}, 0) for _, node := range selectedNodes { + node = node.DeepCopy() c.annotateNode(node, nonTerminatedPodsList) result = append(result, node) } @@ -162,25 +172,13 @@ func (c *nodesGetter) filter(object runtime.Object, filter query.Filter) bool { } // annotateNode adds cpu/memory requests usage data to node's annotations -// this operation is resource consuming, so avoid annotating on every query +// this operation mutates the *v1.Node passed in +// so DO A DEEPCOPY before calling func (c *nodesGetter) annotateNode(node *v1.Node, pods []*v1.Pod) { - // only annotate node when two consecutive annotating gap bigger than 30s - c.mutex.Lock() if node.Annotations == nil { node.Annotations = make(map[string]string) } - if lastAnnotatedAt, ok := node.Annotations[nodeAnnotatedAt]; ok { - if lastAnnotationTimeStamp, err := time.Parse(time.RFC3339, lastAnnotatedAt); err != nil { - if lastAnnotationTimeStamp.Add(30 * time.Second).After(time.Now()) { - c.mutex.Unlock() - return - } - } - } - node.Annotations[nodeAnnotatedAt] = time.Now().Format(time.RFC3339) - c.mutex.Unlock() - if len(pods) == 0 { return } diff --git a/pkg/models/resources/v1alpha3/node/nodes_test.go b/pkg/models/resources/v1alpha3/node/nodes_test.go index 6464f30e0..92be399e1 100644 --- a/pkg/models/resources/v1alpha3/node/nodes_test.go +++ b/pkg/models/resources/v1alpha3/node/nodes_test.go @@ -145,8 +145,6 @@ func TestNodesGetterGet(t *testing.T) { } nodeGot := got.(*corev1.Node) - // ignore last-annotated-at annotation - delete(nodeGot.Annotations, nodeAnnotatedAt) if diff := cmp.Diff(nodeGot.Annotations, expectedAnnotations); len(diff) != 0 { t.Errorf("%T, diff(-got, +expected), %v", expectedAnnotations, nodeGot.Annotations) } @@ -170,7 +168,7 @@ func TestListNodes(t *testing.T) { }, &api.ListResult{ Items: []interface{}{ - node2, + node2Expected, }, TotalItems: 1, }, @@ -187,7 +185,7 @@ func TestListNodes(t *testing.T) { }, &api.ListResult{ Items: []interface{}{ - node1, + node1Expected, }, TotalItems: 1, }, @@ -204,7 +202,7 @@ func TestListNodes(t *testing.T) { }, &api.ListResult{ Items: []interface{}{ - node2, + node2Expected, }, TotalItems: 1, }, @@ -239,6 +237,17 @@ var ( Unschedulable: true, }, } + + node1Expected = &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + Annotations: map[string]string{}, + }, + Spec: corev1.NodeSpec{ + Unschedulable: true, + }, + } + node2 = &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ Name: "node2", @@ -247,6 +256,17 @@ var ( Unschedulable: false, }, } + + node2Expected = &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node2", + Annotations: map[string]string{}, + }, + Spec: corev1.NodeSpec{ + Unschedulable: false, + }, + } + nodes = []*corev1.Node{node1, node2} ) diff --git a/pkg/models/resources/v1alpha3/persistentvolumeclaim/persistentvolumeclaim.go b/pkg/models/resources/v1alpha3/persistentvolumeclaim/persistentvolumeclaim.go index 02d27e4b4..7c7993f06 100644 --- a/pkg/models/resources/v1alpha3/persistentvolumeclaim/persistentvolumeclaim.go +++ b/pkg/models/resources/v1alpha3/persistentvolumeclaim/persistentvolumeclaim.go @@ -53,6 +53,8 @@ func (p *persistentVolumeClaimGetter) Get(namespace, name string) (runtime.Objec if err != nil { return pvc, err } + // we should never mutate the shared objects from informers + pvc = pvc.DeepCopy() p.annotatePVC(pvc) return pvc, nil } @@ -65,6 +67,7 @@ func (p *persistentVolumeClaimGetter) List(namespace string, query *query.Query) var result []runtime.Object for _, pvc := range all { + pvc = pvc.DeepCopy() p.annotatePVC(pvc) result = append(result, pvc) } diff --git a/pkg/models/resources/v1alpha3/persistentvolumeclaim/persistentvolumeclaim_test.go b/pkg/models/resources/v1alpha3/persistentvolumeclaim/persistentvolumeclaim_test.go index 6d9c8d321..be6ea1486 100644 --- a/pkg/models/resources/v1alpha3/persistentvolumeclaim/persistentvolumeclaim_test.go +++ b/pkg/models/resources/v1alpha3/persistentvolumeclaim/persistentvolumeclaim_test.go @@ -37,7 +37,7 @@ var ( testStorageClassName = "test-csi" ) -func TestListPods(t *testing.T) { +func TestListPVCs(t *testing.T) { tests := []struct { description string namespace string @@ -58,7 +58,7 @@ func TestListPods(t *testing.T) { Filters: map[query.Field]query.Value{query.FieldNamespace: query.Value("default")}, }, &api.ListResult{ - Items: []interface{}{pvc3, pvc2, pvc1}, + Items: []interface{}{pvc3Expected, pvc2Expected, pvc1Expected}, TotalItems: len(persistentVolumeClaims), }, nil, @@ -79,7 +79,7 @@ func TestListPods(t *testing.T) { }, }, &api.ListResult{ - Items: []interface{}{pvc1}, + Items: []interface{}{pvc1Expected}, TotalItems: 1, }, nil, @@ -100,7 +100,7 @@ func TestListPods(t *testing.T) { }, }, &api.ListResult{ - Items: []interface{}{pvcGet2}, + Items: []interface{}{pvc2Expected}, TotalItems: 1, }, nil, @@ -121,7 +121,7 @@ func TestListPods(t *testing.T) { }, }, &api.ListResult{ - Items: []interface{}{pvcGet3}, + Items: []interface{}{pvc3Expected}, TotalItems: 1, }, nil, @@ -154,6 +154,21 @@ var ( Phase: corev1.ClaimPending, }, } + + pvc1Expected = &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvc-1", + Namespace: "default", + Annotations: map[string]string{ + annotationInUse: "false", + annotationAllowSnapshot: "false", + }, + }, + Status: corev1.PersistentVolumeClaimStatus{ + Phase: corev1.ClaimPending, + }, + } + pvc2 = &corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ Name: "pvc-2", @@ -167,7 +182,7 @@ var ( }, } - pvcGet2 = &corev1.PersistentVolumeClaim{ + pvc2Expected = &corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ Name: "pvc-2", Namespace: "default", @@ -189,7 +204,7 @@ var ( }, } - pvcGet3 = &corev1.PersistentVolumeClaim{ + pvc3Expected = &corev1.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ Name: "pvc-3", Namespace: "default",