Use k8s.io/utils/lru instead of github.com/golang/groupcache/lru #128507

dims · 2024-11-02T02:19:44Z

What type of PR is this?

Drop last remaining reference to github.com/golang/groupcache/lru since we already have k8s.io/utils/lru for a while now.

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

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

dims · 2024-11-04T13:29:48Z

/kind cleanup

aojea · 2024-11-04T13:42:55Z

pkg/controller/garbagecollector/garbagecollector_test.go

@@ -2654,7 +2654,7 @@ func assertState(s state) step {
 				}
 				if len(s.absentOwnerCache) != ctx.gc.absentOwnerCache.cache.Len() {
 					// only way to inspect is to drain them all, but that's ok because we're failing the test anyway
-					ctx.gc.absentOwnerCache.cache.OnEvicted = func(key lru.Key, item interface{}) {
+					err := ctx.gc.absentOwnerCache.cache.SetEvictionFunc(func(key lru.Key, item interface{}) {


only needed for a test?

I still think this can be racy without a lock in SetEvictionFunc, but in this specific use, it doesn't really matter (this is helping debug an already-failed test). I do think a follow-up in utils to make SetEvictionFunc threadsafe would be good

pohly · 2024-11-04T13:44:16Z

vendor/k8s.io/utils/lru/lru.go

+	if c.cache.OnEvicted != nil {
+		return fmt.Errorf("lru cache eviction function is already set")
+	}
+	c.cache.OnEvicted = f


Does this not need a mutex lock?

The assumption seems to be that this gets called before concurrent use starts, but a one-time mutex lock would trivial and avoid that assumption.

The overall PR LGTM, so not a blocker as far as I am concerned.

asked exactly the same https://github.com/kubernetes/utils/pull/318/files#r1826961649 😄

great minds think alike @pohly and @aojea :)

/pony party

Agree it's not a blocker for this PR since we only use this in a single spot in an already-failed test, but a follow-up to make it threadsafe sgtm

aojea · 2024-11-04T13:45:59Z

LGTM

k8s-ci-robot · 2024-11-04T13:48:51Z

@dims:

In response to this:

great minds think alike @pohly and @aojea :)

/pony party

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-sigs/prow repository.

liggitt · 2024-11-04T14:33:46Z

staging/src/k8s.io/client-go/tools/record/events_cache.go

-	f.Lock()
-	defer f.Unlock()


this is guarding more than just the cache get/add ... also the rate limiter assignment to the record (which is not thread-safe, and ensuring we don't get concurrent accept calls to the record's rate limiter

the golang rate limiter is thread safe, that is wrapped by us

liggitt

/lgtm
/approve

liggitt · 2024-11-04T14:51:32Z

vendor/k8s.io/utils/lru/lru.go

+	if c.cache.OnEvicted != nil {
+		return fmt.Errorf("lru cache eviction function is already set")
+	}
+	c.cache.OnEvicted = f


Agree it's not a blocker for this PR since we only use this in a single spot in an already-failed test, but a follow-up to make it threadsafe sgtm

k8s-ci-robot · 2024-11-04T14:51:43Z

LGTM label has been added.

Git tree hash: c9008c3ddd07f09e649bb3692fb8287f0482d274

k8s-ci-robot · 2024-11-04T14:51:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, liggitt, saschagrunert

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:

~~OWNERS~~ [dims,liggitt]
~~hack/OWNERS~~ [dims,liggitt]
~~pkg/controller/garbagecollector/OWNERS~~ [dims,liggitt]
~~pkg/controller/resourceclaim/OWNERS~~ [dims,liggitt]
~~pkg/kubelet/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/apimachinery/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/cli-runtime/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/client-go/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/cloud-provider/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/cluster-bootstrap/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/code-generator/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/component-base/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/component-helpers/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/controller-manager/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/cri-client/OWNERS~~ [liggitt,saschagrunert]
~~staging/src/k8s.io/csi-translation-lib/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/dynamic-resource-allocation/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/endpointslice/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/kube-controller-manager/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/kube-proxy/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/kube-scheduler/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/kubectl/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/kubelet/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/metrics/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/mount-utils/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/pod-security-admission/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/sample-cli-plugin/OWNERS~~ [dims,liggitt]
~~staging/src/k8s.io/sample-controller/OWNERS~~ [dims,liggitt]
~~vendor/OWNERS~~ [dims,liggitt]

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

Signed-off-by: Davanum Srinivas <davanum@gmail.com>

liggitt · 2024-11-04T16:05:36Z

/lgtm

k8s-ci-robot · 2024-11-04T16:05:42Z

LGTM label has been added.

Git tree hash: 434ead3dd0fcc477db365ca7481018f91c3ac97b

fedebongio · 2024-11-05T21:11:05Z

/triage accepted

k8s-ci-robot requested review from bart0sh and caesarxuchao November 2, 2024 02:20

dims force-pushed the use-k8s.io/utils/lru-instead-of-github.com/golang/groupcache/lru branch from e692401 to 2ae2bd0 Compare November 2, 2024 02:23

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

dims mentioned this pull request Nov 2, 2024

Add a helper method to set eviction function after construction kubernetes/utils#318

Merged

dims force-pushed the use-k8s.io/utils/lru-instead-of-github.com/golang/groupcache/lru branch from 2ae2bd0 to 3e04366 Compare November 4, 2024 12:05

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Nov 4, 2024

saschagrunert approved these changes Nov 4, 2024

View reviewed changes

aojea reviewed Nov 4, 2024

View reviewed changes

pohly reviewed Nov 4, 2024

View reviewed changes

liggitt reviewed Nov 4, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2024

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 4, 2024

Use k8s.io/utils/lru instead of github.com/golang/groupcache/lru

2b0592e

Signed-off-by: Davanum Srinivas <davanum@gmail.com>

dims force-pushed the use-k8s.io/utils/lru-instead-of-github.com/golang/groupcache/lru branch from af098ac to 2b0592e Compare November 4, 2024 15:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2024

k8s-ci-robot requested review from liggitt and saschagrunert November 4, 2024 15:51

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

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 4, 2024

k8s-ci-robot merged commit 7a4d755 into kubernetes:master Nov 4, 2024
22 checks passed

k8s-ci-robot added this to the v1.32 milestone Nov 4, 2024

pohly mentioned this pull request Nov 5, 2024

kube-scheduler: enhance performance for DRA structured parameters #127277

Merged

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use k8s.io/utils/lru instead of github.com/golang/groupcache/lru #128507

Use k8s.io/utils/lru instead of github.com/golang/groupcache/lru #128507

dims commented Nov 2, 2024 •

edited

Loading

dims commented Nov 4, 2024

aojea Nov 4, 2024

dims Nov 4, 2024

aojea Nov 4, 2024

liggitt Nov 4, 2024

pohly Nov 4, 2024

aojea Nov 4, 2024

dims Nov 4, 2024

liggitt Nov 4, 2024

aojea commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

liggitt Nov 4, 2024

aojea Nov 4, 2024

liggitt left a comment

liggitt Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

liggitt commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

fedebongio commented Nov 5, 2024

Use k8s.io/utils/lru instead of github.com/golang/groupcache/lru #128507

Use k8s.io/utils/lru instead of github.com/golang/groupcache/lru #128507

Conversation

dims commented Nov 2, 2024 • edited Loading

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

dims commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

liggitt commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

fedebongio commented Nov 5, 2024

dims commented Nov 2, 2024 •

edited

Loading