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

Proposal: Add API Server Service Information to KarmadaStatus #5599

Merged
merged 1 commit into from
Nov 2, 2024

Conversation

jabellard
Copy link
Contributor

What type of PR is this?

/kind design

What this PR does / why we need it:

  • Details are contained in the proposal.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added the kind/design Categorizes issue or PR as related to design. label Sep 24, 2024
@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 24, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.30%. Comparing base (5f7fc4f) to head (e8c81c5).
Report is 231 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    #5599      +/-   ##
==========================================
+ Coverage   34.84%   42.30%   +7.46%     
==========================================
  Files         645      655      +10     
  Lines       44861    55756   +10895     
==========================================
+ Hits        15633    23590    +7957     
- Misses      28021    30652    +2631     
- Partials     1207     1514     +307     
Flag Coverage Δ
unittests 42.30% <ø> (+7.46%) ⬆️

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.

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

@jabellard jabellard force-pushed the api_server_service_info branch 2 times, most recently from b994313 to dc35f83 Compare October 30, 2024 11:51
@chaosi-zju
Copy link
Member

The services created by Karmada can have four different types.

In order to make better decisions regarding the most suitable API form, I conducted tests on different service types and documented for reference: doc link

I hope this will be helpful to all !

@RainbowMango
Copy link
Member

Thanks @chaosi-zju for your effort. Yes, we noticed that currently, karmada-operator allows people to config all kinds of Service types, and @jabellard analyzed it in the discussion above.
Glad to see the test report, I believe it can help us figure out the situation more clearly.

But, I can't access the doc, just sent a request for that :) :)

@jabellard
Copy link
Contributor Author

I conducted tests on different service types and documented for reference: doc link

@chaosi-zju , thank you for this. I'd love to learn more about your findings. I just sent you a request to get access to the doc.

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.

Hi @jabellard
Generally looks good to me.

From the test report @chaosi-zju provided on above, we can tell that:

  1. Currently we supports 2 Service types, there are ClusterIP and NodePort.
  2. The Loadbalancer is not supported yet, but I think it makes sense to enhance it this time. See the use case on When installing karmada using the operator, provide a way to annotate karmadaAPIServer services #4634.

So I just opened #5767 to track all of this. Any thoughts?

@karmada-bot karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 1, 2024
@jabellard
Copy link
Contributor Author

jabellard commented Nov 1, 2024

Hi @jabellard Generally looks good to me.

From the test report @chaosi-zju provided on above, we can tell that:

  1. Currently we supports 2 Service types, there are ClusterIP and NodePort.
  2. The Loadbalancer is not supported yet, but I think it makes sense to enhance it this time. See the use case on When installing karmada using the operator, provide a way to annotate karmadaAPIServer services #4634.

So I just opened #5767 to track all of this. Any thoughts?

@RainbowMango , thanks for sharing this and also to @chaosi-zju for generating this thorough report. I'll keep on eye on these action items. I just pushed some changes to address your comments.

@RainbowMango
Copy link
Member

Thanks Joe, I think we are ready to go after squashing the commits.

We can start to implemente it now.

Signed-off-by: Joe Nathan Abellard <contact@jabellard.com>
@jabellard
Copy link
Contributor Author

Thanks Joe, I think we are ready to go after squashing the commits.

We can start to implemente it now.

Just squashed. I started the implementation, and will send a PR for that soon.

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.

/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2024
@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 Nov 2, 2024
@RainbowMango
Copy link
Member

RainbowMango commented Nov 2, 2024

The failing test is unrelated. We can retest it by leaving a comment /retest after all tests are finished.

tracked by #5136

@RainbowMango
Copy link
Member

/retest

@karmada-bot karmada-bot merged commit 5cd3c1a into karmada-io:master Nov 2, 2024
18 checks passed
@chaosi-zju
Copy link
Member

  1. Currently we supports 2 Service types, there are ClusterIP and Loadbalancer.

This statement seems to be misspelled, Loadbalancer should be NodePort

@RainbowMango
Copy link
Member

Right, thanks. Updated on #5599 (review).

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/design Categorizes issue or PR as related to design. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants