Bug#618920: debootstrap: needs more robust download error handling

2012-03-13 Thread Colin Watson 
On Sat, Mar 19, 2011 at 11:18:14AM -0400, Michael Gilbert wrote:
 debootstrap's current download error handling isn't very robust.  It
 declares success just for the presence of a downloaded file, which may
 be a partial download, or one for which the checksum doesn't match.
 Eventually those conditions will lead to unhandled failures elsewhere
 within debootstrap.

Thanks for the patch!  I've attached a test proxy which (with various
modifications depending on exactly what you're trying to test) can be
useful to forcibly recreate this kind of bug by making certain downloads
be truncated or fail in whatever other way you like.

I'm particularly disturbed that I've found it possible for debootstrap
to succeed in some cases despite some corrupted downloads.  This should
definitely not be possible, and I agree that we need something along the
lines of your patch.

 It may also be useful to expand a bit on these d-i debootstrap error
 messages: when an error happens, the right answer that the user wants is
 to hit 'go back' twice in a row to start the debootstrap all over again,
 but the dialogs are confusing, and 'continue' seems to be the obvious
 choice, but that will lead to the broken debootstrap continuing to
 completion with various brokenness. Anyway, that maybe should be
 submitted as another bug.

This is http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=283600, I
think.

 So, back to the original issue, I've created a patch that will retry
 downloads whenever anything in the get routine fails, which I
 believe is much more robust than the current situation.  Please see
 attached patch.

 --- newhd/source/debootstrap-1.0.28/functions 2011-02-21 19:25:08.0 
 -0500
 +++ /usr/share/debootstrap/functions  2011-03-19 10:58:57.0 -0400
 @@ -1,3 +1,5 @@
 +MAXATTEMPTS=10
 +
  ### smallutils
  
  smallyes() {
 @@ -241,6 +243,13 @@
  }
  
  get () {
 + for iters in $(seq 1 $MAXATTEMPTS); do
 + if single_get $@; then break; fi
 + warning RETRYING Retrying failed download.
 + done
 +}
 +
 +single_get () {
   # args: from dest 'nocache'
   # args: from dest [checksum size] [alt {checksum size type}]
   local displayname
 @@ -331,13 +340,6 @@
   # http/ftp mirror
   if wgetprogress -O $dest $from; then
   return 0
 - elif [ -s $dest ]; then
 - local iters=0
 - while [ $iters -lt 3 ]; do
 - warning RETRYING Retrying failed download of 
 %s $from
 - if wgetprogress -c -O $dest $from; then 
 break; fi
 - iters=$(($iters + 1))
 - done
[...]

This isn't quite right, though.  You can tell by the change in the
signature of the RETRYING warning; compare that with
base-installer/debian/bootstrap-base.templates and you'll see that
that's going to confuse d-i because there's one parameter too few for
the warning.  In any case, I think the warning should indicate which
download it's retrying.

This can be fixed by moving the retry loop into the original get
function, inside the 'for a in $order' loop.  That way we know $from
when issuing the RETRYING warning.  A further refinement is to remove
$dest2 on failure, so that it doesn't inadvertently confuse something
else later.

I've committed my improved version, which you're welcome to review:

  
http://anonscm.debian.org/gitweb/?p=d-i/debootstrap.git;a=commitdiff;h=733069bb97bdfe3f9c16ca4c9ef58685205eabf3;hp=412608c1fbe28074f56d930242e4269d5588d101

Thanks,

-- 
Colin Watson   [cjwat...@debian.org]
#! /usr/bin/perl
use warnings;
use strict;
use HTTP::Proxy;
use HTTP::Proxy::HeaderFilter::simple;
use HTTP::Proxy::BodyFilter::simple;

sub truncate {
my ($self, $dataref, $message, $protocol, $buffer) = @_;
$$dataref = '';
#$$dataref = substr $$dataref, 0, 1;
}

my $proxy = HTTP::Proxy-new(port = 7890, logmask = HTTP::Proxy::STATUS);
$proxy-push_filter(
path = qr/klibc/,
mime = undef,
response = HTTP::Proxy::BodyFilter::simple-new(\truncate));
$proxy-start;

Bug#269656: Bug#618920: debootstrap: needs more robust download error handling

2012-03-13 Thread Michael Gilbert 
On Tue, Mar 13, 2012 at 1:06 PM, Colin Watson wrote:
 I've committed my improved version, which you're welcome to review:

Cool!  Thanks for looking into this.  This problem has reared its head
on me too many times to count over the past few years ;)

Thanks again!
Mike



-- 
To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Archive: 
http://lists.debian.org/CANTw=MMbHx+zma5Vxz5wZq_Vwif=ml9u4_zm-8v9vogfqnr...@mail.gmail.com

Bug#618920: debootstrap: needs more robust download error handling

2011-03-19 Thread Michael Gilbert 
package: debootstrap
version: 1.0.28
severity: important
tags: patch

debootstrap's current download error handling isn't very robust.  It
declares success just for the presence of a downloaded file, which may
be a partial download, or one for which the checksum doesn't match.
Eventually those conditions will lead to unhandled failures elsewhere
within debootstrap.

This can make d-i very rocky on certain mirrors since one bad package
download ultimately lead to a complete failure of the debootstrap
process, and thus a failed install.  An expert can recover from this,
but an average user will get rather frustrated (especially since the
dialogs for debootstrap errors are rather confusing).  

It may also be useful to expand a bit on these d-i debootstrap error
messages: when an error happens, the right answer that the user wants is
to hit 'go back' twice in a row to start the debootstrap all over again,
but the dialogs are confusing, and 'continue' seems to be the obvious
choice, but that will lead to the broken debootstrap continuing to
completion with various brokenness. Anyway, that maybe should be
submitted as another bug.

So, back to the original issue, I've created a patch that will retry
downloads whenever anything in the get routine fails, which I
believe is much more robust than the current situation.  Please see
attached patch.

Best wishes,
Mike
--- newhd/source/debootstrap-1.0.28/functions	2011-02-21 19:25:08.0 -0500
+++ /usr/share/debootstrap/functions	2011-03-19 10:58:57.0 -0400
@@ -1,3 +1,5 @@
+MAXATTEMPTS=10
+
 ### smallutils
 
 smallyes() {
@@ -241,6 +243,13 @@
 }
 
 get () {
+	for iters in $(seq 1 $MAXATTEMPTS); do
+		if single_get $@; then break; fi
+		warning RETRYING Retrying failed download.
+	done
+}
+
+single_get () {
 	# args: from dest 'nocache'
 	# args: from dest [checksum size] [alt {checksum size type}]
 	local displayname
@@ -331,13 +340,6 @@
 		# http/ftp mirror
 		if wgetprogress -O $dest $from; then
 			return 0
-		elif [ -s $dest ]; then
-			local iters=0
-			while [ $iters -lt 3 ]; do
-warning RETRYING Retrying failed download of %s $from
-if wgetprogress -c -O $dest $from; then break; fi
-iters=$(($iters + 1))
-			done
 		else
 			rm -f $dest
 			return 1
@@ -346,13 +348,6 @@
 		# http/ftp mirror
 		if wgetprogress $CHECKCERTIF $CERTIFICATE $PRIVATEKEY -O $dest $from; then
 			return 0
-		elif [ -s $dest ]; then
-			local iters=0
-			while [ $iters -lt 3 ]; do
-warning RETRYING Retrying failed download of %s $from
-if wgetprogress $CHECKCERTIF $CERTIFICATE $PRIVATEKEY -c -O $dest $from; then break; fi
-iters=$(($iters + 1))
-			done
 		else
 			rm -f $dest
 			return 1

Bug#618920: debootstrap: needs more robust download error handling

2011-03-19 Thread Michael Gilbert 
I probably should have mentioned that these errors are much more
likely within d-i for some reason.  The best way to reproduce this is
to do an installation up through partitioning, then do:

$ debootstrap wheezy /target/wheezy-chroot
http://snapshot.debian.org/archive/debian/20110318T093210Z

At least a few package downloads should fail from that mirror, and
they'll be different every time.

Executing the same command in a full system seems to work fine for
that exact same mirror, so maybe its some other variability within d-i
that is the cause of this issue.

Best wishes,
Mike



-- 
To UNSUBSCRIBE, email to debian-boot-requ...@lists.debian.org
with a subject of unsubscribe. Trouble? Contact listmas...@lists.debian.org
Archive: 
http://lists.debian.org/AANLkTinic9TK-G7pu+=a5aafaboifjhzhwhirebns...@mail.gmail.com