From 3fb1ab2fdd5898e5f306e6774e86e5d738ffd6bd Mon Sep 17 00:00:00 2001 From: Krisztian Litkey Date: Wed, 4 Dec 2024 13:25:36 +0200 Subject: [PATCH] agent,resmgr: merge PodResources{List,Map}, cache list. Merge PodResourceMap into PodResourcesList. Avoid unnecessary re-listing of pod resources by caching the result of the last list and trying to reuse it to look up resources. Signed-off-by: Krisztian Litkey --- pkg/agent/pod-resource-api.go | 6 +- pkg/agent/podresapi/client.go | 11 +- pkg/agent/podresapi/resources.go | 76 +++--- pkg/agent/podresapi/resources_test.go | 318 ++++++++++++++++++++++++++ pkg/resmgr/cache/cache.go | 9 +- 5 files changed, 384 insertions(+), 36 deletions(-) create mode 100644 pkg/agent/podresapi/resources_test.go diff --git a/pkg/agent/pod-resource-api.go b/pkg/agent/pod-resource-api.go index 83a7dc4ec..15883f3cb 100644 --- a/pkg/agent/pod-resource-api.go +++ b/pkg/agent/pod-resource-api.go @@ -51,7 +51,7 @@ func (a *Agent) GoGetPodResources(ns, pod string, timeout time.Duration) <-chan } // ListPodResources lists all pods' resources. -func (a *Agent) ListPodResources(timeout time.Duration) (podresapi.PodResourcesList, error) { +func (a *Agent) ListPodResources(timeout time.Duration) (*podresapi.PodResourcesList, error) { ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() @@ -59,12 +59,12 @@ func (a *Agent) ListPodResources(timeout time.Duration) (podresapi.PodResourcesL } // GoListPodResources lists all pods' resources asynchronously. -func (a *Agent) GoListPodResources(timeout time.Duration) <-chan podresapi.PodResourcesList { +func (a *Agent) GoListPodResources(timeout time.Duration) <-chan *podresapi.PodResourcesList { if !a.podResCli.HasClient() { return nil } - ch := make(chan podresapi.PodResourcesList, 1) + ch := make(chan *podresapi.PodResourcesList, 1) go func() { defer close(ch) diff --git a/pkg/agent/podresapi/client.go b/pkg/agent/podresapi/client.go index fc946cbba..777f63ae6 100644 --- a/pkg/agent/podresapi/client.go +++ b/pkg/agent/podresapi/client.go @@ -35,6 +35,7 @@ type Client struct { conn *grpc.ClientConn cli api.PodResourcesListerClient noGet bool + cached *PodResourcesList } const ( @@ -105,7 +106,7 @@ func (c *Client) HasClient() bool { } // List lists all pods' resources. -func (c *Client) List(ctx context.Context) (PodResourcesList, error) { +func (c *Client) List(ctx context.Context) (*PodResourcesList, error) { if !c.HasClient() { return nil, nil } @@ -115,7 +116,9 @@ func (c *Client) List(ctx context.Context) (PodResourcesList, error) { return nil, fmt.Errorf("failed to get pod resources by list: %w", err) } - return PodResourcesList(reply.GetPodResources()), nil + c.cached = NewPodResourcesList(reply.GetPodResources()) + + return c.cached, nil } // Get queries the given pod's resources. @@ -144,6 +147,10 @@ func (c *Client) Get(ctx context.Context, namespace, pod string) (*PodResources, c.noGet = true } + if r := c.cached.GetPodResources(namespace, pod); r != nil { + return r, nil + } + l, err := c.List(ctx) if err != nil { return nil, fmt.Errorf("failed to get pod resources: %w", err) diff --git a/pkg/agent/podresapi/resources.go b/pkg/agent/podresapi/resources.go index 03c6b47fd..a624b1061 100644 --- a/pkg/agent/podresapi/resources.go +++ b/pkg/agent/podresapi/resources.go @@ -32,11 +32,11 @@ type ContainerResources struct { *api.ContainerResources } -// PodResourcesList is a list of PodResources. -type PodResourcesList []*api.PodResources - -// PodResourceMap is a map representation of PodResourcesList. -type PodResourcesMap map[string]map[string]*PodResources +// PodResourcesList containers the result of a pod resources list query. +type PodResourcesList struct { + l []*api.PodResources + m map[string]map[string]*PodResources +} // GetContainer returns resources for the given container. func (p *PodResources) GetContainer(ctr string) *ContainerResources { @@ -53,41 +53,65 @@ func (p *PodResources) GetContainer(ctr string) *ContainerResources { return nil } -// GetPodResources returns resources for the given pod. -func (l PodResourcesList) GetPodResources(ns, pod string) *PodResources { - for _, p := range l { - if p.GetNamespace() == ns && p.GetName() == pod { - return &PodResources{p} - } +func NewPodResourcesList(l []*api.PodResources) *PodResourcesList { + return &PodResourcesList{ + l: l, + m: make(map[string]map[string]*PodResources), } +} - return nil +func (l *PodResourcesList) Len() int { + if l == nil { + return 0 + } + + cnt := len(l.l) + for _, m := range l.m { + cnt += len(m) + } + + return cnt } -// Map returns a PodResourcesMap for the pod resources list. -func (l PodResourcesList) Map() PodResourcesMap { - m := make(PodResourcesMap) +// GetPodResources returns resources for the given pod. +func (l *PodResourcesList) GetPodResources(ns, pod string) *PodResources { + if l == nil { + return nil + } + + if p, ok := l.m[ns][pod]; ok { + return p + } + + for i, p := range l.l { + var ( + podNs = p.GetNamespace() + podName = p.GetName() + ) - for _, p := range l { - podMap, ok := m[p.GetNamespace()] + podMap, ok := l.m[podNs] if !ok { podMap = make(map[string]*PodResources) - m[p.GetNamespace()] = podMap + l.m[podNs] = podMap + } + + r := &PodResources{p} + podMap[podName] = r + + if podNs == ns && podName == pod { + l.l = l.l[i+1:] + return r } - podMap[p.GetName()] = &PodResources{p} } - return m -} + l.l = nil -// GetPod returns resources for the given pod. -func (m PodResourcesMap) GetPod(ns, pod string) *PodResources { - return m[ns][pod] + return nil } // GetContainer returns resources for the given container. -func (m PodResourcesMap) GetContainer(ns, pod, ctr string) *ContainerResources { - return m.GetPod(ns, pod).GetContainer(ctr) +func (l *PodResourcesList) GetContainer(ns, pod, ctr string) *ContainerResources { + return l.GetPodResources(ns, pod).GetContainer(ctr) } // GetDeviceTopologyHints returns topology hints for the given container. checkDenied diff --git a/pkg/agent/podresapi/resources_test.go b/pkg/agent/podresapi/resources_test.go new file mode 100644 index 000000000..d042e6a03 --- /dev/null +++ b/pkg/agent/podresapi/resources_test.go @@ -0,0 +1,318 @@ +// Copyright The NRI Plugins Authors. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package podresapi_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + api "k8s.io/kubelet/pkg/apis/podresources/v1" + + . "github.com/containers/nri-plugins/pkg/agent/podresapi" +) + +func TestGetContainer(t *testing.T) { + type testCase struct { + name string + podResources *api.PodResources + containerName string + expectedContainer *ContainerResources + } + + for _, tc := range []*testCase{ + { + name: "no containers", + podResources: &api.PodResources{}, + containerName: "test", + expectedContainer: nil, + }, + { + name: "container not found", + podResources: &api.PodResources{ + Containers: []*api.ContainerResources{ + { + Name: "test1", + }, + }, + }, + containerName: "test", + expectedContainer: nil, + }, + { + name: "the only container found", + podResources: &api.PodResources{ + Containers: []*api.ContainerResources{ + { + Name: "test", + }, + }, + }, + containerName: "test", + expectedContainer: &ContainerResources{ + &api.ContainerResources{ + Name: "test", + }, + }, + }, + { + name: "one of many containers found", + podResources: &api.PodResources{ + Containers: []*api.ContainerResources{ + { + Name: "test1", + }, + { + Name: "test2", + }, + { + Name: "test3", + }, + }, + }, + containerName: "test2", + expectedContainer: &ContainerResources{ + &api.ContainerResources{ + Name: "test2", + }, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + p := &PodResources{tc.podResources} + + ct := p.GetContainer(tc.containerName) + require.Equal(t, tc.expectedContainer, ct) + }) + } +} + +func TestPodResourcesList(t *testing.T) { + type lookup struct { + Namespace string + Pod string + } + type testCase struct { + name string + podResources []*api.PodResources + lookup []*lookup + expect []*PodResources + } + + for _, tc := range []*testCase{ + { + name: "no pod resources", + podResources: []*api.PodResources{}, + lookup: []*lookup{ + { + Namespace: "test", + Pod: "pod1", + }, + }, + expect: []*PodResources{ + nil, + }, + }, + { + name: "pod not found", + podResources: []*api.PodResources{ + { + Namespace: "test", + Name: "pod1", + }, + }, + lookup: []*lookup{ + { + Namespace: "test", + Pod: "pod2", + }, + }, + expect: []*PodResources{ + nil, + }, + }, + { + name: "the only pod found", + podResources: []*api.PodResources{ + { + Namespace: "test", + Name: "pod1", + }, + }, + lookup: []*lookup{ + { + Namespace: "test", + Pod: "pod1", + }, + }, + expect: []*PodResources{ + { + &api.PodResources{ + Namespace: "test", + Name: "pod1", + }, + }, + }, + }, + { + name: "one of many pods found", + podResources: []*api.PodResources{ + { + Namespace: "test", + Name: "pod1", + }, + { + Namespace: "test", + Name: "pod2", + }, + { + Namespace: "test", + Name: "pod3", + }, + }, + lookup: []*lookup{ + { + Namespace: "test", + Pod: "pod2", + }, + }, + expect: []*PodResources{ + { + &api.PodResources{ + Namespace: "test", + Name: "pod2", + }, + }, + }, + }, + { + name: "all of many pods found", + podResources: []*api.PodResources{ + { + Namespace: "test1", + Name: "pod1", + }, + { + Namespace: "test1", + Name: "pod2", + }, + { + Namespace: "test1", + Name: "pod3", + }, + { + Namespace: "test2", + Name: "pod4", + }, + { + Namespace: "test3", + Name: "pod5", + }, + { + Namespace: "test3", + Name: "pod6", + }, + { + Namespace: "test1", + Name: "pod7", + }, + }, + lookup: []*lookup{ + { + Namespace: "test3", + Pod: "pod5", + }, + { + Namespace: "test1", + Pod: "pod7", + }, + { + Namespace: "test3", + Pod: "pod6", + }, + { + Namespace: "test2", + Pod: "pod4", + }, + { + Namespace: "test1", + Pod: "pod3", + }, + + { + Namespace: "test1", + Pod: "pod2", + }, + + { + Namespace: "test1", + Pod: "pod1", + }, + }, + expect: []*PodResources{ + { + &api.PodResources{ + Namespace: "test3", + Name: "pod5", + }, + }, + { + &api.PodResources{ + Namespace: "test1", + Name: "pod7", + }, + }, + { + &api.PodResources{ + Namespace: "test3", + Name: "pod6", + }, + }, + { + &api.PodResources{ + Namespace: "test2", + Name: "pod4", + }, + }, + { + &api.PodResources{ + Namespace: "test1", + Name: "pod3", + }, + }, + { + &api.PodResources{ + Namespace: "test1", + Name: "pod2", + }, + }, + { + &api.PodResources{ + Namespace: "test1", + Name: "pod1", + }, + }, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + r := NewPodResourcesList(tc.podResources) + + for i, l := range tc.lookup { + p := r.GetPodResources(l.Namespace, l.Pod) + require.Equal(t, tc.expect[i], p) + } + }) + } +} diff --git a/pkg/resmgr/cache/cache.go b/pkg/resmgr/cache/cache.go index cee39af62..e32ff29e6 100644 --- a/pkg/resmgr/cache/cache.go +++ b/pkg/resmgr/cache/cache.go @@ -423,7 +423,7 @@ type Cache interface { Save() error // RefreshPods purges/inserts stale/new pods/containers using a pod sandbox list response. - RefreshPods([]*nri.PodSandbox, <-chan podresapi.PodResourcesList) ([]Pod, []Pod, []Container) + RefreshPods([]*nri.PodSandbox, <-chan *podresapi.PodResourcesList) ([]Pod, []Pod, []Container) // RefreshContainers purges/inserts stale/new containers using a container list response. RefreshContainers([]*nri.Container) ([]Container, []Container) @@ -641,7 +641,7 @@ func (cch *cache) LookupContainerByCgroup(path string) (Container, bool) { } // RefreshPods purges/inserts stale/new pods/containers into the cache. -func (cch *cache) RefreshPods(pods []*nri.PodSandbox, resCh <-chan podresapi.PodResourcesList) ([]Pod, []Pod, []Container) { +func (cch *cache) RefreshPods(pods []*nri.PodSandbox, resCh <-chan *podresapi.PodResourcesList) ([]Pod, []Pod, []Container) { valid := make(map[string]struct{}) add := []Pod{} @@ -673,10 +673,9 @@ func (cch *cache) RefreshPods(pods []*nri.PodSandbox, resCh <-chan podresapi.Pod if resCh != nil { podResList := <-resCh - if len(podResList) > 0 { - podResMap := podResList.Map() + if podResList.Len() > 0 { for _, pod := range cch.Pods { - pod.setPodResources(podResMap.GetPod(pod.GetNamespace(), pod.GetName())) + pod.setPodResources(podResList.GetPodResources(pod.GetNamespace(), pod.GetName())) } } }