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

🌱 Reduce GH fixture test GINKGO_NODES to 1 #1543

Merged
merged 1 commit into from
Feb 13, 2024
Merged

🌱 Reduce GH fixture test GINKGO_NODES to 1 #1543

merged 1 commit into from
Feb 13, 2024

Conversation

mquhuy
Copy link
Member

@mquhuy mquhuy commented Feb 2, 2024 •
edited
Loading

We're facing many failures in GH-action-based E2E fixture tests. This is likely due to the github VM is too small and cannot handle two gingko threads.

@metal3-io-bot metal3-io-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 2, 2024
@mquhuy mquhuy force-pushed the mquhuy/reduce-e2e-fixture-threads branch from e4019f2 to 15b94c0 Compare February 2, 2024 06:34
@mquhuy mquhuy changed the title WIP: Reduce GH fixture test GINKGO_NODES to 1 Reduce GH fixture test GINKGO_NODES to 1 Feb 2, 2024
@metal3-io-bot metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 2, 2024
@mquhuy mquhuy changed the title Reduce GH fixture test GINKGO_NODES to 1 🐛 Reduce GH fixture test GINKGO_NODES to 1 Feb 2, 2024
@mquhuy mquhuy force-pushed the mquhuy/reduce-e2e-fixture-threads branch from 15b94c0 to 8793429 Compare February 2, 2024 06:48
tuminoid
tuminoid reviewed Feb 2, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

LGTM, but

/cc @lentzi90

@tuminoid
Copy link
Member

tuminoid commented Feb 2, 2024

/retitle 🌱 Reduce GH fixture test GINKGO_NODES to 1

@metal3-io-bot metal3-io-bot changed the title 🐛 Reduce GH fixture test GINKGO_NODES to 1 🌱 Reduce GH fixture test GINKGO_NODES to 1 Feb 2, 2024
@mquhuy mquhuy force-pushed the mquhuy/reduce-e2e-fixture-threads branch from 8793429 to b4b5b2b Compare February 2, 2024 07:50
lentzi90
lentzi90 reviewed Feb 2, 2024
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Let's try to figure out if/why this is an issue first. The tests should not be heavy when running with BMO in test mode. Perhaps the test mode is just not handling concurrency well?

Edit: If we just trigger the test many times on this PR and it is stable, I'm also happy to merge it just to get more stable CI, but I would still want to figure out the "why" 🙂

@mquhuy
Reduce GH fixture test GINKGO_NODES to 1
c6f5312 
Signed-off-by: Huy Mai <huy.mai@est.tech>
@mquhuy mquhuy force-pushed the mquhuy/reduce-e2e-fixture-threads branch from b4b5b2b to c6f5312 Compare February 2, 2024 09:55
@tuminoid
Copy link
Member

tuminoid commented Feb 2, 2024

This might actually be bug in BMO as found by @lentzi90 . Let's investigate that a bit more, and this might not be needed.
/hold

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2024
@lentzi90
Copy link
Member

lentzi90 commented Feb 2, 2024

Yup, I can put it here also so it is more obvious what we are discussing.
Normally the live-ISO test takes <40 seconds in the fixture job. When we hit this particular issue, it times out after 300 seconds!
I have seen this happen when going from deprovisioning to available and also from provisioning to provisioned. I have a feeling we are dealing with a subtle bug rather than performance issues

@tuminoid
Copy link
Member

tuminoid commented Feb 8, 2024

/test-centos-e2e-integration-main
Random test.

@tuminoid tuminoid mentioned this pull request Feb 8, 2024
Closed
@mquhuy
Copy link
Member Author

mquhuy commented Feb 13, 2024

/unhold Let's merge this as a quick fix for CI, then work on the bugs afterwards

@mquhuy
Copy link
Member Author

mquhuy commented Feb 13, 2024

/test metal3-bmo-e2e-test-pull

@mquhuy
Copy link
Member Author

mquhuy commented Feb 13, 2024

/cc @lentzi90 @tuminoid @kashifest

tuminoid
tuminoid reviewed Feb 13, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

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

/lgtm
/unhold

@metal3-io-bot metal3-io-bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 13, 2024
@lentzi90
Copy link
Member

/cc @kashifest
Could you approve? We believe this could stabilize the fixture tests a bit

@kashifest
Copy link
Member

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2024
@metal3-io-bot metal3-io-bot merged commit 8fcf061 into metal3-io:main Feb 13, 2024
19 checks passed
@metal3-io-bot metal3-io-bot deleted the mquhuy/reduce-e2e-fixture-threads branch February 13, 2024 12:12
@mquhuy mquhuy mentioned this pull request Feb 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tuminoid tuminoid tuminoid left review comments

@kashifest kashifest Awaiting requested review from kashifest

@lentzi90 lentzi90 Awaiting requested review from lentzi90

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mquhuy @tuminoid @lentzi90 @kashifest @metal3-io-bot