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

fix: make netsim run from examples #2959

Merged

Conversation

Arqu
Copy link
Collaborator

@Arqu Arqu commented Nov 22, 2024

Description

Moves our netsim tests to run from an example and only test the lib part of the code, not the full CLI & blobs.

Things left to do:

  • rename the example from new to anything that makes more sense netsim runs it under iroh-transfer
  • clean up the example code
  • add the option to provide either a relay config or pass in at least the relay url as an argument
  • continue the CI adjustment and move all invocations of the netsim runner to run from iroh_v2 and integration_v2 sims (we want a less abrupt netsim switchover)
  • convert the remaining sims in chuck/netsim/sims to the new format
  • after some time flip back the CI invocations to be regular iroh and integration sims which includes doing the same on netsim and removing the old ones

Breaking Changes

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

@Arqu Arqu self-assigned this Nov 22, 2024
@Arqu Arqu changed the base branch from main to refactor-drop-external-protocols November 22, 2024 11:35
@Arqu Arqu changed the title Arqu/netsim from example [WIP] make netsim run from examples Nov 22, 2024
Copy link

github-actions bot commented Nov 22, 2024

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2959/docs/iroh/

Last updated: 2024-11-25T14:05:10Z

Copy link

github-actions bot commented Nov 22, 2024

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: b7c9928

@Arqu
Copy link
Collaborator Author

Arqu commented Nov 22, 2024

test case throughput_gbps throughput_transfer
iroh 1_to_1 1.32 1.33
iroh 1_to_3 4.28 4.28
iroh 1_to_5 6.89 6.90
iroh 1_to_10 11.65 11.67
iroh 2_to_2 2.77 2.78
iroh 2_to_4 5.42 5.43
iroh 2_to_6 8.22 8.23
iroh 2_to_10 13.55 13.58

@Arqu Arqu requested a review from dignifiedquire November 25, 2024 11:24
@Arqu Arqu marked this pull request as ready for review November 25, 2024 11:24
@Arqu Arqu changed the title [WIP] make netsim run from examples fix: make netsim run from examples Nov 25, 2024
Copy link
Contributor

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

looks good overall

@Arqu
Copy link
Collaborator Author

Arqu commented Nov 25, 2024

Waiting for CI and will merge.

@Arqu Arqu requested a review from flub November 25, 2024 11:56
iroh/examples/transfer.rs Outdated Show resolved Hide resolved
iroh/examples/transfer.rs Show resolved Hide resolved
iroh/examples/transfer.rs Outdated Show resolved Hide resolved
let duration = start.elapsed();
println!(
"Received {} B in {:.4}s with ttfb {}s in {} chunks",
HumanBytes(len as u64),
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to use HumanBytes here? the B in the format string suggests you just want an integer number of bytes that are easy to parse? The next line seems to do the human-friendly printing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, mostly this is a precursor for moving the current netsim parsing to a different format. This just shows we have more data now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Did fix the extra B though.

iroh/examples/transfer.rs Outdated Show resolved Hide resolved
duration.as_secs_f64(),
HumanBytes((len as f64 / duration.as_secs_f64()) as u64)
);

Copy link
Contributor

Choose a reason for hiding this comment

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

You now need to explicitly call endpoint.close().await to make a best effort to let the other side know you closed the connection. If you don't do this and the datagram carrying the close was lost the other side will trigger a timeout. Awaiting the endpoint.close makes sure to wait until the datagram carrying the close is acknowledged (or times out on the acknowledgement).

Currently you probably also want to put this endpoint.close().await wrapped in a application-level timeout like you do on the other side because our API WRT timeouts is a bit crap right now...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Something like now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or should I move the endpoint.close also into the timeout?

Copy link
Contributor

@flub flub Nov 25, 2024

Choose a reason for hiding this comment

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

Something like this here:

Suggested change
tokio::time::timeout(Duration::from_secs(3), async move {
endpoint.close(0u8.into(), b"closing").await
})??;

@Arqu Arqu requested a review from flub November 25, 2024 14:02
@dignifiedquire dignifiedquire added this to the v0.29.0 milestone Nov 25, 2024
@dignifiedquire dignifiedquire merged commit f4adb94 into refactor-drop-external-protocols Nov 25, 2024
20 of 25 checks passed
@dignifiedquire dignifiedquire deleted the arqu/netsim_from_example branch November 25, 2024 15:40
dignifiedquire pushed a commit that referenced this pull request Nov 25, 2024
## Description

Moves our netsim tests to run from an example and only test the lib part
of the code, not the full CLI & blobs.

Things left to do:
- [x] rename the example from `new` to anything that makes more sense
`netsim` runs it under `iroh-transfer`
- [x] clean up the example code
- [x] add the option to provide either a relay config or pass in at
least the relay url as an argument
- [x] continue the CI adjustment and move all invocations of the netsim
runner to run from `iroh_v2` and `integration_v2` sims (we want a less
abrupt netsim switchover)
- [x] convert the remaining sims in `chuck/netsim/sims` to the new
format
- [ ] after some time flip back the CI invocations to be regular `iroh`
and `integration` sims which includes doing the same on `netsim` and
removing the old ones

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the PR.
-->

## Change checklist

- [ ] Self-review.
- [ ] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [ ] Tests if relevant.
- [ ] All breaking changes documented.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants