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

performance: optimize memory usage #94

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

performance: optimize memory usage #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module sigs.k8s.io/kube-network-policies

go 1.22.0
go 1.23.0

require (
github.com/florianl/go-nfqueue v1.3.2
Expand Down
46 changes: 43 additions & 3 deletions pkg/networkpolicy/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,17 @@ package networkpolicy
import (
"context"
"fmt"
"runtime"
"strings"
"time"
"unique"

nfqueue "github.com/florianl/go-nfqueue"
"github.com/mdlayher/netlink"

v1 "k8s.io/api/core/v1"
Copy link

Choose a reason for hiding this comment

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

I prefer it with the blank line there.

(But we should adopt a consistent style. Currently:

  • pkg/networkpolicy/controller.go has a blank line between the github and k8s imports
  • pkg/networkpolicy/metrics.go puts them together
  • cmd/main.go is a mess

)

Copy link
Contributor

Choose a reason for hiding this comment

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

do your magic :)

networkingv1 "k8s.io/api/networking/v1"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/sets"
Expand Down Expand Up @@ -173,6 +176,24 @@ func newController(client clientset.Interface,

// reduce memory usage only care about Labels and Status
trim := func(obj interface{}) (interface{}, error) {
if po, ok := obj.(*v1.Pod); ok {
ips := make([]v1.PodIP, 0, len(po.Status.PodIPs))
for _, i := range po.Status.PodIPs {
ips = append(ips, v1.PodIP{IP: intern(i.IP)})
Copy link

Choose a reason for hiding this comment

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

Pod IPs don't get reused very quickly... This probably doesn't help much...
(Is there a performance tradeoff between "memory saved by interning" vs "extra allocations used in the process of interning"?)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah this is definitely likely to be dropped when I try to minimize the diff here. I did too much, mostly because I was misreading some pprofs before I added the explicitly GC() step. I doubt this adds any value.

}
return &v1.Pod{
Copy link
Contributor

Choose a reason for hiding this comment

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

is not replacing in place cheaper?

Copy link

Choose a reason for hiding this comment

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

ah, apparently SetTransform is the exception to the normal "you can't modify objects from the informer cache" rule

Copy link
Contributor

Choose a reason for hiding this comment

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

it runs before goes to the informer cache IIRC

Copy link
Author

Choose a reason for hiding this comment

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

The reason I didn't replace in place (and made sure to copy every item entirely) is due to how Go will sometimes not GC a full struct if you hold a reference to something within the struct. https://stackoverflow.com/a/55018900

However, I doubt this applies here, or if it is it can be avoided in a simpler manner -- I was measuring things wrong so took an overkill approach. ill clean it up

ObjectMeta: metav1.ObjectMeta{
Name: intern(po.Name),
Namespace: intern(po.Namespace),
Labels: internm(po.Labels),
},
Spec: v1.PodSpec{Hostname: intern(po.Spec.Hostname), NodeName: intern(po.Spec.NodeName)},
Copy link

Choose a reason for hiding this comment

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

the code doesn't seem to use Hostname

Status: v1.PodStatus{
Phase: v1.PodPhase(intern(string(po.Status.Phase))),
PodIPs: ips,
},
Comment on lines +185 to +194
Copy link
Contributor

Choose a reason for hiding this comment

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

what I love to do is this server side, so you can trim the objects before sending them through the wire, there is a metadata informer only, but still need these values on Spec and Status

Copy link

Choose a reason for hiding this comment

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

to do is this server side, so you can trim the objects before sending them through the wire

long term, the most efficient thing would be to have a central controller that processes the Pods and distributes processed data about them to the per-node kube-network-policy processes (in the same way kcm processes Pods into EndpointSlices so kube-proxy only needs to watch the latter). Using an API object (possibly even EndpointSlice) would be one way to do that, but using a grpc API (like xDS) would probably be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I started that path, and have a branch with that, but I didn't like moving to a two components models ... xDS is cool but in this case this is a cache synchronization problem, so current informers we'll be enough for efficient data transmission and will remove the additional dependencies

Copy link
Author

Choose a reason for hiding this comment

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

One other thing you could do is to have, on the client side, a custom Pod type that is only the subset. While the apiserver will send the full pod, it will only end up as bytes and not deserialized into a Pod object. I did similar a while back with decent results (but it wasn't with Kubernetes; its likely a pain to do with client-go).

Then you probably don't need the GC thing either.

Obviously server side is best though

}, nil
}
if accessor, err := meta.Accessor(obj); err == nil {
accessor.SetManagedFields(nil)
}
Expand All @@ -183,9 +204,16 @@ func newController(client clientset.Interface,
return nil, err
}

pods := 0
// process only local Pods that are affected by network policices
_, _ = podInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
AddFunc: func(obj interface{}) {
_, _ = podInformer.Informer().AddEventHandler(cache.ResourceEventHandlerDetailedFuncs{
AddFunc: func(obj interface{}, isInitialList bool) {
if isInitialList {
pods++
if pods%200 == 0 {
runtime.GC()
Copy link
Contributor

Choose a reason for hiding this comment

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

this scares me

Copy link

Choose a reason for hiding this comment

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

it doesn't scare me but it seems kind of ugly and maybe would fit better in the the workqueue-processing code (eg processNextItem) rather than the informer itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe GOMEMLIMIT would be a more conventional approach?

}
}
pod := obj.(*v1.Pod)
if pod.Spec.NodeName != c.config.NodeName {
return
Expand Down Expand Up @@ -775,3 +803,15 @@ func (c *Controller) cleanNFTablesRules() {
klog.Infof("error deleting nftables rules %v", err)
}
}

func intern(s string) string {
return unique.Make(strings.Clone(s)).Value()
}

func internm(s map[string]string) map[string]string {
nm := make(map[string]string, len(s))
for k, v := range s {
nm[intern(k)] = intern(v)
}
return nm
}
Loading