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

Jump to bottom

Trim managedFields in controller-manager #118455

Merged

k8s-ci-robot merged 3 commits into kubernetes:master from linxiulei:managedFields

Aug 15, 2023

Contributor

linxiulei commented Jun 5, 2023 •

edited

Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Improve memory efficiency for kube-controller-manager

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Improve memory usage of kube-controller-manager by dropping the `.metadata.managedFields` field that kube-controller-manager doesn't require.

k8s-ci-robot added do-not-merge/work-in-progress release-note-none size/L kind/cleanup cncf-cla: yes do-not-merge/needs-sig needs-triage needs-ok-to-test labels

Contributor

k8s-ci-robot commented Jun 5, 2023

Hi @linxiulei. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added needs-priority area/code-generation sig/api-machinery sig/cloud-provider and removed do-not-merge/needs-sig labels

k8s-ci-robot requested review from andrewsykim and deads2k

June 5, 2023 09:19

Contributor Author

linxiulei commented Jun 5, 2023

/cc @wojtek-t @apelisse @alexzielenski

k8s-ci-robot requested review from alexzielenski, apelisse and wojtek-t

June 5, 2023 09:20

Member

apelisse commented Jun 5, 2023

cc @alexzielenski

Member

apelisse commented Jun 5, 2023

The PR description could be improved since a single word doesn't describe it properly. And somehow I don't see any change related to ManagedFields, nor how it fixes the linked issue? Am I missing something?

linxiulei changed the title ~~Managed fields~~ Trim managedFields in controller-manager

linxiulei commented

View reviewed changes

staging/src/k8s.io/controller-manager/pkg/informerfactory/informer_factory.go Outdated Show resolved Hide resolved

alexzielenski reviewed

View reviewed changes

staging/src/k8s.io/code-generator/cmd/informer-gen/generators/factory.go Outdated Show resolved Hide resolved

Contributor

nayihz commented Jun 6, 2023

should we modify every informer according on your way which we don't use managedFileld?

Contributor

alexzielenski commented Jun 6, 2023

/triage accepted

k8s-ci-robot added triage/accepted and removed needs-triage labels

linxiulei force-pushed the managedFields branch from 3cc8d8e to 2551f4a Compare

June 6, 2023 21:13

k8s-ci-robot added this to the v1.29 milestone

linxiulei mentioned this pull request

Trim managedFields in shared informer factory kubernetes/autoscaler#6159

Merged

dcbw added a commit to dcbw/ovn-kubernetes-1 that referenced this pull request


          factory: strip Managed Fields to reduce memory usage

583f9d0

We don't care about them, so why cache them and use a ton
of memory.

Inspired by kubernetes/kubernetes#118455

Signed-off-by: Dan Williams <dcbw@redhat.com>

dcbw pushed a commit to dcbw/ovn-kubernetes that referenced this pull request


          CARRY: Add 'WithTransform' SharedInformerOption

ac26e9d

kubernetes/kubernetes#118455

Kubernetes-commit: c86f562f29b6b7498ea962d2ac596e6d26ecd723
Kubernetes-commit: 9c4651bd0479b84f5e5913649207476717f3f13e

dcbw added a commit to dcbw/ovn-kubernetes that referenced this pull request


          factory: strip Managed Fields to reduce memory usage

93a9bac

We don't care about them, so why cache them and use a ton
of memory.

Inspired by kubernetes/kubernetes#118455

Signed-off-by: Dan Williams <dcbw@redhat.com>

dcbw added a commit to dcbw/ovn-kubernetes that referenced this pull request


          factory: strip Managed Fields to reduce memory usage

231847d

We don't care about them, so why cache them and use a ton
of memory.

Inspired by kubernetes/kubernetes#118455

Signed-off-by: Dan Williams <dcbw@redhat.com>

dcbw pushed a commit to dcbw/ovn-kubernetes that referenced this pull request


          CARRY: Add 'WithTransform' SharedInformerOption

4f50535

kubernetes/kubernetes#118455

Kubernetes-commit: c86f562f29b6b7498ea962d2ac596e6d26ecd723
Kubernetes-commit: 9c4651bd0479b84f5e5913649207476717f3f13e

Signed-off-by: Eric Lin <exlin@google.com>

dcbw added a commit to dcbw/ovn-kubernetes that referenced this pull request


          factory: strip Managed Fields to reduce memory usage

e7101f1

We don't care about them, so why cache them and use a ton
of memory.

Inspired by kubernetes/kubernetes#118455

Signed-off-by: Dan Williams <dcbw@redhat.com>

dcbw added a commit to dcbw/ovn-kubernetes that referenced this pull request


          factory: strip Managed Fields to reduce memory usage

d9a7732

We don't care about them, so why cache them and use a ton
of memory.

Inspired by kubernetes/kubernetes#118455

Signed-off-by: Dan Williams <dcbw@redhat.com>

dcbw added a commit to dcbw/ovn-kubernetes that referenced this pull request


          factory: strip Managed Fields to reduce memory usage

e17e8f2

We don't care about them, so why cache them and use a ton
of memory.

Inspired by kubernetes/kubernetes#118455

Signed-off-by: Dan Williams <dcbw@redhat.com>

dcbw added a commit to dcbw/ovn-kubernetes that referenced this pull request


          factory: strip Managed Fields and some Pod fields to reduce memory usage

da7d04e

We don't care about them, so why cache them and use a ton
of memory.

Inspired by kubernetes/kubernetes#118455

Signed-off-by: Dan Williams <dcbw@redhat.com>

dcbw pushed a commit to dcbw/ovn-kubernetes that referenced this pull request


          CARRY: Add 'WithTransform' SharedInformerOption

aa4b720

kubernetes/kubernetes#118455

Kubernetes-commit: c86f562f29b6b7498ea962d2ac596e6d26ecd723
Kubernetes-commit: 9c4651bd0479b84f5e5913649207476717f3f13e

Signed-off-by: Eric Lin <exlin@google.com>

dcbw added a commit to dcbw/ovn-kubernetes that referenced this pull request


          factory: strip Managed Fields and some Pod fields to reduce memory usage

7198fad

We don't care about them, so why cache them and use a ton
of memory.

Inspired by kubernetes/kubernetes#118455

Signed-off-by: Dan Williams <dcbw@redhat.com>

aojea mentioned this pull request

Add a hack to iptables kube-proxy to work around certain missed updates #122048

Closed

dcbw pushed a commit to dcbw/ovn-kubernetes-1 that referenced this pull request


          CARRY: Add 'WithTransform' SharedInformerOption

5ab766a

kubernetes/kubernetes#118455

Kubernetes-commit: c86f562f29b6b7498ea962d2ac596e6d26ecd723
Kubernetes-commit: 9c4651bd0479b84f5e5913649207476717f3f13e

Signed-off-by: Eric Lin <exlin@google.com>

dcbw added a commit to dcbw/ovn-kubernetes-1 that referenced this pull request


          factory: strip Managed Fields and some Pod fields to reduce memory usage

147c2ad

We don't care about them, so why cache them and use a ton
of memory.

Inspired by kubernetes/kubernetes#118455

Signed-off-by: Dan Williams <dcbw@redhat.com>

dcbw pushed a commit to dcbw/ovn-kubernetes that referenced this pull request


          CARRY: Add 'WithTransform' SharedInformerOption

23e8b88

kubernetes/kubernetes#118455

Kubernetes-commit: c86f562f29b6b7498ea962d2ac596e6d26ecd723
Kubernetes-commit: 9c4651bd0479b84f5e5913649207476717f3f13e

Signed-off-by: Eric Lin <exlin@google.com>

machine424 mentioned this pull request

Reduce Kubernetes SD informers cache prometheus/prometheus#13305

Open

vthapar added a commit to vthapar/submariner-addon that referenced this pull request


          Reduce k8s sharedInformer memory usage

3a97d83

Trim ManagedFields to reduce shared informer memory usage.
Currently it is only for kubernetes client based informers. Once
other generated clients use newer version of k8s/client-go [0.29]
we can apply the trim to them too.

Refer: kubernetes/kubernetes#118455

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>

vthapar mentioned this pull request

Reduce sharedInformer memory usage stolostron/submariner-addon#1316

Merged

vthapar added a commit to vthapar/submariner-addon that referenced this pull request


          Reduce k8s sharedInformer memory usage

4c871a2

Trim ManagedFields to reduce shared informer memory usage.
Currently it is only for kubernetes client based informers. Once
other generated clients use newer version of k8s/client-go [0.29]
we can apply the trim to them too.

Refer: kubernetes/kubernetes#118455

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>

vthapar added a commit to vthapar/submariner-addon that referenced this pull request


          Reduce sharedInformer memory usage

fb30e71

Trim ManagedFields to reduce shared informer memory usage.
This requires informers generated with  newer version of k8s/client-go [0.29]
so currently it is done for following informers:
 - k8s
 - submarinerconfig

Rest informers should be updated once WithTransform() is available.

Refer: kubernetes/kubernetes#118455

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>

openshift-merge-bot bot pushed a commit to stolostron/submariner-addon that referenced this pull request


          Reduce sharedInformer memory usage

40ca49e

Trim ManagedFields to reduce shared informer memory usage.
This requires informers generated with  newer version of k8s/client-go [0.29]
so currently it is done for following informers:
 - k8s
 - submarinerconfig

Rest informers should be updated once WithTransform() is available.

Refer: kubernetes/kubernetes#118455

Signed-off-by: Vishal Thapar <5137689+vthapar@users.noreply.github.com>

liggitt mentioned this pull request

DeltaFIFO TransformFunc can result in a data race on re-sync #124337

Closed

Member

liggitt commented Apr 16, 2024

A data race on resync was just reported in use of WithTransform in #124337 that may affect this use

Member

wojtek-t commented Apr 16, 2024

heh...
the problem is that for Resync operation we take object that is already existing in the store (DeltaFifo in this case).
And there is chance of touching (reading) that object at the same time.

We effectively can't call Transform for SYNC events here:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/cache/delta_fifo.go#L456

Member

liggitt commented Apr 16, 2024 •

edited

Loading

We effectively can't call Transform for SYNC events here:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/cache/delta_fifo.go#L456

Or delete events where the object is DeletedFinalStateUnknown, and maybe also for Replace (the docs there are unclear - "In cases involving Replace(), such an object can come through multiple times.")

Member

wojtek-t commented Apr 16, 2024

Or delete events where the object is DeletedFinalStateUnknown, and maybe also for Replace (the docs there are unclear - "In cases involving Replace(), such an object can come through multiple times.")

Good point on the DeletedFinalStateUnknown.

Regarding Replace() - this is probably true in theory, but in practice we only replace with a net new result of List call, so in practice it's not a problem [although we should clarify the contract there or sth]

linxiulei mentioned this pull request

controlplane/apiserver: Trim managedFields off self-requested informers #124667

Merged

alvaroaleman mentioned this pull request

✨ Add TransformStripManagedFields kubernetes-sigs/controller-runtime#2791

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

apelisse apelisse left review comments

alexzielenski alexzielenski left review comments

andrewsykim Awaiting requested review from andrewsykim

deads2k Awaiting requested review from deads2k

wojtek-t Awaiting requested review from wojtek-t

caesarxuchao Awaiting requested review from caesarxuchao

liggitt Awaiting requested review from liggitt

Labels

approved area/code-generation cncf-cla: yes kind/cleanup lgtm needs-priority ok-to-test release-note sig/api-machinery sig/cloud-provider size/L triage/accepted