JENKINS: test CM's fallback in client-server testing by alinask · Pull Request #6035 · openucx/ucx

alinask · 2020-12-16T12:45:42Z

No description provided.

alinask · 2020-12-17T08:48:55Z

@yosefe can you please review?

yosefe · 2020-12-17T12:17:29Z

+    #get the interface of the ip address that is used to reach The Google Public DNS IP addresses (IPv4).
+    #making the assumption that the pure Ethernet IPv4 address on the host is the one used for this purpose.
+    eth_iface=$(ip route get 8.8.8.8 | awk -F 'dev' '/dev/{print $2}' | awk '{print $1}')
+    ipoib_roce_ifaces=$(ibdev2netdev)


can we just find the default gateway?

isn't this it?

pls simplify the code

https://www.unixmen.com/how-to-find-default-gateway-in-linux/#:~:text=You%20can%20find%20default%20gateway,netstat%20commands%20in%20Linux%20systems.&text=The%20above%20output%20shows%20my,and%20G%20stands%20for%20Gateway.

ip route show|sed -n 's/default via $\S*$ dev $\S*$/\2/p'

yosefe · 2020-12-17T12:19:06Z

+    #the pure Ethernet interface should not appear in the ibdev2netdev output. it should not be an IPoIB or
+    #RoCE interface.
+    if [ "x${ipoib_roce_ifaces}" != "x" ] &&
+       [ "x${eth_iface}" != "x" ] &&
+       [ $(echo "${ipoib_roce_ifaces}" | grep -c "${eth_iface}") -eq 0 ] ; then
+      ipaddr=$(ip addr show "${eth_iface}" | awk '/inet /{print $2}' | awk -F '/' '{print $1}')
+    else
+      echo "Failed to retrieve an IP of a non IPoIB/RoCE interface" >&2
+    fi


could be simplified: ibdev2ntdev|grep -w ${iface}

yosefe · 2020-12-17T12:19:41Z


-	server_ip=$(get_rdma_device_ip_addr)
-	if [ "$server_ip" == "" ]
+	server_ip_ib=$(get_rdma_device_ip_addr)


server_rdma_addr=
server_nonrdma_addr=

yosefe · 2020-12-17T12:21:01Z

+	server_ip_tcp=$(get_tcp_ip_addr)
+	if [ "$server_ip_ib" == "" ] && [ "$server_ip_tcp" == ""  ]
 	then
 		return
 	fi

-	run_client_server_app "./${test_name}" "" "-a ${server_ip}" 1 0
+	for server_ip in $server_ip_ib $server_ip_tcp
+	do
+		run_client_server_app "./${test_name}" "" "-a ${server_ip}" 1 0
+	done


can we simplify this?

what do you mean? i wanted it to test both IP's so just added a loop

somehow do it at once without for loop and another if

alinask · 2020-12-30T07:10:54Z

@yosefe Can you take a look please?

yosefe · 2020-12-30T08:07:02Z

+    if [ -z "$(ibdev2netdev | grep -w ${eth_iface})" ] ; then
+      ipaddr=$(ip addr show "${eth_iface}" | awk '/inet /{print $2}' | awk -F '/' '{print $1}')
+    else
+      echo "Failed to retrieve an IP of a non IPoIB/RoCE interface" >&2


echo ...
exit 1
or
echo ...
false

@yosefe can you please clarify? i'm not sure what you mean to change here

don't use stderr redirection to make script exit

i see.
i didn't want to kill Jenkins if there is no eth IP to test, but ok, will change this.

ok, then no need to redirect to stderr (and also no need to exit 1), just skip this test

this is what happens now if there is no ip - the error will be printed, nothing would be returned and the fallback won't be tested.

but now that you mentioned it, maybe we do need to force testing an eth ip? if we don't, we might end up not testing the fallback in jenkins for a long while due to a misconfiguration or something on the hosts.

but now that you mentioned it, maybe we do need to force testing an eth ip? if we don't, we might end up not testing the fallback in jenkins for a long while due to a misconfiguration or something on the hosts.

ok, we should expect to always have an Eth device for TCP

yosefe · 2020-12-30T08:11:04Z

 	echo $ipaddr
 }

+get_tcp_ip_addr() {


get_non_rdma_ip_addr

yosefe · 2020-12-30T08:11:40Z


 	export UCX_SOCKADDR_CM_ENABLE=y
-	run_client_server_app "./test/apps/iodemo/${test_name}" "${test_args}" "${server_ip}" 1 0
+	for server_ip in $server_rdma_addr $server_nonrdma_addr


remove UCX_SOCKADDR_CM_ENABLE=y ?

yosefe · 2020-12-30T08:12:13Z

+      echo "Failed to retrieve an IP of a non IPoIB/RoCE interface" >&2
+    fi
+
+    echo $ipaddr


why needed? we will see this value when used for client-server

alinask · 2021-01-03T09:08:15Z

@yosefe updated, please review.

yosefe · 2021-01-03T09:37:36Z

+    else
+      echo "Failed to retrieve an IP of a non IPoIB/RoCE interface"
+      exit 1
+    fi


pls use tabs indentation as in the rest of the file

alinask · 2021-01-03T13:18:03Z

@yosefe please review again

yosefe

unresolved: #6035 (comment)

yosefe · 2021-01-03T14:13:20Z

+        exit 1
+    fi
+
+    echo $(get_ifaddr ${eth_iface})


no need echo
just get_ifaddr "${eth_iface}"

yosefe · 2021-01-03T14:14:40Z


 run_client_server_app() {
-	test_name=$1
+	test_name_to_run=$1


alinask · 2021-01-03T18:08:40Z

@yosefe have another look please.

yosefe · 2021-01-03T18:31:28Z

 }

+get_non_rdma_ip_addr() {
+    if ! which ibdev2netdev >&/dev/null


pls use tabs for indentation

I updated.
sorry, didn't notice before that the whole function didn't have the correct indentation

alinask · 2021-01-04T08:03:02Z

squashed

alinask · 2021-01-04T13:42:53Z

failure is #6088

yosefe reviewed Dec 17, 2020

View reviewed changes

yosefe reviewed Dec 30, 2020

View reviewed changes

Comment thread contrib/test_jenkins.sh Outdated

echo $ipaddr

}

get_tcp_ip_addr() {

yosefe Dec 30, 2020

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

get_non_rdma_ip_addr

yosefe reviewed Dec 30, 2020

View reviewed changes

yosefe reviewed Jan 3, 2021

View reviewed changes

yosefe approved these changes Jan 3, 2021

View reviewed changes

JENKINS: test CM's fallback in client-server testing

b89a4e4

alinask force-pushed the topic/jenkins-io-demo-tcp-ip branch from e0a4948 to b89a4e4 Compare January 4, 2021 07:59

yosefe merged commit 8587242 into openucx:master Jan 4, 2021

Conversation

alinask commented Dec 16, 2020

Uh oh!

alinask commented Dec 17, 2020

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!

yosefe Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

alinask commented Dec 30, 2020

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alinask commented Jan 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alinask commented Jan 3, 2021

Uh oh!

yosefe left a comment

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!

alinask commented Jan 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alinask commented Jan 4, 2021

Uh oh!

alinask commented Jan 4, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

yosefe Dec 30, 2020 •

edited

Loading