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

Add proposal for remove derived mcs #3705

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

zishen
Copy link
Member

@zishen zishen commented Jun 26, 2023

What type of PR is this?

/kind design

Which issue(s) this PR fixes:
Fixes issue-2384

@karmada-bot karmada-bot added the kind/design Categorizes issue or PR as related to design. label Jun 26, 2023
@karmada-bot karmada-bot added do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 26, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2023

Codecov Report

Merging #3705 (c21dbb0) into master (2840f91) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #3705      +/-   ##
==========================================
- Coverage   55.69%   55.68%   -0.02%     
==========================================
  Files         222      225       +3     
  Lines       21195    21313     +118     
==========================================
+ Hits        11805    11868      +63     
- Misses       8763     8813      +50     
- Partials      627      632       +5     
Flag Coverage Δ
unittests 55.68% <ø> (-0.02%) ⬇️

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

see 7 files with indirect coverage changes

@karmada-bot karmada-bot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jun 26, 2023
@zishen zishen changed the title Remove derived mcs Add proposal for remove derived mcs Jun 26, 2023
@zishen zishen force-pushed the remove-derived-mcs branch 2 times, most recently from 40a14bb to 800021a Compare July 4, 2023 06:21
@XiShanYongYe-Chang
Copy link
Member

docs/proposals/mcs/remove-derived-mcs.md Outdated Show resolved Hide resolved
docs/proposals/mcs/remove-derived-mcs.md Outdated Show resolved Hide resolved
docs/proposals/mcs/remove-derived-mcs.md Outdated Show resolved Hide resolved
docs/proposals/mcs/remove-derived-mcs.md Outdated Show resolved Hide resolved
@zishen zishen force-pushed the remove-derived-mcs branch 3 times, most recently from d69ad0a to c21dbb0 Compare July 13, 2023 07:27
Copy link
Member

@Garrybest Garrybest left a comment

Choose a reason for hiding this comment

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

Generally LGTM, cc @Tingtal @RainbowMango

docs/proposals/mcs/remove-derived-mcs.md Outdated Show resolved Hide resolved
docs/proposals/mcs/remove-derived-mcs.md Show resolved Hide resolved
docs/proposals/mcs/remove-derived-mcs.md Outdated Show resolved Hide resolved
docs/proposals/mcs/remove-derived-mcs.md Outdated Show resolved Hide resolved
docs/proposals/mcs/remove-derived-mcs.md Outdated Show resolved Hide resolved
docs/proposals/mcs/remove-derived-mcs.md Outdated Show resolved Hide resolved
Comment on lines 235 to 243
The workflow of the additional dnsvserver as follows:

![](.\additional-dns-server.jpg)

Copy link
Member

Choose a reason for hiding this comment

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

This is no image showing here:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

This is no image showing here:

image

oh, it's ok in my brower. I think you need a pull, for this is l just up recently.


1. Load balancing can be achieved. Since the extended dns resolver is customized, customers can achieve load balancing here.

2. Access in the original way can be realized. Rules for local services and remote services can be determined in custom plug-ins. Therefore, the original access method can be maintained, in this case is `foo.default.svc.cluster.local`.
Copy link
Member

Choose a reason for hiding this comment

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

How can achieve this point? How to distinguish the single-cluster service or multi-cluster service?

Copy link
Member Author

Choose a reason for hiding this comment

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

How can achieve this point? How to distinguish the single-cluster service or multi-cluster service?

I mean it's all up to the user to customize the development, so there's a lot of flexibility.


#### coredns compile

Refer to [Compile Instructions](#Compilation Instructions).
Copy link
Member

Choose a reason for hiding this comment

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

Expired Link

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the difference between github and typora. Let me find a solution


#### coredns configuration

Refer to [Configuration Description](#Configuration Description).
Copy link
Member

Choose a reason for hiding this comment

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

Expired Link

### Test Plan

- All current testing should be passed, no break change would be involved by this feature.
- Add new E2E tests to cover the feature, the scope should include:
Copy link
Member

Choose a reason for hiding this comment

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

E2E testing is not easy to add, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's difficult. I haven't figured it out yet.

@karmada-bot karmada-bot added the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jul 19, 2023
@karmada-bot karmada-bot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Jul 20, 2023
@zishen zishen force-pushed the remove-derived-mcs branch 4 times, most recently from 9674338 to b1ab179 Compare August 16, 2023 01:43
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.

I think we might need to postpone this and gather more feedback for this approach.
The biggest challenge with this proposal is that users have to replace CoreDNS, which might be hard to accept for most users.

docs/proposals/mcs/remove-derived-mcs.md Outdated Show resolved Hide resolved
Signed-off-by: Tanggui Bian <softwarebtg@163.com>
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 Aug 18, 2023
@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 Aug 18, 2023
@karmada-bot karmada-bot merged commit a4e3957 into karmada-io:master Aug 18, 2023
12 checks passed
@zishen zishen deleted the remove-derived-mcs branch August 22, 2023 08:09
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.

[MCS] Remove "derived-" prefix of MCS
7 participants