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
Unset gated podinfo InitialAttemptTimestamp in addToActiveQ #118049
Unset gated podinfo InitialAttemptTimestamp in addToActiveQ #118049
Conversation
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
/ok-to-test |
3ea2543
to
8d1849b
Compare
8d1849b
to
46687c3
Compare
/retest |
I'll leave this review to @Huang-Wei |
/test pull-kubernetes-verify |
@helayoty the latest logic looks good, it's just some UTs may have assumed Timestamp/InitialAttemptTimestamp in another way, so may need to adapt those UTs. |
@Huang-Wei , This UT doesn’t make sense with the change as it tests that the |
I'm wondering if it's possible to leverage help a fakeClock? Like, initialize the podInfo with a fakeClock to ensure the time is the same when it's created and firstly time added to activeQ, and then |
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. Would you mind squashing the commits?
And could you fill the release note section? as it changes how pod_scheduling_duration_seconds
is calculated.
cc @sanposhiho for awareness.
Yes I've reached out to Heba offline. Let's hold this pr for now. |
Per discussion in https://github.com/kubernetes/enhancements/pull/4065/files#r1231487092, we reached a concensus to change the semantics of /lgtm |
LGTM label has been added. Git tree hash: dd97b62bc824836afb219596dc1aecaa2efbb811
|
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
@@ -1870,6 +1870,38 @@ func TestPerPodSchedulingMetrics(t *testing.T) { | |||
t.Fatalf("Failed to pop a pod %v", err) | |||
} | |||
checkPerPodSchedulingMetrics("Attempt twice with update", t, pInfo, 2, timestamp) | |||
|
|||
// Case 4: A gated pod is created and scheduled after lifting gate. The queue operations are |
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.
We should be using a test table for these cases or split into separate test functions. That should make tests easier to debug in the future.
This is beyond the scope of this PR, but please consider working on this after this has been merged and cherry-picked.
Although, I would prefer this new test goes into a separate function in this PR.
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.
we do have some test code not following the table-driven test style. it can be followed-up with other tests.
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.
I agree to follow the table-driven test style. I didn't want to have an out-of-scope change in this PR. I will follow up with another PR to address this comment.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, helayoty, Huang-Wei 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 |
@helayoty could you squash the commits, and it will be ease cherrypicking. |
3a314ba
to
4e01556
Compare
4e01556
to
fb973f6
Compare
@Huang-Wei Done. |
Signed-off-by: Heba Elayoty <hebaelayoty@gmail.com>
63974a0
to
902c711
Compare
/lgtm And feel free to /unhold when the CIs are all green. Thanks @helayoty . Sorry for the discussion back and forth, but I think it helps clarify the overall picture. |
LGTM label has been added. Git tree hash: 20b865fd72eb4874a6fa5bc36e1be9558181870e
|
/retest |
/unhold |
Can you prepare a cherry-pick for the |
…8049-upstream-release-1.27 Automated cherry pick of #118049: Unset gated pod info timestamp in addToActiveQ
InitialAttemptTimestamp
to exclude the preEnqueue failure time from the pod_scheduling_duration_seconds metric.InitialAttemptTimestamp
.TestPerPodSchedulingMetrics
unit testWhat type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #117979
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: