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

CI: move dist-arm-linux to an ARM runner #133902

Merged
merged 2 commits into from
Dec 10, 2024
Merged

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Dec 5, 2024

First, I want to test whether we could actually move this to a free runner, vs moving to the 8-core ARM runner.

Fixes: rust-lang/infra-team#181

r? @MarcoIeni

try-job: dist-arm-linux

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Dec 5, 2024
@Kobzol
Copy link
Contributor Author

Kobzol commented Dec 5, 2024

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2024
CI: move `dist-arm-linux` to a free runner

Related issue: rust-lang/infra-team#181

First, I want to test whether we could actually move this to a free runner, vs moving to the 8-core ARM runner.

r? `@MarcoIeni`

try-job: dist-arm-linux
@bors
Copy link
Contributor

bors commented Dec 5, 2024

⌛ Trying commit 67c6629 with merge a806c74...

@MarcoIeni
Copy link
Member

I tried to move it to free runners in #133256 (comment) but it was taking a bit too long.
but feel free to try again 👍

@Kobzol
Copy link
Contributor Author

Kobzol commented Dec 5, 2024

True, but since then we have merged several changes where 2h 37m would be deemed acceptable, right? So at least wanted to try it again.

@MarcoIeni
Copy link
Member

By looking at the average ci time, it looks like the minimum is 2h 40min, so yes, it's acceptable, but in some cases it might be the slowest job.

We can merge this and monitor over time to check if it becomes an issue 👍

image

@MarcoIeni
Copy link
Member

Also, it would be cool to be able to split dist jobs, e.g. one job could build rust-analyzer, another one could build clippy, rustfmt, etc.

But I'm not sure if it's feasible

@fmease
Copy link
Member

fmease commented Dec 5, 2024

sync @bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2024
@fmease fmease closed this Dec 5, 2024
@fmease fmease reopened this Dec 5, 2024
@fmease
Copy link
Member

fmease commented Dec 5, 2024

Sorry for the inconvenience, bors seems stuck.

@bors try

@fmease fmease added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 5, 2024
@bors
Copy link
Contributor

bors commented Dec 5, 2024

⌛ Trying commit 67c6629 with merge 346bf4d...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2024
CI: move `dist-arm-linux` to a free runner

Related issue: rust-lang/infra-team#181

First, I want to test whether we could actually move this to a free runner, vs moving to the 8-core ARM runner.

r? `@MarcoIeni`

try-job: dist-arm-linux
@rust-log-analyzer

This comment has been minimized.

@Kobzol Kobzol force-pushed the ci-dist-arm-runner branch from 67c6629 to 28767a1 Compare December 5, 2024 11:41
@Kobzol
Copy link
Contributor Author

Kobzol commented Dec 5, 2024

Not enough space. Nevermind.

@bors try

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 5, 2024
CI: move `dist-arm-linux` to a free runner

First, I want to test whether we could actually move this to a free runner, vs moving to the 8-core ARM runner.

Fixes: rust-lang/infra-team#181

r? `@MarcoIeni`

try-job: dist-arm-linux
@bors
Copy link
Contributor

bors commented Dec 5, 2024

⌛ Trying commit 28767a1 with merge ab1508a...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 5, 2024

💔 Test failed - checks-actions

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 5, 2024
@MarcoIeni
Copy link
Member

LGTM. can you change the PR title (we aren't using a free runner) and squash your commits?

@Kobzol Kobzol changed the title CI: move dist-arm-linux to a free runner CI: move dist-arm-linux to an ARM runner Dec 6, 2024
Kobzol and others added 2 commits December 6, 2024 15:31
To make it more consistent with the rest of the anchors.
@Kobzol Kobzol force-pushed the ci-dist-arm-runner branch from ba0c5ac to 9490297 Compare December 6, 2024 14:31
@Kobzol
Copy link
Contributor Author

Kobzol commented Dec 6, 2024

Done (I kept two commits, since they are independent).

@MarcoIeni
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Dec 6, 2024

📌 Commit 9490297 has been approved by MarcoIeni

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 6, 2024
@workingjubilee
Copy link
Member

@bors rollup=iffy

@@ -19,7 +19,7 @@ RUN sh /scripts/rustbuild-setup.sh
WORKDIR /tmp

COPY scripts/crosstool-ng-build.sh /scripts/
COPY host-x86_64/dist-arm-linux/arm-linux-gnueabi.defconfig /tmp/crosstool.defconfig
COPY host-aarch64/dist-arm-linux/arm-linux-gnueabi.defconfig /tmp/crosstool.defconfig
Copy link
Contributor

Choose a reason for hiding this comment

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

So it sounds like this is still a normal cross-compilation job, it now just happens to use an aarch64 host in CI. Can we drop this host-aarch64/host-x86_64 distinction for jobs where it is not relevant? Otherwise it will not be possible to test this image locally (on x86_64) anymore.

Copy link
Contributor

@nikic nikic Dec 8, 2024

Choose a reason for hiding this comment

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

Basically, as far as I can tell, there is no relation at all here between the fact that this is dist-arm-linux and we're running on ARM. You could do exactly the same move for all/most other crosstool-ng based jobs, if the ARM runners are cheaper for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. But based on your comment from the other PR, moving to a native build would raise the minimum glibc version, right? Since this already used crosstool before, we could make use of it to keep glibc lower.

@nikic
Copy link
Contributor

nikic commented Dec 8, 2024

The cost calculation here is that, as long as it takes less than 60% longer to run, it's cheaper to use the aarch64 runners instead of the x86_64 ones, right?

@Kobzol
Copy link
Contributor Author

Kobzol commented Dec 8, 2024

Yes, the ARM64 runners are in fact cheaper than x64 runners at the moment (somewhat surprisingly to me). In theory, we could start moving all/most large runners from x64 to ARM, but we wanted to test it first with jobs that are the "most relevant" (dist-arm/aarch64), where we could get rid of cross compilation.

However, as you correctly pointed out, this PR does not switch away from it, and more importantly, switching away from it in the native way, by just using ubuntu 22.04, would increase our minimum glibc support, which is bad. So we can either keep using crosstool, or migrate the ARM jobs to a similar design as we use for x64. But that would be quite annoying, I can imagine, because on the CentOS we need to install a bunch of things from scratch.

In the longer term, we'd like to unify the Dockerfile base images to avoid so much duplication; it might be better to delay a large change like this until then.

@nikic
Copy link
Contributor

nikic commented Dec 8, 2024

Right. I think switching from crosstool-ng to a native build only makes sense for tier 1 targets (like dist-aarch64-linux) where we want to apply PGO optimizations. For things like dist-arm-linux, sticking to the cross-compiled build gives us a lot more control over the target requirements.

Just moving the images to a different host, while keeping cross-compilation, still makes sense from a cost perspective. If we do that, I'd mainly just want to retain the ability to easily run the images on x86_64. Probably just having a host-any directory next to host-x86_64 and host-aarch64 would be enough for that.

@Kobzol
Copy link
Contributor Author

Kobzol commented Dec 8, 2024

Just moving the images to a different host, while keeping cross-compilation, still makes sense from a cost perspective. If we do that, I'd mainly just want to retain the ability to easily run the images on x86_64. Probably just having a host-any directory next to host-x86_64 and host-aarch64 would be enough for that.

Yes, this PR was done because of the reduced cost, just to test it out (switching a single runner won't exactly save our CI budget).

Our current way of running CI jobs locally isn't great, and with the latest CI changes, it's actually becoming worse, as some jobs are being split (although this isn't happening yet for dist builders, only for test builders). It would be great to have an easy way to run any CI job locally based on the source of truth in jobs.yml, but we haven't got around to that yet.

On a related note, we should probably have some tests that check what glibc do our dist toolchains require, as currently it's not being tested anywhere, AFAIK. And if you haven't caught this on the PR, we could have landed a glibc bump inadvertedly (although probably sooner or later someone would complain). I created an issue for that: #134037.

@lqd
Copy link
Member

lqd commented Dec 8, 2024

It would be great to have an easy way to run any CI job locally based on the source of truth in jobs.yml

(sorry for the off-topic but: this would be extremely valuable for contributors and imo sounds worthy of a t-infra project goal; possibly orphaned if no one has availability to work on it but can help with mentoring and reviews of external contributions)

jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 9, 2024
…Ieni

CI: move `dist-arm-linux` to an ARM runner

First, I want to test whether we could actually move this to a free runner, vs moving to the 8-core ARM runner.

Fixes: rust-lang/infra-team#181

r? `@MarcoIeni`

try-job: dist-arm-linux
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2024
Rollup of 5 pull requests

Successful merges:

 - rust-lang#128004 (codegen `#[naked]` functions using global asm)
 - rust-lang#132789 (add some debug-assertion crash tests)
 - rust-lang#133456 (Add licenses + Run `cargo update`)
 - rust-lang#133853 (use vendor sources by default on dist tarballs)
 - rust-lang#133902 (CI: move `dist-arm-linux` to an ARM runner)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Dec 10, 2024

⌛ Testing commit 9490297 with merge b597d2a...

@bors
Copy link
Contributor

bors commented Dec 10, 2024

☀️ Test successful - checks-actions
Approved by: MarcoIeni
Pushing b597d2a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 10, 2024
@bors bors merged commit b597d2a into rust-lang:master Dec 10, 2024
7 checks passed
@rustbot rustbot added this to the 1.85.0 milestone Dec 10, 2024
@Kobzol Kobzol deleted the ci-dist-arm-runner branch December 10, 2024 10:49
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b597d2a): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -3.6%, secondary 4.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.7% [4.7%, 4.7%] 1
Improvements ✅
(primary)
-3.6% [-3.6%, -3.6%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.6% [-3.6%, -3.6%] 1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 767.281s -> 767.346s (0.01%)
Artifact size: 330.96 MiB -> 330.96 MiB (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move the dist-arm-linux job to arm runner
10 participants