On Wednesday 05 March 2008, Philip Hands wrote:
> The reason I need this is in preseeding scripts, where one can then
> write code that says:
Right, that seems like a valid use case.
> If what you're really saying is that you don't understand it, that's OK,
Yes, that is what I'm saying: I'm unable to judge the impact/risks myself.
> I've obviously not been clear enough in the README -- Perhaps you could
> tell which bit's not sufficiently explained and I'll try again.
No, that's not the problem. The problem is my lack of background here.
The code is a bit over my head and just "looks" a bit fragile to me. It's on
a somewhat lower technical level than what we usually do in shell script
and that makes me a bit uncomfortable.
If others can ack your code, then it would be perfectly fine with me.
Another possible solution could be to have an option to quickly fall back to
a simpler version if we see breakage. For example: do this change in two
stages: first stage would be to move existing fetch_url functions to
di-utils, second stage to implement wget404. That way we could revert the
second stage if needed with less risk of conflicts, without needing to
change/upload multiple components and without having to change
dependencies.
The two stages would only have to be separate commits, there would not have
to be time between them.
> > So basically you now first try a restart and if that fails a new full
> > download and that three times, while in the old code we only did the
> > full retry.
>
> Well, sort-of -- in the old code, this was only used for preseed_fetch
> which did as you say. In the new code, preseed_fetch wipes out the
> target file before we get here (I'm not sure if that's really needed,
> but it preserves the old behaviour most closely) and then does the
> retries.
>
> On the other hand, the code that's no longer needed in net-retriever
> used to look like it was trying to do the above but actually implemented:
>
> If the file does not exist, try one wget and return with the result.
>
> If it does exist, try a wget -c.
> If that fails, try a wget and return with it's result.
Hmm. I see what you mean. The old code in net-retriever has a retry loop,
but then always jumps out of the loop on a failure instead of doing any
retries.
> Perhaps that's what was intended, given that the continuation branch
> seems to imply that there are loops at a higher level deciding to retry
> the download via net-retriever (presumably anna is willing to retry
> net-retriever?).
Looking at the code there is an option to retry in anna, but it's only done
if the user tells the installer to do so (by choosing retry or e.g. after
selecting another mirror).
What I'm a bit worried about here is seeing huge pauses from a user PoV on
wget failures because of all the retries acting as a multiplier of wget's
internal timeouts...
I'd really like to hear Joey's take on this. He's probably the best judge to
decide between keeping what was implemented or changing to what was
intended.
> So, perhaps we need a -c option for fetch-url to indicate that it should
> use wget -c, and have that option specified when calling fetch-url from
> net-retriever -- I certainly don't see it being particularly useful in
> preseeding most of the time, although for people who start grabbing
> things like custom kernels using preseed_fetch it might prove helpful to
> have a continuation retry.
Problem with that is that it would just shift the problem to D-I developers
having to choose whether -c is appropriate for some usage of fetch-url.
Same goes for a -r (retry) option that I considered while writing this to
allow to enable or disable the retry loop.
> > I'm not sure if that is wise, given the many caveats listed in the wget
> > manpage for the -c option...
>
> Me neither, but that's how it is in net-retriever, and I'd have thought
> that trying to change that as little as possible makes sense, given that
> it's pretty vital to the install.
Right. I overlooked that.
The retry option from anna does mitigate the caveats a bit: if there was a
problem _because_ of a continuation, it will mostly be detected (ms5sum of
package does not match) and the user will be asked if he wants to retry.
The same is not true for other uses of wget, so using -c might be more of a
problem there, though the risk is probably not huge.
The main problem here of course is that it is almost impossible to trace a
report of an installation problem to the use of wget -c. It would certainly
not be something that would pop into my mind if a user reported that his
installation failed because file X failed to download or was corrupted.
Again, I find it really hard to judge the risks and pros and cons here.
Maybe it's better to keep things simple. If the user really has a flaky
network connection, maybe it's even better to have him notice that early in
the installation rather than during debootstrap or tasksel. I have no idea
what e.g. debootstrap does when it comes to retrying.
[/me checks debootstrap's code]
Hmmm. This is illuminating. debootstrap's functions library has this in the
just_get function (wgetprogress calls wget):
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
fi
Guess it would make a lot of sense to emulate that. Note that it does not do
any full retries, but only a repeated continuation retry.
Care to take a closer look at that yourself too?
What I'm starting to think is that maybe we should have two retry loops: one
default continuation retry loop as in debootstrap, and an optional full
retry loop (if a -r option is passed) that does the full retry, probably
only twice.
I'm thinking that net-retriever probably should /not/ use that -r option
(which would match current actual behavior), but maybe it would be useful
in other cases.
Does that make sense?
[...]
> No -- fetch-url deals with proxies already -- see the lines in
> ...method.../http:
OK. Missed that too. Sorry.
Thanks for the elaborate reply,
FJP
Attachment:
signature.asc
Description: This is a digitally signed message part.