Move Taint/Toleration helpers to component-helpers repo #98445

damemi · 2021-01-26T19:05:16Z

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This is part of the goal for scheduling to remove dependencies on internal
packages for the scheduling framework. It also provides these functions in an
external location for other components and projects to import.

Which issue(s) this PR fixes:
Ref #89930 and #91782

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

/sig scheduling

k8s-ci-robot · 2021-01-26T19:05:23Z

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

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.

damemi

/cc @ingvagabund

ingvagabund · 2021-01-27T12:11:51Z

staging/src/k8s.io/component-helpers/scheduling/corev1/helpers.go

+// TolerationsTolerateTaintsWithFilter checks if given tolerations tolerates
+// all the taints that apply to the filter in given taint list.
+// DEPRECATED: Please use FindMatchingUntoleratedTaint instead.
+func TolerationsTolerateTaintsWithFilter(tolerations []v1.Toleration, taints []v1.Taint, applyFilter taintsFilterFunc) bool {


Time to drop it given it's deprecated?

Yeah I thought about that. Updated with it in a new commit

staging/src/k8s.io/component-helpers/scheduling/corev1/helpers_test.go

damemi · 2021-01-28T17:26:48Z

/retest

damemi · 2021-02-01T15:15:06Z

rebased
/cc @lavalamp
please take a look for API review, thanks!

This is part of the goal for scheduling to remove dependencies on internal packages for the scheduling framework. It also provides these functions in an external location for other components and projects to import.

damemi · 2021-02-01T17:01:20Z

/retest

lavalamp · 2021-02-01T22:39:56Z

/lgtm
/approve

k8s-ci-robot · 2021-02-01T22:40:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, lavalamp

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:

~~pkg/apis/core/OWNERS~~ [lavalamp]
~~pkg/controller/daemon/OWNERS~~ [lavalamp]
~~pkg/kubelet/OWNERS~~ [lavalamp]
~~pkg/scheduler/OWNERS~~ [damemi,lavalamp]
~~staging/src/k8s.io/component-helpers/scheduling/OWNERS~~ [damemi,lavalamp]
~~test/OWNERS~~ [lavalamp]

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

k8s-ci-robot · 2021-02-01T23:17:38Z

@damemi: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kind-ipv6	`578ff3e`	link	`/test pull-kubernetes-e2e-kind-ipv6`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

fejta-bot · 2021-02-02T01:49:09Z

/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 or /hold comment for consistent failures.

Huang-Wei · 2021-02-02T23:52:12Z

This seems to be cause regression in scheduler perf integration test:

F0202 23:48:44.559482 3887158 perf_utils.go:104] Error listing nodes: there are currently no ready, schedulable nodes in the cluster
goroutine 92 [running]:
k8s.io/kubernetes/vendor/k8s.io/klog/v2.stacks(0xc000a7b001, 0xc004d62000, 0x85, 0x14d)
        /root/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:1026 +0xb9
k8s.io/kubernetes/vendor/k8s.io/klog/v2.(*loggingT).output(0x71fffc0, 0xc000000003, 0x0, 0x0, 0xc001c7a7e0, 0x70b6404, 0xd, 0x68, 0x0)
        /root/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:975 +0x19b
k8s.io/kubernetes/vendor/k8s.io/klog/v2.(*loggingT).printf(0x71fffc0, 0xc000000003, 0x0, 0x0, 0x0, 0x0, 0x48e4cbb, 0x17, 0xc0077c3770, 0x1, ...)
        /root/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:750 +0x191
k8s.io/kubernetes/vendor/k8s.io/klog/v2.Fatalf(...)
        /root/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/klog/v2/klog.go:1514
k8s.io/kubernetes/test/integration/framework.(*IntegrationTestNodePreparer).PrepareNodes(0xc005f56940, 0x0, 0xc005f0d770, 0x5172660)
        /root/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/integration/framework/perf_utils.go:104 +0xa26
k8s.io/kubernetes/test/integration/scheduler_perf.runWorkload(0xc0000f5680, 0xc000112b40, 0xc000200d00, 0x0, 0x0, 0x0)
        /root/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/integration/scheduler_perf/scheduler_perf_test.go:337 +0x121d
k8s.io/kubernetes/test/integration/scheduler_perf.BenchmarkPerfScheduling.func1.1(0xc0000f5680)
        /root/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/integration/scheduler_perf/scheduler_perf_test.go:299 +0x16a
testing.(*B).runN(0xc0000f5680, 0x1)
        /usr/local/go/src/testing/benchmark.go:191 +0xeb
testing.(*B).run1.func1(0xc0000f5680)
        /usr/local/go/src/testing/benchmark.go:231 +0x57
created by testing.(*B).run1
        /usr/local/go/src/testing/benchmark.go:224 +0x7f

alculquicondor · 2021-02-03T14:13:45Z

staging/src/k8s.io/component-helpers/scheduling/corev1/helpers.go

+// all the filtered taints, and returns the first taint without a toleration
+// Returns true if there is an untolerated taint
+// Returns false if all taints are tolerated
+func FindMatchingUntoleratedTaint(taints []v1.Taint, tolerations []v1.Toleration, inclusionFilter taintsFilterFunc) (v1.Taint, bool) {


A function that returns a negated boolean (Untolerated) is going to cause problems (it already did #98703), and we better fix it before the release is cut. The problem is that you would usually run into situation where you have to say !untolerated, which is a double negation.

In this case, it would be simpler to make this function just return a pointer. And perhaps we can have another exported function that uses this and returns true or false, where true means that it passes all taints.

The problem is that the deprecated function TolerationsTolerateTaintsWithFilter which we removed had the opposite behavior (returned true if all the taints were tolerated). So this should only affect people who were using the deprecated function, where we had to update their semantics (which caused the regression).

On its own, I don't think this function is too confusing. It's searching for Untolerated Taints, returning true if there are any. But if it would be clearer for some use cases to wrap this and negate it to search for Tolerated Taints, returning true if all are tolerated then that's fine.

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 26, 2021

k8s-ci-robot requested review from alculquicondor and gmarek January 26, 2021 19:06

k8s-ci-robot added area/kubelet area/test sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jan 26, 2021

damemi commented Jan 26, 2021

View reviewed changes

k8s-ci-robot requested a review from ingvagabund January 26, 2021 19:11

ingvagabund reviewed Jan 27, 2021

View reviewed changes

ingvagabund reviewed Jan 28, 2021

View reviewed changes

staging/src/k8s.io/component-helpers/scheduling/corev1/helpers_test.go Outdated Show resolved Hide resolved

damemi force-pushed the taint-toleration-internal-helpers branch 2 times, most recently from b436767 to d402165 Compare January 28, 2021 15:50

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2021

damemi force-pushed the taint-toleration-internal-helpers branch from d402165 to 80182aa Compare February 1, 2021 15:12

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2021

damemi force-pushed the taint-toleration-internal-helpers branch from 80182aa to eec5175 Compare February 1, 2021 15:13

k8s-ci-robot requested review from lavalamp and removed request for gmarek February 1, 2021 15:15

damemi force-pushed the taint-toleration-internal-helpers branch from eec5175 to b5d0ff3 Compare February 1, 2021 15:24

Move Taint/Toleration helpers to component-helpers repo

578ff3e

This is part of the goal for scheduling to remove dependencies on internal packages for the scheduling framework. It also provides these functions in an external location for other components and projects to import.

damemi force-pushed the taint-toleration-internal-helpers branch from b5d0ff3 to 578ff3e Compare February 1, 2021 16:06

SergeyKanzhelev added this to Triage in SIG Node CI/Test Board Feb 1, 2021

k8s-ci-robot assigned lavalamp Feb 1, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2021

k8s-ci-robot merged commit f27318c into kubernetes:master Feb 2, 2021

SIG Node CI/Test Board automation moved this from Triage to Done Feb 2, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 2, 2021

Huang-Wei mentioned this pull request Feb 3, 2021

Fix a bug that untolerated variable is used as tolerated #98703

Merged

alculquicondor reviewed Feb 3, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Taint/Toleration helpers to component-helpers repo #98445

Move Taint/Toleration helpers to component-helpers repo #98445

damemi commented Jan 26, 2021

k8s-ci-robot commented Jan 26, 2021

damemi left a comment

ingvagabund Jan 27, 2021

damemi Jan 27, 2021

damemi commented Jan 28, 2021

damemi commented Feb 1, 2021

damemi commented Feb 1, 2021

lavalamp commented Feb 1, 2021

k8s-ci-robot commented Feb 1, 2021

k8s-ci-robot commented Feb 1, 2021

fejta-bot commented Feb 2, 2021

Huang-Wei commented Feb 2, 2021

alculquicondor Feb 3, 2021

damemi Feb 3, 2021

Move Taint/Toleration helpers to component-helpers repo #98445

Move Taint/Toleration helpers to component-helpers repo #98445

Conversation

damemi commented Jan 26, 2021

k8s-ci-robot commented Jan 26, 2021

damemi left a comment

Choose a reason for hiding this comment

ingvagabund Jan 27, 2021

Choose a reason for hiding this comment

damemi Jan 27, 2021

Choose a reason for hiding this comment

damemi commented Jan 28, 2021

damemi commented Feb 1, 2021

damemi commented Feb 1, 2021

lavalamp commented Feb 1, 2021

k8s-ci-robot commented Feb 1, 2021

k8s-ci-robot commented Feb 1, 2021

fejta-bot commented Feb 2, 2021

Huang-Wei commented Feb 2, 2021

alculquicondor Feb 3, 2021

Choose a reason for hiding this comment

damemi Feb 3, 2021

Choose a reason for hiding this comment