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

enable golangci-lint for supported platforms #4411

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

crazy-max
Copy link
Member

follow-up #4261 (comment)

@crazy-max crazy-max force-pushed the lint-multi-platform branch 3 times, most recently from eb01b1d to 01ed1a5 Compare November 9, 2023 10:57
@crazy-max crazy-max force-pushed the lint-multi-platform branch from 01ed1a5 to 88f3b82 Compare November 9, 2023 12:08
@crazy-max crazy-max marked this pull request as ready for review November 9, 2023 12:11
jedevc
jedevc previously approved these changes Nov 9, 2023
@TBBle
Copy link
Collaborator

TBBle commented Nov 9, 2023

Just so I'm clear, it looks like a single lint takes 20 minutes anyway, so there's not much room for gains on GHA anyway? I note that this run took 24 minutes for lint-default, while the final run in #4261 took 20 minutes for the entire lint task.

Either that, or the GHA runner can't actually run all the lint RUN steps for different platforms in parallel, so they end up being in-effect re-serialised. If that's the case, could we distribute the platforms across a GHA matrix, rather than a single GHA task with BuildKit (fruitlessly) parallelising inside it?

Or it's something I'm not familiar-enough with to spot, like this extensive runtime being a cold cache problem that goes away in future runs, or we're just being unlucky in GHA task scheduling or something.

@jedevc jedevc dismissed their stale review November 9, 2023 13:14

Wait, the jobs aren't split up properly

@jedevc
Copy link
Member

jedevc commented Nov 9, 2023

@crazy-max I think we probably want to matrix the platforms as well? The problem with the linting is that it's cpu-intensive and uses multiple cores (I have no idea why, maybe something to dig into sometime 😢). To parallelize, we'd need to spin up a linting job for each platform.

@crazy-max
Copy link
Member Author

or the GHA runner can't actually run all the lint RUN steps for different platforms in parallel

It does run the steps for different platforms in parallel but golangci-lint is quite resource intensive so contention is hardware related (linux public runners are limited to 2 vCPUs).

If that's the case, could we distribute the platforms across a GHA matrix, rather than a single GHA task with BuildKit (fruitlessly) parallelising inside it?

We could be that would increase the number of jobs in our pipeline quite heavily and therefore increase queue for other runs. From 5 lint-* jobs with this PR to 8 platforms x 5 lint-* jobs, that would create 40 jobs.

@thaJeztah
Copy link
Member

The problem with the linting is that it's cpu-intensive and uses multiple cores

I know golangci-lint is always aggressively trying to do as many things in parallel as possible, which can be great to make use of all available resources, but I recall we had to tweak their concurrency option, as well as GOGC (and GOMAXPROCS ?) in the past for the best result (which could result in "less parallel", but faster runs).

Also don't forget build-tags and CGO_ENABLED if needed; I've had some repositories where that was overlooked, and large parts of the code were skipped. 😞

@crazy-max
Copy link
Member Author

Follow-up: as discussed with @jedevc we could look at grouping default, labs and nydus and instead split by platform to reduce build time a bit more.

@crazy-max crazy-max merged commit c63d082 into moby:master Nov 14, 2023
@crazy-max crazy-max deleted the lint-multi-platform branch November 14, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants