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

uninstall metrics adapter when karmada cluster remove #4056

Merged

Conversation

wawa0210
Copy link
Member

@wawa0210 wawa0210 commented Sep 12, 2023

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #4030

When the karmada instance is removed, the metrics-adapter does not have a corresponding de-init, causing the pod to remain.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-operator`: Fixed the issue that karmada-metrics-adapter was not removed after deleting a Karmada instance.

@karmada-bot karmada-bot added the kind/bug Categorizes issue or PR as related to a bug. label Sep 12, 2023
@karmada-bot karmada-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 12, 2023
@wawa0210 wawa0210 force-pushed the remove-mertics-adapter-cluster-rm branch from c15faa9 to 8be8210 Compare September 12, 2023 11:36
@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (2879c8d) 53.81% compared to head (d928c11) 53.81%.
Report is 1 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4056   +/-   ##
=======================================
  Coverage   53.81%   53.81%           
=======================================
  Files         231      231           
  Lines       23013    23013           
=======================================
  Hits        12385    12385           
  Misses       9955     9955           
  Partials      673      673           
Flag Coverage Δ
unittests 53.81% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 28 to 29
newRemoveComponentWithServiceSubTask(constants.KarmadaAPIserverComponent, util.KarmadaAPIServerName),
newRemoveComponentWithServiceSubTask(constants.KarmadaAPIserverComponent, util.KarmadaAPIServerName),
Copy link
Member

Choose a reason for hiding this comment

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

Remove the same component twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Duplicate code,thx

@wawa0210 wawa0210 force-pushed the remove-mertics-adapter-cluster-rm branch from b4e54cf to d928c11 Compare September 13, 2023 01:15
@calvin0327
Copy link

/lgtm

/cc @RainbowMango @jwcesign

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2023
@jwcesign
Copy link
Member

/lgtm

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/assign

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/approve

I added a release-note to the PR description.

@calvin0327 Do you think this patch should be backported to release branches?

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2023
@karmada-bot karmada-bot merged commit 0b3e0d9 into karmada-io:master Sep 13, 2023
11 checks passed
@calvin0327
Copy link

@RainbowMango Yes, I think we should patch to v1.7 release branch, how do you think?

@RainbowMango
Copy link
Member

+1

@wawa0210 Would you like to do this? Here is the guide about how to cherry-pick a PR:
https://karmada.io/docs/contributor/cherry-picks

@wawa0210
Copy link
Member Author

+1

@wawa0210 Would you like to do this? Here is the guide about how to cherry-pick a PR: https://karmada.io/docs/contributor/cherry-picks

sure, let me do it

karmada-bot added a commit that referenced this pull request Sep 15, 2023
…-upstream-release-1.7

Automated cherry pick of #4056: uninstall metrics adapter when karmada cluster remove
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. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When deleting a karmada instance, some pods (metrics-adapters) are still running.
6 participants