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

add stable preempt label #68

Merged
merged 3 commits into from
Apr 25, 2022
Merged

Conversation

flyhighzy
Copy link

related to volcano pr #2149 and issue #2075
add a new label "volcano.sh/preempt-stable-time“ to indicate cool down time for preemptees

@volcano-sh-bot
Copy link
Collaborator

Welcome @flyhighzy!

It looks like this is your first PR to volcano-sh/apis 馃帀.

Thank you, and welcome to Volcano. 😃

@@ -42,6 +42,9 @@ const QueueNameAnnotationKey = GroupName + "/queue-name"
// PodPreemptable is the key of preemptable
const PodPreemptable = "volcano.sh/preemptable"

// PodPreemptStableTime is the key of preempt-stable-time, value's unit is second
const PodPreemptStableTime = "volcano.sh/preempt-stable-time"
Copy link
Member

@shinytang6 shinytang6 Apr 14, 2022

Choose a reason for hiding this comment

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

l am ok about the label name, but l prefer to use “ns”, “us”, “ms”, “s”, “m”, “h” as time units

Copy link
Member

Choose a reason for hiding this comment

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

why do we introduce this label? can we reuse deletiontimestamp?

Copy link
Author

Choose a reason for hiding this comment

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

why do we introduce this label? can we reuse deletiontimestamp?

this label is used by scheduler to protect these pods not to be preempted in some time, not to make them deleted after some time

Copy link
Author

@flyhighzy flyhighzy Apr 15, 2022

Choose a reason for hiding this comment

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

l am ok about the label name, but l prefer to use “ns”, “us”, “ms”, “s”, “m”, “h” as time units

good point, comments updated and I'll add unit support in pr #2149

@shinytang6
Copy link
Member

/cc @Thor-wl @k82cn

@@ -42,6 +42,10 @@ const QueueNameAnnotationKey = GroupName + "/queue-name"
// PodPreemptable is the key of preemptable
const PodPreemptable = "volcano.sh/preemptable"

// PodPreemptStableTime is the key of preempt-stable-time, value's format "600s","10m"
// Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h".
const PodPreemptStableTime = "volcano.sh/preempt-stable-time"
Copy link
Member

Choose a reason for hiding this comment

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

Whether job need a prempt-stable-time as well. If needs, it seems using PreemptStableTime is more reasonable.

Copy link
Author

Choose a reason for hiding this comment

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

yes! already updated

zhaoying added 3 commits April 15, 2022 10:56
Signed-off-by: zhaoying <zhaoying@bilibili.com>
Signed-off-by: zhaoying <zhaoying@bilibili.com>
Signed-off-by: zhaoying <zhaoying@bilibili.com>
Copy link
Member

@shinytang6 shinytang6 left a comment

Choose a reason for hiding this comment

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

/lgtm

@flyhighzy
Copy link
Author

PR is ready, thanks for the advice~
/assign @Thor-wl @k82cn @william-wang

@Thor-wl
Copy link
Contributor

Thor-wl commented Apr 25, 2022

/approve

@volcano-sh-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Thor-wl

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

@volcano-sh-bot volcano-sh-bot merged commit 7459ae8 into volcano-sh:master Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants