Skip to content

HBONE connections benches#631

Merged
istio-testing merged 3 commits intoistio:masterfrom
Stevenjin8:bench/hbone-connect
Aug 25, 2023
Merged

HBONE connections benches#631
istio-testing merged 3 commits intoistio:masterfrom
Stevenjin8:bench/hbone-connect

Conversation

@Stevenjin8
Copy link
Copy Markdown
Contributor

@Stevenjin8 Stevenjin8 commented Aug 2, 2023

Benchmarks how long it takes to establish a new HBONE connection.
Do this by creating n workloads giving us O(n^2) ways to create new HBONE connections. The actual limiting factor here is not the number of workload source/destination pairs, but the number of possible open file descriptions.
This happens to me when I change the duration of the test from 5 to 10 seconds.

I confirmed this is creating new HBONE connections by running tcpdump on lo and seeing the number of TLS handshakes was more or less the same as the number of test iterations.

resolves #623

@istio-testing
Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 2, 2023
@Stevenjin8
Copy link
Copy Markdown
Contributor Author

/test all

1 similar comment
@Stevenjin8
Copy link
Copy Markdown
Contributor Author

/test all

@Stevenjin8
Copy link
Copy Markdown
Contributor Author

/retest

@Stevenjin8
Copy link
Copy Markdown
Contributor Author

/test all

1 similar comment
@Stevenjin8
Copy link
Copy Markdown
Contributor Author

/test all

@Stevenjin8
Copy link
Copy Markdown
Contributor Author

I have no idea how to fix the CI error

error: non-binding `let` on a future
   --> benches/throughput.rs:465:9
    |
465 | /         let _ = tokio::spawn(async move {
466 | |             let _ = tokio::join!(app.wait_termination(), echo.run());
467 | |         });
    | |___________^
    |
    = help: consider awaiting the future or dropping explicitly with `std::mem::drop`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#le

The other benches do the by doing let t = tokio::spawn(...) and then returning t. However, t gets eventually bound to _ so it never gets awaited

echo_addr,
TEST_WORKLOAD_HBONE.parse().unwrap(),
))
.socks5_connect(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This section is repeated often.

e.g.

 let mut s =
    e.ta.socks5_connect(helpers::with_ip(
        e.echo_addr,
        TEST_WORKLOAD_TCP.parse().unwrap(),
    ))
    .await;

Wdyt about abstracting that logic into a helper function, maybe something called newTCPStream or similar. Have it maybe take in the TEST_WORKLOAD_<SOURCE/HBONE/TCP> string value as an argument. Would help to maintain that DRY principle while also making it convenient for the next person to extend this test with more benches?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll think about this. The whole echo server setup is clever but also confusing. You can access the echo server through any ip address on lo, but if you want to use ztunnel/socks5, the address you pick to access the echo server needs to be a registered workload.

Point is, I feel like the way its abstracted right now is already confusing, so I didn't want to abstract it any further. I do think there is probably a better way to do this. My first idea it to make TEST_WORKLOAD_* actual IP address instead of a string that can be parsed into an ip address. Thoughts?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The abstraction I refer to is to really help reduce the repetitive and confusing code in the tests. Using a helper function should actually make it more understandable.

@MorrisLaw
Copy link
Copy Markdown
Contributor

let t won't work for your test? Is that why you did let _?

@Stevenjin8
Copy link
Copy Markdown
Contributor Author

let t won't work for your test? Is that why you did let _?

let t doesn't work because it ends up being an unused variable. But the warning comes from the fact that I never await the result of the spawn, which is the same for the other benches.

}

/// Iterate through possible IP pairs restricted to 0 < ip_pair.0 < ip_pair.1 <= MAX_HBONE_WORKLOADS.
fn next_ip_pair(ip_pair: (u8, u8)) -> (u8, u8) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Note this is only n*(n-1)/2 combos. If we wanted more new HBONE connections from the same number of IPs this could be optimized.

@Stevenjin8 Stevenjin8 marked this pull request as ready for review August 4, 2023 16:45
@Stevenjin8 Stevenjin8 requested a review from a team as a code owner August 4, 2023 16:45
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Aug 4, 2023
let echo_addr = echo.address();
let _ = tokio::spawn(async move {
let _ = tokio::join!(app.wait_termination(), echo.run());
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Adding a .await here should fix the CI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or an explicit drop

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

TODO: Drop works, but hoping that someone has a better solution than just orphaning the tasks.

Steven Jin Xuan added 3 commits August 25, 2023 18:01
Signed-off-by: Steven Jin Xuan <t-stjinxuan@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Benching HBONE handshake performance

6 participants