[Sharding 4] Base testing support for sharding #1369

cberkhoff · 2024-10-24T03:32:48Z

This is the fourth of a series of pull requests (PR) to enable sharding on IPA. See #1364

This change doesn't meaningfully change any tests or the operation of IPA. This is just adding abstractions that are going to be useful later.

Biggest change here is in net/test.rs module, which was enhanced to create Sharded infrastructure. This will become more useful for the sharded shuffle test, but releasing early allows reviewers to check for the intended architecture and its usage.

codecov · 2024-10-24T17:09:49Z

Codecov Report

Attention: Patch coverage is 98.90411% with 4 lines in your changes missing coverage. Please review.

Project coverage is 93.66%. Comparing base (933a902) to head (29258c5).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
ipa-core/src/net/test.rs	98.46%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1369      +/-   ##
==========================================
+ Coverage   93.64%   93.66%   +0.01%     
==========================================
  Files         223      223              
  Lines       37112    37376     +264     
==========================================
+ Hits        34755    35008     +253     
- Misses       2357     2368      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

akoshelev · 2024-10-25T03:04:54Z

ipa-core/src/net/test.rs

+
+impl TestNetwork<Shard> {
+    #[must_use]
+


Suggested change

akoshelev · 2024-10-25T03:10:38Z

ipa-core/src/net/test.rs

+    /// 3 mpc ports.
+    ports_by_ring: Vec<Ports<Vec<u16>>>,
+    /// Describes the number of shards per helper
+    sharding_factor: usize,


when I see factor I think about the multipler. shard_count is easy and straightforward

That was intended because you're getting 3x hosts for the factor.

If you still think shard_count is easier I can do that, but this factor affects all 3 helpers.

then /// Describes the number of shards per helper confuses me even more

Why? it says per helper aka 3x

akoshelev · 2024-10-25T03:14:28Z

ipa-core/src/net/test.rs

    #[must_use]
-    pub fn with_open_ports() -> Self {
+    pub fn with_http_and_default_test_ports() -> Self {


I don't see the functionality to request shard ports from the Kernel, is it planned? Static ports are constant source of pain for tests and I don't think we should go with static. It is fine to test configs, but all unit tests should run with random ports assigned at runtime

Requesting ports from the kernel is part of the default method.

In the past we used to have both, with_open_ports and default, which did the same thing. I thought less code is better.

akoshelev · 2024-10-25T03:15:25Z

ipa-core/src/net/test.rs

+        optional_ports: Option<Vec<u16>>,
+        ring_index: usize,
+    ) -> TestServers {
+        let mut sockets = vec![None, None, None];


why not just Vec::with_capacity(3)?

akoshelev · 2024-10-25T03:20:47Z

ipa-core/src/net/test.rs

        });
-        let (scheme, certs) = if self.disable_https {
-            ("http", [None, None, None])
+        // Ports will always be defined after this. Socks only if there were no ports set.


I don't understand this block of code and comment to it. Why we use all ports available to build servers in HTTPS mode and only 3 for HTTP?

Not sure to what part you're referring...

But this function creates 3 servers.

After line 270 all the port situation will be set. Either you gave it as param or we asked the os.

Not sure to what part you're referring when you say "Why we use all ports available to build servers"... But this function creates 3 servers.

The confusion comes from either using optional_ports or synthesizing ports inside

optional_ports: Option<Vec<u16>> which can be arbitrary size according to this function (it does not enforce size to be 3, if it should, then it would be better to take an array). So ports may have variable size

akoshelev · 2024-10-25T04:07:37Z

ipa-core/src/net/test.rs

+    }
+
+    /// Defines a test network with a single MPC ring (3 helpers).
+    fn build_ring_network(


After this change TestConfig looks a lot more complicated that it was before. I think it will benefit from some refactoring to split MPC and shard network initializations

Yeah, Test config holds now the entire sharded network. But on the other hand, having everything described on this single object makes it easy to operate on the network as a whole which is the use case (check the transport.rs test). Separating this into 2 seems like it will only add complexity on the caller side.

Can you be more explicit on how you want to split the network init?

you don't have to make callers suffer. Providing a single entry point for them to construct the config is reasonable. Doing everything inside one struct after that is not necessary - power through composition

akoshelev · 2024-10-25T04:09:23Z

ipa-core/src/net/test.rs

+        let rings: Vec<TestNetwork<Helper>> = (0..self.sharding_factor)
+            .map(|s| {
+                let ring_ports = self.ports_by_ring.get(s).map(|p| p.ring.clone());
+                let shards_ports = self.ports_by_ring.get(s).map(|p| p.shards.clone());


Suggested change

let rings: Vec<TestNetwork<Helper>> = (0..self.sharding_factor)

.map(|s| {

let ring_ports = self.ports_by_ring.get(s).map(|p| p.ring.clone());

let shards_ports = self.ports_by_ring.get(s).map(|p| p.shards.clone());

let r = zip(ring_ports, shard_ports).map(|(r_port, s_port)| { ... })

there are cases where ports_by_ring won't be set though.

akoshelev · 2024-10-25T04:10:19Z

ipa-core/src/net/test.rs

+}
+
+#[must_use]
+pub fn create_ids(disable_https: bool, id: HelperIdentity, ix: ShardIndex) -> ClientIdentities {


should it be a constructor on ClientIdentities?

Sure. The conditional on disable_https had me on the fence

akoshelev · 2024-10-25T04:12:24Z

ipa-core/src/net/test.rs

-    let (mut certificate, mut private_key) = get_test_certificate_and_key(id);
-    ClientIdentity::from_pkcs8(&mut certificate, &mut private_key).unwrap()
+pub fn get_client_test_identity(id: HelperIdentity, ix: ShardIndex) -> ClientIdentities {
+    let pos = ix.as_index() * 3 + id.as_index();


that deserves a comment and a common function to compute the absolute offset as I believe it is used in more than one place

akoshelev · 2024-10-25T04:13:16Z

ipa-core/src/net/test.rs

+    /// The network itself won't be excersized as that's tested elsewhere.
+    #[test]
+    fn create_4_shard_http_network() {
+        let ports: Vec<Ports<Vec<u16>>> = vec![


do you want to test that overlapping ports are rejected?

Sure, I can add that

akoshelev

I think we shouldn't store a TcpListener inside a configuration struct, so I'd like this to be fixed before pushing it, but other than that lgtm

akoshelev · 2024-10-29T17:13:02Z

ipa-core/src/net/test.rs

+    /// Contains the ports
+    pub config: ServerConfig,
+    /// Sockets are created if no port was specified.
+    pub socket: Option<TcpListener>,


as much as I like the idea of holding onto TcpListener until the moment when server is spawned - minimizes the chances of Kernel re-using that port in between - I don't think the approach of storing TcpListener in the configuration struct is appropriate here. TcpListener does not describe the configuration, it actively uses a system resource (socket)

Maybe we can get away with using file descriptors, but honestly I wouldn't bother as I've never seen an issue with bind/release.

There's a chain of dependencies; PeerConfig requires a port, etc.
Changing this won't be simple. I can add a todo.

just to be clear - I am not suggesting to get rid of the port here. If port is not provided by the caller, you can do the previous workaround (ask kernel for a port and drop the listener after that)

Ahh, I think that might be problematic with many tests running at the same time.

Kernel is good not releasing the port right away

akoshelev · 2024-10-29T17:16:54Z

ipa-core/src/net/test.rs

+                .map(|r| r.shards[id.as_index()])
+                .map(Some)


Suggested change

.map(|r| r.shards[id.as_index()])

.map(Some)

.map(|r| Some(r.shards[id.as_index()]))

In general, do you prefer larger blocks of code inside a map function instead of more map calls?

it is always a balance, cluttered map calls are bad for readability, more map calls result in more complex types used and more work for compiler to unwind them

akoshelev · 2024-10-29T17:18:40Z

ipa-core/src/net/test.rs

+    }
+
+    /// Get all the MPC ports in a ring specified by the shard index.
+    fn get_ports_for_shard_index(&self, ix: ShardIndex) -> Vec<Option<u16>> {


from the ergonomic perspective, a better type to return would be Option<Vec<u16>> here

akoshelev · 2024-10-29T17:19:39Z

ipa-core/src/net/test.rs

@@ -9,46 +9,171 @@

 #![allow(clippy::missing_panics_doc)]
 use std::{
-    array,
+    collections::HashSet,


Consider splitting this file into multiple - it grew too big and Github now hides all the changes for it by default

I'm thinking on separating the TestServer stuff

Test support for sharding

6e431f4

cberkhoff requested review from akoshelev and andyleiserson October 24, 2024 03:32

Fix: rename TestConfig

f0cb507

akoshelev requested changes Oct 25, 2024

View reviewed changes

cberkhoff added 4 commits October 28, 2024 15:05

Addressing comments

d691b06

Clippy

797b35d

Cannot use repeat_n

29a8a4c

Merge branch 'main' into shard-infra-4

7daafdf

akoshelev approved these changes Oct 29, 2024

View reviewed changes

Nit: Some

29258c5

cberkhoff merged commit 4d50a64 into private-attribution:main Oct 29, 2024
12 checks passed

	.map(\|r\| r.shards[id.as_index()])
	.map(Some)
	.map(\|r\| Some(r.shards[id.as_index()]))

[Sharding 4] Base testing support for sharding #1369

[Sharding 4] Base testing support for sharding #1369

Conversation

cberkhoff commented Oct 24, 2024

codecov bot commented Oct 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akoshelev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 24, 2024 •

edited

Loading