AZP: Add multi HCAs iodemo test by Alexey-Rivkin · Pull Request #261 · yosefe/ucx

Alexey-Rivkin · 2022-07-28T10:38:26Z

What

add test for multi HPCs test for iodemo
(cherry-pick from openucx#7971)

yosefe · 2022-07-28T10:42:44Z

-        initial_delay: 9999 # No interference
+        test_name: am_cx6_rc
+        extra_run_args: ""
+      "tag match on CX6/DC":


need to remove DC and AM tests from this branch. keep only tag match on CX4/RC

but this PR was going to fix tag match on CX6/RC

right, keep cx6 but remove AM and DC below

yosefe · 2022-07-28T10:42:58Z

+        tls: "dc_x"
+        test_name: tag_cx6_dc
+        extra_run_args: ""
+      "active messages on CX6/DC with data validation":


remove this case

yosefe · 2022-07-28T10:43:02Z

-        initial_delay: 9999 # No interference
+        test_name: tag_umemh_cx4_rc
+        extra_run_args: ""
+      "active messages on CX6/DC with user memh":


remove this case

NirWolfer · 2026-05-05T15:24:34Z

agent-review

NirWolfer · 2026-05-06T12:17:48Z

agent-review

NirWolfer

Code Review

8 findings — 🚨 3 blockers, ⚠️ 5 minors

NirWolfer · 2026-05-06T12:22:58Z

            test_ucx_tls: ${{ test.Value.tls }}
            test_extra_run_args: ${{ test.Value.extra_run_args }}
            initial_delay: ${{ coalesce(test.Value.initial_delay, parameters.initial_delay) }}
+            test_interference: ${{ test.Value.interference }}


🚨 BLOCKER

The matrix entries use interference as an optional key, but coalesce is not applied here (unlike initial_delay on line 122). If a test entry omits interference, ${{ test.Value.interference }} expands to an empty string and $(test_interference) will be empty when passed to az-stage-io-demo.yaml, causing the eq(variables['interference'], 'Yes') condition to silently skip the corrupter. Add a coalesce(test.Value.interference, 'Yes') default, matching the pattern used for initial_delay.

NirWolfer · 2026-05-06T12:22:58Z

+      net_device=$(ibdev2netdev | sed -ne 's/\(\w*\) port \([0-9]\) ==> '${interface}' .*/\1:\2/p')
+      net_devices="${net_devices} ${net_device}"
+    done
+    export UCX_NET_DEVICES=$(echo ${net_devices##*( )} | tr " ", ",")


🚨 BLOCKER

tr " " "," passes a three-character second argument ("," includes the comma and quote-artefact). The correct invocation is tr ' ' ','. As written, the UCX_NET_DEVICES value will be malformed and UCX will fail to find the device.

NirWolfer · 2026-05-06T12:22:58Z

+			addr=$(echo ${host_ip} | cut -d: -f2)
+			if [ -n "${host}" ] && [ -n "${addr}" ]
+			then
+				ip_address_per_host[${host}${net_if}]=${addr}


🚨 BLOCKER

The associative-array key ${host}${net_if} has no delimiter. A host named node1 with interface eth0 produces the same key as host node + interface 1eth0. Use a separator that cannot appear in either value, e.g. ${host}:${net_if}, and apply the same change at every lookup site (lines 465, 676).

NirWolfer · 2026-05-06T12:22:58Z

-    $(workspace)/buildlib/az-network-corrupter.sh reset=yes interface=${{ parameters.roce_iface }}
+    echo "##vso[task.setvariable variable=interference]${{ parameters.interference }}"
+    for interface in ${{ parameters.roce_iface }} ; do
+      $(workspace)/buildlib/az-network-corrupter.sh reset=yes interfaces=${interface}


⚠️ MINOR

The reset loop calls az-network-corrupter.sh once per interface sequentially. If one interface fails, subsequent interfaces are not reset. The original script already handles multi-interface iteration internally after this PR; passing the full space-separated list as a single interfaces= value would let the script handle it atomically and be consistent with how the corrupter loop is invoked.

NirWolfer · 2026-05-06T12:22:58Z

+        duration: 60
+        interface: $(roce_iface_cx4) $(roce_iface_cx6)
+        tls: "rc_x"
+        test_name: multy_cx6_cx4_rc


⚠️ MINOR

Test name multy_cx6_cx4_rc is a typo; should be multi_cx6_cx4_rc. Artifact names and analyzer output will inherit the misspelling.

NirWolfer · 2026-05-06T12:22:58Z

-        initial_delay: 9999 # No interference
+        test_name: tag_umemh_cx4_rc
+        extra_run_args: ""
+      "Multi HCAs: tag match on CX4 and CX6 RC ":


⚠️ MINOR

The matrix entry name 'Multi HCAs: tag match on CX4 and CX6 RC ' has a trailing space. YAML allows it but it can cause surprising behaviour if the key is referenced or displayed.

NirWolfer · 2026-05-06T12:22:58Z

+        test_name: multy_cx6_cx4_rc
+        extra_run_args: ""
+      "different UCX_IB_NUM_PATH":
+        args: "-A"


⚠️ MINOR

args: "-A" for the 'different UCX_IB_NUM_PATH' test case — the args field is not a declared parameter in az-stage-io-demo.yaml and is not forwarded anywhere in the matrix expansion. If -A is an intended io_demo flag it must be placed in extra_run_args; as written it is silently ignored.

NirWolfer · 2026-05-06T12:22:58Z

+			for net_if in $(split_list ${net_ifs})
+			do
+				port_num=$((base_port_num + i))
+				client_connect_list+=" ${ip_address_per_host[${host}${net_if}]}:${port_num}"


⚠️ MINOR

The server-port list now adds one host:port entry per (server, interface) pair. If the intent is that each server listens on all interfaces at the same port, this multiplies the connect list incorrectly. Confirm whether io_demo expects one entry per server or one per (server × interface).

NirWolfer · 2026-05-06T16:18:24Z

agent-review

NirWolfer · 2026-05-06T16:56:27Z

agent-review

NirWolfer

Code Review

⚠️ MINOR buildlib/pr/io_demo/az-stage-io-demo.yaml:64

tr " ", "," passes " ," (space + comma) as tr's source set instead of the intended " " (space only). The result is that both spaces and commas are mapped to commas. For the current input (net_devices is built with space separators and will never contain commas) this is harmless, but the extra comma is a typo and should be tr " " ",".

NirWolfer · 2026-05-06T17:05:14Z

agent-review

NirWolfer

Code Review

2 findings — 🚨 1 blocker, ⚠️ 1 minor

2 findings posted as inline comments.

NirWolfer · 2026-05-07T06:59:12Z

agent-review

NirWolfer

Code Review

8 findings — 🚨 4 blockers, ⚠️ 4 minors

8 findings posted as inline comments.

NirWolfer · 2026-05-07T07:03:52Z

 		do
-			port_num=$((base_port_num + i))
-			client_connect_list+=" ${ip_address_per_host[${host}]}:${port_num}"
+			for net_if in $(split_list ${net_ifs})


🚨 BLOCKER

The inner loop over interfaces inside the server-slot loop means each server slot i (listening on a single port base_port_num + i) gets one client_connect_list entry per interface, all pointing to the same port but different IPs. With num_tasks=2 (two NICs) and num_servers=2, the list becomes ip_cx4:20000 ip_cx6:20000 ip_cx4:20001 ip_cx6:20001 — four entries for two server processes. Clients are told to connect to four addresses but only two servers exist. The correct model for multi-NIC is one server per NIC, each bound to its own IP, so the connect list should have exactly one entry per server (matching its bound interface). The nesting order should be inverted: outer loop over interfaces, inner loop over server slots per interface, with each interface's server using a distinct port range offset by interface index.

NirWolfer · 2026-05-07T07:03:53Z

            test_ucx_tls: ${{ test.Value.tls }}
            test_extra_run_args: ${{ test.Value.extra_run_args }}
            initial_delay: ${{ coalesce(test.Value.initial_delay, parameters.initial_delay) }}
+            test_interference: ${{ test.Value.interference }}


🚨 BLOCKER

Tests that omit the interference key (e.g. 'tag match on CX4', 'tag match on CX6/RC') will expand ${{ test.Value.interference }} to an empty string. The template parameter default of 'Yes' is never applied because an explicit (empty) value is passed, so $(test_interference) is empty and eq(variables['interference'], 'Yes') evaluates to false — the network corrupter silently never starts for those tests. Fix: test_interference: ${{ coalesce(test.Value.interference, 'Yes') }}, mirroring the initial_delay pattern on the line above.

NirWolfer · 2026-05-07T07:03:54Z

+      net_device=$(ibdev2netdev | sed -ne 's/\(\w*\) port \([0-9]\) ==> '${interface}' .*/\1:\2/p')
+      net_devices="${net_devices} ${net_device}"
+    done
+    export UCX_NET_DEVICES=$(echo ${net_devices##*( )} | tr " ", ",")


🚨 BLOCKER

${net_devices##*( )} uses an extglob pattern to strip leading spaces, but extglob is not enabled by default in bash. Without shopt -s extglob the literal string *( ) is matched instead of 'zero or more spaces', so the stripping silently fails and UCX_NET_DEVICES gets a leading space. Use a POSIX-portable alternative such as echo ${net_devices} | xargs or echo ${net_devices} | sed 's/^ *//'.

NirWolfer · 2026-05-07T07:03:55Z

 - bash: |
    set -eEx
-    $(workspace)/buildlib/az-network-corrupter.sh reset=yes interface=${{ parameters.roce_iface }}
+    echo "##vso[task.setvariable variable=interference]${{ parameters.interference }}"


🚨 BLOCKER

The interference pipeline variable is set via ##vso[task.setvariable] inside the first 'Restore port state' step, which runs under set -eEx and may exit early if a per-interface corrupter-reset call fails before the echo. If that happens, $(test_interference) is never set and all downstream eq(variables['interference'], 'Yes') conditions evaluate to false, silently suppressing the corrupter for all subsequent steps. The variable should be set in a dedicated, unconditional step that precedes any fallible commands.

NirWolfer · 2026-05-07T07:03:56Z

+      net_device=$(ibdev2netdev | sed -ne 's/\(\w*\) port \([0-9]\) ==> '${interface}' .*/\1:\2/p')
+      net_devices="${net_devices} ${net_device}"
+    done
+    export UCX_NET_DEVICES=$(echo ${net_devices##*( )} | tr " ", ",")


⚠️ MINOR

tr " ", "," passes the two-character set " ," (space and comma) as tr's source, mapping both spaces and commas to commas. The intended translation is space→comma only: tr " " ",". With the current input this is benign (no commas in net_devices), but it is a typo.

NirWolfer · 2026-05-07T07:03:56Z

          set -eEx
          ./autogen.sh
-          ./contrib/configure-devel --prefix=$(Build.Repository.LocalPath)/install --without-java
+          ./contrib/configure-devel --prefix=$(Build.Repository.LocalPath)/install


⚠️ MINOR

--without-java was removed from the configure line. If the centos7 build container does not have a JDK installed, the build will fail trying to compile Java bindings. This change should be intentional and verified against the container image, or --without-java should be restored.

NirWolfer · 2026-05-07T07:03:57Z

+        duration: 60
+        interface: $(roce_iface_cx4) $(roce_iface_cx6)
+        tls: "rc_x"
+        test_name: multy_cx6_cx4_rc


⚠️ MINOR

test_name: multy_cx6_cx4_rc contains a typo ('multy' should be 'multi'). Log directories and published artifact names will inherit this misspelling.

NirWolfer · 2026-05-07T07:03:58Z

+        interface: $(roce_iface_cx6)
        tls: "rc_x"
        extra_run_args: "--env server UCX_IB_NUM_PATHS=2 --env client UCX_IB_NUM_PATHS=1"
        test_name: tag_cx4_rc_client_one_path


⚠️ MINOR

test_name: tag_cx4_rc_client_one_path is carried over from the old 'client one path' test but the entry is now named 'different UCX_IB_NUM_PATH'. The log directory and artifact will be misleadingly labelled. Align the test_name with the new entry's purpose.

yosefe reviewed Jul 28, 2022

View reviewed changes

yosefe approved these changes Jul 28, 2022

View reviewed changes

AZP: Add multi HCAs iodemo test

1d56eeb

Alexey-Rivkin force-pushed the integration3 branch from 4115bc9 to 1d56eeb Compare July 31, 2022 10:25

This comment was marked as outdated.

Sign in to view

NirWolfer reviewed May 6, 2026

View reviewed changes

This comment was marked as outdated.

Sign in to view

NirWolfer reviewed May 6, 2026

View reviewed changes

NirWolfer reviewed May 7, 2026

View reviewed changes

Alexey-Rivkin closed this May 11, 2026

Conversation

Alexey-Rivkin commented Jul 28, 2022

What

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NirWolfer commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

NirWolfer commented May 6, 2026

Uh oh!

NirWolfer left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NirWolfer commented May 6, 2026

Uh oh!

This comment was marked as outdated.

Uh oh!

NirWolfer commented May 6, 2026

Uh oh!

NirWolfer left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

NirWolfer commented May 6, 2026

Uh oh!

NirWolfer left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

NirWolfer commented May 7, 2026

Uh oh!

NirWolfer left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NirWolfer commented May 5, 2026 •

edited

Loading