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.

Sign up for GitHub

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

Decrease linter's memory usage #1194

Merged
merged 11 commits into from
Mar 12, 2024
Merged

Decrease linter's memory usage #1194

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c79d7b7
Add build constraints while running the linter to reduce memory consu…
ulucinar Mar 5, 2024
f874627
Add config.ProviderConfiguration
ulucinar Mar 6, 2024
d2aec0a
Test CI pipeline
ulucinar Mar 6, 2024
3e46cfa
Add linter analysis cache restore step before running the linter
ulucinar Mar 6, 2024
513ebf1
Add the go.lint.analysiskey make target to calculate the linter cache…
ulucinar Mar 6, 2024
4e0e596
Skip building the linter runner analysis cache if the cache was succe…
ulucinar Mar 6, 2024
21ff2c2
Initialize the linter analysis cache using the "account" API group
ulucinar Mar 7, 2024
767ba19
Download the buildtagger binary from uptest's release S3 bucket
ulucinar Mar 7, 2024
6975fb6
Use buildtagger by default with "make lint"
ulucinar Mar 8, 2024
ed39e84
Improve lint make target documentation
ulucinar Mar 11, 2024
11deaa7
Increase the concurrency of the first linter runner cache building ph…
ulucinar Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 74 additions & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,80 @@ jobs:
ci:
uses: upbound/uptest/.github/workflows/provider-ci.yml@main
with:
go-version: 1.21
go-version: "1.21"
golangci-skip: true # we will run the linter via "make lint"
secrets:
UPBOUND_MARKETPLACE_PUSH_ROBOT_USR: ${{ secrets.UPBOUND_MARKETPLACE_PUSH_ROBOT_USR }}
UPBOUND_MARKETPLACE_PUSH_ROBOT_PSW: ${{ secrets.UPBOUND_MARKETPLACE_PUSH_ROBOT_PSW }}

detect-noop:
runs-on: ubuntu-22.04
outputs:
noop: ${{ steps.noop.outputs.should_skip }}
steps:
- name: Detect No-op Changes
id: noop
uses: fkirc/skip-duplicate-actions@12aca0a884f6137d619d6a8a09fcc3406ced5281 # v5.3.0
with:
github_token: ${{ secrets.GITHUB_TOKEN }}
paths_ignore: '["**.md", "**.png", "**.jpg"]'
do_not_skip: '["workflow_dispatch", "schedule", "push"]'

lint:
runs-on: ubuntu-22.04
needs: detect-noop
if: needs.detect-noop.outputs.noop != 'true'

steps:
- name: Checkout
uses: actions/checkout@8ade135a41bc03ea155e62e844d188df1ea18608 # v4
with:
submodules: true

- name: Setup Go
uses: actions/setup-go@6edd4406fa81c3da01a34fa6f6343087c207a568 # v3
with:
go-version: "1.21"

- name: Find the Go Build Cache
id: go_cache
run: |
echo "cache=$(make go.cachedir)" >> $GITHUB_OUTPUT && \
echo "mod_cache=$(make go.mod.cachedir)" >> $GITHUB_OUTPUT && \
echo "analysis_cache=$HOME/.cache/golangci-lint" >> $GITHUB_OUTPUT && \
echo "analysis_cache_key=$(make go.lint.analysiskey)" >> $GITHUB_OUTPUT && \
echo "analysis_cache_key_int=$(make go.lint.analysiskey-interval)" >> $GITHUB_OUTPUT


- name: Cache the Go Build Cache
uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84 # v3
with:
path: ${{ steps.go_cache.outputs.cache }}
key: ${{ runner.os }}-build-lint-${{ hashFiles('**/go.sum') }}
restore-keys: ${{ runner.os }}-build-lint-

- name: Cache Go Dependencies
uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84 # v3
with:
path: ${{ steps.go_cache.outputs.mod_cache }}
key: ${{ runner.os }}-pkg-${{ hashFiles('**/go.sum') }}
restore-keys: ${{ runner.os }}-pkg-

- name: Cache Linter Analysis
uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84 # v3
id: cache-analysis
with:
path: ${{ steps.go_cache.outputs.analysis_cache }}
key: ${{ steps.go_cache.outputs.analysis_cache_key }}
restore-keys: |
${{ steps.go_cache.outputs.analysis_cache_key_int }}

- name: Vendor Dependencies
run: make vendor vendor.check

- name: Lint
env:
GOLANGCI_LINT_CACHE: ${{ steps.go_cache.outputs.analysis_cache }}
SKIP_LINTER_ANALYSIS: ${{ steps.cache-analysis.outputs.cache-hit }}
RUN_BUILDTAGGER: true
run: make lint
7 changes: 3 additions & 4 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
run:
timeout: 20m

skip-files:
- "zz_.+\\.go$"
timeout: 60m
show-stats: true

output:
# colored-line-number|line-number|json|tab|checkstyle|code-climate, default is "colored-line-number"
format: colored-line-number
print-linter-name: true

linters-settings:
errcheck:
Expand Down
30 changes: 28 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,14 @@ GO_REQUIRED_VERSION ?= 1.21
# Uncomment below if you need to override the version.
GOLANGCILINT_VERSION ?= 1.55.2

# if running in a CI job, we will use build constraints and use the buildtagger
# to generate the build tags.
ifeq ($(RUN_BUILDTAGGER),true)
GO_LINT_ARGS ?= -v --build-tags all
BUILDTAGGER_VERSION ?= v0.12.0-rc.0.28.gdc5d6f3
BUILDTAGGER_DOWNLOAD_URL ?= https://s3.us-west-2.amazonaws.com/upbound.official-providers-ci.releases/main/$(BUILDTAGGER_VERSION)/bin/$(SAFEHOST_PLATFORM)/buildtagger
sergenyalcin marked this conversation as resolved.
Show resolved Hide resolved
endif

# SUBPACKAGES ?= $(shell find cmd/provider -type d -maxdepth 1 -mindepth 1 | cut -d/ -f3)
SUBPACKAGES ?= monolith
GO_STATIC_PACKAGES ?= $(GO_PROJECT)/cmd/generator ${SUBPACKAGES:%=$(GO_PROJECT)/cmd/provider/%}
Expand Down Expand Up @@ -311,7 +319,14 @@ go.cachedir:
go.mod.cachedir:
@go env GOMODCACHE

.PHONY: cobertura reviewable submodules fallthrough go.mod.cachedir go.cachedir run crds.clean $(TERRAFORM_PROVIDER_SCHEMA)
go.lint.analysiskey-interval:
@# cache is invalidated at least every 7 days
@echo -n golangci-lint.cache-$$(( $$(date +%s) / (7 * 86400) ))-

go.lint.analysiskey:
@echo $$(make go.lint.analysiskey-interval)$$(sha1sum go.sum | cut -d' ' -f1)

.PHONY: cobertura reviewable submodules fallthrough go.mod.cachedir go.cachedir go.lint.analysiskey-interval go.lint.analysiskey run crds.clean $(TERRAFORM_PROVIDER_SCHEMA)

build.init: kustomize-crds

Expand All @@ -325,7 +340,18 @@ kustomize-crds: output.init $(KUSTOMIZE) $(YQ)
XDG_CONFIG_HOME=$(PWD)/package $(KUSTOMIZE) build --enable-alpha-plugins $(OUTPUT_DIR)/package/kustomize -o $(OUTPUT_DIR)/package/crds.yaml || $(FAIL)
@$(OK) Kustomizing CRDs.

.PHONY: kustomize-crds

checkout-to-old-api:
CHECKOUT_RELEASE_VERSION=$(CHECKOUT_RELEASE_VERSION) hack/check-duplicate.sh

.PHONY: kustomize-crds
ifeq ($(RUN_BUILDTAGGER),true)
lint.init: build-lint-cache

build-lint-cache: $(GOLANGCILINT)
@$(INFO) Running golangci-lint with the analysis cache building phase.
@(BUILDTAGGER_DOWNLOAD_URL=$(BUILDTAGGER_DOWNLOAD_URL) ./scripts/tag.sh && \
(([[ "${SKIP_LINTER_ANALYSIS}" == "true" ]] && $(OK) "Skipping analysis cache build phase because it's already been populated") && \
[[ "${SKIP_LINTER_ANALYSIS}" == "true" ]] || $(GOLANGCILINT) run -v --build-tags account,configregistry,configprovider,linter_run -v --concurrency 1 --disable-all --exclude '.*')) || $(FAIL)
sergenyalcin marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we consider increasing the Concurrency?

Copy link
Collaborator Author

@ulucinar ulucinar Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've not done extensive tests here. We already know the default concurrency of 4 on Github hosted runners is not working for us even when using the build constraints, it still consumes too much memory. Decreasing the concurrency to 1 without employing the build constraints also did not allow us to fit the linter into a hosted runner with 16 GB of physical memory. But using the build constraints and limiting the concurrency to 1 made it possible to fit onto a 16 GB machine. So, looks like we can try increasing the concurrency to 2 or 3.

The full linting is still done with the maximum available concurrency (in our case, with a concurrency of 4) with the constructed analysis cache. This concurrency limitation is only for the first cache construction phase.

Let me try with a concurrency of 2 and 3. We were initially doing our observations with a relatively larger resource provider (ec2). Switching to account may have helped.

Copy link
Collaborator Author

@ulucinar ulucinar Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A concurrency of 4 for the initial analysis cache build phase on cold cache has failed here:
https://github.com/upbound/provider-aws/actions/runs/8246986803/job/22554209938?pr=1194

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a run with a concurrency of 1. It took ~21 min and ~14 GB of peak memory to complete the initial phase:
image

And here's another run with a concurrency of 3. It took ~14 min ~19 GB of peak memory to complete the initial phase:
image

Let's switch to a concurrency of 3 for the initial phase (concurrency of 4 has failed). The second phase already uses the max available concurrency.

If we observe any memory issues for the initial phase, we can decrease it later again.

@$(OK) Running golangci-lint with the analysis cache building phase.
endif
17 changes: 17 additions & 0 deletions apis/linter_run.go
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, this file was brought up in the context of the fragmentation of the registration phase. It will be needed when we want to add all the resources to the schema. I can't find any use of this function in the repo. Am I missing something, or do we really need this file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained here, the code that the linter runner is processing (analyzing) must compile. The API registration code (apis/zz_register.go) is one of the hot spots, which imports all the API packages and is thus costly to analyze. The apis/linter_run.go keeps the linter happy by providing a definition of apis.AddToScheme & apis.AddToSchemes and the original definitions from apis/zz_register.go are excluded by the supplied build constraints to the linter. So, the definitions in this file are only used by the linter and the actual build uses the definitions from apis/zz_register.go.

This also means that the generated file apis/zz_register.go is not analyzed. We can in theory distribute API registration across the individual resource provider (API group) modules but the cross-resource references necessitate a form of dependency resolution as discussed under the Implemented Strategy in this PR section of the PR description. So avoiding these hot spots at the expense of not linting them is a trade-off we make here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! Thanks for the clarification. This part can also be a bit tricky for other contributors. So, I am leaving this comment unresolved for documenting purposes.

Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//go:build linter_run

// SPDX-FileCopyrightText: 2024 The Crossplane Authors <https://crossplane.io>
//
// SPDX-License-Identifier: Apache-2.0

package apis

import "k8s.io/apimachinery/pkg/runtime"

// AddToSchemes may be used to add all resources defined in the project to a Scheme
var AddToSchemes runtime.SchemeBuilder

// AddToScheme adds all Resources to the Scheme
func AddToScheme(s *runtime.Scheme) error {
panic(`Must not be called in provider runtime. The provider should not have been built with the "linter_run" build constraint.`)
}
Loading
Loading