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

Jump to bottom

Use tmpfs for integration tests #5959

Merged
merged 4 commits into from
Feb 19, 2025
Merged

Use tmpfs for integration tests #5959

merged 4 commits into from
Feb 19, 2025

Conversation

flouthoc
Copy link
Collaborator

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

@flouthoc flouthoc force-pushed the integrate-experiment branch from 0f7b48c to 1c2b97f Compare January 30, 2025 20:20
@flouthoc flouthoc marked this pull request as draft January 30, 2025 21:16
@flouthoc flouthoc force-pushed the integrate-experiment branch 2 times, most recently from 7ad3de4 to df76c15 Compare January 31, 2025 16:50
@flouthoc
Copy link
Collaborator Author

@Luap99 It seems increasing CPU for vfs has some improvement.

@flouthoc
Copy link
Collaborator Author

Also containerized_integration came from 27 something to 19m

@Luap99
Copy link
Member

Luap99 commented Feb 3, 2025

image

cirrus graph for the container task seems broken as this usage makes no sense.

image

Looking at another int test we do not seem max out the cpu much so I am not sure if 8 cores make much sense. Also memory usage seems quite low so I think we can reduce that as well.

Overall the first question is what is the goal here? 30 mins total CI like podman? What is the target time we are aiming for?
Once we know that we can see how much we want to bump the cores, because we also need to keep in mind of costs. If double the cores means 2/3 of the time then the costs will most likely be higher.

@flouthoc
Copy link
Collaborator Author

flouthoc commented Feb 3, 2025

Overall the first question is what is the goal here? 30 mins total CI like podman

I think we are already under 30mins, this PR was just an experiment. Increasing cores reduces time by a lot but yeah I don't think it's a good idea to keep throwing money at the problem if its not needed.

@Luap99
Copy link
Member

Luap99 commented Feb 3, 2025

Overall the first question is what is the goal here? 30 mins total CI like podman

I think we are already under 30mins, this PR was just an experiment. Increasing cores reduces time by a lot but yeah I don't think it's a good idea to keep throwing money at the problem if its not needed.

I am talking about total time for CI to finish. Not just a single task time. There is a bit of overhead in scheduling tasks.

You need to look at the cirrus build page at the top it shows you Finished in 57:37 on this PR (because it was not rebased on the unit test speedup)

@flouthoc
Copy link
Collaborator Author

flouthoc commented Feb 3, 2025

Overall the first question is what is the goal here? 30 mins total CI like podman

I think we are already under 30mins, this PR was just an experiment. Increasing cores reduces time by a lot but yeah I don't think it's a good idea to keep throwing money at the problem if its not needed.

I am talking about total time for CI to finish. Not just a single task time. There is a bit of overhead in scheduling tasks.

You need to look at the cirrus build page at the top it shows you Finished in 57:37 on this PR (because it was not rebased on the unit test speedup)

Yes I am only talking about integration tests, waiting for this to merge for unit tests here #5954

@flouthoc flouthoc force-pushed the integrate-experiment branch 5 times, most recently from 5020a4b to c7dbcd8 Compare February 3, 2025 18:23
@flouthoc
Copy link
Collaborator Author

flouthoc commented Feb 3, 2025

@Luap99 I see minor improvement in other tests but not in containerized_integration.

@flouthoc flouthoc force-pushed the integrate-experiment branch 4 times, most recently from e548e62 to fe0750f Compare February 6, 2025 14:56
@flouthoc flouthoc marked this pull request as ready for review February 6, 2025 14:56
@flouthoc flouthoc changed the title time integration tests Use tmpfs for integration tests and bump resources Feb 6, 2025
@flouthoc
Copy link
Collaborator Author

flouthoc commented Feb 6, 2025

@Luap99 @nalind PTAL

@flouthoc flouthoc requested a review from Luap99 February 7, 2025 16:44
Luap99
Luap99 reviewed Feb 7, 2025
View reviewed changes
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.

Does the tmpfs change actually do anything? AFAICT we set TMPDIR=/var/tmp so nothing ends up on tmpfs?

In general a PR like this benefits from precise numbers before/after. A commit like "bump cpu and memory" is not helping any future reader. It is missing the why we do this and the numbers on much we safe.

@Luap99
Copy link
Member

Luap99 commented Feb 7, 2025

Actually never mind we only set TMPDIR: '/var/tmp' on the conformance vfs task per 8b0ecd7

So I think the tests were already using tmpfs

@flouthoc flouthoc force-pushed the integrate-experiment branch from fe0750f to 4547c96 Compare February 7, 2025 20:01
@TomSweeneyRedHat
Copy link
Member

LGTM

@TomSweeneyRedHat
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 17, 2025
@TomSweeneyRedHat
Copy link
Member

@flouthoc you may need a rebase here.

@flouthoc flouthoc force-pushed the integrate-experiment branch from 91fc35a to 391ba38 Compare February 17, 2025 22:23
@openshift-ci openshift-ci bot removed the lgtm label Feb 17, 2025
@flouthoc
Copy link
Collaborator Author

@flouthoc you may need a rebase here.

Done.

@flouthoc
Copy link
Collaborator Author

@rhatdan @nalind @TomSweeneyRedHat PTAL

This was referenced Feb 18, 2025
Open
Open
@flouthoc flouthoc force-pushed the integrate-experiment branch 2 times, most recently from 1d432ff to a8c42b1 Compare February 18, 2025 21:26
@flouthoc flouthoc requested a review from nalind February 18, 2025 21:27
@nalind
Copy link
Member

nalind commented Feb 18, 2025

LGTM

@flouthoc
test: heredoc remove python dependency from test
c86f554 
Use regular `cat` to test the same functionality instead
of using python image specifically for this part of test.

Signed-off-by: flouthoc <[email protected]>
@flouthoc
heredoc: create temp subdirs for each build
efb28dc 
Signed-off-by: flouthoc <[email protected]>
@flouthoc
test: use /tmp as TMPDIR
d7d7878 
use /tmp as TMPDIR so tests use tmpfs

Signed-off-by: flouthoc <[email protected]>
@flouthoc
mkcw: mkcw_check_image use bats run_with_log
c87fd8e 
Add `run_with_log` to mkcw tests.

Add `sleep 1` during cleanup between attempting `luksClose`
and unmounting the filesystem mounted on the device /dev/mapper/"$uuid".
Without this somehow we end up in a state where mount is still being
used by the kernel because when we do `lsof /dev/mapper/"$uuid"` it
shows nothing but `dmsetup info -c $uuid` shows the device is still
under use. Adding `sleep 1` in between somehow fixes this.

Also this problem with `cryptsetup` is pretty common for reference
one thread which I found https://lore.kernel.org/all/[email protected]/T/

Signed-off-by: flouthoc <[email protected]>
@flouthoc flouthoc force-pushed the integrate-experiment branch from a8c42b1 to c87fd8e Compare February 18, 2025 22:50
@flouthoc
Copy link
Collaborator Author

Rebased with main.

giuseppe
giuseppe approved these changes Feb 19, 2025
View reviewed changes
Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 19, 2025
@giuseppe
Copy link
Member

/approve

1 similar comment
@rhatdan
Copy link
Member

rhatdan commented Feb 19, 2025

/approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Feb 19, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, giuseppe, Luap99, rhatdan

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

@openshift-merge-bot openshift-merge-bot bot merged commit 3d14858 into containers:main Feb 19, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giuseppe giuseppe giuseppe approved these changes

@Luap99 Luap99 Luap99 approved these changes

@rhatdan rhatdan Awaiting requested review from rhatdan

@nalind nalind Awaiting requested review from nalind

Assignees

@giuseppe giuseppe

@TomSweeneyRedHat TomSweeneyRedHat

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@flouthoc @Luap99 @TomSweeneyRedHat @nalind @giuseppe @rhatdan