Multiple network interface binding #281

al-rigazzi · 2023-04-26T20:20:52Z

This PR adds the possibility of binding Orchestrators and co-located DBs to multiple network interfaces. It also adds the --bind-source-addr argument to any Redis DB we deploy, making sure shards bind to an address we know a priori.

codecov · 2023-04-26T20:43:52Z

Codecov Report

Merging #281 (12d21ac) into develop (950ef6b) will increase coverage by 0.04%.
The diff coverage is 92.30%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #281      +/-   ##
===========================================
+ Coverage    87.65%   87.70%   +0.04%     
===========================================
  Files           60       60              
  Lines         3386     3374      -12     
===========================================
- Hits          2968     2959       -9     
+ Misses         418      415       -3

Impacted Files	Coverage Δ
smartsim/_core/control/manifest.py	`94.78% <0.00%> (ø)`
smartsim/_core/config/config.py	`98.38% <100.00%> (+3.45%)`	⬆️
smartsim/_core/launcher/colocated.py	`93.10% <100.00%> (+0.16%)`	⬆️
smartsim/database/orchestrator.py	`85.13% <100.00%> (+0.15%)`	⬆️

MattToast

Looks great! a couple of minor style things and some concerns around the test that was changed, but otherwise lgtm!!

MattToast · 2023-05-01T16:32:21Z

smartsim/_core/entrypoints/colocated.py

        cmd = command + [f"--bind {lo_address}"]
    else:
        # bind to both addresses if the user specified a network
        # address that exists and is not the loopback address
-        cmd = command + [f"--bind {lo_address} {ip_address}"]
+        cmd = command + [f"--bind {lo_address} " + " ".join(ip_addresses)]


Minor knit pick, can we make this use either entierly f-strs or string concatanation just be be consistant? Maybe

[f"--bind {lo_address} {' '.join(ip_addresses)}"]

?

MattToast · 2023-05-01T16:32:23Z

smartsim/_core/entrypoints/colocated.py

@@ -206,7 +212,7 @@ def main(
            "\n\nCo-located database information\n"
            + "\n".join(
                (
-                    f"\tIP Address: {ip_address}",
+                    f"\tIP Address(es): " + " ".join(ip_addresses + [lo_address]),


Same unify f-str vs str-concat knit pick here. Would prefer f-str to be consistent with line 218

Makes sense.

MattToast · 2023-05-01T16:55:56Z

smartsim/database/orchestrator.py

-            )
-            logger.warning(f"Found network interfaces are: {available}")
+        for interface in self._interfaces:
+            if interface not in net_if_addrs and self._interfaces != "lo":


In this line

if interface not in net_if_addrs and self._interfaces != "lo": # ^^^^^^^^^^^^^^^^ # Should this be `interface` instead of `self._interfaces`?

It makes sense.

MattToast · 2023-05-01T17:13:06Z

tests/backends/test_dbmodel.py

-    exp.start(colo_model, block=True)
-    statuses = exp.get_status(colo_model)
-    assert all([stat == status.STATUS_COMPLETED for stat in statuses])
+    exp.start(colo_model, block=False)
+
+    completed = False
+    timeout = 90
+    check_interval = 5
+    while timeout and not completed:
+        timeout -= check_interval
+        time.sleep(check_interval)
+        statuses = exp.get_status(colo_model)
+        if all([stat == status.STATUS_COMPLETED for stat in statuses]):
+            completed = True
+
+    assert completed


Not sure I understand that change here from block=True to block=False with a timeout. Was this test hanging and this was a way to "fail fast"?

Yeah, I think it was hanging forever (even in CI/CD). Do you think there is a better solution?

Nope! just wanted make sure I understood the reason for the change!

MattToast · 2023-05-01T17:15:05Z

tests/backends/test_dbmodel.py

+    completed = False
+    timeout = 90
+    check_interval = 5
+    while timeout and not completed:


Just confirming: If the timeout is expended here and the test fails, is the colo_model run killed and cleaned up, or is the process left running?

Good question, I'd say it could be left running. I changed the way we handle it now.

MattToast · 2023-05-01T17:59:53Z

smartsim/_core/config/config.py

+                for net_if_addr in net_if_addrs:
+                    if net_if_addr.startswith("hsn"):
+                        interfaces.append(net_if_addr)
+                return interfaces


This return type does not match the type annotation of the fn: expected str but got list[str]

Unionized, do you think it is fine? We could actually always return a list (even with just one item).

ooh I kinda like the idea of unifying the type and making it always return a list just for consistency's sake. That way I could always count on being doing something like

for network in Config().test_interface: # `network` is guaranteed to be a str of name of network # not chars in a str # Not sure why I would want to do this, but could ¯\_(ツ)_/¯ ...

Shrug is the way. I'll fix it.

MattToast · 2023-05-01T18:03:59Z

smartsim/_core/entrypoints/colocated.py

+        ip_addresses = []
+        ip_addresses.extend(
+            [current_ip(interface) for interface in network_interface.split(",")]
+        )


Can these lines be simplified to just be

ip_addresses = [current_ip(interface) for interface in network_interface.split(",")]

Yes, I think it makes sense.

MattToast · 2023-05-01T18:31:29Z

smartsim/_core/entrypoints/redis.py

+            current_ip(net_if) for net_if in network_interface.split(",")
+        ]
+        ip_address = ip_addresses[0]
+        cmd = command + ["--bind " + " ".join(ip_addresses)]


Now purely stylistic: Can we also make this an f-str to be consistent with the colo script?

Yeah, it always bothered me.

MattToast · 2023-05-01T18:40:25Z

smartsim/database/orchestrator.py

-        :param interface: network interface, defaults to "lo"
-        :type interface: str, optional
+        :param interface: network interface(s), defaults to "lo"
+        :type interface: str, list[str], optional


minor doc thing: earlier in the docstrings, the type "string or list of strings" was conveyed as str | list[str]. Should we do that here as well/update the earlier one to be str, list[str] just to be consistent?

Should it be Optional[str] | Optional[list[str]]?

Good question! The "proper" (i.e. mypy accepted) type hint would be Optional[Union[str, list[str]]] in py<=3.9 syntax or str | list[str] | None in py>=3.10 syntax

But this also brings up our somewhat inconsistent use of , optional. For the most part, when , optional is provided in the :type: tag we usually imply that a type of None is accepted and we simply do not annotate the None in the docstr :type:. I swear I've seen other packages do something similar, but cannot seem to recall any offhand.

Any preference between :type interface: str | list[str] | None, optional or :type interface: str | list[str], optional? I honestly could be swayed either way...

I'd say we either put None or optional... both seem redundant. Maybe keep optional for now?

MattToast · 2023-05-01T18:46:19Z

smartsim/_core/entrypoints/colocated.py


-    if lo_address == ip_address or not ip_address:
+    if (
+        all([lo_address == ip_address for ip_address in ip_addresses])


minor "performance" thing: If you drop the square brackets here, all will evaluate this lazily and will return on the first instance of False

That said, this list is small and we are unlikely to see literally any difference, so this is an entirely optional refactor

Dropping characters is always good.

MattToast

Two last conversations to close out, but otherwise LGTM!

Approving in advance bc everything else looks great!

al-rigazzi added 2 commits April 26, 2023 14:43

Misc test fixes

c99fc86

Add multiple interfaces and bind source address

d54b058

al-rigazzi added 2 commits April 27, 2023 13:50

Switch to list of strings for interface arg type

fedcfd5

Fix interface parsing in tests

8e6f884

al-rigazzi marked this pull request as ready for review April 27, 2023 18:03

al-rigazzi requested a review from MattToast April 27, 2023 18:27

Fix automatic network interface discovery in tests

a732456

MattToast requested changes May 1, 2023

View reviewed changes

al-rigazzi added 2 commits May 2, 2023 11:22

Address reviewer's comments.

73a688e

Make style of edited files

202b69e

al-rigazzi requested a review from MattToast May 2, 2023 18:25

Use List for older py versions

7067e5d

MattToast approved these changes May 2, 2023

View reviewed changes

al-rigazzi added 8 commits May 3, 2023 11:30

Make return type consistent in config function

6d07a6b

Exclude config test functions from coverage

3dec189

Exclude function from coverage

59e9d19

Remove wrong coverage hint

f6eddf5

List comprehension for config func

4371db5

Add multiple-interface-specific test

edad9dc

Fix experiment name in test

7bca2b0

Fix multi-interface test for MacOS

12d21ac

al-rigazzi merged commit f0ed512 into CrayLabs:develop May 3, 2023

al-rigazzi deleted the multi-interface branch May 10, 2023 19:15

MattToast mentioned this pull request Jun 22, 2023

Update add_ml_model and add_script #304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple network interface binding #281

Multiple network interface binding #281

al-rigazzi commented Apr 26, 2023 •

edited

Loading

codecov bot commented Apr 26, 2023 •

edited

Loading

MattToast left a comment

MattToast May 1, 2023

al-rigazzi May 2, 2023

MattToast May 1, 2023

al-rigazzi May 2, 2023

MattToast May 1, 2023

al-rigazzi May 2, 2023

MattToast May 1, 2023

al-rigazzi May 2, 2023

MattToast May 2, 2023

MattToast May 1, 2023

al-rigazzi May 2, 2023

MattToast May 1, 2023

al-rigazzi May 2, 2023

MattToast May 2, 2023 •

edited

Loading

al-rigazzi May 3, 2023

MattToast May 1, 2023

al-rigazzi May 2, 2023

MattToast May 1, 2023

al-rigazzi May 2, 2023

MattToast May 1, 2023

al-rigazzi May 2, 2023

MattToast May 2, 2023

al-rigazzi May 3, 2023

MattToast May 1, 2023

al-rigazzi May 2, 2023

MattToast left a comment

Multiple network interface binding #281

Multiple network interface binding #281

Conversation

al-rigazzi commented Apr 26, 2023 • edited Loading

codecov bot commented Apr 26, 2023 • edited Loading

Codecov Report

MattToast 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattToast May 2, 2023 • edited Loading

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

MattToast left a comment

Choose a reason for hiding this comment

al-rigazzi commented Apr 26, 2023 •

edited

Loading

codecov bot commented Apr 26, 2023 •

edited

Loading

MattToast May 2, 2023 •

edited

Loading