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

TAS: introduce dedicated TAS label #3271

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

mimowo
Copy link
Contributor

@mimowo mimowo commented Oct 21, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:

This label will be used in two places:

  • to trigger reconcile from TopologyUngater only for pods managed by TAS, without relying on the fact that the "workload"
    annotation or "podset" label is only used for TAS
  • used when computing usage from non-TAS pods, by selector which avoids TAS pods

Which issue(s) this PR fixes:

Part of #2724

Special notes for your reviewer:

No release note here, as I added one already for TAS in the API PR.

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. labels Oct 21, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo

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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 21, 2024
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 21, 2024
@mimowo
Copy link
Contributor Author

mimowo commented Oct 21, 2024

/assign @PBundyra @gabesaba @tenzen-y

Copy link

netlify bot commented Oct 21, 2024

Deploy Preview for kubernetes-sigs-kueue canceled.

Name Link
🔨 Latest commit 41f961f
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67163e54c7153400096b8de8

@mimowo mimowo mentioned this pull request Oct 21, 2024
@PBundyra
Copy link
Contributor

/lgtm
/hold for at least one more pair of eyes

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 21, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: f8ffe1da94130543750110e71ea0a08145dd95a8

@mimowo
Copy link
Contributor Author

mimowo commented Oct 21, 2024

/hold
I synced on this with @tenzen-y offline and I will investigate if we could use indexer to use a "virtual" field for it based on the Workload object indicated by the kueue.x-k8s.io/workload annotation

@tenzen-y
Copy link
Member

I discussed this dedicated label with @mimowo offline.
In conclusion, we will investigate the solution to store TAS usage in the Workload object.

So, for a while, we will keep this open.

@PBundyra
Copy link
Contributor

I discussed this dedicated label with @mimowo offline. In conclusion, we will investigate the solution to store TAS usage in the Workload object.

So, for a while, we will keep this open.

I believe this label in contrary to storing this information in Workload object, brings value from the user perspective. Users can easily filter out pods that were scheduled by TAS using kubectl, which can be useful for scripts and debugging

@mimowo
Copy link
Contributor Author

mimowo commented Oct 21, 2024

I agree, which is another argument with the label rather than indexed virtual field, wdyt @tenzen-y ?

@mimowo mimowo force-pushed the tas-dedicated-label branch from 1a61a69 to 41f961f Compare October 21, 2024 11:43
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2024
@tenzen-y
Copy link
Member

tenzen-y commented Oct 21, 2024

I agree, which is another argument with the label rather than indexed virtual field, wdyt @tenzen-y ?

I discussed this with @mimowo offline.
The TAS label is mainly used by internal mechanisms, and if we justify the label to identify which kueue scheduling mechanism handles the Pod, that will justify adding labels kueue.x-k8s.io/generic-scheduler: "true", or future new Kueue scheduling mechanism labels.

The TAS label implies giving special treatment to the TAS in the Kueue, but I don't think we should do it. We should handle all scheduling mechanisms fairly, and users should not care about the internal mechanism (generic scheduling vs. TAS).
If batch admins want to deeply debug the Kueue behavior, admins can refer to the Workload object.

Hence, if possible, we should store the information in the Workload object.
But, I will approve this label solution for now since the release schedule is slightly tight, and the TAS label is completely guarded by the Alpha feature gate, and the label can be removed in the next release.

cc: @PBundyra

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.

Thanks!
/lgtm

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

LGTM label has been added.

Git tree hash: 0a0db8a2f26c2cf21e719179b0da52910941f814

@tenzen-y
Copy link
Member

/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 21, 2024
@k8s-ci-robot k8s-ci-robot merged commit a8188b5 into kubernetes-sigs:main Oct 21, 2024
16 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v0.9 milestone Oct 21, 2024
PBundyra pushed a commit to PBundyra/kueue that referenced this pull request Nov 5, 2024
kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024
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. 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-none Denotes a PR that doesn't merit a release note. 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.

5 participants