Use checksums instead of fsyncs to avoid slow discovery caching on MacOS #110851

negz · 2022-06-29T03:54:10Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Short version: Avoids kubectl "hanging" for 10+ seconds while writing to cache when an API server has many API groups.

Part of the API discovery cache uses an HTTP RoundTripper that transparently caches responses to disk. The upstream implementation of the disk cache is hard coded to call Sync() on every file it writes. This has noticeably poor performance on modern Macs, which ask their controllers to flush all the way to persistent storage because Go uses the F_FULLFSYNC fnctl. Apple recommends minimizing this behavior in order to avoid degrading performance and increasing disk wear.

The content of the discovery cache is not critical; it is indeed just a cache and can be recreated by hitting the API servers' discovery endpoints. This commit replaces upstream httpcache's diskcache implementation with a similar implementation that can use SHA256 sums to detect corrupted cache entries at read-time. When such an entry is detected (e.g. because it was only partially flushed to permanent storage before the host lost power) the cache will report a miss. This causes httpcache to fall back to its underlying HTTP transport (i.e. the real API server) and re-cache the resulting value.

Apart from adding SHA256 sums and avoiding calling fsync this implementation differs from upstream httpcache's diskcache package in that it also uses SHA256 sums rather than MD5 sums of cache keys in order to sanitize cache filenames.

Note that I'm adding this implementation here rather than submitting it upstream because httpcache does not appear to have been touched for the last three years - it seems unlikely that they are accepting contributions.

Which issue(s) this PR fixes:

Fixes #110753

Special notes for your reviewer:

Since this is intended to improve slow performance I've added a benchmark test. You can see there's a dramatic speed improvement on MacOS:

# Before
$ go test -v -bench . -run '^Bench'
goos: darwin
goarch: arm64
pkg: k8s.io/client-go/discovery/cached/disk
BenchmarkDiskCache
BenchmarkDiskCache-10    	      60	  22272642 ns/op
PASS
ok  	k8s.io/client-go/discovery/cached/disk	2.582s

# After
$ go test -v -bench . -run '^Bench'
goos: darwin
goarch: arm64
pkg: k8s.io/client-go/discovery/cached/disk
BenchmarkDiskCache
BenchmarkDiskCache-10    	    1534	    761469 ns/op
PASS
ok  	k8s.io/client-go/discovery/cached/disk	1.483s

I also see a decent speed improvement on Linux, though note that I'm running this test inside a Parallels VM:

# Before
$ go test -v -bench . -run '^Bench'
goos: linux
goarch: arm64
pkg: k8s.io/client-go/discovery/cached/disk
BenchmarkDiskCache
BenchmarkDiskCache-6         812           1316323 ns/op
PASS
ok      k8s.io/client-go/discovery/cached/disk  1.240s

# After
$ go test -v -bench . -run '^Bench'
goos: linux
goarch: arm64
pkg: k8s.io/client-go/discovery/cached/disk
BenchmarkDiskCache
BenchmarkDiskCache-6        1513            772180 ns/op
PASS
ok      k8s.io/client-go/discovery/cached/disk  1.267s

Finally, invoking kubectl against an API server with 300+ API groups now takes about a second rather than 10+ (as demonstrated in #110753).

$ bin/kubectl version
WARNING: This version information is deprecated and will be replaced with the output from kubectl version --short.  Use --output=yaml|json to get the full version.
Client Version: version.Info{Major:"1", Minor:"25+", GitVersion:"v1.25.0-alpha.2.14+4fddaf7eb2cf4e", GitCommit:"4fddaf7eb2cf4ee904e500f4af4f01f0a409d19f", GitTreeState:"clean", BuildDate:"1980-01-01T00:00:00Z", GoVersion:"go1.18.2", Compiler:"gc", Platform:"darwin/arm64"}
Kustomize Version: v4.5.4
Server Version: version.Info{Major:"1", Minor:"22+", GitVersion:"v1.22.9-eks-a64ea69", GitCommit:"540410f9a2e24b7a2a870ebfacb3212744b5f878", GitTreeState:"clean", BuildDate:"2022-05-12T19:15:31Z", GoVersion:"go1.16.15", Compiler:"gc", Platform:"linux/amd64"}
WARNING: version difference between client (1.25) and server (1.22) exceeds the supported minor version skew of +/-1

$ rmd ~/.kube/cache

# Cold cache
$ time bin/kubectl get nodes
NAME                                            STATUS   ROLES    AGE    VERSION
ip-192-168-103-188.us-west-2.compute.internal   Ready    <none>   7d6h   v1.22.9-eks-810597c
ip-192-168-63-15.us-west-2.compute.internal     Ready    <none>   7d6h   v1.22.9-eks-810597c
ip-192-168-93-0.us-west-2.compute.internal      Ready    <none>   7d6h   v1.22.9-eks-810597c
bin/kubectl get nodes  0.41s user 0.47s system 61% cpu 1.428 total

# Warm cache
$ time bin/kubectl get nodes
NAME                                            STATUS   ROLES    AGE    VERSION
ip-192-168-103-188.us-west-2.compute.internal   Ready    <none>   7d6h   v1.22.9-eks-810597c
ip-192-168-63-15.us-west-2.compute.internal     Ready    <none>   7d6h   v1.22.9-eks-810597c
ip-192-168-93-0.us-west-2.compute.internal      Ready    <none>   7d6h   v1.22.9-eks-810597c
bin/kubectl get nodes  0.17s user 0.05s system 160% cpu 0.139 total

$ bin/kubectl api-versions|wc -l
     343

$ du -hd1 ~/.kube/cache
1.7M    /Users/negz/.kube/cache/discovery
1.7M    /Users/negz/.kube/cache/http
3.4M    /Users/negz/.kube/cache

Does this PR introduce a user-facing change?

Use checksums instead of fsyncs to ensure discovery cache integrity

k8s-ci-robot · 2022-06-29T03:54:19Z

Hi @negz. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

leilajal · 2022-06-30T16:38:19Z

/assign @Jefftree
/triage accepted

Jefftree · 2022-06-30T17:07:29Z

/ok-to-test

Jefftree · 2022-06-30T17:10:04Z

/cc @lavalamp @seans3

staging/src/k8s.io/client-go/discovery/cached/disk/round_tripper.go

lavalamp · 2022-07-06T21:20:05Z

Feedback:

Hash the whole file
Use a cryptographic hash (note that, if you don't like how big they are, it's OK to only use some of the bytes for the key)

seans3 · 2022-07-25T07:03:43Z

/priority important-soon

apelisse · 2022-07-25T15:38:25Z

So by changing this hash algorithm, all currently disk-cached files will no longer be found.

Yeah, we never use that for discovery (yet), and OpenAPI changes all the time. This is as bad as installing a CRD on a cluster. I don't know if it warrants a release note.

staging/src/k8s.io/client-go/discovery/cached/disk/round_tripper.go

negz · 2022-07-27T07:05:38Z

/retest

negz · 2022-07-27T07:06:11Z

@lavalamp @seans3 PTAL - we're now SHA256 summing all the things. 😄 I've updated the PR description with the latest benchmarks - as expected it's trivially slower than the previous FNV/CRC implementation but still much much faster than the original fsync based approach to cache integrity.

This benchmark is intended to demonstrate a performance improvement gained by removing fsyncs. Refer to the below issue for more detail. kubernetes#110753 Signed-off-by: Nic Cope <nicc@rk0n.org>

Part of the API discovery cache uses an HTTP RoundTripper that transparently caches responses to disk. The upstream implementation of the disk cache is hard coded to call Sync() on every file it writes. This has noticably poor performance on modern Macs, which ask their disk controllers to flush all the way to persistant storage because Go uses the `F_FULLFSYNC` fnctl. Apple recommends minimizing this behaviour in order to avoid degrading performance and increasing disk wear. The content of the discovery cache is not critical; it is indeed just a cache and can be recreated by hitting the API servers' discovery endpoints. This commit replaces upstream httpcache's diskcache implementation with a similar implementation that can use CRC-32 checksums to detect corrupted cache entries at read-time. When such an entry is detected (e.g. because it was only partially flushed to permanent storage before the host lost power) the cache will report a miss. This causes httpcache to fall back to its underlying HTTP transport (i.e. the real API server) and re-cache the resulting value. Apart from adding CRC-32 checksums and avoiding calling fsync this implementation differs from upstream httpcache's diskcache package in that it uses FNV-32a hashes rather than MD5 hashes of cache keys in order to generate filenames. Signed-off-by: Nic Cope <nicc@rk0n.org>

This helps avoid (potentially malicious) collisions when reading and writing cache data. Signed-off-by: Nic Cope <nicc@rk0n.org>

This is a little more computationally expensive but reduces the likelihood of a potentially malicious cache collision. Signed-off-by: Nic Cope <nicc@rk0n.org>

dims · 2022-07-27T14:14:30Z

/retest

seans3 · 2022-07-27T15:27:33Z

/lgtm

negz · 2022-07-29T22:22:30Z

Hey folks, just checking in ahead of code freeze next week. Do you feel this is mergeable as is?

lavalamp · 2022-07-29T22:25:07Z

/approve

k8s-ci-robot · 2022-07-29T22:25:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lavalamp, negz, seans3

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:

~~staging/src/k8s.io/client-go/OWNERS~~ [lavalamp]
~~vendor/OWNERS~~ [lavalamp]

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

sftim · 2022-08-16T08:56:23Z

Should this PR have a release note?

csantanapr · 2022-08-18T13:59:44Z

@negz Could you provide a one-liner for release notes?

negz · 2022-08-18T20:54:20Z

@csantanapr Not sure how much I'd consider this a user facing change, but I've added a one-liner nonetheless. Thanks!

sftim · 2022-08-19T07:27:45Z

Thanks @negz

End users can observe the effects of this change without using debug tools.

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jun 29, 2022

k8s-ci-robot requested review from deads2k and logicalhan June 29, 2022 03:57

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 29, 2022

This was referenced Jun 29, 2022

Replaces http disk cache with CompactableDiskCache #109701

Closed

Add ability to disable certain CRDs on installation. crossplane/crossplane#2869

Closed

k8s-ci-robot assigned Jefftree Jun 30, 2022

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 Jun 30, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 30, 2022

k8s-ci-robot requested review from lavalamp and seans3 June 30, 2022 17:10

negz commented Jul 6, 2022

View reviewed changes

staging/src/k8s.io/client-go/discovery/cached/disk/round_tripper.go Show resolved Hide resolved

negz changed the title ~~Use checksums instead of fsyncs to manage discovery cache corruption~~ Use checksums instead of fsyncs to avoid slow discovery caching on MacOS Jul 6, 2022

lavalamp reviewed Jul 6, 2022

View reviewed changes

staging/src/k8s.io/client-go/discovery/cached/disk/round_tripper.go Outdated Show resolved Hide resolved

negz requested a review from lavalamp July 8, 2022 02:22

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2022

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 25, 2022

lavalamp reviewed Jul 26, 2022

View reviewed changes

staging/src/k8s.io/client-go/discovery/cached/disk/round_tripper.go Outdated Show resolved Hide resolved

lavalamp reviewed Jul 26, 2022

View reviewed changes

staging/src/k8s.io/client-go/discovery/cached/disk/round_tripper.go Outdated Show resolved Hide resolved

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2022

negz added 4 commits July 27, 2022 00:13

Add a benchmark for the discovery cache RoundTripper

eace469

This benchmark is intended to demonstrate a performance improvement gained by removing fsyncs. Refer to the below issue for more detail. kubernetes#110753 Signed-off-by: Nic Cope <nicc@rk0n.org>

Use sha256 to sanitize discovery HTTP cache keys

288a17f

This helps avoid (potentially malicious) collisions when reading and writing cache data. Signed-off-by: Nic Cope <nicc@rk0n.org>

Use SHA256 sums to verify discovery cache integrity

c5957c2

This is a little more computationally expensive but reduces the likelihood of a potentially malicious cache collision. Signed-off-by: Nic Cope <nicc@rk0n.org>

negz force-pushed the more-like-mac-slow-s branch from d0b1b63 to c5957c2 Compare July 27, 2022 07:15

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 27, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2022

k8s-ci-robot merged commit 8cecf18 into kubernetes:master Jul 30, 2022

k8s-ci-robot added this to the v1.25 milestone Jul 30, 2022

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use checksums instead of fsyncs to avoid slow discovery caching on MacOS #110851

Use checksums instead of fsyncs to avoid slow discovery caching on MacOS #110851

negz commented Jun 29, 2022 •

edited

k8s-ci-robot commented Jun 29, 2022

leilajal commented Jun 30, 2022

Jefftree commented Jun 30, 2022

Jefftree commented Jun 30, 2022

lavalamp commented Jul 6, 2022

seans3 commented Jul 25, 2022

apelisse commented Jul 25, 2022

negz commented Jul 27, 2022

negz commented Jul 27, 2022 •

edited

dims commented Jul 27, 2022

seans3 commented Jul 27, 2022

negz commented Jul 29, 2022

lavalamp commented Jul 29, 2022

k8s-ci-robot commented Jul 29, 2022

sftim commented Aug 16, 2022

csantanapr commented Aug 18, 2022

negz commented Aug 18, 2022

sftim commented Aug 19, 2022

Use checksums instead of fsyncs to avoid slow discovery caching on MacOS #110851

Use checksums instead of fsyncs to avoid slow discovery caching on MacOS #110851

Conversation

negz commented Jun 29, 2022 • edited

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?

k8s-ci-robot commented Jun 29, 2022

leilajal commented Jun 30, 2022

Jefftree commented Jun 30, 2022

Jefftree commented Jun 30, 2022

lavalamp commented Jul 6, 2022

seans3 commented Jul 25, 2022

apelisse commented Jul 25, 2022

negz commented Jul 27, 2022

negz commented Jul 27, 2022 • edited

dims commented Jul 27, 2022

seans3 commented Jul 27, 2022

negz commented Jul 29, 2022

lavalamp commented Jul 29, 2022

k8s-ci-robot commented Jul 29, 2022

sftim commented Aug 16, 2022

csantanapr commented Aug 18, 2022

negz commented Aug 18, 2022

sftim commented Aug 19, 2022

negz commented Jun 29, 2022 •

edited

negz commented Jul 27, 2022 •

edited