Merge ~mwhudson/ubuntu/+source/initramfs-tools:refactor-networking into ubuntu/+source/initramfs-tools:ubuntu/devel

[Michael Hudson-Doyle]

This is a larger rewrite of configure_networking, as an alternative to https://code.launchpad.net/~mwhudson/ubuntu/+source/initramfs-tools/+git/initramfs-tools/+merge/353782

[Mathieu Trudel-Lapierre (cyphermox)]

Why are you getting rid of all_netbootable_devices() and the loop for multiple devices? This tended to be important since the BOOTIF might not actually be the one where DHCP will respond (somehow?). That's why we tried to bring up DHCP on any interface possible.

review: Needs Information

[Michael Hudson-Doyle (mwhudson)]

Because if you call DHCP with no interfaces it does ~the same scanning
itself.

On Tue, 28 Aug 2018, 00:37 Mathieu Trudel-Lapierre, <email address hidden>
wrote:

> Review: Needs Information
>
> Why are you getting rid of all_netbootable_devices() and the loop for
> multiple devices? This tended to be important since the BOOTIF might not
> actually be the one where DHCP will respond (somehow?). That's why we tried
> to bring up DHCP on any interface possible.
> --
>
> https://code.launchpad.net/~mwhudson/ubuntu/+source/initramfs-tools/+git/initramfs-tools/+merge/353783
> You are the owner of
> ~mwhudson/ubuntu/+source/initramfs-tools:refactor-networking.
>

[Michael Hudson-Doyle (mwhudson)]

Oh wait, I think I see what you're asking. Will answer more when awake.

On Tue, 28 Aug 2018, 06:43 Michael Hudson-Doyle, <
<email address hidden>> wrote:

> Because if you call DHCP with no interfaces it does ~the same scanning
> itself.
>
> On Tue, 28 Aug 2018, 00:37 Mathieu Trudel-Lapierre, <email address hidden>
> wrote:
>
>> Review: Needs Information
>>
>> Why are you getting rid of all_netbootable_devices() and the loop for
>> multiple devices? This tended to be important since the BOOTIF might not
>> actually be the one where DHCP will respond (somehow?). That's why we tried
>> to bring up DHCP on any interface possible.
>> --
>>
>> https://code.launchpad.net/~mwhudson/ubuntu/+source/initramfs-tools/+git/initramfs-tools/+merge/353783
>> You are the owner of
>> ~mwhudson/ubuntu/+source/initramfs-tools:refactor-networking.
>>
>

[Scott Moser (smoser)]

A few comments inline + these here.

> Tidy up configure_networking a bit:
>
> - Just let dhclient scan the interfaces if none is specified for IPv6.

'a' below also applies here even though the change you've made does not change 'b' as we're already doing it for ipv6.

> - Do not handle ip=rarp specially (ipconfig hasn't supported it for some
> time)

this seems like we should forward it to debian. just to reduce delta, even
though we are high-delta here. I assume the fix applies in debian also.

> - Call dhclient in simple cases for IPv4 (this makes some small observable
> changes -- for example ip=bootp will now make a DHCP request too -- but
> nothing that seems important).

The reasons that we kept using ipconfig previously for ipv4 were
a.) it has known behavior. The loop over existing interfaces means only one is "found", and timeouts of ipconfig are specified on the command line versus hard coded into dhclient. That means that if you have 4 nics and the last one is the one that has a dhcp server on it, then you'll end up waiting 3 (failed) ROUNTTTT and then a hit. With dhclient, the value I think is 60 seconds meaning you'd wait much more than that.

b.) At least as it is right now, the dhclient process is not at all cleaned up. ipconfig exits after it receives a lease, dhclient backgrounds itself. Then we pivot root out of that initramfs and the process stil lives. What happens when it goes to renew a lease? Or when it fails? Those were unknowns.

[Michael Hudson-Doyle (mwhudson)]

> A few comments inline + these here.
>
> > Tidy up configure_networking a bit:
> >
> > - Just let dhclient scan the interfaces if none is specified for IPv6.
>
> 'a' below also applies here even though the change you've made does not change
> 'b' as we're already doing it for ipv6.
>
> > - Do not handle ip=rarp specially (ipconfig hasn't supported it for some
> > time)
>
> this seems like we should forward it to debian. just to reduce delta, even
> though we are high-delta here. I assume the fix applies in debian also.

Yes I guess so.

>
> > - Call dhclient in simple cases for IPv4 (this makes some small
> observable
> > changes -- for example ip=bootp will now make a DHCP request too -- but
> > nothing that seems important).
>
> The reasons that we kept using ipconfig previously for ipv4 were

At the same time, I guess the argument for getting off ipconfig is to reduce the number of dhcp implementations we have to worry about. Although having read the dhclient source a bit now I'm not sure I'm happy about relying on it either :(

> a.) it has known behavior. The loop over existing interfaces means only one
> is "found", and timeouts of ipconfig are specified on the command line versus
> hard coded into dhclient. That means that if you have 4 nics and the last one
> is the one that has a dhcp server on it, then you'll end up waiting 3 (failed)
> ROUNTTTT and then a hit. With dhclient, the value I think is 60 seconds
> meaning you'd wait much more than that.

I'm faiiirly sure that if dhclient is trying multiple interfaces, it tries them all in parallel. So I think this branch improves the situation for IPv6.

I think the default timeout for dhclient is 300 seconds btw (!)

> b.) At least as it is right now, the dhclient process is not at all cleaned
> up. ipconfig exits after it receives a lease, dhclient backgrounds itself.
> Then we pivot root out of that initramfs and the process stil lives. What
> happens when it goes to renew a lease? Or when it fails? Those were
> unknowns.

Hm yes, this is true. I'd thought -1 made it exit when it found an address, but that only means it exits on failure.

1cbcdf7... by Michael Hudson-Doyle on 2018-08-29

review comments

a93733b... by Michael Hudson-Doyle on 2018-08-30

Do not let dhclient processes hang around past the pivot and have them respect the shorter timeouts ipconfig was given.

[Michael Hudson-Doyle (mwhudson)]

> > A few comments inline + these here.
> >
> > > Tidy up configure_networking a bit:
> > >
> > > - Just let dhclient scan the interfaces if none is specified for IPv6.
> >
> > 'a' below also applies here even though the change you've made does not
> change
> > 'b' as we're already doing it for ipv6.
> >
> > > - Do not handle ip=rarp specially (ipconfig hasn't supported it for
> some
> > > time)
> >
> > this seems like we should forward it to debian. just to reduce delta, even
> > though we are high-delta here. I assume the fix applies in debian also.
>
> Yes I guess so.
>
> >
> > > - Call dhclient in simple cases for IPv4 (this makes some small
> > observable
> > > changes -- for example ip=bootp will now make a DHCP request too --
> but
> > > nothing that seems important).
> >
> > The reasons that we kept using ipconfig previously for ipv4 were
>
> At the same time, I guess the argument for getting off ipconfig is to reduce
> the number of dhcp implementations we have to worry about. Although having
> read the dhclient source a bit now I'm not sure I'm happy about relying on it
> either :(
>
> > a.) it has known behavior. The loop over existing interfaces means only one
> > is "found", and timeouts of ipconfig are specified on the command line
> versus
> > hard coded into dhclient. That means that if you have 4 nics and the last
> one
> > is the one that has a dhcp server on it, then you'll end up waiting 3
> (failed)
> > ROUNTTTT and then a hit. With dhclient, the value I think is 60 seconds
> > meaning you'd wait much more than that.
>
> I'm faiiirly sure that if dhclient is trying multiple interfaces, it tries
> them all in parallel. So I think this branch improves the situation for IPv6.
>
> I think the default timeout for dhclient is 300 seconds btw (!)

Ah I see that it's actually 30s as you say for the initramfs.

> > b.) At least as it is right now, the dhclient process is not at all cleaned
> > up. ipconfig exits after it receives a lease, dhclient backgrounds itself.
> > Then we pivot root out of that initramfs and the process stil lives. What
> > happens when it goes to renew a lease? Or when it fails? Those were
> > unknowns.
>
> Hm yes, this is true. I'd thought -1 made it exit when it found an address,
> but that only means it exits on failure.

I hacked up a fix for both the timeout and this part, what do you think?

Not tested yet.

e8d30bc... by Michael Hudson-Doyle on 2018-09-02

fix name of dhclient.conf

0adcd8e... by Michael Hudson-Doyle on 2018-09-02

check for stray dhclient processes in check-results

[Michael Hudson-Doyle (mwhudson)]

I've tested this now, and am happy with it. I'd still appreciate a re-review :)

d06550d... by Michael Hudson-Doyle on 2018-09-04

semicolons--

[Scott Moser (smoser)]

Minor comments inline.

I think this looks good though. Thank you for handling the timeout and the cleaning up of the process.

[Michael Hudson-Doyle (mwhudson)]

Thanks for the comments. I've addressed your comments, will run the tests again locally and upload.

81ceabc... by Michael Hudson-Doyle on 2018-09-06

more review feedback

e7ddf7f... by Michael Hudson-Doyle on 2018-09-06

tyop

fc47f9a... by Michael Hudson-Doyle on 2018-09-06

stuff

44a728c... by Michael Hudson-Doyle on 2018-09-06

and things

[Scott Moser (smoser)]

I've only done code review, not tested any of this, but this "looks good to me".

review: Approve

[Dimitri John Ledkov (xnox)]

Was this merged?

[Michael Hudson-Doyle (mwhudson)]

yes

diff --git a/debian/changelog b/debian/changelog
index 1e97be0..7b57d04 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,19 @@
+initramfs-tools (0.131ubuntu10) UNRELEASED; urgency=medium
+
+  * Tidy up configure_networking a bit:
+    - Just let dhclient scan the interfaces if none is specified for IPv6.
+    - Do not handle ip=rarp specially (ipconfig hasn't supported it for some
+      time)
+    - Call dhclient in simple cases for IPv4 (this makes some small observable
+      changes -- for example ip=bootp will now make a DHCP request too -- but
+      nothing that seems important).
+    - Do not let dhclient processes hang around past the pivot and have them
+      respect the shorter timeouts ipconfig was given.
+  * debian/tests/control: add some dependencies that happened to already be
+    installed on the test runners used so far.
+
+ -- Michael Hudson-Doyle <[e-mail]>  Mon, 27 Aug 2018 13:57:05 +1200
+
 initramfs-tools (0.131ubuntu9) cosmic; urgency=medium
 
   [ John Gallagher ]
diff --git a/debian/tests/check-results b/debian/tests/check-results
index 9016baa..0cb58ec 100755
--- a/debian/tests/check-results
+++ b/debian/tests/check-results
@@ -57,6 +57,12 @@ with open(os.path.join(result_dir, 'link.json')) as fp:
 with open(os.path.join(result_dir, 'addr.json')) as fp:
     addrs = json.load(fp)
 
+with open(os.path.join(result_dir, 'ps.txt'), encoding='utf-8', errors='replace') as fp:
+    ps_output = fp.read()
+
+if 'dhclient' in ps_output:
+    error("dhclient appears to be running")
+
 i = 2
 while i < len(sys.argv):
     a = sys.argv[i]
diff --git a/debian/tests/control b/debian/tests/control
index 9d49717..9f505fa 100644
--- a/debian/tests/control
+++ b/debian/tests/control
@@ -1,3 +1,3 @@
 Tests: net
-Depends: initramfs-tools, linux-image-generic, python3, qemu-system, curl
+Depends: initramfs-tools, linux-image-generic, python3, qemu-system, curl, lsb-release, parted
 Restrictions: needs-root, allow-stderr
diff --git a/debian/tests/prep-image b/debian/tests/prep-image
index a173c9c..377a99f 100755
--- a/debian/tests/prep-image
+++ b/debian/tests/prep-image
@@ -46,6 +46,7 @@ for file in /run/net-*.conf /run/net6-*.conf; do
 done
 ip -json addr > /result/addr.json
 ip -json link > /result/link.json
+ps aux | tee /result/ps.txt
 sync
 exec /lib/systemd/systemd-shutdown poweroff
 EOF
diff --git a/scripts/functions b/scripts/functions
index 9b51c32..1d1845b 100644
--- a/scripts/functions
+++ b/scripts/functions
@@ -216,32 +216,24 @@ get_fstype ()
 	return ${RET}
 }
 
-all_netbootable_devices()
-{
-	for device in /sys/class/net/* ; do
-		if [ ! -e $device/flags ]; then
-			continue
-		fi
+run_dhclient() {
+        local timeout conffile pidfile pid
 
-		loop=$(($(cat $device/flags) & 0x8 && 1 || 0))
-		bc=$(($(cat $device/flags) & 0x2 && 1 || 0))
-		ptp=$(($(cat $device/flags) & 0x10 && 1 || 0))
-
-		# Skip any device that is a loopback
-		if [ $loop = 1 ]; then
-			continue
-		fi
+	timeout=$1
+	shift
 
-		# Skip any device that isn't a broadcast
-		# or point-to-point.
-		if [ $bc = 0 ] && [ $ptp = 0 ]; then
-			continue
-		fi
+        conffile="/etc/dhcp/dhclient.conf.$timeout"
+        pidfile="/tmp/dhclient.pid.$timeout"
+	cp /etc/dhcp/dhclient.conf $conffile
+	echo "timeout $timeout;" >> $conffile
 
-		DEVICE="$DEVICE $(basename $device)"
-	done
+        rm -f $pidfile
+	dhclient -v -1 -cf $conffile -pf $pidfile "$@"
 
-	echo $DEVICE
+        # We do not want the dhclient persisting past the initramfs so
+	# just kill it here (the pid file will only be written if
+	# dhclient daemonized, i.e. found an address).
+	[ -f $pidfile ] && read pid < $pidfile && [ -n "$pid" ] && kill $pid
 }
 
 configure_networking()
@@ -319,12 +311,9 @@ configure_networking()
 			# Do nothing
 			IP=done
 			;;
-		""|on|any)
-			# Bring up device
-			ipconfig -t ${ROUNDTTT} "${DEVICE}"
-			;;
-		dhcp|bootp|rarp|both)
-			ipconfig -t ${ROUNDTTT} -c ${IP} -d "${DEVICE}"
+		""|on|any|dhcp|bootp|both)
+			run_dhclient $ROUNDTTT -4 ${DEVICE:+"${DEVICE}"}
+
 			;;
 		*)
 			ipconfig -t ${ROUNDTTT} -d $IP
@@ -349,28 +338,17 @@ configure_networking()
 			IP6=done
 			;;
 		*)
-			# if this is not the first loop, sleep to provide the backoff.
-			[ "$(($ROUNDTTT-2))" = "0" ] || sleep $ROUNDTTT
-
-			# check the content of IP6, if we have something other
-			# than a device name there and BOOTIF isn't set, clear
-			# DEVICE6 and we'll try all available devices.
-			if echo "${IP6}" | grep -v '^\(on\|dhcp\|any\)$'; then
-				DEVICE6="$IP6"
-			fi
-
-			# if we don't have a device specified, try to bring up
-			# any eligible device.
-			if [ -z "${DEVICE6}" ]; then
-				DEVICE6=$(all_netbootable_devices)
-			fi
-
-			# Bring up device
-			for dev in ${DEVICE6} ; do
-				dhclient -6 -1 -v "${dev}"
-			done
+			# check the content of IP6 and if it is not on/dhcp/any use it as
+			# a device name. Otherwise all devices will be tried (unless
+			# BOOTIF was set, see above).
+			case "${IP6}" in
+			on|dhcp|any)
+				;;
+			*)
+				DEVICE6="$IP6" ;;
+			esac
 
-			DEVICE6=$dev
+			run_dhclient $ROUNDTTT -6 ${DEVICE6:+"${DEVICE6}"}
 			;;
 		esac
 	done