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

Simplify taint manager workqueue keys #65350

Merged
merged 1 commit into from Oct 18, 2018

Conversation

@liggitt
Member

liggitt commented Jun 22, 2018

This greatly reduces the memory size of the workqueue keys, and allows the queue to dedupe rapid events from the same objects, like other controllers do

Builds on #65339

Fixes #65325

/sig scheduling

improves memory use and performance when processing large numbers of pods containing tolerations
@liggitt

This comment has been minimized.

Member

liggitt commented Jun 22, 2018

@liggitt

This comment has been minimized.

Member

liggitt commented Jun 22, 2018

/retest

@wojtek-t

This comment has been minimized.

Member

wojtek-t commented Jun 22, 2018

/lgtm
/approve

Thanks!

@fejta-bot

This comment has been minimized.

fejta-bot commented Jun 22, 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.

@liggitt

This comment has been minimized.

Member

liggitt commented Jun 22, 2018

/retest

@dims

This comment has been minimized.

Member

dims commented Jun 22, 2018

@liggitt the change in pkg/controller/nodelifecycle/scheduler/taint_manager.go is in this PR as well as #65339 i am guessing we don't need the 65339 then?

@dims

This comment has been minimized.

Member

dims commented Jun 22, 2018

/test pull-kubernetes-e2e-kops-aws

@liggitt

This comment has been minimized.

Member

liggitt commented Jun 22, 2018

i am guessing we don't need the 65339 then?

I am planning to cherrypick that one back to 1.8

@fejta-bot

This comment has been minimized.

fejta-bot commented Jun 22, 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.

@liggitt

This comment has been minimized.

Member

liggitt commented Jun 22, 2018

/retest

@fejta-bot

This comment has been minimized.

fejta-bot commented Jun 22, 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.

fejta-bot commented Jun 23, 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.

@liggitt liggitt force-pushed the liggitt:simplify-taint-manager-key branch from cc8e67e to d2d3149 Jun 23, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jun 23, 2018

@wgliang

This comment has been minimized.

Member

wgliang commented Jun 23, 2018

/lgtm

@fejta-bot

This comment has been minimized.

fejta-bot commented Jun 23, 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.

@liggitt liggitt force-pushed the liggitt:simplify-taint-manager-key branch from a1f0aa9 to 6ff04a0 Oct 16, 2018

@spiffxp

This comment has been minimized.

Member

spiffxp commented Oct 17, 2018

/milestone v1.13
I'm adding this to the v1.13 milestone since it relates to kubernetes/enhancements#166 which is currently planned for v1.13

@k8s-ci-robot k8s-ci-robot added this to the v1.13 milestone Oct 17, 2018

@AishSundar

This comment has been minimized.

Contributor

AishSundar commented Oct 17, 2018

/retest

@Huang-Wei Huang-Wei referenced this pull request Oct 17, 2018

Open

Taint Based Eviction #166

@wojtek-t

This comment has been minimized.

Member

wojtek-t commented Oct 17, 2018

the failures look related, which is concerning, because it seems to mean something is depending on level-driven processing of the nodes/pod updates. will keep digging as I have time

Yes - those seem to be real failures not flakes.
What is even more terrifying is that for e.g. gce-100-performance is (in terms of taints) preatty much relying only on making nodes ready/schedulable.

So something is really terrifying in this PR.

@AishSundar

This comment has been minimized.

Contributor

AishSundar commented Oct 17, 2018

@wojtek-t @liggitt based on the comment above I am assuming this issue is serious enough to block Taint based eviction going to Beta in 1.13. @Huang-Wei 's comment indicated this PR to be a "nice to have" performance enhancement.

Considering the scalability regressions we saw with taint nodes in 1.12 I am concerned with the signals here already. Please let us know of how critical is this fix for the feature to go to Beta in 1.13. Thanks

@liggitt liggitt force-pushed the liggitt:simplify-taint-manager-key branch from 6ff04a0 to 3f8c033 Oct 17, 2018

@liggitt liggitt force-pushed the liggitt:simplify-taint-manager-key branch from 3f8c033 to 9503c64 Oct 17, 2018

@liggitt liggitt changed the title from WIP - Simplify taint manager workqueue keys to Simplify taint manager workqueue keys Oct 17, 2018

@liggitt

This comment has been minimized.

Member

liggitt commented Oct 17, 2018

/retest

@liggitt

This comment has been minimized.

Member

liggitt commented Oct 17, 2018

/test pull-kubernetes-e2e-gke

@liggitt

This comment has been minimized.

Member

liggitt commented Oct 17, 2018

@AishSundar - the issues with this PR have been resolved, and tests are green. this should help with taint/toleration scaleability issues

@liggitt

This comment has been minimized.

Member

liggitt commented Oct 17, 2018

/hold cancel

@Huang-Wei

This comment has been minimized.

Member

Huang-Wei commented Oct 17, 2018

Thanks @liggitt !!

@AishSundar

This comment has been minimized.

Contributor

AishSundar commented Oct 17, 2018

Thanks @liggitt and @Huang-Wei

@wojtek-t

This comment has been minimized.

Member

wojtek-t commented Oct 17, 2018

great.

/lgtm

@Huang-Wei

This comment has been minimized.

Member

Huang-Wei commented Oct 17, 2018

/test pull-kubernetes-e2e-kops-aws

@liggitt

This comment has been minimized.

Member

liggitt commented Oct 17, 2018

/retest

1 similar comment
@Huang-Wei

This comment has been minimized.

Member

Huang-Wei commented Oct 18, 2018

/retest

@k8s-ci-robot k8s-ci-robot merged commit 6f4b768 into kubernetes:master Oct 18, 2018

18 checks passed

cla/linuxfoundation liggitt 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 Job succeeded.
Details
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
@@ -211,8 +198,8 @@ func (tc *NoExecuteTaintManager) Run(stopCh <-chan struct{}) {
glog.V(0).Infof("Starting NoExecuteTaintManager")
for i := 0; i < UpdateWorkerSize; i++ {
tc.nodeUpdateChannels = append(tc.nodeUpdateChannels, make(chan *nodeUpdateItem, NodeUpdateChannelSize))
tc.podUpdateChannels = append(tc.podUpdateChannels, make(chan *podUpdateItem, podUpdateChannelSize))
tc.nodeUpdateChannels = append(tc.nodeUpdateChannels, make(chan nodeUpdateItem, NodeUpdateChannelSize))

This comment has been minimized.

@k82cn

k82cn Oct 18, 2018

Member

Do we still need channel group? The performance of TaintNodeByCondition is acceptable by one channel.

This comment has been minimized.

@wojtek-t

wojtek-t Oct 18, 2018

Member

I would say "now".
I think that once we will be able to bump default qps limits in large clusters, it may appear to be too slow. But yeah - it's mostly guessing...

This comment has been minimized.

@liggitt

liggitt Oct 18, 2018

Member

TaintNodeByCondition is distributing a single update queue among multiple workers.

This has two update queues to coordinate, so the exact solution will look different. I think we can do something simpler than this two-layer async, but we need to do it in a way that still lets us fan out workers and give node update handling priority when handling pod update events.

@liggitt liggitt deleted the liggitt:simplify-taint-manager-key branch Oct 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment