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

support job scheduling #898

Merged
merged 1 commit into from
Nov 12, 2021
Merged

support job scheduling #898

merged 1 commit into from
Nov 12, 2021

Conversation

Garrybest
Copy link
Member

Signed-off-by: Garrybest [email protected]

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add Job scheduling in scheduler. We divide replicas by spec.parallelism. And the spec.completions could be divided weighted by scheduling result.

Which issue(s) this PR fixes:
Fixes #893

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

support job scheduling

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Nov 1, 2021
@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 1, 2021
@pigletfly
Copy link
Contributor

will #899 cover this case?

@Garrybest
Copy link
Member Author

will #899 cover this case?

I'm not sure. Do you have any ideas? @RainbowMango

@RainbowMango
Copy link
Member

Not sure, let me take a look.
I didn't notice Job should be scheduled.

@mrlihanbo
Copy link

I wonder how to aggregate status of job. We collect job status in detector before: https://github.com/karmada-io/karmada/blob/master/pkg/detector/aggregate_status.go#L193. We will set JobComplete condition when all job run in member cluster succeed. But with job scheduling, the meaning here need to adapt.

@Garrybest
Copy link
Member Author

But with job scheduling, the meaning here need to adapt.

Hi @mrlihanbo, this is an interesting question. Should we need more adaption? I thought the aggregation does not need any more changes. When we divide replicas, we still mark job completed after all jobs in member clusters succeed right? Could you please give more details?

@Garrybest
Copy link
Member Author

Leave a user story here.

@Garrybest
Copy link
Member Author

Well, I think the startTime and completionTime of job status should be adapted when job replicas are divided. I have tested a job that has been divided into 2 member clusters. When the job finished, I didn't see the startTime and completionTime in karmada control plane.

@mrlihanbo
Copy link

But with job scheduling, the meaning here need to adapt.

Hi @mrlihanbo, this is an interesting question. Should we need more adaption? I thought the aggregation does not need any more changes. When we divide replicas, we still mark job completed after all jobs in member clusters succeed right? Could you please give more details?

Hi @Garrybest , for parallel Jobs with a fixed completion count, you should set .spec.completions to the number of completions needed. So the aggregation need more adaption, if we still mark job completed after all jobs in member clusters succeed, the behavior is not the same with native kubernetes, right?

Hi, @Garrybest my fault, it seems like we can still mark job completed after all jobs in member clusters succeed.

@Garrybest
Copy link
Member Author

Hi, @Garrybest my fault, it seems like we can still mark job completed after all jobs in member clusters succeed.

Hi, @mrlihanbo. I was just about to reply😄. I found the relevant code here. If succeeded is more than completions, the job could be considered as Complete.

@mrlihanbo
Copy link

Hi, @Garrybest my fault, it seems like we can still mark job completed after all jobs in member clusters succeed.

Hi, @mrlihanbo. I was just about to reply😄. I found the relevant code here. If succeeded is more than completions, the job could be considered as Complete.

Hi, @Garrybest. I just wonder if there exists a scenario that complete = succeeded >= *job.Spec.Completions and I found that the scenario seems to be not exist. For example, cluster A specifies completions as 10, and finally 11 pod complete. cluster B specifies completions as 10, and finally 9 pod complete. The job in cluster B failed, but when aggregate status, the number of complete pod will be 20 which equal to job.Spec.Completions. But it seems like the scenario will not happen.

@Garrybest
Copy link
Member Author

Hi, @Garrybest. I just wonder if there exists a scenario that complete = succeeded >= *job.Spec.Completions and I found that the scenario seems to be not exist.

Got it. I guess this scenario rarely happens. I'd prefer to consider this situation as JobFailed.

@RainbowMango
Copy link
Member

/assign @mrlihanbo

@RainbowMango
Copy link
Member

Generlly looks good.

diff --git a/pkg/controllers/binding/common.go b/pkg/controllers/binding/common.go
index 422efde5..21b00e9a 100644
--- a/pkg/controllers/binding/common.go
+++ b/pkg/controllers/binding/common.go
@@ -84,12 +84,13 @@ func ensureWork(c client.Client, workload *unstructured.Unstructured, overrideMa
        var jobCompletions []workv1alpha2.TargetCluster
        var jobHasCompletions = false
        if workload.GetKind() == util.JobKind {
-               completions, ok, err := unstructured.NestedInt64(workload.Object, util.SpecField, util.CompletionsField)
+               completions, found, err := unstructured.NestedInt64(workload.Object, util.SpecField, util.CompletionsField)
                if err != nil {
                        return err
                }
-               if jobHasCompletions = ok; jobHasCompletions {
+               if found {
                        jobCompletions = util.DivideReplicasByTargetCluster(targetClusters, int32(completions))
+                       jobHasCompletions = true
                }
        }

@@ -125,6 +126,9 @@ func ensureWork(c client.Client, workload *unstructured.Unstructured, overrideMa
                                        clonedWorkload.GetKind(), clonedWorkload.GetNamespace(), clonedWorkload.GetName(), targetCluster.Name, err)
                                return err
                        }
+
+                       // For a work queue Job that usually leaves .spec.completions unset, in that case, we skip setting this field.
+                       // Refer to: https://kubernetes.io/docs/concepts/workloads/controllers/job/#parallel-jobs.
                        if jobHasCompletions {
                                err = applyReplicaSchedulingPolicy(clonedWorkload, int64(jobCompletions[i].Replicas), util.CompletionsField)
                                if err != nil {

Signed-off-by: Garrybest <[email protected]>
@RainbowMango
Copy link
Member

@mrlihanbo Do you have any questions or comments?

@RainbowMango
Copy link
Member

/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2021
@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 Nov 12, 2021
@karmada-bot karmada-bot merged commit 48c2bfb into karmada-io:master Nov 12, 2021
@Garrybest Garrybest deleted the pr_job branch November 12, 2021 02:15
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/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Job Scheduling
5 participants