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

test: disable NAT port mapping, outbound dials, inbound connections #12591

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Oct 11, 2024

Proposed Changes

My poor network. It deserves to be treated with respect and dignity and doesn't deserve to be spammed.

  1. Disable NAT port mapping. Because no, we don't want our integration tests nodes to be reachable.
  2. Disable all but a single localhost/quic transport. No need to do more work than necessary.
  3. Set the connection manager limits to be really high. This probably doesn't matter, but there's no need to be killing connections in our integration tests.
  4. Reject all outbound dials to non-localhost addresses.

Checklist

Before you mark the PR ready for review, please make sure that:

@Stebalien Stebalien added the skip/changelog This change does not require CHANGELOG.md update label Oct 11, 2024
itests/kit/ensemble.go Outdated Show resolved Hide resolved
@Stebalien

This comment was marked as resolved.

My poor network. It deserves to be treated with respect and dignity and
doesn't deserve to be spammed.

1. Disable NAT port mapping. Because no, we don't want our integration
tests nodes to be reachable.
2. Disable all but a single localhost/quic transport. No need to do more
work than necessary.
3. Set the connection manager limits to be really high. This probably
doesn't matter, but there's no need to be killing connections in our
integration tests.
4. Reject all outbound dials to non-localhost addresses.
Copy link
Collaborator

@ribasushi ribasushi left a comment

Choose a reason for hiding this comment

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

this is great - want.

@rvagg
Copy link
Member

rvagg commented Oct 14, 2024

The ddo test being flaky here is weird (rerun made it pass) and makes me sus. But is that because of #12592?

@Stebalien
Copy link
Member Author

The ddo test being flaky here is weird (rerun made it pass) and makes me sus. But is that because of #12592?

Shouldn't be, this branch is up to date. But I'm also not sure how this change could affect DDO...

@rvagg
Copy link
Member

rvagg commented Oct 14, 2024

Yeah, just a quick look at the logs it seems like it's something about the miner<>node cooperation in that test. It might not be odd for a flaky, but I don't recall seeing that particular test as flaky, ever.

@Stebalien
Copy link
Member Author

Yeah, just a quick look at the logs it seems like it's something about the miner<>node cooperation in that test. It might not be odd for a flaky, but I don't recall seeing that particular test as flaky, ever.

Yeah, I haven't seen that before. I'll rerun it a few times and see.

@Stebalien
Copy link
Member Author

So, 1 failure for DDO against 4 successes. Looking at the failure, it looks like it took slightly too long:

2024-10-12T14:32:43.3573014Z         	Error:      	Should not be: 199
2024-10-12T14:32:43.3573236Z         	Test:       	TestOnboardRawPieceSnap
2024-10-12T14:32:43.3573713Z         	Messages:   	block at height 4512 never managed to sync to node, which is at height 4511

@Stebalien
Copy link
Member Author

Hm. Although TestOnboardRawPieceSnap is timing out after 10m. Waiting on

miner.WaitSectorsProving(ctx, toCheck)
.

@Stebalien Stebalien merged commit a89cf6d into master Oct 15, 2024
85 checks passed
@Stebalien Stebalien deleted the steb/quiet-itests branch October 15, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip/changelog This change does not require CHANGELOG.md update
Projects
Status: ☑️ Done (Archive)
Development

Successfully merging this pull request may close these issues.

5 participants