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

TAS: Introduce API #3235

Merged
merged 1 commit into from
Oct 16, 2024
Merged

TAS: Introduce API #3235

merged 1 commit into from
Oct 16, 2024

Conversation

mimowo
Copy link
Contributor

@mimowo mimowo commented Oct 15, 2024 •
edited by k8s-ci-robot
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Part of #2724

Special notes for your reviewer:

Some of the field API comments are improved compared to KEP. I will update the KEP accordingly to make them in-sync.

Does this PR introduce a user-facing change?

Support Topology Aware Scheduling (TAS) in Kueue in the Alpha version, along with the new Topology API
to specify the ordered list of node labels corresponding to the different levels of hierarchy in data-centers
(like racks or blocks).

Additionally, we introduce the pair of Job-level annotations: `http://kueue.x-k8s.io/podset-required-topology`
and `kueue.x-k8s.io/podset-preferred-topology` which users can use to indicate their preference for the
Jobs to run all their Pods within a topology domain at the indicated level.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 15, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 15, 2024
@mimowo
Copy link
Contributor Author

mimowo commented Oct 15, 2024

/cc @PBundyra @tenzen-y

@netlify Netlify
Copy link

netlify bot commented Oct 15, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 5d3b497
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/670ff42369bedc0008996654

@k8s-ci-robot k8s-ci-robot requested a review from PBundyra October 15, 2024 11:19
tenzen-y
tenzen-y reviewed Oct 15, 2024
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

It seems that the Workload API is quite different from the KEP, right?
https://github.com/kubernetes-sigs/kueue/tree/main/keps/2724-topology-aware-schedling#internal-apis

apis/kueue/v1alpha1/tas_types.go Show resolved Hide resolved
apis/kueue/v1alpha1/tas_types.go Outdated Show resolved Hide resolved
apis/kueue/v1alpha1/tas_types.go Outdated Show resolved Hide resolved
apis/kueue/v1alpha1/tas_types.go Outdated Show resolved Hide resolved
apis/kueue/v1alpha1/tas_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/resourceflavor_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/workload_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/workload_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/workload_types.go Outdated Show resolved Hide resolved
apis/kueue/v1beta1/workload_types.go Outdated Show resolved Hide resolved
@mimowo
Copy link
Contributor Author

mimowo commented Oct 15, 2024 •
edited
Loading

It seems that the Workload API is quite different from the KEP, right?
https://github.com/kubernetes-sigs/kueue/tree/main/keps/2724-topology-aware-schedling#internal-apis

Yes, it is changed a bit, I will send the Workload API level in a separate PR, and will send a PR to update the KEP.

@tenzen-y
Copy link
Member

It seems that the Workload API is quite different from the KEP, right?
https://github.com/kubernetes-sigs/kueue/tree/main/keps/2724-topology-aware-schedling#internal-apis

Yes, it is changed a bit, I will send the Workload API level in a separate PR, and will send a PR to update the KEP.

I would like to merge this PR after we check how this Workload API changes give to the TAS scheduler in the updated KEP.
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 15, 2024
@mimowo mimowo changed the title TAS: Introduce Alpha API TAS: Introduce API Oct 15, 2024
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 15, 2024
@mimowo mimowo force-pushed the tas-alpha-api branch 2 times, most recently from 4f190aa to 8979606 Compare October 15, 2024 14:00
@mimowo
Copy link
Contributor Author

mimowo commented Oct 15, 2024

I would like to merge this PR after we check how this Workload API changes give to the TAS scheduler in the updated KEP.

Makes sense, I prepared the update: #3237

@mimowo mimowo force-pushed the tas-alpha-api branch 2 times, most recently from 24dc823 to a9111f8 Compare October 16, 2024 10:48
@mimowo
Copy link
Contributor Author

mimowo commented Oct 16, 2024 •
edited
Loading

FYI, the content here is in-sync with the KEP update PR (after the sync today).

@PBundyra
Copy link
Contributor

PBundyra commented Oct 16, 2024 •
edited
Loading

nit: Could we start comments with lowercase letters?

@mimowo
Copy link
Contributor Author

mimowo commented Oct 16, 2024

nit: Could we start comments with lowercase letters?

I use lowercase for fields which follows https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status. It makes sense so tat in the generated user-facing docs it looks consistent with the json fields.

As for labels and annotations I don't think there is such a convention. We also don't generate docs based on the declarations. Finally all consts here are commented with upper-case.

tenzen-y
tenzen-y reviewed Oct 16, 2024
View reviewed changes
Copy link
Member

@tenzen-y tenzen-y left a comment

Choose a reason for hiding this comment

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

Thank you!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 16, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: cd5fe89295a9751d5580fc9733fea98140fa9755

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, tenzen-y

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

@mimowo
Copy link
Contributor Author

mimowo commented Oct 16, 2024

I would like to merge this PR after we check how this Workload API changes give to the TAS scheduler in the updated KEP.
/hold

As the API update in KEP is merged
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2024
@k8s-ci-robot k8s-ci-robot merged commit 587a6fc into kubernetes-sigs:main Oct 16, 2024
16 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.9 milestone Oct 16, 2024
@mimowo
Copy link
Contributor Author

mimowo commented Oct 28, 2024

/release-note-edit

Support Topology Aware Scheduling (TAS) in Kueue in the Alpha version, along with the new Topology API
to specify the ordered list of node labels corresponding to the different levels of hierarchy in data-centers
(like racks or blocks).

Additionally, we introduce the pair of Job-level annotations: `http://kueue.x-k8s.io/podset-required-topology`
and `kueue.x-k8s.io/podset-preferred-topology` which users can use to indicate their preference for the
Jobs to run all their Pods within a topology domain at the indicated level.

PBundyra pushed a commit to PBundyra/kueue that referenced this pull request Nov 5, 2024
@mimowo @PBundyra
TAS API (kubernetes-sigs#3235)
b1cd145
kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024
@mimowo @kannon92
TAS API (kubernetes-sigs#3235)
7f637d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PBundyra PBundyra PBundyra left review comments

@tenzen-y tenzen-y tenzen-y left review comments

@trasc trasc Awaiting requested review from trasc

Assignees

@tenzen-y tenzen-y

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.9
Development

Successfully merging this pull request may close these issues.
4 participants
@mimowo @tenzen-y @PBundyra @k8s-ci-robot