feat: prevent updates to mcs.types or when multiple types are involved. #4454
Conversation
karmada-bot
added
the
kind/feature
karmada-bot
added
the
size/S
jwcesign
force-pushed
the
forbid-mcs-update
branch
2 times, most recently
from
283dfc3 to
4f52253
Compare
karmada-bot
added
size/M
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #4454 +/- ##
==========================================
- Coverage 51.86% 51.86% -0.01%
==========================================
Files 243 243
Lines 24167 24180 +13
==========================================
+ Hits 12535 12540 +5
- Misses 10950 10958 +8
Partials 682 682
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: jwcesign <jwcesign@gmail.com>
jwcesign
force-pushed
the
forbid-mcs-update
branch
2 times, most recently
from
532940d to
dc43383
Compare
There was a problem hiding this comment.
I understand this is a temporary protection measure to avoid conflicts between CrossCluster(MCS) and Loadbalancer(MCI). However, this conflict issue should be resolved in the release-1.9 cycle.
cc @GitHubxsy @chaunceyjiang Please look at this, need your comments here.
| if len(typesSet) > 1 { | ||
| allErrs = append(allErrs, field.Invalid(typesPath, mcs.Spec.Types, "MultiClusterService types should not contain more than one type")) | ||
| } |
There was a problem hiding this comment.
Maybe we don't need to introduce another set, how about this:
| if len(typesSet) > 1 { | |
| allErrs = append(allErrs, field.Invalid(typesPath, mcs.Spec.Types, "MultiClusterService types should not contain more than one type")) | |
| } | |
| // temporarily disable two expose types | |
| if len(mcs.Spec.Types) > 1 { | |
| allErrs = append(allErrs, field.Invalid(typesPath, mcs.Spec.Types, "MultiClusterService types should not contain more than one type")) | |
| } |
There was a problem hiding this comment.
mcs.Spec.Types can be configured as {LoadBalancer, LoadBalancer}. In this situation, it's the same type.
There was a problem hiding this comment.
BTW, we should deduplicate, but in another PR.
There was a problem hiding this comment.
We need to consider whether to prohibit users from filling in the same type of element. My idea is that it should be prohibited.
|
lgtm |
|
[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 |
karmada-bot
added
the
approved
What type of PR is this?
/kind feature
What this PR does / why we need it:
Currently, the north-south traffic routing is implemented through ServiceImport/ServiceExport by collecting EndpointSlice from member clusters to Karmada control plane for Work resources.
The east-west MCS will also collect EndpointSlice for Work, and both sides will jointly manage the overlay of Work operations.
If both types are configured at the same time, when the north-south Work is overlaid (mcs label is deleted), the east-west MCS cannot synchronize the provider cluster's endpointslice to consumer clusters.
This PR temporarily prohibits mixed configuration and will resolve related conflict issues in future updates.
Which issue(s) this PR fixes:
Part of #4292
Special notes for your reviewer:
none
Does this PR introduce a user-facing change?: