Skip to content
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

Support Remote Cache for golangci-lint #4986

Closed
2 of 3 tasks
ryancurrah opened this issue Sep 5, 2024 · 12 comments · Fixed by #5098
Closed
2 of 3 tasks

Support Remote Cache for golangci-lint #4986

ryancurrah opened this issue Sep 5, 2024 · 12 comments · Fixed by #5098
Assignees
Labels
area: cache enhancement New feature or improvement

Comments

@ryancurrah
Copy link
Member

ryancurrah commented Sep 5, 2024

Welcome

Your feature request related to a problem? Please describe

Yes, this feature request is related to the problem of long linting times, especially in large codebases or CI environments where there are multiple runners/agents.

Currently, golangci-lint lacks a mechanism to cache results remotely, which could help in reducing the overall time taken for linting when running it repeatedly over the same or similar code. This lack of caching increases build times and resource usage, especially when dealing with distributed and large build environments.

Describe the solution you'd like

I propose introducing a feature in golangci-lint to support remote caching by abstracting the cache mechanism. Similar to how Go has implemented the GOCACHEPROG environment variable, which allows a custom cache implementation via a child process that communicates using JSON over stdin/stdout, golangci-lint could introduce a GOLANGCICACHEPROG environment variable. This would allow users to specify a binary responsible for handling cache operations, thereby enabling remote caching similar to https://go-review.googlesource.com/c/go/+/486715.

Describe alternatives you've considered

Persisting cache on individual agents. In our environment, multiple agents are used, and they may not always have an up-to-date cache, leading to inconsistencies and longer build times.

Additional context

Willing to implement this myself if it is deemed an acceptable proposal. Remote caching has proven for us to reduce build times in Go projects. By using a similar approach in golangci-lint, users could experience similar reductions in linting times, especially in CI/CD environments or large projects when linting a lot of code. The ability to cache results remotely would lead to more efficient use of resources and faster feedback loops during builds.

Supporter

@ryancurrah ryancurrah added the enhancement New feature or improvement label Sep 5, 2024
@ldez ldez added no decision No decision to fix or not proposal and removed enhancement New feature or improvement labels Sep 5, 2024
@ryancurrah
Copy link
Member Author

Note this feature relies on golang/go#64876

@ryancurrah ryancurrah self-assigned this Oct 29, 2024
@ldez ldez self-assigned this Oct 31, 2024
@ldez
Copy link
Member

ldez commented Oct 31, 2024

Just to clarify why I put no-decision: I'm working on a re-sync of the internal code related to cache.

It requires a lot of time to ensure it is non-breaking and provides a reviewable PR.

My priority is maintainability, so the new feature (if not added by the sync) will be evaluated after the sync because this a really really really long and complex job to just re-sync the code.

The changes in the original code have never been documented, I already added a "summary" but this is not enough to work quickly on this.
I don't want (because I'm not able to do it) to follow 2 things on the same complex topic at the same time.

@ryancurrah
Copy link
Member Author

ryancurrah commented Oct 31, 2024

Did not realize you were refactoring or reimplementing the caching behaviour. Will wait for that and then we can see how to extend it for remote cache.

@ldez ldez added enhancement New feature or improvement area: cache and removed no decision No decision to fix or not proposal labels Oct 31, 2024
@ldez
Copy link
Member

ldez commented Oct 31, 2024

@ryancurrah I updated the internals inside #5098.

As a side effect, the programmatic cache is supported but I didn't test it.

If you plan to create a cache implementation, I'm interested in playing with it or contributing.

@ldez
Copy link
Member

ldez commented Nov 1, 2024

I didn't saw before but there is a test implementation: https://github.com/bradfitz/go-tool-cache/

$ go install github.com/bradfitz/go-tool-cache/cmd/go-cacher@latest
go: downloading github.com/bradfitz/go-tool-cache v0.0.0-20230425225207-ef6c7b1b26e9

$ GOLANGCI_LINT_CACHEPROG="$(which go-cacher) --verbose" ./golangci-lint run
2024/11/01 19:11:42 Defaulting to cache dir /home/ldez/.cache/go-cacher ...
2024/11/01 19:12:03 cacher: closing; 8009 gets (0 hits, 8009 misses, 0 errors); 9233 puts (0 errors)

$ GOLANGCI_LINT_CACHEPROG="$(which go-cacher) --verbose" ./golangci-lint run
2024/11/01 19:12:05 Defaulting to cache dir /home/ldez/.cache/go-cacher ...
2024/11/01 19:12:06 cacher: closing; 153 gets (153 hits, 0 misses, 0 errors); 0 puts (0 errors)

And it works as expected, it's funny.

@ldez ldez closed this as completed in #5098 Nov 2, 2024
@ryancurrah
Copy link
Member Author

Crazy good sir. Sorry I didn't get to review it. I will definitely try it out though. We have a large staging environment for testing all our CI tools. I can test this release on 30k builds in varying projects.

@ryancurrah
Copy link
Member Author

ryancurrah commented Nov 14, 2024

I had a chance to implement this in our staging environment I am getting errors. I am using the same test implementation but slightly refactored to actually work under high load. It's working well for several months using regular Go. I'm going to look into it but figured I'd post my early results.

GOLANGCI_LINT_CACHEPROG="/usr/local/bin/go-cacher -cache-dir=/golangcicache -cache-server=http://go-remote-cache.acme.com:31364"

golangci-lint run --concurrency 2 --verbose --out-format checkstyle --timeout 30m ./... > golangci-checkstyle.xml
level=info msg="golangci-lint has version 1.62.0 built with go1.23.2 from 22b58c9b on 2024-11-10T19:09:02Z"
level=info msg="[config_reader] Config search paths: [./ /workspace/repo /workspace / /home/cs]"
level=info msg="[config_reader] Used config file .golangci.yml"
level=info msg="[lintersdb] Active 29 linters: [bodyclose errcheck exhaustive exportloopref funlen gochecknoinits gocognit goconst gocritic gofmt goimports gomodguard goprintffuncname gosec gosimple govet ineffassign interfacebloat lll misspell nakedret nolintlint revive rowserrcheck sqlclosecheck staticcheck unconvert unparam unused]"
level=info msg="[loader] Using build tags: [stack exclude_graphdriver_btrfs exclude_graphdriver_devicemapper containers_image_openpgp]"
level=info msg="[loader] Go packages loading at mode 8767 (types_sizes|deps|exports_file|imports|compiled_files|files|name) took 22.368794447s"
level=info msg="[runner/filename_unadjuster] Pre-built 0 adjustments in 5.216411ms"
2024/11/14 21:38:26 unable to unmarshal index entry for action id "84e23898c0e2ab98e0e4a16e044603dfa71b98a0a4f4187eb16cad1ec6146c54": invalid character 'v' looking for beginning of value
2024/11/14 21:38:26 unable to unmarshal index entry for action id "af4ad533882caa5ce0f774ccf668d4270eccfff002a402ab8c30843a26030e68": invalid character 'v' looking for beginning of value
2024/11/14 21:38:26 unable to unmarshal index entry for action id "6e5cd1869732d666afd5509dada4e4e626a47c2ee8bc820e6808f19bbccef4b1": invalid character 'v' looking for beginning of value
2024/11/14 21:38:26 unable to unmarshal index entry for action id "3ca2040a93b58cdad548cfc867af8ed2c230150aa01ce827c698546096fc5e92": invalid character 'v' looking for beginning of value

@ryancurrah
Copy link
Member Author

ryancurrah commented Nov 14, 2024

I found a strange behavior running it locally.

With remote cache enabled no cache is saved locally.

export GOLANGCI_LINT_CACHEPROG="go-cacher -verbose -cache-dir=$HOME/golangcicache -cache-server=http://go-remote-cache.acme.com:31364"export GOLANGCI_LINT_CACHE=$HOME/golangcicacherm -rf ~/golangcicache/*golangci-lint run ./...
2024/11/14 18:08:29 cacher: closing; 6 gets (6 hits, 0 misses, 0 errors); 0 puts (0 errors)ls -ail ~/golangcicache/00
total 0
111072968 drwxr--r--   2 rcurrah staff   64 Nov 14 18:08 .
111045876 drwxr-xr-x 259 rcurrah staff 8.1K Nov 14 18:08 ..

With remote cache disabled cache is saved locally.

unset GOLANGCI_LINT_CACHEPROGexport GOLANGCI_LINT_CACHE=$HOME/golangcicacherm -rf ~/golangcicache/*golangci-lint run ./...
2024/11/14 18:08:29 cacher: closing; 6 gets (6 hits, 0 misses, 0 errors); 0 puts (0 errors)ls -ail ~/golangcicache/00
total 28K
111074391 drwxr--r--   9 rcurrah staff  288 Nov 14 18:11 .
111045876 drwxr-xr-x 260 rcurrah staff 8.2K Nov 14 18:11 ..
111075435 -rw-r--r--   1 rcurrah staff  175 Nov 14 18:11 000456273503f35422aed41e272e69be39f750e373d866b255e96b156ccf5e7d-a
111076113 -rw-r--r--   1 rcurrah staff  246 Nov 14 18:11 0007b60a2f84a384e62c579c1b47b6c86abd7ca231bffe605330cdaf07dbd2f9-d
111076643 -rw-r--r--   1 rcurrah staff  175 Nov 14 18:11 00375f634d7b08484971d5156a148704996b6c7ad52f83924dac74164dc43987-a
111077089 -rw-r--r--   1 rcurrah staff  175 Nov 14 18:11 008d924f8b610c4ceaf00912f86eab12a41a29c7387868e1501c054bd0a0499f-a
111076328 -rw-r--r--   1 rcurrah staff  175 Nov 14 18:11 00c182efe5844ea94f44bb4980d6e316873f7047324e077cf75e3cb95cb1e128-a
111075054 -rw-r--r--   1 rcurrah staff  175 Nov 14 18:11 00f1e8d8e194fd0ccd948c560ce8ae01d16b20a30a4dfcc9409638d0ed21e08d-a
111075630 -rw-r--r--   1 rcurrah staff  299 Nov 14 18:11 00febc81575a587d2d62f0bc14c505df460adf7bb83732158531f23da222b3ea-d

I expected the cache to be saved locally when using remote cache.

@ldez
Copy link
Member

ldez commented Nov 14, 2024

I expected the cache to be saved locally when using remote cache.

The implementation inside golangci-lint is the same as Go.
So if you are using the prog cache, the standard/local cache is not enabled.

But maybe you are talking about the "cacher"?

@ryancurrah
Copy link
Member Author

Yeah the cacher implementation caches locally but also on the remote.

@ryancurrah
Copy link
Member Author

ryancurrah commented Nov 15, 2024

Whats also interesting is the first run says 2295 gets.

golangci-lint run ./...
2024/11/14 18:54:21 cacher: closing; 2295 gets (0 hits, 2295 misses, 0 errors); 2337 puts (0 errors)

Than the subsequent runs always says 6 gets.

golangci-lint run ./...
2024/11/14 18:54:37 cacher: closing; 6 gets (6 hits, 0 misses, 0 errors); 0 puts (0 errors)

When doing something similar with go test ./... it always reports similar 1601 gets.

First run...

go test ./...
?       go.acme.dev/redacted/redacted/cmd/redacted  [no test files]
?       go.acme.dev/redacted/redacted/internal/metrics   [no test files]
ok      go.acme.dev/redacted/redacted/internal/errorz    0.863s
ok      go.acme.dev/redacted/redacted/internal/remoteio  0.619s
ok      go.acme.dev/redacted/redacted/internal/service   0.612s
ok      go.acme.dev/redacted/redacted/internal/spec      0.573s
2024/11/14 18:45:56 cacher: closing; 919 gets (55 hits, 864 misses, 0 errors); 1568 puts (0 errors)

Subsequent runs...

go test ./...
?       go.acme.dev/redacted/redacted/cmd/redacted  [no test files]
?       go.acme.dev/redacted/redacted/internal/metrics   [no test files]
ok      go.acme.dev/redacted/redacted/internal/errorz    (cached)
ok      go.acme.dev/redacted/redacted/internal/remoteio  (cached)
ok      go.acme.dev/redacted/redacted/internal/service   (cached)
ok      go.acme.dev/redacted/redacted/internal/spec      (cached)
2024/11/14 18:58:17 cacher: closing; 1601 gets (1601 hits, 0 misses, 0 errors); 6 puts (0 errors)

@ldez
Copy link
Member

ldez commented Nov 15, 2024

I think it's related to how keys and entries are built inside golangci-lint (for fact and package).

Go uses the cache "globally" whereas golangci-lint will use it by package and fact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cache enhancement New feature or improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants