Skip to content

Fix leak in resourceimport_controller.go#4251

Merged
tnqn merged 1 commit into
antrea-io:mainfrom
Dyanngg:fix-installed-imports-leak
Oct 17, 2022
Merged

Fix leak in resourceimport_controller.go#4251
tnqn merged 1 commit into
antrea-io:mainfrom
Dyanngg:fix-installed-imports-leak

Conversation

@Dyanngg
Copy link
Copy Markdown
Contributor

@Dyanngg Dyanngg commented Sep 27, 2022

The ResourceImport reconciler only adds/updates installed ResourceImports to the installedResImports cache, but never removes them when they are deleted from the K8s API and all corresponding resources are deleted successfully. This PR fixes the above issue.

Signed-off-by: Dyanngg dingyang@vmware.com

@Dyanngg Dyanngg added kind/bug Categorizes issue or PR as related to a bug. area/multi-cluster Issues or PRs related to multi cluster. labels Sep 27, 2022
@Dyanngg Dyanngg requested review from luolanzone and tnqn September 27, 2022 21:15
@codecov
Copy link
Copy Markdown

codecov Bot commented Sep 27, 2022

Codecov Report

Merging #4251 (bd7b407) into main (ac70351) will increase coverage by 0.53%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4251      +/-   ##
==========================================
+ Coverage   63.16%   63.69%   +0.53%     
==========================================
  Files         388      400      +12     
  Lines       55147    55215      +68     
==========================================
+ Hits        34833    35170     +337     
+ Misses      17766    17458     -308     
- Partials     2548     2587      +39     
Flag Coverage Δ
e2e-tests 62.17% <41.66%> (?)
integration-tests 34.33% <20.83%> (-0.09%) ⬇️
kind-e2e-tests 48.80% <ø> (+0.25%) ⬆️
unit-tests 44.84% <41.66%> (-0.05%) ⬇️
Impacted Files Coverage Δ
...uster/commonarea/acnp_resourceimport_controller.go 69.74% <40.00%> (-0.08%) ⬇️
...lticluster/commonarea/resourceimport_controller.go 80.21% <41.66%> (-0.97%) ⬇️
...rs/multicluster/commonarea/clusterinfo_importer.go 69.64% <42.85%> (-4.87%) ⬇️
pkg/controller/externalippool/controller.go 86.16% <0.00%> (-6.25%) ⬇️
pkg/controller/networkpolicy/tier.go 50.00% <0.00%> (-5.00%) ⬇️
pkg/agent/consistenthash/consistenthash.go 93.75% <0.00%> (-3.75%) ⬇️
pkg/agent/route/route_linux.go 50.21% <0.00%> (-3.07%) ⬇️
pkg/controller/ipam/antrea_ipam_controller.go 75.25% <0.00%> (-3.02%) ⬇️
pkg/agent/controller/networkpolicy/packetin.go 74.32% <0.00%> (-2.71%) ⬇️
...g/agent/controller/serviceexternalip/controller.go 79.18% <0.00%> (-2.39%) ⬇️
... and 51 more

Copy link
Copy Markdown
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

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

LGTM, two nits.

Comment thread multicluster/controllers/multicluster/commonarea/resourceimport_controller.go Outdated
Comment thread multicluster/controllers/multicluster/commonarea/clusterinfo_importer_test.go Outdated
@Dyanngg Dyanngg force-pushed the fix-installed-imports-leak branch from 1d4ed7f to c3c4c4f Compare September 28, 2022 17:16
@Dyanngg Dyanngg requested a review from luolanzone September 28, 2022 17:16
Comment thread multicluster/controllers/multicluster/commonarea/clusterinfo_importer.go Outdated
Comment thread multicluster/controllers/multicluster/commonarea/clusterinfo_importer_test.go Outdated
@Dyanngg Dyanngg force-pushed the fix-installed-imports-leak branch from c3c4c4f to af9f381 Compare September 29, 2022 18:05
Comment thread multicluster/controllers/multicluster/commonarea/resourceimport_controller.go Outdated
@luolanzone luolanzone added this to the Antrea v1.9 release milestone Oct 12, 2022
@Dyanngg Dyanngg force-pushed the fix-installed-imports-leak branch from af9f381 to 4f68a1d Compare October 12, 2022 17:14
The ResourceImport reconciler only adds/updates installed ResourceImports to
the installedResImports cache, but never removes them when they are deleted
from the K8s API and all corresponding resources are deleted successfully.
This PR fixes the above issue.

Signed-off-by: Dyanngg <dingyang@vmware.com>
@Dyanngg Dyanngg force-pushed the fix-installed-imports-leak branch from 4f68a1d to bd7b407 Compare October 12, 2022 17:16
@Dyanngg Dyanngg requested a review from luolanzone October 12, 2022 17:16
@Dyanngg
Copy link
Copy Markdown
Contributor Author

Dyanngg commented Oct 12, 2022

/test-all

Copy link
Copy Markdown
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn
Copy link
Copy Markdown
Member

tnqn commented Oct 17, 2022

/test-multicluster-e2e

@tnqn tnqn merged commit cd4ccdd into antrea-io:main Oct 17, 2022
@Dyanngg Dyanngg deleted the fix-installed-imports-leak branch February 23, 2023 20:10
heanlan pushed a commit to heanlan/antrea that referenced this pull request Mar 29, 2023
The ResourceImport reconciler only adds/updates installed ResourceImports to
the installedResImports cache, but never removes them when they are deleted
from the K8s API and all corresponding resources are deleted successfully.
This PR fixes the above issue.

Signed-off-by: Dyanngg <dingyang@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/multi-cluster Issues or PRs related to multi cluster. kind/bug Categorizes issue or PR as related to a bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants