Bug #20823
closedMake -c (continue) wget flag optional when downloading kernel/initram
Description
Wget does not detect changed files correctly and -c option corrupts downloaded files. This is expected behavior and documented in wget man page (see -c option). Users who switch OS variant (Server to Workstation) very often or refresh kernel/initram disk very often are running into issues.
This patch is a simple workaround to make the -c flag optional via settings, until we refactor the downloading code completely which is I believe planned.
I would like to propose this patch into 1.15/1.16, it will be just oneliner.
Updated by Lukas Zapletal about 7 years ago
- Related to Feature #19389: Change TFTP filename pattern to include unique installation media ID added
Updated by Anonymous about 7 years ago
I don't think we can safely remove "-c" option. Tftp orchestration (of which downloading kernel and initrd files is part of) is executed during every provisioning. Removing "-c" would result in those files being downloaded every time (not a huge problem on its own), which in turn would lead to provisioning failing due to kernel and initrd files unavailable/partially downloaded.
Without changes in operating systems and media management we have to choose which is a lesser problem: orchestration failures due to kernel files being unavailable/incomplete or due to users changing media without updating OS. I think the former is a bigger issue, while the latter can be worked around.
Updated by Lukas Zapletal about 7 years ago
This ticket is about option to configure that, I am not requesting changing the default behavior, let's default with "-c". This is a workaround enabler, not a solution.
Honestly, I tried to do the patch, but I got stuck in dependency injection and initializers, so I gave up. All I want is a setting that users can flip in case they want to re-download everytime. I was onsite with a customer who had fast enough network (kernel/image download under one second) but was hit by the error when changing media. So this request is not some artificial theory, but real demand.
Updated by Ivan Necas about 7 years ago
+1 for providing the option to optionally not use the `-c` flag.
Updated by Lukas Zapletal about 7 years ago
Alternatively, this can be a setting named "wget_options" with default value of "-c" so this is more flexible.
Updated by Anonymous about 7 years ago
I most strongly disagree. I realise this must feel like I'm nit-picking, I'm not.
This workaround will not work for everyone. In all likelihood it will only work in a small number of instances. Those who do not have a fast enough internet connection will experience intermittent? host creation failures, or won't be able to create hosts at all. Documentation of side-effects and ways to diagnose when (not) to use the option will take more space than the feature itself. My biggest concern is the amount of support this will require.
Ultimately, we cannot defer resolution of technical problems to our users, which is what this option does. My suggestion would be: create a patch for the user who experienced the problem and get them to apply it after every upgrade. At the same time rewrite both downloads and host provisioning to fix issues with current implementation.
Updated by Marek Hulán about 7 years ago
If we default to "-c" and put the Dmitris explanation as a comment above the setting, I don't see it as a big problem. In worst case, add a comment like "this is unsupported, use at your own risk" which I still find better than asking user to apply patch after each update.
Removing that option after rewriting download part should be easy. The fact that we haven't started with the rewrite yet convinces me that this setting is required.
Updated by Lukas Zapletal about 7 years ago
Yes, what Marek says, this is enabler, we need to backport this to two downstream versions and multiple customers hit this, symptoms are very hard to figure out (Anaconda freezing, weird errors). The plan I suggest is:
1) Add the flag as optional setting
2) We can backport where we want
3) Work on proper solution that will essentially drop this temporary code, whatever this is
Jeez guys we are talking about few lines, we can even add comment that the option is only recommended for repositories which are local (e.g. when using Pulp).
Updated by Anonymous about 7 years ago
If we are to implement this fix, it will have to be a temporary one. I'd like to see a plan for a permanent fix/redesign (even if a high-level one) before this is implemented.
Updated by Lukas Zapletal about 7 years ago
The plan is already linked to this issue, see #19389
Updated by Lukas Zapletal about 7 years ago
There is a discussion in the ticket #19389 about how to do this right, I would love to have this temporary fix in. Is there any chance you can guide me Dmitri how to propagate the setting through initializers? It is a bit of code that I don't understand. The place where to put "-c" (or optional_wget_options setting) is obvious to me, I just fail in propagating that through initializers/injectors. I can then file a PR for this one, should be pretty easy (oneliner kinda).
Updated by Anonymous about 7 years ago
I can give you a hand with this; I would like to have a (mostly complete) plan for a permanent solution before this temporary change goes in, however.
Updated by Rahul Bajaj about 7 years ago
- Assignee set to Rahul Bajaj
- Target version set to 220
Updated by Rahul Bajaj about 7 years ago
- Target version changed from 220 to 230
Updated by Lukas Zapletal over 6 years ago
- Status changed from New to Rejected
- Assignee deleted (
Rahul Bajaj) - Target version deleted (
230)
I am closing this ticket because #19389 solves the problem and there is no need of such an option now. Thanks.