Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve memory usage of Istio manager #852

Merged
merged 11 commits into from
Jun 3, 2024
+39 −23
Merged

Improve memory usage of Istio manager #852

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5567e43
Use distinct path for Istio telemetries
barchw May 20, 2024
760b13e
Remove deepcopy from gathering pods for restart
barchw May 29, 2024
006cb1f
Revert unneded
barchw May 29, 2024
110efb2
Remove caching for objects
barchw Jun 3, 2024
4b2445f
Post review
barchw Jun 3, 2024
792d60d
Use distinct path for Istio telemetries
barchw May 20, 2024
e4f4cbf
Remove deepcopy from gathering pods for restart
barchw May 29, 2024
2ecdfb5
Revert unneded
barchw May 29, 2024
5b26c43
Remove caching for objects
barchw Jun 3, 2024
fd65944
Post review
barchw Jun 3, 2024
565bb7e
Merge branch 'remove-deepcopy' of https://github.com/barchw/istio int…
barchw Jun 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ package main

import (
"flag"
v1 "k8s.io/api/apps/v1"
"os"
"sigs.k8s.io/controller-runtime/pkg/client"
"time"

networkingv1alpha3 "istio.io/client-go/pkg/apis/networking/v1alpha3"
Expand Down Expand Up @@ -107,6 +109,16 @@ func main() {
HealthProbeBindAddress: flagVar.probeAddr,
LeaderElection: flagVar.enableLeaderElection,
LeaderElectionID: "76223278.kyma-project.io",
Client: client.Options{
Cache: &client.CacheOptions{
DisableFor: []client.Object{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a comment that explains why we disable the cache for this types?

&v1.DaemonSet{},
&v1.Deployment{},
&v1.StatefulSet{},
&v1.ReplicaSet{},
},
},
},
})
if err != nil {
setupLog.Error(err, "unable to start manager")
Expand Down
16 changes: 8 additions & 8 deletions pkg/lib/sidecars/pods/get.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,32 +51,32 @@ func getAllRunningPods(ctx context.Context, c client.Client) (*v1.PodList, error
return podList, nil
}

func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) (outputPodsList v1.PodList, err error) {
func GetPodsToRestart(ctx context.Context, c client.Client, expectedImage SidecarImage, expectedResources v1.ResourceRequirements, predicates []filter.SidecarProxyPredicate, logger *logr.Logger) (outputPodsList *v1.PodList, err error) {
podList, err := getAllRunningPods(ctx, c)
if err != nil {
return outputPodsList, err
return nil, err
}

podList.DeepCopyInto(&outputPodsList)
outputPodsList.Items = []v1.Pod{}

//Add predicate for image version and resources configuration
predicates = append(predicates, NewRestartProxyPredicate(expectedImage, expectedResources))

for _, predicate := range predicates {
evaluator, err := predicate.NewProxyRestartEvaluator(ctx)
if err != nil {
return v1.PodList{}, err
return &v1.PodList{}, err
}

outputPodsList = &v1.PodList{}
for _, pod := range podList.Items {
if evaluator.RequiresProxyRestart(pod) {
outputPodsList.Items = append(outputPodsList.Items, *pod.DeepCopy())
outputPodsList.Items = append(outputPodsList.Items, pod)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a micro-optimization since the pod objects are not that big, but I think we will create multiple backing arrays, since the capacity of the slices will increase multiple times. That will lead to multiple arrays being kept in memory until the outputPodList isn't referenced any longer and can be garbage collected.
We could initialize the outputPodsList.Items with a capacity derived from podList.Items length.

}
}
}

logger.Info("Pods to restart", "number of pods", len(outputPodsList.Items))
if outputPodsList != nil {
logger.Info("Pods to restart", "number of pods", len(outputPodsList.Items))
}
return outputPodsList, nil
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/lib/sidecars/remove/remove.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ func RemoveSidecars(ctx context.Context, k8sclient client.Client, logger *logr.L
return nil, err
}

return restart.Restart(ctx, k8sclient, *toRestart, logger)
return restart.Restart(ctx, k8sclient, toRestart, logger)
}
2 changes: 1 addition & 1 deletion pkg/lib/sidecars/restart/restart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func newRestartWarning(o actionObject, message string) RestartWarning {
}
}

func Restart(ctx context.Context, c client.Client, podList v1.PodList, logger *logr.Logger) ([]RestartWarning, error) {
func Restart(ctx context.Context, c client.Client, podList *v1.PodList, logger *logr.Logger) ([]RestartWarning, error) {
warnings := make([]RestartWarning, 0)
processedActionObjects := make(map[string]bool)

Expand Down
26 changes: 13 additions & 13 deletions pkg/lib/sidecars/restart/restart_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ var _ = Describe("Restart Pods", func() {
}

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand All @@ -67,7 +67,7 @@ var _ = Describe("Restart Pods", func() {
}

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand All @@ -88,7 +88,7 @@ var _ = Describe("Restart Pods", func() {
}

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand All @@ -109,7 +109,7 @@ var _ = Describe("Restart Pods", func() {
}

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand All @@ -134,7 +134,7 @@ var _ = Describe("Restart Pods", func() {
}

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand All @@ -158,7 +158,7 @@ var _ = Describe("Restart Pods", func() {
}

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand All @@ -181,7 +181,7 @@ var _ = Describe("Restart Pods", func() {
}

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand All @@ -204,7 +204,7 @@ var _ = Describe("Restart Pods", func() {
}

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand All @@ -230,7 +230,7 @@ var _ = Describe("Restart Pods", func() {
}

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand All @@ -256,7 +256,7 @@ var _ = Describe("Restart Pods", func() {
c := fakeClient(&pod)

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -291,7 +291,7 @@ var _ = Describe("Restart Pods", func() {
}})

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand All @@ -316,7 +316,7 @@ var _ = Describe("Restart Pods", func() {
}

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand Down Expand Up @@ -351,7 +351,7 @@ var _ = Describe("Restart Pods", func() {
}})

// when
warnings, err := restart.Restart(ctx, c, podList, &logger)
warnings, err := restart.Restart(ctx, c, &podList, &logger)

// then
Expect(err).NotTo(HaveOccurred())
Expand Down
8 changes: 4 additions & 4 deletions tests/ui/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion tests/ui/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"start-k3d": "./scripts/k3d-local-dev.sh"
},
"devDependencies": {
"cypress": "13.9.0",
"cypress": "13.10.0",
"cypress-file-upload": "5.0.8",
"js-yaml": "4.1.0",
"typescript": "5.4.5",
Expand Down
Loading