feat: replace instruction annotation with work suspendDispatching field #6043

vie-serendipity · 2025-01-14T03:27:17Z

What type of PR is this?

/kind deprecation
What this PR does / why we need it:
Replace propagation.karmada.io/instruction with suspendDispatching field. Annotation was just a temporary solution. With the native support of the work API, we should deprecate the use of annotations and utilize the work field instead.
Which issue(s) this PR fixes:
Fixes #5386

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Users who are still using annotations to suspend work should migrate to the `suspendDispatching` field as soon as possible.

codecov-commenter · 2025-01-14T03:41:22Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 21.27660% with 37 lines in your changes missing coverage. Please review.

Project coverage is 48.07%. Comparing base (006cf70) to head (eee26ca).

Files with missing lines	Patch %	Lines
pkg/controllers/mcs/common.go	0.00%	10 Missing ⚠️
pkg/controllers/mcs/service_export_controller.go	0.00%	9 Missing ⚠️
pkg/util/helper/predicate.go	0.00%	6 Missing and 2 partials ⚠️
cmd/agent/app/agent.go	0.00%	3 Missing ⚠️
cmd/controller-manager/app/controllermanager.go	0.00%	3 Missing ⚠️
pkg/util/helper/work.go	0.00%	2 Missing ⚠️
...clusterservice/endpointslice_collect_controller.go	50.00%	1 Missing ⚠️
.../controllers/multiclusterservice/mcs_controller.go	0.00%	0 Missing and 1 partial ⚠️

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

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6043   +/-   ##
=======================================
  Coverage   48.07%   48.07%           
=======================================
  Files         668      669    +1     
  Lines       55327    55348   +21     
=======================================
+ Hits        26597    26608   +11     
- Misses      26992    27003   +11     
+ Partials     1738     1737    -1

Flag	Coverage Δ
unittests	`48.07% <21.27%> (+<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.

chaosi-zju · 2025-01-14T10:09:03Z

/retest

XiShanYongYe-Chang · 2025-02-06T07:24:04Z

Thanks~
/assign

XiShanYongYe-Chang

Thanks a lot~

XiShanYongYe-Chang · 2025-02-06T07:37:15Z

pkg/util/work.go

+)
+
+// GetWorkSuspendDispatching will get suspendDispatching field from work spec
+func GetWorkSuspendDispatching(spec *workv1alpha1.WorkSpec) []string {


How about moving this function to the pkg/util/helper/work.go file?

XiShanYongYe-Chang · 2025-02-06T08:24:06Z

pkg/controllers/mcs/common.go

+)
+
+const (
+	workKeyIndex = "workSpec.suspendDispatching"


How about renaming this to workSuspendDispatchdingIndex?

XiShanYongYe-Chang · 2025-02-06T08:52:44Z

pkg/util/constants.go

@@ -46,7 +46,7 @@ const (
 	//
 	// Note: This instruction is intended to set on Work objects to indicate the Work should be ignored by
 	// execution controller. The instruction maybe deprecated once we extend the Work API and no other scenario want this.
-	PropagationInstruction = "propagation.karmada.io/instruction"
+	PropagationInstruction = "propagation.karmada.io/instruction" // deprecated


There are still some places in the warehouse that use the variable. Should we replace all the places that use the variable?

Now I think we should retain the handling of this label. I just replaced the usage of PropagationInstruction with suspendDispatching in the repository when creating new resources, ensuring NO new resources with the PropagationInstruction label are created. And we can remove it entirely later.

Thanks for the explanation, I think we can clean them up in the next version(v1.14).

By the way, can we mark the PropagationInstruction and the related value to be deprecated like this:

// Deprecated PropagationInstruction = "propagation.karmada.io/instruction"

XiShanYongYe-Chang · 2025-02-06T08:55:35Z

pkg/controllers/mcs/service_export_controller.go

-			util.ServiceNamespaceLabel: endpointSlice.GetNamespace(),
-			util.ServiceNameLabel:      endpointSlice.GetLabels()[discoveryv1.LabelServiceName],
-			// indicate the Work should be not propagated since it's collected resource.
-			util.PropagationInstruction:          util.PropagationInstructionSuppressed,


Can we delete this variable in the next version to consider compatibility?

I think removing this tag won't cause compatibility issues, because I've retained the handling logic for PropagationInstruction, which will still work as intended.

Yes, you are right!

XiShanYongYe-Chang · 2025-02-07T08:36:44Z

pkg/controllers/mcs/common.go

+)
+
+const (
+	workSuspendDispatchdingIndex = "workSpec.suspendDispatching"


Sorry I made a mistake in my previous comment, a typo Dispatchding -> Dispatching

ok, my mistake, I didn't double check.

XiShanYongYe-Chang · 2025-02-07T08:54:28Z

pkg/util/helper/predicate.go

+		if IsWorkSuspendDispatching(obj) {
+			klog.V(5).Infof("Ignored Work(%s/%s) %s event as dispatching is suspended.", obj.Namespace, obj.Name, eventType)
+			return false
+		}


Should such a judgment be added to other places where judgment util.GetLabelValue(obj.Labels, util.PropagationInstruction) == util.PropagationInstructionSuppressed is used, because for users who directly use this version (v1.13), the work in the environment will not have the propagation.karmada.io/instruction label.

For example:

--- a/pkg/controllers/ctrlutil/work.go +++ b/pkg/controllers/ctrlutil/work.go @@ -36,7 +36,8 @@ import ( // CreateOrUpdateWork creates a Work object if not exist, or updates if it already exists. func CreateOrUpdateWork(ctx context.Context, client client.Client, workMeta metav1.ObjectMeta, resource *unstructured.Unstructured, options ...WorkOption) error { - if workMeta.Labels[util.PropagationInstruction] != util.PropagationInstructionSuppressed { + if workMeta.Labels[util.PropagationInstruction] != util.PropagationInstructionSuppressed || + IsWorkSuspendDispatching(obj) {

I think the handling here is not necessary, we can directly remove it if we use suspendDispatching. For some other places, we cannot use suspendDispatching filter because the suspendDispatching condition for work needs to be updated in reconcile.

https://github.com/karmada-io/karmada/blob/4982157d65188c45916b06b080c46761eac89155/pkg/util/helper/predicate.go#L82C1-L85C4

I think the handling here is not necessary, we can directly remove it if we use suspendDispatching.

For a new work formed by collecting Endpointslices, this condition judgment is always valid, which is equivalent to invalid judgment.

For some other places, we cannot use suspendDispatching filter because the suspendDispatching condition for work needs to be updated in reconcile.

I'm sorry, I don't quite understand this point. Could you explain it a little bit?

I mean, when using PropagationInstruction for the execution controller's predicate, the suspendDispatching field cannot be used as a filtering condition because the execution controller needs to update the Dispatching condition.

For a new work formed by collecting Endpointslices, this condition judgment is always valid, which is equivalent to invalid judgment.

So I think we can remove this judgment directly, according to my understanding adding labels and annotations to the work's manifest should be fine.

I'm worried that if we add IsWorkSuspendDispatching(obj) in the jugement here, user-created suspended works might lose these label and annotation information. If we add IsWorkSuspendDispatching(obj) here, we might add some handle in the mutating webhook.

karmada/pkg/webhook/work/mutating.go

Lines 74 to 77 in 4982157

// Skip label/annotate the workload of Work that is not intended to be propagated.

if work.Labels[util.PropagationInstruction] != util.PropagationInstructionSuppressed {

setLabelsAndAnnotationsForWorkload(workloadObj, work)

}

Sorry about progress, I'm busy lately, I'll probably test it next week, are there any releases planned lately that require this pr?

Version 1.13 will be released at the end of February. I'm not sure if we can catch up with it. Let's follow your schedule.

I tested and it works fine. I followed the following steps to test:

Use local-up-karmada script to make a test enviroment.

Build my karmada controller manager image and use it.

Follow https://karmada.io/docs/userguide/service/multi-cluster-service to test multi cluster discovery.

Endpointslice is propagted correctly.

Thanks for your test!

I think we can move on. The current pr and master branch conflict, can you help solve it?

karmada-bot · 2025-02-25T01:46:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from xishanyongye-chang. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

feat: rename workIndex and mv GetWorkSuspendDispatching to helper feat: modify the style of Deprecated Signed-off-by: vie-serendipity <[email protected]> feat: label and annotate workloads even though work is suspended Signed-off-by: vie-serendipity <[email protected]> test: fix ut for CreateOrUpdateWork Signed-off-by: vie-serendipity <[email protected]>

karmada-bot added the kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. label Jan 14, 2025

karmada-bot requested review from chaunceyjiang, lonelyCZ and whitewindmills January 14, 2025 03:27

karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 14, 2025

vie-serendipity force-pushed the feat/suspend branch from 982414c to 2f2ae0a Compare January 14, 2025 06:32

chaosi-zju mentioned this pull request Jan 14, 2025

CI failed: Error: common chart not found in repo https://charts.bitnami.com/bitnami #6046

Closed

karmada-bot assigned XiShanYongYe-Chang Feb 6, 2025

XiShanYongYe-Chang reviewed Feb 6, 2025

View reviewed changes

vie-serendipity force-pushed the feat/suspend branch from 59e2e2b to 85f3dca Compare February 7, 2025 01:50

XiShanYongYe-Chang reviewed Feb 7, 2025

View reviewed changes

vie-serendipity force-pushed the feat/suspend branch from 85f3dca to 75d0f1d Compare February 25, 2025 01:45

vie-serendipity force-pushed the feat/suspend branch from 75d0f1d to 50dafe2 Compare February 25, 2025 01:49

vie-serendipity force-pushed the feat/suspend branch from 50dafe2 to eee26ca Compare February 25, 2025 02:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: replace instruction annotation with work suspendDispatching field #6043

feat: replace instruction annotation with work suspendDispatching field #6043

vie-serendipity commented Jan 14, 2025

codecov-commenter commented Jan 14, 2025 •

edited

Loading

chaosi-zju commented Jan 14, 2025

XiShanYongYe-Chang commented Feb 6, 2025

XiShanYongYe-Chang left a comment

XiShanYongYe-Chang Feb 6, 2025

vie-serendipity Feb 6, 2025

XiShanYongYe-Chang Feb 6, 2025

vie-serendipity Feb 6, 2025

XiShanYongYe-Chang Feb 6, 2025

vie-serendipity Feb 6, 2025

XiShanYongYe-Chang Feb 6, 2025

vie-serendipity Feb 7, 2025

XiShanYongYe-Chang Feb 6, 2025

vie-serendipity Feb 6, 2025

XiShanYongYe-Chang Feb 6, 2025

XiShanYongYe-Chang Feb 7, 2025

vie-serendipity Feb 8, 2025

XiShanYongYe-Chang Feb 7, 2025

vie-serendipity Feb 8, 2025

XiShanYongYe-Chang Feb 8, 2025

vie-serendipity Feb 8, 2025

vie-serendipity Feb 8, 2025

vie-serendipity Feb 22, 2025

XiShanYongYe-Chang Feb 22, 2025

vie-serendipity Feb 24, 2025

XiShanYongYe-Chang Feb 24, 2025

vie-serendipity Feb 24, 2025

karmada-bot commented Feb 25, 2025

	// Skip label/annotate the workload of Work that is not intended to be propagated.
	if work.Labels[util.PropagationInstruction] != util.PropagationInstructionSuppressed {
	setLabelsAndAnnotationsForWorkload(workloadObj, work)
	}

feat: replace instruction annotation with work suspendDispatching field #6043

Are you sure you want to change the base?

feat: replace instruction annotation with work suspendDispatching field #6043

Conversation

vie-serendipity commented Jan 14, 2025

codecov-commenter commented Jan 14, 2025 • edited Loading

Codecov Report

chaosi-zju commented Jan 14, 2025

XiShanYongYe-Chang commented Feb 6, 2025

XiShanYongYe-Chang 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmada-bot commented Feb 25, 2025

codecov-commenter commented Jan 14, 2025 •

edited

Loading