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.

Sign up for GitHub

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

Jump to bottom

Fix: TAS assignment error #3937

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Fix: TAS assignment error #3937

wants to merge 2 commits into from

Conversation

kerthcet
Copy link
Contributor

@kerthcet kerthcet commented Jan 7, 2025

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #3887

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 7, 2025
@kerthcet
Copy link
Contributor Author

kerthcet commented Jan 7, 2025

/test

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kerthcet
Once this PR has been reviewed and has the lgtm label, please assign tenzen-y for approval. For more information see the 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:

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

@k8s-ci-robot
Copy link
Contributor

@kerthcet: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-kueue-build-image-main

/test pull-kueue-test-e2e-main-1-29

/test pull-kueue-test-e2e-main-1-30

/test pull-kueue-test-e2e-main-1-31

/test pull-kueue-test-integration-main

/test pull-kueue-test-multikueue-e2e-main

/test pull-kueue-test-scheduling-perf-main

/test pull-kueue-test-tas-e2e-main

/test pull-kueue-test-unit-main

/test pull-kueue-verify-main

Use /test all to run all jobs.

In response to this:

/test

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.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 7, 2025
@kerthcet
Copy link
Contributor Author

kerthcet commented Jan 7, 2025

/test all

@netlify Netlify
Copy link

netlify bot commented Jan 7, 2025 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 4f61106
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/677de1a07c1db50008757dd2

@kerthcet
fix nil map
4f61106 
Signed-off-by: kerthcet <[email protected]>
@kerthcet
Copy link
Contributor Author

kerthcet commented Jan 7, 2025

/test all

mimowo
mimowo reviewed Jan 7, 2025
View reviewed changes
Copy link
Contributor

@mimowo mimowo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a unit test in scheduler_test.go TestScheduleForTAS for the problematic scenario.

pkg/cache/tas_flavor_snapshot.go
for k, v := range singlePodRequest {
usage[k] = v * int64(domain.state)
}
s.addUsage(domain.id, usage)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this will work fine for now as TAS is not combined with cohorts, and so only one workload is considered in each scheduling cycle on the snapshot. However, this will not work well when we start supporting cohorts - in that case usage coming from the assignment phase for a workload that is de-prioritized in a cohort will consume the capacity, but scheduler should operate on "clean" capacity.

One way to go about it in the future would be to remember usage coming from "Inflight" PodSets, and clean it at the end of the assignment phase, before the actual scheduling phase.

Since I don't see it problematic in the current assumptions (no cohorts) I believe a TODO with #3761 is sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add tests later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean when having time, or in a follow up PR? My preference would be to add the test in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean when I have time later, have codes without tests is terrible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I would like to include the fix in 0.10.1, and tentatively planning next week. Do you think you would add the test by then? I can also help to write it

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 8, 2025
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 8, 2025
@mimowo mimowo mentioned this pull request Jan 9, 2025
Open
34 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mimowo mimowo mimowo left review comments

@mbobrovskyi mbobrovskyi Awaiting requested review from mbobrovskyi

@PBundyra PBundyra Awaiting requested review from PBundyra

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Discussion][TAS] Best effort placements for pods in lower tier of topology
3 participants
@kerthcet @k8s-ci-robot @mimowo