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: remove some inter-job dependencies, run cross-compile task with make -j, use /tmp for Go build cache #5856

Merged
merged 4 commits into from
Dec 6, 2024

Conversation

nalind
Copy link
Member

@nalind nalind commented Dec 4, 2024

What type of PR is this?

/kind other

What this PR does / why we need it:

As a quick attempt to speed up our CI's total start-to-finish time, this hits what we think will be low-hanging fruit.

How to verify it

Check the clock duration for https://cirrus-ci.com/github/containers/buildah/pull%2F5856 compared to builds for other pull requests.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

@rhatdan
Copy link
Member

rhatdan commented Dec 4, 2024

LGTM

@nalind nalind force-pushed the ci-deps branch 4 times, most recently from ebe486a to f7b1a39 Compare December 4, 2024 21:39
@nalind nalind marked this pull request as ready for review December 4, 2024 21:40
@nalind nalind force-pushed the ci-deps branch 2 times, most recently from 49ab45e to cdb2120 Compare December 5, 2024 15:38
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Thanks mostly looks good. That is a great improvement.

.cirrus.yml Outdated
- make cross CGO_ENABLED=0

- make -j cross CGO_ENABLED=0
- du -sh `go env GOCACHE`
Copy link
Member

Choose a reason for hiding this comment

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

I think we can drop the du or do you still need that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm concerned about using /tmp for the Go cache, but you're probably right that we don't need to be worrying about that in every task.

.cirrus.yml Outdated
@@ -170,6 +170,7 @@ unit_task:
setup_script: '${SCRIPT_BASE}/setup.sh |& ${_TIMESTAMP}'
build_script: '${SCRIPT_BASE}/build.sh |& ${_TIMESTAMP}'
unit_test_script: '${SCRIPT_BASE}/test.sh unit |& ${_TIMESTAMP}'
du_script: du -sh `go env GOCACHE`
Copy link
Member

Choose a reason for hiding this comment

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

same here and in the other places?

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropped it.

.cirrus.yml Outdated
Comment on lines 157 to 160
depends_on: &smoke_vendor_cross
- smoke
- vendor
- cross_build
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should at least keep the dependency for vendor & smoke test as those are fast enough.

In podman we have basically two stages build/validate -> actual tests:
https://raw.githubusercontent.com/wiki/containers/podman/cirrus-map.svg

So I think it makes sense for buildah to follow that, unless we really like to safe the the 7 mins form the smoke test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Having added depends_on bits back, it looks like we're getting just about everything else stalled until the cross-build check finishes. Replacing some of the "only_if" directives with "skip" directives looks like it will avoid this weirdness.

@@ -93,7 +94,7 @@ smoke_task:
name: "Smoke Test"

gce_instance:
memory: "12Gb"
Copy link
Member

Choose a reason for hiding this comment

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

pre existing but defining a task with no cpu seems odd

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. We'll try 4, maybe it'll run this task a bit faster.

Tell make to run all of the cross-compile tasks at once. This uses a lot
of space in /tmp, and a later patch will also use it for the build
cache, so we request more memory for the task.  Also request more CPUs.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@nalind nalind changed the title CI: remove most inter-job dependencies, run cross-compile task with make -j CI: remove some inter-job dependencies, run cross-compile task with make -j, use /tmp for Go build cache Dec 5, 2024
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

openshift-ci bot commented Dec 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, nalind

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Remove the bits of the CI configuration that prevent some jobs from
starting unless/until the cross-compile tests have passed.  That costs
more time than we're willing to wait, at this time at least.  Use the
skip directive to avoid having to get all of the dependencies and
only_if directives to agree.

Signed-off-by: Nalin Dahyabhai <[email protected]>
Point the compiler's build cache at /tmp.  This generally requires more
memory, but it's faster, and the build cache would be discarded anyway.

Signed-off-by: Nalin Dahyabhai <[email protected]>
The unit tests don't use the binary, so we shouldn't be spending time
compiling it.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Dec 6, 2024

/lgtm

@openshift-merge-bot openshift-merge-bot bot merged commit a3a7ece into containers:main Dec 6, 2024
32 checks passed
@nalind nalind deleted the ci-deps branch December 6, 2024 15:17
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.

3 participants