Skip to content

CI, build 2: Move GHA to zstd #3984

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

Merged
merged 1 commit into from
Mar 7, 2025

Conversation

apostasie
Copy link
Contributor

@apostasie apostasie commented Mar 5, 2025

For context, see #3940, #3924, and the start of this PR cycle at #3983.

On top of #3983


This moves caching to zstd.

Given the bottleneck with gha is IO and not compute, this is not likely to have an impact on speed - but it will on cache size.

@apostasie apostasie changed the title Move GHA to zstd [WIP] Move GHA to zstd Mar 5, 2025
@apostasie apostasie changed the title [WIP] Move GHA to zstd [WIP] CI, build 2: Move GHA to zstd Mar 5, 2025
@apostasie
Copy link
Contributor Author

EL8 did time-out. Apparently stuck somewhere in containers tests.

@apostasie apostasie marked this pull request as ready for review March 5, 2025 01:04
@apostasie apostasie changed the title [WIP] CI, build 2: Move GHA to zstd CI, build 2: Move GHA to zstd Mar 5, 2025
@apostasie apostasie closed this Mar 6, 2025
@apostasie apostasie reopened this Mar 6, 2025
@apostasie apostasie closed this Mar 6, 2025
@apostasie apostasie reopened this Mar 6, 2025
@AkihiroSuda AkihiroSuda added this to the v2.0.4 milestone Mar 7, 2025
@AkihiroSuda
Copy link
Member

Needs rebase.

(For small amount of changes you do not need to split them into multiple PRs)

@apostasie
Copy link
Contributor Author

(For small amount of changes you do not need to split them into multiple PRs)

Really.

#3944 (comment)

So, which is it?

@apostasie
Copy link
Contributor Author

x

(For small amount of changes you do not need to split them into multiple PRs)

Really.

#3944 (comment)

So, which is it?

I don't want to get into an argument here.
But really, these kind of comments going both ways are not helping anyone.
I know that being a maintainer is hard - but you are not the only one putting in efforts on this.

@AkihiroSuda
Copy link
Member

x

(For small amount of changes you do not need to split them into multiple PRs)

Really.
#3944 (comment)
So, which is it?

I don't want to get into an argument here. But really, these kind of comments going both ways are not helping anyone. I know that being a maintainer is hard - but you are not the only one putting in efforts on this.

Sorry, it is hard to have a consistent convention, but I thought the following ones could be optionally merged to a single PR because they are quite small, modify the same file, and unlikely controversial:

The situation seemed different for other ones though:

Anyway, feel free to submit PRs in the finer granularity when you are not sure.
The only drawback is the cost of rebasing.

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 92e37db into containerd:main Mar 7, 2025
30 checks passed
@apostasie
Copy link
Contributor Author

x

(For small amount of changes you do not need to split them into multiple PRs)

Really.
#3944 (comment)
So, which is it?

I don't want to get into an argument here. But really, these kind of comments going both ways are not helping anyone. I know that being a maintainer is hard - but you are not the only one putting in efforts on this.

Sorry, it is hard to have a consistent convention, but I thought the following ones could be optionally merged to a single PR because they are quite small, modify the same file, and unlikely controversial:

Changing compression is a huge deal.
It is highly impactful for cache and likely to trigger CI issues, or serious disruption if it goes south.
Departing from "default" is always a risky proposal.

Same for how we shard our cache.
This specific change will have a significant impact on build duration, cache usage and how GHA will evict entries, with cascading effects on how much more we are going to use it or not, in turn shaping how GHA will behave wrt throttling.

The situation seemed different for other ones though:

Renaming variables for readability has no impact whatsoever on the product, nor on the CI behavior.
It is purely about quality of life, and either objectively increases readability, or it does not.
Shorter, non-stuttering, more explicit variable names increase readability.
Longer, stuttering, more obscure variable names decrease readability.

IMHO "controversial" should not be defined by potential for bike-shed (assuming that was your point), but by "risk" and "impact".

Anyway, feel free to submit PRs in the finer granularity when you are not sure. The only drawback is the cost of rebasing.

Sorry, I will not.
It is just too much extra work to put up upfront with no upside.

I am fine doing it for maintainers quality of life, when there is clear and rational reasons.

Thanks

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants