Code review comment for lp:~dpb/orange-box/use-nc-not-ping-1361714

Review: Needs Fixing

On Wed, Aug 27, 2014 at 6:10 PM, David Britton
<email address hidden> wrote:
> David Britton has proposed merging lp:~davidpbritton/orange-box/use-nc-not-ping-1361714 into lp:orange-box.
>
> Requested reviews:
> Orange Box (orange-box)
> Related bugs:
> Bug #1361714 in Orange Box: "orange-box postinst uses ping to test connectivity to maas.ubuntu.com"
> https://bugs.launchpad.net/orange-box/+bug/1361714
>
> For more details, see:
> https://code.launchpad.net/~davidpbritton/orange-box/use-nc-not-ping-1361714/+merge/232490
>
> Introduce new script ob-test-uplink and call this instead of using icmp ping.
>
> Please let me know if you would like any more hosts added.
> --
> https://code.launchpad.net/~davidpbritton/orange-box/use-nc-not-ping-1361714/+merge/232490
> You are subscribed to branch lp:orange-box.
>
> === modified file 'debian/postinst'
> --- debian/postinst 2014-08-25 16:40:02 +0000
> +++ debian/postinst 2014-08-27 23:09:22 +0000
> @@ -37,7 +37,8 @@
> fi
> # Setup nat
> # Ensure we have an external connection
> - run-one-until-success ping -c 3 maas.ubuntu.com
> + orange-box-test-uplink
> + run-one-until-success

This isn't quite right. You would need to put these both on the same
line, like this:

run-one-until-success orange-box-test-uplink

Also, check that you're using tabs here, and not 8 white spaces.

> orange-box-setup-nat
> # Redirect to MAAS web interface
> cat >/var/www/html/index.html <<EOF
> @@ -96,8 +97,7 @@
> # MAAS won't work very well until this is done, so we're going to block
> # until this completes
> # Network access may not be immediately available;
> - # wait until we can talk to maas
> - run-one-until-success ping -c 3 maas.ubuntu.com
> + orange-box-test-uplink

Use a tab. And wrap with run-one-until-success.

> # Support MAAS 1.5, and 1.6
> maas_ver=$(dpkg -l maas | tail -n1 | awk '{print $3}')
> if dpkg --compare-versions $maas_ver lt 1.6; then
>
> === added file 'usr/bin/orange-box-test-uplink'
> --- usr/bin/orange-box-test-uplink 1970-01-01 00:00:00 +0000
> +++ usr/bin/orange-box-test-uplink 2014-08-27 23:09:22 +0000
> @@ -0,0 +1,46 @@
> +#!/bin/sh
> +#
> +# orange-box-amt-recover - recover a dead AMT by dropping all connections

Thanks for copying the template. Make sure you update this line
appropriately ;-)

> +# Copyright (C) 2014 Canonical Ltd.
> +#
> +# Authors: David Britton <email address hidden>
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, version 3 of the License.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +# Wait until tcp connection established to specified host:port
> +# Expects: (host, port)
> +wait_tcp_connect() {
> + timeout 2m run-one-until-success nc -w 10 -zvv $@

Holy smokes. You just blew my mind. The timeout command is awesome!

> +}
> +
> +wait_udp_connect() {
> + # Best effort on UDP, it's connectionless. Short
> + # of starting up a real client, it's prone to false positives
> + timeout 2m run-one-until-success nc -w 5 -uzvv $@
> +}
> +

I think we should probably set -e this, so that we'll exit non-zero if
any of these faile.

And we're using set -x pretty much everywhere in the orange-box, in
the interest of transparency.

> +tcp_servers="maas.ubuntu.com:80 archive.ubuntu.com:80"
> +udp_servers="ntp.ubuntu.com:123"
> +
> +for entry in $tcp_servers; do
> + server=$(echo $entry | awk -F : '{ print $1 }')
> + port=$(echo $entry | awk -F : '{ print $2 }')
> + wait_tcp_connect $server $port
> +done
> +
> +for entry in $udp_servers; do
> + server=$(echo $entry | awk -F : '{ print $1 }')
> + port=$(echo $entry | awk -F : '{ print $2 }')
> + wait_udp_connect $server $port
> +done
> +

Fantastic work, David! This is great stuff ;-)