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

Preemption e2e test #71281

Merged
merged 3 commits into from Nov 30, 2018

Conversation

@Huang-Wei
Copy link
Member

Huang-Wei commented Nov 20, 2018

What type of PR is this?

/kind bug

What this PR does / why we need it:

A follow up on #70898 to ensure code coverage.

Special notes for your reviewer:

Integration test is really challenging to reproduce original issue because:

  • Preemptor pod is (always) head of activeQ, and will be firstly re-tried

    This is because when preemptor pod is trying to preempt low-priority pods, it's firstly updated (status.NominatedNode), hence put back into activeQ (a max-heap) at the head.

  • Retry of preemptor pod is always succeeded

    This is because in integration, kubelet isn't involved, you have to set deleteGracePeriod=0 to get the pod deleted (see slack link for details). But with that, it's actually not mimicing the real world - in real world, pod have terminationGracePeriod, so they get into Terminating status first, and then be deleted. Say they're sort of deleted asynchronously. But in integration test, it's sort of synchronous (required to have 0 terminationGracePeriod)

Based on above reasons, I switched back to using an e2e test to simulate the issue. And I've tweaked it from checking podEvents to checking number of "podNamesSeen" of innocent pods only, which should be good.

BTW, there is a separate commit to address #70898 (comment).

Does this PR introduce a user-facing change?:

NONE

/sig scheduling

Huang-Wei added some commits Nov 16, 2018

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Nov 20, 2018

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Nov 20, 2018

I suppose the plan is to merge it in 1.13, so

/priority critical-urgent

@k82cn

This comment has been minimized.

Copy link
Member

k82cn commented Nov 26, 2018

still for 1.13?

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Nov 26, 2018

@k82cn maybe impossible...

@bsalamat should we postpone it to next release?

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Nov 28, 2018

Friendly ping @bsalamat..

@k82cn

This comment has been minimized.

Copy link
Member

k82cn commented Nov 28, 2018

should be 1.14 or 1.13.1 at least :)

@bsalamat
Copy link
Contributor

bsalamat left a comment

Thanks, @Huang-Wei!

Show resolved Hide resolved test/e2e/scheduling/preemption.go
Show resolved Hide resolved test/e2e/scheduling/preemption.go Outdated
Show resolved Hide resolved test/e2e/scheduling/preemption.go Outdated
framework.Logf("length of pods created so far: %v", len(podNamesSeen))

// count pods number of RepliaSet3, if it's more than orignal replicas (4)
// then means its pods has been preempted once or more

This comment has been minimized.

@bsalamat

bsalamat Nov 29, 2018

Contributor

You rely on the fact that preempted pods are not garbage collected yet. I think this should be fine with the current implementation of the GC. Please at least update the comment and explain this logic for future reference.

This comment has been minimized.

@Huang-Wei

Huang-Wei Nov 29, 2018

Author Member

I don't see how GC is involved. Isn't this just (1) scheduler preempts (call clientset.Delete() to delete) pod(s), and then (2) RelicaSet controller tries to reconcile to spawn new pod(s)?

// then means its pods has been preempted once or more
rs3PodsSeen := 0
for podName := range podNamesSeen {
if strings.HasPrefix(podName, "rs-pod3") {

This comment has been minimized.

@bsalamat

bsalamat Nov 29, 2018

Contributor

Why does this apply only to RS3? It should apply to RS1 and RS2 as well, right? We should ensure that no more than 10 replicas of RS1 and no more than 8 replicas of RS2 exist and all those replicas are either deleted or unscheduled.

This comment has been minimized.

@Huang-Wei

Huang-Wei Nov 29, 2018

Author Member

True, that's exactly what I did in previous commit... (thought it's subtle)

Will update.

Show resolved Hide resolved test/e2e/scheduling/preemption.go
@bsalamat
Copy link
Contributor

bsalamat left a comment

/lgtm
/approve

Thanks, @Huang-Wei!

Show resolved Hide resolved test/e2e/scheduling/preemption.go

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 29, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 29, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Nov 29, 2018

/retest

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

Huang-Wei commented Nov 30, 2018

/retest

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 30, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 30, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit dda9637 into kubernetes:master Nov 30, 2018

18 checks passed

cla/linuxfoundation Huang-Wei authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@Huang-Wei Huang-Wei deleted the Huang-Wei:preemption-e2e-test branch Dec 4, 2018

k8s-ci-robot added a commit that referenced this pull request Dec 5, 2018

Merge pull request #71724 from Huang-Wei/automated-cherry-pick-of-#70…
…898-#71281-upstream-release-1.12

Automated cherry pick of #70898: ensure scheduler preemptor behaves in an efficient/correct #71281: add an e2e test to verify preemption running path

k8s-ci-robot added a commit that referenced this pull request Dec 10, 2018

Merge pull request #71884 from Huang-Wei/automated-cherry-pick-of-#70…
…898-#71281-upstream-release-1.11

Automated cherry pick of #70898: ensure scheduler preemptor behaves in an efficient/correct #71281: add an e2e test to verify preemption running path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment