Unset gated podinfo InitialAttemptTimestamp in addToActiveQ by helayoty · Pull Request #118049 · kubernetes/kubernetes

helayoty · 2023-05-16T15:44:48Z

Unset podInfo InitialAttemptTimestamp to exclude the preEnqueue failure time from the pod_scheduling_duration_seconds metric.
Use a pointer to an InitialAttemptTimestamp.
Add a new case for the TestPerPodSchedulingMetrics unit test

What 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?

the `pod_scheduling_duration_seconds` metrics won't consider the time when a Pod fails PreEnqueue (like being gated).

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2023-05-16T15:44:56Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Details

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.

helayoty · 2023-05-16T15:47:13Z

/ok-to-test

helayoty · 2023-05-16T18:39:02Z

/retest

alculquicondor · 2023-05-16T19:27:34Z

I'll leave this review to @Huang-Wei

helayoty · 2023-05-16T19:51:18Z

/test pull-kubernetes-verify

Huang-Wei · 2023-05-16T22:44:30Z

@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.

helayoty · 2023-05-16T23:20:00Z

@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 InitialAttemptTimestamp is not changed with a different number of attempts while it will change now. Should we remove it? Or reverse the logic to make sure it fails if it didn't change?

Huang-Wei · 2023-05-17T20:53:40Z

@Huang-Wei , This UT doesn’t make sense with the change as it tests that the InitialAttemptTimestamp is not changed with a different number of attempts while it will change now. Should we remove it? Or reverse the logic to make sure it fails if it didn't change?

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 step() when it needs to do another attempt or movement.

Huang-Wei

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.

Huang-Wei · 2023-05-23T21:34:28Z

Yes I've reached out to Heba offline. Let's hold this pr for now.

Huang-Wei · 2023-06-20T21:19:18Z

Per discussion in https://github.com/kubernetes/enhancements/pull/4065/files#r1231487092, we reached a concensus to change the semantics of pod_scheduling_duration_seconds to exclude PreEnqueue by default, and then cherrypick the changes along with a few follow-ups.

/lgtm
/approve

cc @alculquicondor

k8s-ci-robot · 2023-06-20T21:19:24Z

LGTM label has been added.

Details

Git tree hash: dd97b62bc824836afb219596dc1aecaa2efbb811

alculquicondor

/approve

alculquicondor · 2023-06-21T18:00:48Z

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.

we do have some test code not following the table-driven test style. it can be followed-up with other tests.

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.

k8s-ci-robot · 2023-06-21T18:03:50Z

[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

Details

Needs approval from an approver in each of these files:

~~pkg/scheduler/OWNERS~~ [Huang-Wei,alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Huang-Wei · 2023-06-21T18:51:34Z

@helayoty could you squash the commits, and it will be ease cherrypicking.

helayoty · 2023-06-21T20:42:45Z

@helayoty could you squash the commits, and it will be ease cherrypicking.

@Huang-Wei Done.

Signed-off-by: Heba Elayoty <hebaelayoty@gmail.com>

Huang-Wei · 2023-06-21T21:20:15Z

/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.

k8s-ci-robot · 2023-06-21T21:20:22Z

LGTM label has been added.

Details

Git tree hash: 20b865fd72eb4874a6fa5bc36e1be9558181870e

helayoty · 2023-06-21T22:18:07Z

/retest

Huang-Wei · 2023-06-21T23:35:09Z

/unhold

alculquicondor · 2023-06-22T12:12:04Z

Can you prepare a cherry-pick for the release-1.27 branch?

…8049-upstream-release-1.27 Automated cherry pick of #118049: Unset gated pod info timestamp in addToActiveQ

helayoty added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label May 16, 2023

helayoty requested review from Huang-Wei and alculquicondor May 16, 2023 15:44

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label May 16, 2023

k8s-ci-robot requested review from denkensk and kerthcet May 16, 2023 15:45

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 16, 2023

Huang-Wei reviewed May 16, 2023

View reviewed changes

Comment thread pkg/scheduler/framework/plugins/queuesort/priority_sort.go Outdated

helayoty force-pushed the sched-queue-gated-timestamp branch from 3ea2543 to 8d1849b Compare May 16, 2023 18:35

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 16, 2023

helayoty force-pushed the sched-queue-gated-timestamp branch from 8d1849b to 46687c3 Compare May 16, 2023 18:37

helayoty requested a review from Huang-Wei May 16, 2023 21:18

Huang-Wei reviewed May 16, 2023

View reviewed changes

Comment thread pkg/scheduler/internal/queue/scheduling_queue.go Outdated

Comment thread pkg/scheduler/internal/queue/scheduling_queue.go Outdated

Comment thread pkg/scheduler/internal/queue/scheduling_queue.go Outdated

helayoty requested a review from Huang-Wei May 16, 2023 21:55

helayoty changed the title ~~Unset gated pod info timestamp in addToActiveQ~~ Unset gated podinfo InitialAttemptTimestamp in addToActiveQ May 18, 2023

Huang-Wei reviewed May 19, 2023

View reviewed changes

helayoty removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 24, 2023

Huang-Wei mentioned this pull request Jun 15, 2023

KEP-3521: Add new metric for excluding PreEnqueue duration time kubernetes/enhancements#4065

Merged

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2023

alculquicondor reviewed Jun 21, 2023

View reviewed changes

helayoty force-pushed the sched-queue-gated-timestamp branch from 3a314ba to 4e01556 Compare June 21, 2023 20:36

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2023

helayoty force-pushed the sched-queue-gated-timestamp branch from 4e01556 to fb973f6 Compare June 21, 2023 20:42

helayoty requested a review from alculquicondor June 21, 2023 21:05

Unset gated pod info timestamp in addToActiveQ

902c711

Signed-off-by: Heba Elayoty <hebaelayoty@gmail.com>

helayoty force-pushed the sched-queue-gated-timestamp branch from 63974a0 to 902c711 Compare June 21, 2023 21:16

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2023

helayoty removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2023

k8s-ci-robot merged commit d0accc3 into kubernetes:master Jun 22, 2023

k8s-ci-robot added this to the v1.28 milestone Jun 22, 2023

helayoty deleted the sched-queue-gated-timestamp branch June 22, 2023 01:03

helayoty mentioned this pull request Jun 22, 2023

Automated cherry pick of #118049: Unset gated pod info timestamp in addToActiveQ #118821

Merged

k8s-ci-robot added a commit that referenced this pull request Jun 23, 2023

Merge pull request #118821 from helayoty/automated-cherry-pick-of-#11…

0539a6a

…8049-upstream-release-1.27 Automated cherry pick of #118049: Unset gated pod info timestamp in addToActiveQ

helayoty mentioned this pull request Jun 24, 2023

test: Use table-driven test for TestPerPodSchedulingMetrics #118842

Merged

Conversation

helayoty commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented May 16, 2023

Uh oh!

helayoty commented May 16, 2023

Uh oh!

Uh oh!

helayoty commented May 16, 2023

Uh oh!

alculquicondor commented May 16, 2023

Uh oh!

helayoty commented May 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Huang-Wei commented May 16, 2023

Uh oh!

helayoty commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Huang-Wei commented May 17, 2023

Uh oh!

Huang-Wei left a comment

Choose a reason for hiding this comment

Uh oh!

Huang-Wei commented May 23, 2023

Uh oh!

Huang-Wei commented Jun 20, 2023

Uh oh!

k8s-ci-robot commented Jun 20, 2023

Uh oh!

alculquicondor left a comment

Choose a reason for hiding this comment

Uh oh!

alculquicondor Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

Huang-Wei Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

helayoty Jun 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 21, 2023

Uh oh!

Huang-Wei commented Jun 21, 2023

Uh oh!

helayoty commented Jun 21, 2023

Uh oh!

Huang-Wei commented Jun 21, 2023

Uh oh!

k8s-ci-robot commented Jun 21, 2023

Uh oh!

helayoty commented Jun 21, 2023

Uh oh!

Huang-Wei commented Jun 21, 2023

Uh oh!

alculquicondor commented Jun 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

helayoty commented May 16, 2023 •

edited

Loading

helayoty commented May 16, 2023 •

edited

Loading