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

Replace work propagation.karmada.io/instruction annotation with work Suspension field #5386

Open
XiShanYongYe-Chang opened this issue Aug 16, 2024 · 19 comments · May be fixed by #6043
Open
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Milestone

Comments

@XiShanYongYe-Chang
Copy link
Member

In the past, we've used the annotation propagation.karmada.io/instruction to mark Work propagation instruction, when it's value is suppressed, it indicates that the resource should not be propagated.

// PropagationInstruction is used to mark a resource(like Work) propagation instruction.
// Valid values includes:
// - suppressed: indicates that the resource should not be propagated.
//
// 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"

In the proposal Support for cluster-level resource propagation pause and resume capabilities, we add the Suspension filed in the Work API, as stated in the proposal, we can replace work propagation.karmada.io/instruction annotation with work Suspension field.

We can mark the propagation.karmada.io/instruction annotation as deprecated, replace the annotation with the Suspension field in the code logic, and finally remove the annotation.

@XiShanYongYe-Chang
Copy link
Member Author

/help

Looking forward to interested people to participate in this work. Feel free to communicate if you have any questions.

@karmada-bot
Copy link
Collaborator

@XiShanYongYe-Chang:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

Looking forward to interested people to participate in this work. Feel free to communicate if you have any questions.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@karmada-bot karmada-bot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Aug 16, 2024
@a7i
Copy link
Contributor

a7i commented Aug 16, 2024

I would be interested in contributing :)

/assign

@XiShanYongYe-Chang
Copy link
Member Author

Thanks a lot ^-^ @a7i

@a7i
Copy link
Contributor

a7i commented Sep 24, 2024

Two areas I could use some guidance on:

  1. Migration of existing Work that is suspended via annotation. How do you think this should be done?

  2. Querying Work by label Selector. It seems to me that we should leave the label for work created by MultiClusterService. Thoughts?

    workList, err := helper.GetWorksByLabelsSet(ctx, c, labels.Set{
    networkingv1alpha1.MultiClusterServicePermanentIDLabel: mcsID,
    })

@XiShanYongYe-Chang
Copy link
Member Author

Hi @a7i

For 1: Existing works that use the propagation.karmada.io/instruction label are collected from member clusters and are used to wrap the EndpointSlice objects; they do not have corresponding resource templates in the Karmada control plane. Therefore, I understand that they will not introduce upgrade issues and can be processed directly.

For 2, I‘m sorry, I didn't quite understand the meaning of the question.

@a7i
Copy link
Contributor

a7i commented Nov 14, 2024

Hi @XiShanYongYe-Chang I meant to reference this line of code:

err = c.List(ctx, workList, client.MatchingLabels{util.PropagationInstruction: util.PropagationInstructionSuppressed})

As you can see above, it performs a List using label-selector. Do you propose leaving that label for a more efficient query?

@XiShanYongYe-Chang
Copy link
Member Author

Hi @a7i, Good question. From the list work here, keeping the label is really helpful for efficient query. Unless a new label replaces the current label, let's pause the task and wait for a more complete plan before starting it. How do you think?

@vie-serendipity
Copy link
Contributor

vie-serendipity commented Dec 25, 2024

From the list work here, keeping the label is really helpful for efficient query.

Why not directly use field selector? I don‘t know whether label query is way more efficient than field query.

@XiShanYongYe-Chang
Copy link
Member Author

Why not directly use field selector?

Which field is used for filtering?

@RainbowMango RainbowMango added this to the v1.13 milestone Dec 25, 2024
@vie-serendipity
Copy link
Contributor

Which field is used for filtering?

I mean spec.suspendDispatching, does it achieve original purpose?

@XiShanYongYe-Chang
Copy link
Member Author

Thanks @vie-serendipity, maybe it's possible.

After thinking about it, we can follow the following steps:

  1. Set the suspendDispatching field for the collected work.
  2. Replace the logic of the labelselector.
  3. Deprecated the propagation.karmada.io/instruction label.

How do you think?

@vie-serendipity
Copy link
Contributor

I think it could work.

@XiShanYongYe-Chang
Copy link
Member Author

Would you like to do it?

@vie-serendipity
Copy link
Contributor

Sure, I will submit a PR after I implement it.

@XiShanYongYe-Chang
Copy link
Member Author

Thanks a lot @vie-serendipity~
/assign @vie-serendipity

Hi @a7i, @vie-serendipity would like to move forward with this task using the filed selector. How do you think?

@XiShanYongYe-Chang
Copy link
Member Author

Hi @vie-serendipity I want to know if you have a plan of action for the next step, not to rush, just to know.

@vie-serendipity
Copy link
Contributor

I am currently developing and expect to submit a PR before the Spring Festival.

@a7i
Copy link
Contributor

a7i commented Jan 8, 2025

That's a good idea, thanks @vie-serendipity !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants