feat: List&watch mcs and service, reconcile the work in execution namespace #4323

Rains6 · 2023-11-25T10:23:32Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Referring to #4287

Which issue(s) this PR fixes:
Part of #4292

Special notes for your reviewer:
none

Does this PR introduce a user-facing change?:

`karmada-controller-manager`: Introduced `multiclusterservice` controller to sync MultiClusterService.

codecov-commenter · 2023-11-25T10:34:30Z

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (dd9f54c) 51.99% compared to head (2802916) 51.98%.
Report is 6 commits behind head on master.

Files	Patch %	Lines
pkg/util/helper/mcs.go	0.00%	6 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4323      +/-   ##
==========================================
- Coverage   51.99%   51.98%   -0.01%     
==========================================
  Files         242      242              
  Lines       23984    23990       +6     
==========================================
+ Hits        12471    12472       +1     
- Misses      10832    10837       +5     
  Partials      681      681

Flag	Coverage Δ
unittests	`51.98% <0.00%> (-0.01%)`	⬇️

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.

pkg/controllers/mcs/mcs_controller.go

jwcesign · 2023-11-27T09:37:17Z

pkg/controllers/mcs/mcs_controller.go

+limitations under the License.
+*/
+
+package mcs


To differentiate the current mcs controller, I believe it would be best to move it to a different directory.

cc @RainbowMango for better suggestions.

Options:

mcsnative

It's ok to put the controller in this package. the user-facing controller name is defined at https://github.com/karmada-io/karmada/pull/4323/files#diff-a111978a679a9f03e49c67a43ecb913299d153e59ce48d2d30f5029f27ab601aR227.

I think we should also put it in another folder for easier code analysis and reading.

pkg/controllers/mcs/mcs_controller.go

jwcesign · 2023-11-27T09:39:05Z

Review continually

jwcesign · 2023-11-27T10:32:46Z

pkg/controllers/mcs/mcs_controller.go

+const ControllerName = "mcs-controller"
+
+// MCSControllerFinalizer is added to Cluster to ensure work is deleted before itself is deleted.
+const MCSControllerFinalizer = "karmada.io/mcs-controller"


Put this in pkg/util/constants.go

jwcesign · 2023-11-27T10:33:14Z

pkg/controllers/mcs/mcs_controller.go

+const MCSControllerFinalizer = "karmada.io/mcs-controller"
+
+// CrossClusterEventReason is indicates the reason of CrossCluster event.
+const CrossClusterEventReason string = "MCSCrossCluster"


Put this in pkg/events/events.go

jwcesign · 2023-11-27T10:37:43Z

pkg/controllers/mcs/mcs_controller.go

+	return controllerruntime.Result{}, nil
+}
+
+func (c *MCSController) syncSvcWorkToClusters(


How about using name syncSVCWorkToClusters?

jwcesign · 2023-11-28T01:42:11Z

pkg/controllers/mcs/mcs_controller.go

+limitations under the License.
+*/
+
+package mcs


cc @RainbowMango for better suggestions.

Options:

mcsnative

pkg/controllers/mcs/mcs_controller.go

jwcesign · 2023-11-28T01:43:04Z

pkg/controllers/mcs/mcs_controller.go

+const MCSServiceAppliedConditionType = "ServiceApplied"
+
+// MCSWorkIDLabel is indicates the label of work created by mcs.
+const MCSWorkIDLabel = "mcs.karmada.io/mcs-id"


Let's use MultiClusterServicePermanentIDLabel instead of MCSWorkIDLabel

jwcesign · 2023-11-28T01:47:25Z

pkg/controllers/mcs/mcs_controller.go

+	})
+}
+
+func (c *MCSController) getMultiClusterServiceID(mcs *networkingv1alpha1.MultiClusterService) (string, error) {


This ID will be added in mutating webhook: https://github.com/karmada-io/karmada/blob/02b9ff4e11f6bbf67655335fd3c4d1b45f223b25/pkg/webhook/multiclusterservice/mutating.go.

But I am not sure whether we should have this part, Just in case the user forgets to configure the webhook. cc @RainbowMango

jwcesign · 2023-11-28T01:49:12Z

pkg/controllers/mcs/mcs_controller.go

+}
+
+// MCSContainCrossClusterType checks weather the MultiClusterService contains CrossCluster type.
+func MCSContainCrossClusterType(mcs *networkingv1alpha1.MultiClusterService) bool {


Let's put this in file

karmada/pkg/util/helper/mcs.go

Line 103 in 02b9ff4

func MultiClusterServiceCrossClusterEnabled(mcs *networkingv1alpha1.MultiClusterService) bool {

jwcesign · 2023-11-28T01:53:55Z

pkg/controllers/mcs/mcs_controller.go

+			}
+
+			// We only care about the update events below:
+			if equality.Semantic.DeepEqual(mcsOld.Annotations, mcsNew.Annotations) &&


Why is this judgment necessary?

Currently, there don't need to pay attention to other changes, such as label.

jwcesign · 2023-11-28T02:31:25Z

pkg/controllers/mcs/mcs_controller.go

+func (c *MCSController) deleteServiceWork(
+	ctx context.Context,
+	mcs *networkingv1alpha1.MultiClusterService,
+	retainClusters sets.Set[string],


retainClusters look useless?

It's useful to exclude clusters where we don't want to delete service work.

RainbowMango

/assign

pkg/controllers/mcs/mcs_controller.go

RainbowMango · 2023-11-29T07:09:05Z

pkg/controllers/mcs/mcs_controller.go

+)
+
+// ControllerName is the controller name that will be used when reporting events.
+const ControllerName = "mcs-controller"


Suggested change

const ControllerName = "mcs-controller"

const ControllerName = "multiclusterservice-controller"

RainbowMango · 2023-11-29T07:09:30Z

pkg/util/constants.go

@@ -89,6 +89,9 @@ const (
 	// ClusterResourceBindingControllerFinalizer is added to ClusterResourceBinding to ensure related Works are deleted
 	// before ClusterResourceBinding itself is deleted.
 	ClusterResourceBindingControllerFinalizer = "karmada.io/cluster-resource-binding-controller"
+
+	// MCSControllerFinalizer is added to Cluster to ensure service work is deleted before itself is deleted.
+	MCSControllerFinalizer = "karmada.io/mcs-controller"


Suggested change

MCSControllerFinalizer = "karmada.io/mcs-controller"

MCSControllerFinalizer = "karmada.io/multiclusterservice-controller"

RainbowMango · 2023-11-29T07:18:53Z

pkg/apis/networking/v1alpha1/service_types.go

+	// MCSServiceAppliedConditionType is indicates the condition type of mcs service applied.
+	MCSServiceAppliedConditionType = "ServiceApplied"


Please move the events to pkg/events/events.go.

@jwcesign In my opinion, if we report a warning event and we need to report another normal event to cancel the event, right?

I am not sure, but we should report the normal event.

pkg/controllers/mcs/mcs_controller.go

jwcesign

I am ok now, cc @RainbowMango for checking

Signed-off-by: z00623291 <[email protected]>

RainbowMango

/lgtm
/approve

karmada-bot · 2023-11-29T11:20:01Z

[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:

~~OWNERS~~ [RainbowMango]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 25, 2023

karmada-bot requested review from lonelyCZ and mrlihanbo November 25, 2023 10:23

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 25, 2023

Rains6 force-pushed the dev-mcs-zhc branch from 6593ecf to 8eb3735 Compare November 27, 2023 01:13

RainbowMango added this to the v1.8 milestone Nov 27, 2023

RainbowMango reviewed Nov 27, 2023

View reviewed changes

pkg/controllers/mcs/mcs_controller.go Outdated Show resolved Hide resolved

Rains6 force-pushed the dev-mcs-zhc branch 5 times, most recently from 0258393 to cd0c339 Compare November 27, 2023 06:43

jwcesign reviewed Nov 27, 2023

View reviewed changes

Rains6 force-pushed the dev-mcs-zhc branch from cd0c339 to d94a62b Compare November 27, 2023 12:08

jwcesign mentioned this pull request Nov 27, 2023

[umbrella] Add Implementation for Service discovery with native Kubernetes naming and resolution #4292

Open

25 tasks

jwcesign reviewed Nov 28, 2023

View reviewed changes

Rains6 force-pushed the dev-mcs-zhc branch from d94a62b to 40b6fa6 Compare November 28, 2023 02:50

RainbowMango reviewed Nov 29, 2023

View reviewed changes

karmada-bot assigned RainbowMango Nov 29, 2023

RainbowMango reviewed Nov 29, 2023

View reviewed changes

pkg/controllers/mcs/mcs_controller.go Outdated Show resolved Hide resolved

RainbowMango reviewed Nov 29, 2023

View reviewed changes

Rains6 force-pushed the dev-mcs-zhc branch from 40b6fa6 to 5872d7e Compare November 29, 2023 08:16

jwcesign reviewed Nov 29, 2023

View reviewed changes

pkg/controllers/mcs/mcs_controller.go Outdated Show resolved Hide resolved

Rains6 force-pushed the dev-mcs-zhc branch 2 times, most recently from 682dcfb to 2802916 Compare November 29, 2023 09:36

jwcesign reviewed Nov 29, 2023

View reviewed changes

List&watch mcs and service, reconcile the work in execution namespace

33ae852

Signed-off-by: z00623291 <[email protected]>

Rains6 force-pushed the dev-mcs-zhc branch from 2802916 to 33ae852 Compare November 29, 2023 09:50

RainbowMango approved these changes Nov 29, 2023

View reviewed changes

karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 29, 2023

karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 29, 2023

karmada-bot merged commit 7032767 into karmada-io:master Nov 29, 2023
12 checks passed

Rains6 deleted the dev-mcs-zhc branch November 29, 2023 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: List&watch mcs and service, reconcile the work in execution namespace #4323

feat: List&watch mcs and service, reconcile the work in execution namespace #4323

Rains6 commented Nov 25, 2023 •

edited by RainbowMango

Loading

codecov-commenter commented Nov 25, 2023 •

edited

Loading

jwcesign Nov 27, 2023

jwcesign Nov 28, 2023

RainbowMango Nov 29, 2023

jwcesign Nov 29, 2023

jwcesign commented Nov 27, 2023

jwcesign Nov 27, 2023

Rains6 Nov 28, 2023

jwcesign Nov 27, 2023

Rains6 Nov 27, 2023

jwcesign Nov 27, 2023

Rains6 Nov 27, 2023

jwcesign Nov 28, 2023

jwcesign Nov 28, 2023

Rains6 Nov 28, 2023

jwcesign Nov 28, 2023

jwcesign Nov 28, 2023

Rains6 Nov 28, 2023

jwcesign Nov 28, 2023

Rains6 Nov 28, 2023

jwcesign Nov 28, 2023

Rains6 Nov 28, 2023

RainbowMango left a comment

RainbowMango Nov 29, 2023

Rains6 Nov 29, 2023

RainbowMango Nov 29, 2023

Rains6 Nov 29, 2023

RainbowMango Nov 29, 2023

jwcesign Nov 29, 2023 •

edited

Loading

jwcesign left a comment

RainbowMango left a comment

karmada-bot commented Nov 29, 2023

	const ControllerName = "mcs-controller"
	const ControllerName = "multiclusterservice-controller"

	MCSControllerFinalizer = "karmada.io/mcs-controller"
	MCSControllerFinalizer = "karmada.io/multiclusterservice-controller"

		// MCSServiceAppliedConditionType is indicates the condition type of mcs service applied.
		MCSServiceAppliedConditionType = "ServiceApplied"

feat: List&watch mcs and service, reconcile the work in execution namespace #4323

feat: List&watch mcs and service, reconcile the work in execution namespace #4323

Conversation

Rains6 commented Nov 25, 2023 • edited by RainbowMango Loading

codecov-commenter commented Nov 25, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwcesign commented Nov 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwcesign Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

jwcesign left a comment

Choose a reason for hiding this comment

RainbowMango left a comment

Choose a reason for hiding this comment

karmada-bot commented Nov 29, 2023

Rains6 commented Nov 25, 2023 •

edited by RainbowMango

Loading

codecov-commenter commented Nov 25, 2023 •

edited

Loading

jwcesign Nov 29, 2023 •

edited

Loading