-
Notifications
You must be signed in to change notification settings - Fork 228
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
[metrics] Add quota_reserved_wait_time_seconds #1977
[metrics] Add quota_reserved_wait_time_seconds #1977
Conversation
Hi @mbobrovskyi. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
/assign |
@trasc: /release-note-edit must be used with a release note block. In response to this:
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/test-infra repository. |
@trasc: /release-note-edit must be used with a single release note block. In response to this:
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/test-infra repository. |
@trasc: /release-note-edit must be used with a release note block. In response to this:
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/test-infra repository. |
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
LGTM label has been added. Git tree hash: c9c1f4ba99a1d94dc4b267733da06e284f80195a
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost lgtm
queuedTime := wl.CreationTimestamp.Time | ||
if c := apimeta.FindStatusCondition(wl.Status.Conditions, kueue.WorkloadRequeued); c != nil { | ||
queuedTime = c.LastTransitionTime.Time | ||
} | ||
queuedWaitTime := time.Since(queuedTime) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extract this as a helper in workload.go to commonize the analogous code with scheduler.go. Maybe QueuedWaitTime(workload)
pkg/workload/workload.go
Outdated
condition := metav1.Condition{ | ||
Type: kueue.WorkloadRequeued, | ||
Status: metav1.ConditionTrue, | ||
LastTransitionTime: metav1.Now(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is set inside apimeta.SetStatusCondition(
apis/kueue/v1beta1/workload_types.go
Outdated
// WorkloadRequeued means that the Workload was requeued. | ||
// The possible reasons for this condition are: | ||
// - On setting QuotaReserved=False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// WorkloadRequeued means that the Workload was requeued. | |
// The possible reasons for this condition are: | |
// - On setting QuotaReserved=False | |
// WorkloadRequeued means that the Workload was requeued due to eviction. |
Let's make the comment meaningful for the users who may observe the conditions.
Kueue will set QuotaReserved=False
for evicted workloads.
Request(corev1.ResourceCPU, "2"). | ||
Obj() | ||
|
||
ginkgo.By("checking the first workload gets created and admitted", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ginkgo.By("checking the first workload gets created and admitted", func() { | |
ginkgo.By("checking the first workload gets created and gets quota reserved", func() { |
apis/kueue/v1beta1/workload_types.go
Outdated
@@ -302,6 +302,11 @@ const ( | |||
// more detailed information. The more detailed reasons should be prefixed | |||
// by one of the "base" reasons. | |||
WorkloadPreempted = "Preempted" | |||
|
|||
// WorkloadRequeued means that the Workload was requeued. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make sure we have an integration test which checks that:
Requeued=True
whenQuotaReserved=True
andAdmitted=False
Requeued=False
when the workload is evicted (IIUC this may still need implementation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requeued=False when the workload is evicted (IIUC this may still need implementation)
Could you please explain this case? Is it possible to set Requeued to false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, we could set it to false
, and when we set Admitted=True
condition. We do something similar for the other conditions when settting QuotaReserved: true
:
kueue/pkg/workload/workload.go
Lines 405 to 417 in 7880819
if evictedCond := apimeta.FindStatusCondition(w.Status.Conditions, kueue.WorkloadEvicted); evictedCond != nil { | |
evictedCond.Status = metav1.ConditionFalse | |
evictedCond.Reason = "QuotaReserved" | |
evictedCond.Message = "Previously: " + evictedCond.Message | |
evictedCond.LastTransitionTime = metav1.Now() | |
} | |
// reset Preempted condition if present. | |
if preemptedCond := apimeta.FindStatusCondition(w.Status.Conditions, kueue.WorkloadPreempted); preemptedCond != nil { | |
preemptedCond.Status = metav1.ConditionFalse | |
preemptedCond.Reason = "QuotaReserved" | |
preemptedCond.Message = "Previously: " + preemptedCond.Message | |
preemptedCond.LastTransitionTime = metav1.Now() | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTOH, setting it to False
is not required to compute the metric, so maybe it does not need to be in this PR. WDYT @alculquicondor ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, we don't want to change the condition to False on admission.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you just add an assert the it remains True when QuotaReserved=True, and Admitted=True?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition should transition to False
in the following scenarios:
- When workload.spec.active=false. It should return to
True
when it's reactivated. - When evicted due to WaitForPodsReady, the workload will temporarily be in backoff. During this time,
Requeued
should be false.
Although I think we can leave these for a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
Leaving lgtm to @mimowo
@@ -633,6 +633,12 @@ func TestReconciler(t *testing.T) { | |||
Reason: "Pending", | |||
Message: "The workload is deactivated", | |||
}). | |||
Condition(metav1.Condition{ | |||
Type: kueue.WorkloadRequeued, | |||
Status: metav1.ConditionTrue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct. If the workload is deactivated, Requeued should be False.
apis/kueue/v1beta1/workload_types.go
Outdated
@@ -302,6 +302,11 @@ const ( | |||
// more detailed information. The more detailed reasons should be prefixed | |||
// by one of the "base" reasons. | |||
WorkloadPreempted = "Preempted" | |||
|
|||
// WorkloadRequeued means that the Workload was requeued. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition should transition to False
in the following scenarios:
- When workload.spec.active=false. It should return to
True
when it's reactivated. - When evicted due to WaitForPodsReady, the workload will temporarily be in backoff. During this time,
Requeued
should be false.
Although I think we can leave these for a follow up.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, mbobrovskyi 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 |
/lgtm |
LGTM label has been added. Git tree hash: d813f5fde2705e8a3177eeb00b5269ec4a993645
|
I opened the issue to track the remaining work for transitioning: #2038. |
/release-note-edit
|
@mimowo As I can see this PR, the semantics of the existing metric ( |
I don't think |
Oh, I see. Thank you for the clarifications. I was thinking that the metric had been changed.
That makes sense. |
What type of PR is this?
/kind bug
/kind feature
What this PR does / why we need it:
Fixed metrics admitted_workloads_total, admission_wait_time_seconds.
Added metrics quota_reserved_workloads_total, quota_reserved_wait_time_seconds.
Which issue(s) this PR fixes:
Fixes #1961
Does this PR introduce a user-facing change?