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.

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

Correctly close port-forwarders #4879

Merged
merged 2 commits into from
Dec 16, 2019
Merged

Correctly close port-forwarders #4879

merged 2 commits into from
Dec 16, 2019

Conversation

alvaroaleman
Copy link
Contributor

@alvaroaleman alvaroaleman commented Dec 16, 2019

What this PR does / why we need it:

So far, we close the individual listeners in the portforwarding but not the underlying spdy connection to the kubelet, resulting in memory leaking.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #4862

Special notes for your reviewer:

Documentation:

Does this PR introduce a user-facing change?:

A memory leak in the port-forwarding of the Kubernetes dashboard and Openshift console endpoints was fixed

@kubermatic-bot kubermatic-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. labels Dec 16, 2019
@kubermatic-bot kubermatic-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. team/ui labels Dec 16, 2019
@alvaroaleman alvaroaleman changed the title Portforward mem leak Correctly close port-forwarders Dec 16, 2019
@alvaroaleman
Copy link
Contributor Author

/cherrypick release/v2.12

@kubermatic-bot
Copy link
Contributor

@alvaroaleman: once the present PR merges, I will cherry-pick it on top of release/v2.12 in a new PR and assign it to you.

In response to this:

/cherrypick release/v2.12

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.

@floreks
Copy link
Contributor

floreks commented Dec 16, 2019

Nice catch!

/lgtm
/approve

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2019
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 6fc77499b1725a557b86e514004672cda78664dc

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, floreks

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubermatic-bot kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 16, 2019
@alvaroaleman
Copy link
Contributor Author

/hold

This results in E1216 10:54:14.571486 1 portforward.go:409] error closing listener: close tcp4 127.0.0.1:32977: use of closed network connection which isn't a real problem but will irritate ppl

@kubermatic-bot kubermatic-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2019
@kubermatic-bot kubermatic-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2019
@alvaroaleman
Copy link
Contributor Author

/hold cancel
PTAL

@kubermatic-bot kubermatic-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2019
@floreks
Copy link
Contributor

floreks commented Dec 16, 2019

/lgtm

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 16, 2019
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: d283a94484324f2817c177af7ec166c92d0e4ddd

@alvaroaleman
Copy link
Contributor Author

/retest

1 similar comment
@alvaroaleman
Copy link
Contributor Author

/retest

@kubermatic-bot kubermatic-bot merged commit 541dbd4 into kubermatic:master Dec 16, 2019
@kubermatic-bot kubermatic-bot added this to the v2.13 milestone Dec 16, 2019
@kubermatic-bot
Copy link
Contributor

@alvaroaleman: new pull request created: #4883

In response to this:

/cherrypick release/v2.12

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.

@floreks floreks deleted the portforward-mem-leak branch December 16, 2019 12:02
@kdomanski
Copy link
Contributor

@alvaroaleman Does this allow us to revert #4524 ?

@alvaroaleman
Copy link
Contributor Author

alvaroaleman commented Dec 16, 2019

@alvaroaleman Does this allow us to revert #4524 ?

Yeah i think so, when testing this the mem usage never went above 50m

kdomanski added a commit that referenced this pull request Jan 13, 2020
The memory leak which caused the increased memory requirements has
been plugged in #4879
kubermatic-bot pushed a commit that referenced this pull request Jan 16, 2020
)

The memory leak which caused the increased memory requirements has
been plugged in #4879
kubermatic-bot pushed a commit that referenced this pull request Mar 11, 2020
* Use in-tree clusterexposer (#4826)

* Initial rework of cluster-exposer

* Plug clusteexposer in

* Debug

* Fix metric address, use cache

* Fix npd

* Correctly set target port of outer service

* Fix failed to write body

* Need that pkill

* add tests for user cluster RBAC (#4824)

* Make node-exporter optional addon (#4832)

* Make node-exporter optional addon

* Update operator
Bump version

* Update YAML examples

* Retry getting OIDC token (#4834)

* add e2e test for cluster roles (#4833)

* Share seed managers across controllers in seed-controller-lifecycle (#4823)

* share seed managers across controllers in seed-controller-lifecycle

* use a shared context for all seed controllers

* prevent panics when restarting the controllers

* fix leaking context

* PR feedback

* Make setup-kind-in-kubermatic.sh usable for UI (#4836)

* Make kubermatic-in-kind.sh usable for dashboard tests

* Add go to PATH

* Allow overwriting cache version

* Dont make clusterexposer building part of its run timeout

* Use relative path for configmap

* Add ADITIONAL_HELM_ARGS

* Do not overwrite additional helm args

* Remove superfluous hostsfile hack

* Add BYO node datacenter

* E2E tests: Get request and auth token in one go (#4845)

* E2E tests: Get request and auth token in one go

* Build OIDC proxy ahead of time

* Fix operator cleanup watches (#4837)

* fix missing watches for Seeds

* fix seed-sync controller garbage collection

* fix indentation

* use retry instead of wait loops for the seed-sync cleanup

* only reconcile the affected seed instead of all in seed-operator

* Api e2e tests: Use kubermatic-in-kind.sh (#4843)

* Api e2e tests: Use kubermatic-in-kind.sh

* Add missing vault preset

* Docker push preset

* Add preset

* Newlines are hard

* Add user

* Format swagger errors

* improve oidc proxy client (#4846)

* vsphere: fix incorrect VM folder in cloud-config (#4737)

* vsphere: fix incorrect VM folder in cloud-config

* e2e: add scenario option to use a custom folder

* deploy Kubermatic Operator in parallel on dev (#4838)

* Add parent cluster readable name to default worker names (#4839)

* Bump Kubernetes Dashboard version for the user cluster. (#4844)

* AWS: Be a bit more helpful when tagging fails (#4848)

* Updates for kube 1.17 (#4852)

* Updates for kube 1.17

* Bump dat chart

* fix missing deployment selector (#4853)

* remove dead code Machine(...) (#4847)

* Use kind for Openshift and all providers (#4842)

* Use kind for Openshift and all providers

* Add both pull and push preset everywhere

* Add service_account_key

* Set OIDC args for openshift

* Debug project creation failure

* Remove unused OIDC settings

* Remove unused dex CA

* Remove OIDC config from Openshift

* Revert "Debug project creation failure"

This reverts commit 4f879f3.

* Add Openstack datacenter

* improve log flag handling (#4855)

* bump cert-manager to 0.12.0 (#4857)

* Remove conformance tester as its unused (#4861)

* update Grafana to 6.5.2 (#4858)

* update Alertmanager to 0.20.0 (#4864)

* update karma to 0.52 (#4859)

* kube-state-metrics 1.8.0 (#4860)

* update Helm chart

* update user-cluster resources

* Use etcd v3.4 for Kube 1.17+ (#4856)

* Use etcd v3.4 for Kube 1.17+

* Make backup cronjob cope with custom images

* Update envoy to v1.12.2 (#4865)

* update Dex to 2.21.0 (#4869)

* Update machine-controller to v1.8.1 (#4866)

* fix misdetecting tags when retagging images (#4872)

* Add v1.17 openstack ccm (#4871)

* Upgrade endpoint: Fix no kind match error (#4870)

* Ginkgo: Add timeout (#4877)

* Leaderelection: Always exist on lost leader lease (#4874)

* Add kube 1.17 presubmit (#4873)

* Add kube 1.17 presubmit

* Run presubmits on prow.yaml changes

* Exclude tests that require open network on 1.17 as well

* Minio 2019-10-12T01-39-57Z (#4868)

* update Minio to RELEASE.2019-10-12T01-39-57Z

* disable authentication for metrics endpoint

* add Minio dashboard

* Correctly close port-forwarders (#4879)

* Close portforwarders

* Close listeners before closing portforwarder

* Portforwarding: Use upstream port-parsing (#4884)

* Close portforwarders

* Remove GetLocalPortFromPortForwardOutput, upstream got fixed

* Fix merge

* extend global settings for new flags (#4878)

* Allow access to Kubernetes Dashboard proxy based on global settings (#4889)

* Use kind for upgrade tests (#4863)

* Make ci-setup-kubermatic-in-kind.sh idempotent

* Update kind wrapper script for upgrade tests

* Use kind in upgrade test

* Debug: only test creation

* Fix port-forwarder killing

* Add some more debug output

* More killing

* Do not test creation only

* Stack traps instead of replacing

* Add logging

* Add more logging

* Run conformance tests again

* Kill OIDC proxy if it still runs

* Only run ginkgo for head version

* Do not create a second cluster

* Sleep

* Re-use existing project in upgrade test

* Remove debug sleep

* BYO: Wait for rbac (#4892)

* Openshift console: Fix login (#4887)

* Openshift console: Fix login

* Encrypt the password

* Test encryption

* Encrypt using ca UID as key

* Fix world

* Deduplicate GlobalSecretKeySelector getter (#4895)

* Unify check if cluster is openshift (#4898)

* Fix upgrade tests when upgrading from older kubermatic version (#4900)

* Make user-ssh-keys-agent building optional in tests

* Default namespace in tests

* Fix ci-download-gocache for periodics (#4899)

* Conformance tests: Give packet more time for provisioning (#4904)

* Use go 12.12 for all presubmits (#4906)

* Addon controller: Allow deleting addons (#4908)

* Update conformance tests image for all presubmits (#4907)

* add dynamic presets (#4903)

* add dynamic presets

* fix doc comments

* fix unit tests

* fix doc comment

* Make controllers not override resource requests set by VPA (#4810)

* Apply correct requirements if resources are managed by vpa-updater

* Add updated-by-vpa annotation to the API

* Apply correct requirements in controllers when managed by vpa-updater

* Add new test cases

* Make default requirements less verbose

* Fix lint error

* Add GetOverrides function

* Run gofmt

* Refactor GetOverrides function

* Use GetOverrides for Scheduler in OpenShift controller

* Remove obsolete CI e2e scripts (#4910)

These have been replaced with new, kind-based scripts.

* Move rancher controller below seed-controller-manager pkg (#4914)

* Add christoph to team lifecycle (#4916)

* Fix setting overrides for kube-controller-manager in OpenShift controller (#4912)

* Minor fixes

* Address review comment

* Fix test failure

* Addons: Allow specyfing required api resources (#4913)

* Safe LastSuccessfulDeploymentTimestamp on addons

* Revert "Safe LastSuccessfulDeploymentTimestamp on addons"

This reverts commit eb98f505163154f73d5391fa60bc3443c2e8eee3.

* Addons: Allow specyfing required resources

* Pass through option to specify rewquires dircetive

* Revert "Revert "Safe LastSuccessfulDeploymentTimestamp on addons""

This reverts commit 64e1f24460f969e79fa9d0ea513519ee4ba8c648.

* Read addons file from disk

* Revert "Revert "Revert "Safe LastSuccessfulDeploymentTimestamp on addons"""

This reverts commit 875e03d83e586e00de0545bb56f3820d3d38eab7.

* Bump chart

* Fix logging

* Fix openshift-addons.yaml path

* Fix hack script

* Join correctly

* Fix flag help text

* Update operator

* Update generated samples

* Verify operator defaults match chart (#4918)

* Add initial tests

* Test versions and updates yaml

* Validate openshift addons

* Bump chart version

* Update samples

* Remove stale upgrade testing scripts (#4921)

* Operator: Fix addon defaulting (#4919)

* Operator: Fix addon defaulting

* Fix log

* Move master controllers into common directory (#4920)

* Move controllers than run in the master-controller-manager into common dir

* Move seed controller lifecycle into shared dir

* Add doc.go

* enable dynamic presets (#4915)

* enable dynamic presets

* extend operator for dynamic presets

* remove static presets from the operator

* init cluster binding for the cluster creator (#4922)

* init cluster binding for the cluster creator

* extend watch for cluster role bindings

* review fixes

* add Youssef to cluster-lifecycle team (#4926)

* check default cluster role binding for the cluster creator (#4924)

* remove seed-team from OWNERS file (#4925)

RIP

* Fix GCP orphaned routes issue (#4885)

* Fix GCP orphaned routes issue

* Add gcp route cleanup finalizer

* address review comments

* Remove alvaroaleman as owner (#4928)

* replace oidc-proxy-client with a simple Dex client (#4886)

* Update the allowed licensed (#4893)

* Add env variable debug flag for hack/run* scripts (#4930)

* add OWNER_EMAIL in the run-userclustercontroller.sh script (#4929)

Signed-off-by: Moath Qasim <moad.qassem@gmail.com>

* Add docs for all controllers (#4927)

* Initial docs

* More docs

* Spelling

* don't create cluster role binding for API cluster roles (#4931)

* refactor get/create project method (#4851)

* refactor get/create project method

* mend

* use MatchingLabels instead label selector (#4934)

* Replica/Ingress-Class Support for Operator (#4932)

* allow to configure replica counts

* fix bad admission webhook reconciling

* move domain and cert issuer in a new "ingress" section

* update example YAML

* make disabling the Ingress more explicit

* enabling node csr approver for kubernetes clusters (#4935)

Signed-off-by: Moath Qasim <moad.qassem@gmail.com>

* Replace presets.yaml with CRD in API e2e tests (#4933)

* replace presets.yaml with Preset CRD in API e2e tests

* enable dynamic presets

* fix typo when cleaning up AWS tags (#4940)

* Bump machine-controller to v1.9.0 (#4939)

Signed-off-by: Artiom Diomin <artiom@loodse.com>

* Add endpoints to fetch gcp networks (#4938)

* fetch gcp networks from api

* update swagger & api-client

* format

* fix typo

* Add configurable ExposeStrategy on Seed cluster level (#4936)

* Add configurable ExposeStrategy on Seed cluster level

* review comments

* fix local API startup script (#4943)

enable --dynamic-preset when starting a local instance of kubermatic API

Signed-off-by: Marcin Franczyk <marcin0franczyk@gmail.com>

* add an API field to switch on dynamic kubelet config (#4946)

* add an API field to switch on dynamic kubelet config

* testcase for getting a dynbamically configured MD from API

* regenerate swagger

* updated api client

* Set dynamic-presets to true in run-api.sh script and give possibility to (#4941)

override PPROF port in run-controller script to be able to run api and
controller on the same host

* changelog for v2.12.5 (#4948)

* changelog for v2.12.5

* fixed quoting

* fixed quoting

* fixed quoting

* Use Operator for e2e Tests (#4911)

* use the operator to setup Kubermatic in e2e tests when KUBERMATIC_USE_OPERATOR is set

* define new e2e job for the operator

* Loki grafana (#4949)

* Added Loki datasource to Grafana's chart

* Added Loki datasource

* Added an empty line

* Chart revision number

* Configuring Loki to be disabled by default

* ci-setup-kubermatic-in-kind.sh: checkout corresponding charts when deploying older kubermatic (#4952)

* Revert "ci-setup-kubermatic-in-kind.sh: checkout corresponding charts when deploying older kubermatic (#4952)" (#4957)

This reverts commit d2b40e2.

* do not let a single bad Seed prevent listing clusters (#4961)

* do not tag :latest when doing canary deployments in presubmits (#4970)

* Revert "Increase kubermatic api memory requests and limits #4524" (#4960)

The memory leak which caused the increased memory requirements has
been plugged in #4879

* addons/kubelet-configmap: allow configuring QPS (#4854)

* ci-setup-kubermatic-in-kind.sh: checkout corresponding charts when deploying older kubermatic (#4959)

* add helm->crd converter for the operator (#4963)

Co-authored-by: Alvaro Aleman <alvaroaleman@users.noreply.github.com>
Co-authored-by: Lukasz Zajaczkowski <zreigz@gmail.com>
Co-authored-by: Marcin Maciaszczyk <maciaszczykm@icloud.com>
Co-authored-by: Christoph Mewes <christoph@loodse.com>
Co-authored-by: Kamil Domański <kamil@domanski.co>
Co-authored-by: Sebastian Florek <Sebastian.Florek@loodse.com>
Co-authored-by: Marko Mudrinić <mudrinic.mare@gmail.com>
Co-authored-by: Mohamed El Sayed <m.elsayed@gmail.com>
Co-authored-by: Sebastian Scheele <sebastian@loodse.com>
Co-authored-by: MqueMazz <moad.qassem@gmail.com>
Co-authored-by: Artiom Diomin <kron82@gmail.com>
Co-authored-by: Kristin Groschoff <kgroschoff@gmail.com>
Co-authored-by: Marcin Franczyk <marcin0franczyk@gmail.com>
Co-authored-by: irozzo-1A <iacopo@loodse.com>
Co-authored-by: Youssef Azrak <30932864+youssefazrak@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Resource leak when creating Kubernetes dashboard connections from UI
4 participants