Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Once #538660: Move update manager upgrade process into new authorize.php file (and make it actually work) is in, we should revisit how we're recovering from errors, especially in terms of cleaning up files and directories that we unpacked into the temp location which we then failed to install for whatever reason.
Comment | File | Size | Author |
---|---|---|---|
#67 | allow-use-of-cached-non-stale-file-605318-66.patch | 1.35 KB | Alan D. |
#62 | 605318-61.update-manager-garbage-collection.patch | 3.12 KB | bojanz |
#61 | 605318-61.update-manager-garbage-collection.patch | 3.12 KB | dww |
#61 | 605318-61.update-manager-garbage-collection.interdiff.txt | 1.02 KB | dww |
#59 | Screengrab1.jpg | 73.15 KB | Tor Arne Thune |
Comments
Comment #1
dwwTagging so this doesn't get lost/forgotten.
Comment #2
dwwMarking #951426: Update-cache is never cleared and holds stale module downloads duplicate and escalating.
Comment #3
hass CreditAttribution: hass commentedLooks like there is more detailed info in the duplicate case than here :-)
Comment #4
mermentau CreditAttribution: mermentau commented@ hass Could be the wrong one was closed and the result is that it's not critical anymore. To me a smooth module update process is a key feature of Drupal 7.
Comment #5
hass CreditAttribution: hass commentedYes, I'm for critical but kept it as dww marked it... :-)
Comment #6
dwwIf you read the handbook page about issue status values you'll see that for something to be critical it has to basically render the entire system unusable. However, "major" is defined as such:
Update manager is optional functionality. Furthermore, the lack of garbage collection is only breaking support for installing -dev releases. Official releases (strongly recommended all over the place) still work. Therefore, the bug here only affects a small percentage of all users.
So, while a big deal, and something we should address ASAP, this bug isn't actually critical and shouldn't block 7.0 if no one gets to it in time...
That said, instead of spending energy on deciding which issue is the right one or how high the priority should be set, it'd be nicer to see that energy spent actually resolving the bug. ;)
Cheers,
-Derek
Comment #7
JoshOrndorff CreditAttribution: JoshOrndorff commentedI agree, let's focus on getting this one worked out. I'm running a drupal7-beta2 install with styles7.x-1.0-alpha4. Update manager suggests that I update to styles7.x-1.0-alpha1. Alpha 4 is the very first and only version of styles that I've installed.
I'm working on porting another module right now, but whenever there is a patch, I'll be ready to test it out.
-Josh
Comment #8
verbosity CreditAttribution: verbosity commentedSubscribing.
I'd mark this as assigned to me, but at the moment I've no idea if i can make any real difference on this issue.
Comment #9
EvanDonovan CreditAttribution: EvanDonovan commentedSubscribing.
If it turns out we can't fix this in two weeks, could we make it so that Update Manager would not allow the installation of dev releases (unless perhaps a hidden setting was added to the $conf array in settings.php)?
That way, we would have the usability benefits of the Update Manager and could possibly fix its behavior in a point release.
EDIT: Removed hass' quote since justinrandell quoted it below.
Comment #10
verbosity CreditAttribution: verbosity commentedChanging to critical on the basis of : #971546 [meta] Battle plan for Update Manager being critical and this ( seemingly ) being the most important of the issues listed in #971546
Comment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedi'm looking at this. i thought i'd bring some context over from #951426: Update-cache is never cleared and holds stale module downloads. in that issue, hass said:
how long should we wait before treating a cached file as out of date? maybe a variable_get() and an admin form?
also, i guess we need a call from update_cron() to something like (but probably better named) update_manager_garbage_collect_downloads() ?
Comment #12
babbage CreditAttribution: babbage commentedI think this is an extremely sensible suggestion... you could make an argument for that as being a "feature", even if it wasn't required for other reasons. You probably wouldn't win that argument, given how often -dev releases are used in our community... but you could at least *make* the argument. :)
Comment #13
geerlingguy CreditAttribution: geerlingguy commentedI think not allowing dev releases via the Update Manager would be a very amenable solution. Those who most often complain about the manager are those who would (a) be more prone to using dev releases, and (b) would probably not use Update Manager regardless.
Comment #14
hass CreditAttribution: hass commentedHere is a patch that downloads locally cached files again after 6 hours, but does not implement the garbage collection. It worked in a very short test on my Windows box.
I guess we all have some more general ideas in mind, but this is a workaround if we are not able to implement the big solution within the required time.
Comment #15
dww#14 is a reasonable start. A few things:
A) I'd rather this new behavior only happened on -dev releases, although we don't really have a reliable way to know other than parsing the filename (which isn't super elegant). However, if people aren't using standard d.o release tarballs, it's an edge case, and they can be careful not to incorrectly name them so they end in ".x-dev.tar.gz". It really is pointless to re-download an official release if we have it locally, even after 6 hours. However, maybe that's over-complicating things at this stage, so I'd be willing to retract this concern. That's why I'm not setting this to needs work.
B) If #14 (or something based on it) is committed, we still can't mark this issue fixed. Even if we're successfully re-downloading -dev releases, we'll just slowly accumulate more junk in the update manager temp folders over time, slowly filling up disks all over the web. So, we really need some garbage collection, not just a work-around to get -dev releases functional. If the only thing this issue was concerned with was making sure installing/updating -dev works, I wouldn't call this critical.
Comment #16
hass CreditAttribution: hass commentedA) Yeah, we can also filter for dev in the filename. Shouldn't be a big trick. The patch is only a quick hack.
B) Yes, as said I also do not see this patch as a solution as the disks are filled with garbage and we need to free up the used disk space. I'm only asking me where we should do this?
system_cron()
? What rules apply? Simply kill all files older than 6 hours (DRUPAL_MAXIMUM_TEMP_FILE_AGE) or any other constant?I know we also have 100% the same garbage collection issue with JS and CSS caches in #721400: Order JS files according to weight, don't change filenames for aggregated JS/CSS, #174. So we need to have a better solution.
Comment #17
hass CreditAttribution: hass commentedAside, I guess we should also check the file size or MD5 hash. Sometimes things are rotten and than a file is only downloaded half or with 0kb. In this case a file exists is TRUE, but the TAR could be corrupt. Not sure how we can grab the MD5 generated with every release from d.o... I only say this as I do expect that such issues may also occur (e.g. if d.o goes down while downloading) at least in theory... You never know, anything can happen :-)
Comment #18
dww@hass: Adding MD5 verification is outside the scope of this issue, but we should discuss that at #974676: Verify MD5 hashes when downloading release tarballs for the Update manager. Instead of verifying the MD5 during garbage collection, we should check it as soon as we download it.
Comment #19
rschwab CreditAttribution: rschwab commentedSo what are we looking for here? Deleting files for failed installs? Clearing out the cache_update table? Lets articulate a plan.
Comment #20
dwwThis issue is *only* about the filesystem. No one needs to touch the {cache_update} table in the database (and if they do, they'll probably break something).
Furthermore, we can't just remove files on failed installs, since that would leave the tarballs around for successful installs, too. Maybe we should just remove any file in this cache directory older than 6 hours inside update_cron(). And, it wouldn't hurt to keep something like the code from #14 as a defensive measure just in case hook_cron() isn't working for some reason.
Comment #21
dwwWe can return to #14 if nothing else materializes, but at least for now, this issue needs more work to flesh out and implement #20.
Comment #22
Welsby CreditAttribution: Welsby commentedIs it not worth doing a CRC check rather than just redownloading based on duration passed?
Comment #23
dww@welsby: The problem is that we never remove any of the files we download. In the normal case, once we download something and install it, there'd never be a reason to try to re-install it. The Update manager doesn't let you remove code from the filesystem, only install new stuff or upgrade it. So, the only time you'd ever hit the "redownload" case is the edge case of a failure during installation. And, it's not worth bloating the filesystem forever just in case there was a chance you might need to re-install something you already downloaded once.
Comment #24
hass CreditAttribution: hass commentedComment #25
hass CreditAttribution: hass commentedFinal patch that removes all stale files from
'temporary://update-cache'
and'temporary://update-extraction'
.Comment #26
hass CreditAttribution: hass commented@dww: Aside of this case, could you take a look to http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_de..., please? It uses
filemtime()
and I guess this is a bug if I understood your filectime() comments correctly...Comment #27
mermentau CreditAttribution: mermentau commentedI tested the patch with an attempt to update the dev version of the Views module. It threw this message:
I checked the /update-cache/ directory and the old tar of Views was deleted. I ran the update again and everything worked with no warning.
In the end /update-cache/ had no tar file for Views. The /update-extraction/ directory in the end was left with the updated files for the Views module. Going to Administration>Reports>Available Updates shows that Views is the latest Dev version now.
Comment #28
hass CreditAttribution: hass commentedThe download/archiving failure sounds not related to this patch. Also see my patch in #977408: Subdirectory in "update-extraction" is not deleted before extraction of other versions, please.
Comment #29
hass CreditAttribution: hass commentedAfter some more thinking about the patch I think we need to exclude DEV versions from the 6 hours check. Otherwise people may download at 2am and will not receive the version build from 1am.
@dww: Any chance that you find some time for a review? :-)
Comment #30
dwwNot sure about the -dev exclusion yet, I'd need to think about that.
No time for a careful review, but one quick concern:
This function has a very confusing name. There's already a {cache_update} DB cache table. I'd prefer if this function's name referenced the fact we're talking about the filesystem or disk, not the database. Something like:
(That's not great, but it's better.)
I also have my recurring fear about functions that call
file_unmanaged_delete_recursive($path)
without any attempts to ensure we're not about to do something incredibly destructive or malicious, although I'm not sure exactly what to suggest.Comment #31
hass CreditAttribution: hass commentedNew patch with function name change to
update_clear_update_disk_cache()
and DEV versions are deleted after 5 minutes what seems to be an appropiate timeframe to me.I do not really understand your comment about
file_unmanaged_delete_recursive($path)
as we know what files and directory structures are inside temporary://update-cache and temporary://update-extraction. For me the function works like a charm.Comment #32
hass CreditAttribution: hass commentedI hate this buggy test exclusion... D7 == 7.x-dev and there is nothing in the description that D7's are ignored like D5 and D6 if the case is set to 7.x-dev.
Comment #33
cosmicdreams CreditAttribution: cosmicdreams commentedIs the only thing holding this patch back a test?
Comment #34
dwwYes. This needs testing on a variety of platforms and configurations, since the actual operation of the update manager can't be tested via simpletest alone.
Comment #35
hass CreditAttribution: hass commentedWe should not hold back this patch any longer - it can only become better than today! I don't see a reason why this should fail on a platform as it's only using existing core functions. If there is a problem other core code must have a bug... :-)
Comment #36
rschwab CreditAttribution: rschwab commented5 minutes passes and my update-cache directory was cleared out of -dev tarballs as expected, but not update-extraction. Maybe this will happen in 6 hours? I'll let you know when its tomorrow (and too late, doh!).
This setup:
MAMP server 1.7.2
php 5.2.6
mysql: 5.0.41
Comment #37
cosmicdreams CreditAttribution: cosmicdreams commentedI can try to give this ago tonight as well.
Comment #38
carlos8f CreditAttribution: carlos8f commentedsubscribing.
Comment #39
hass CreditAttribution: hass commented@rschwab: yes, update-extraction subdir is only killed after 6 hours and on install with patch #977408: Subdirectory in "update-extraction" is not deleted before extraction of other versions.
Comment #40
carlos8f CreditAttribution: carlos8f commentedsubscribing
Comment #41
rschwab CreditAttribution: rschwab commentedin that case +1 for #32. It worked like a charm on my system. I'm not going to RTBC this though - we need more people verifying this works as intended.
Comment #42
threewestwinds CreditAttribution: threewestwinds commentedI was trying to test this yesterday, but ran into http://drupal.org/node/935036 and put it to rest for the night. I'm trying again right now. I'll be back here in bit with a report, and hopefully RTBC.
Comment #43
bendiy CreditAttribution: bendiy commentedI can verify that #32 is working for me.
Steps used to test:
Comment #44
idflood CreditAttribution: idflood commentedpatch #32 works fine for me ( I only checked the 5 minutes update-cache by installing new modules from url ).
Tested on:
mamp 1.7.1 ( Apache/2.0.59 (Unix) PHP/5.2.5 DAV/2 MYSQL/5.0.41-log )
ovh ( Apache/2.2.X (Unix) PHP/5.2.14 MYSQL/5.0.90-log )
Comment #45
cosmicdreams CreditAttribution: cosmicdreams commentedsounds like a RTBC to me.
Comment #46
carlos8f CreditAttribution: carlos8f commentedThis is missing the "R" in RTBC :) Let's make sure the code is sound, too.
Comment #47
dwwOnly problem I see in the patch is this:
The PHPDoc summary for the function needs to be only one line.
I'll quickly re-roll for this, then it's RTBC.
Comment #48
dwwWell, while I was at it, I fixed a few other things:
A) The PHPDoc summary I mentioned in #47.
B) Improved and reorganized the rest of the comments for update_delete_file_if_stale().
C) update_delete_file_if_stale() used to always try to compute the ctime() on a file, even if it didn't exist. Fixed the logic to test for the file existing first.
D) Moved these two new functions outside of the update_status_cache defgroup, since they're not related to the database cache functions that defgroup is documenting.
Someone else should probably RTBC, since this is a bit more extensive of a change than a trivial PHPDoc summary fix.
Comment #49
aspilicious CreditAttribution: aspilicious commentedFunctions should start with 3th person verb...
But I won't start a fight a day before RC ;)
Comment #50
dww@aspilicious: No fight needed. Both of the functions added with this patch begin (after the module namespace, of course) with a 3rd person verb: "clear" and "delete".
Comment #51
webchickThis is the last remaining critical bug in the queue. I'd like to commit it, and then put a "code freeze" on HEAD for a few hours. I'd also like to eat supper. How close do you feel we are? Looks like many people tested it and confirmed it working.
Comment #52
carlos8f CreditAttribution: carlos8f commentedI'd like clarification on this: the regex checking for dev snapshots matches .tar.gz/.zip files, but would it also be good to match *-dev directories as well, so file_unmanaged_delete_recursive() can clean up the unpacked dev snapshot, too?
Comment #53
carlos8f CreditAttribution: carlos8f commentedOr maybe there is no "-dev" on the directory to check for...
from update_manager_install_form_submit()
Does that mean it's impossible to separately stage multiple versions of a module within the garbage collection period? If you've used update manager to install foo-2.0 and then you go to foo-2.x-dev, the folder "foo" is used to stage both versions, without clearing anything in between?
Comment #54
dwwRe:
That particular case doesn't work at all. See #977414: Allow updating to a -dev or new major branch with Update manager
However, I think the patch you care about would be this: #977408: Subdirectory in "update-extraction" is not deleted before extraction of other versions
Comment #55
carlos8f CreditAttribution: carlos8f commenteddww explained the situation in IRC, and now I understand that the shortcomings I mentioned are ongoing issues with this module. Even so, I'm going to investigate this patch tonight and see if there's anything to improve.
Comment #56
webchickWhile this is indeed a nasty little critter, it doesn't meet the actual definition of a critical bug; Drupal as a whole is not unusable without this, though if you hit this behaviour it certainly seems buggy. There's also nothing in here that affects strings, so this can still be committed post-RC1. Downgrading to (solidly) "major."
Looks like we're pretty close here! Needs more testing though, methinks.
Comment #57
carlos8f CreditAttribution: carlos8f commentedThat sums it up nicely ... not incredibly usable either! ;)
I tried to review this but got frustrated by the fact that none of my dev environments even support Update manager (SSH with pubkey access only, no FTP daemon, webserver doesn't own any module files). And seeing that this is no longer blocking an RC, It's a bit lower on my priorities so I'm unassigning for now.
Comment #58
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedI would be happy to test the patch, as I have a local FTP server running. Unfortunately, #988898: Can never use my local FTP server or SSH login. stops me from testing it.
Comment #59
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedTested #48 and it is working for me. I used bendiy's test notes as a base and ran through them with the new patch.
Xubuntu 10.10
Apache 2.2.16
PHP 5.3.3
MySQL 5.1.49
Steps used to test:
The "update-cache" folder was created in /tmp and the file "devel-7.x-1.0-beta2.tar.gz" was contained within (see Screengrab 1.)
The "update-extraction" folder was created in /tmp and "devel" folder was extracted inside it (see Screengrab 1.)
The file "devel-7.x-1.0-rc1.tar.gz" was in the "update-cache" folder. (see Screengrab 2.)
Devel folder in "update-extraction" folder was replaced with the new one (see Screengrab 2.)
Folder "update-cache" contained only "devel-7.x-1.0-rc1.tar.gz" (see Screengrab 3.)
The "update-extraction" folder contained the "devel" folder.
The "update-cache" folder was empty (see Screengrab 4.)
The "update-extraction" folder was empty (see Screengrab 4.)
Comment #60
bojanz CreditAttribution: bojanz commented1V, you are officially awesome.
Read through the patch and noticed this:
That's not really grammatically correct (tar implementations -> it creates, and the "will" is not needed), let's change it to:
Might not be a bad idea to replace "files" with "tarballs" or "archives", makes it more clear.
Thoughts?
Comment #61
dwwNormally I'm glad core patches get such close gramatical scrutiny for code comments. But this is a major/critical bug that might not get into 7.0 at this point, and if I reroll the patch for such tiny details, the maintainers might want it to all be tested again, etc. Plus, no, this is incorrect:
You misunderstand the comment. When you ask tar to untar an archive file, it goes out of its way to set the modification times on the files it creates for you on your filesystem to match the modification times on those files that were true when the archive/tarball was first created. We don't care about modification times on tarball files themselves. We care about the modification times on the files that are "in" the tarball once they show up on your disk.
Anyway, here's a trivial re-roll of #48 (and interdiff) to fix s/it/they/. I didn't try to re-word the rest of it since I still think it's pretty clear.
Comment #62
bojanz CreditAttribution: bojanz commentedOkay, thanks.
I tend to reroll myself when nitpicking about comments, didn't do so here because I wasn't sure about the proposed changes. Sorry!
I now see "they creates", changed to "they create", and setting status to RTBC.
I think RTBC is justified, we've seen many iterations and tests of the patch, including the latest in #59.
Comment #63
geerlingguy CreditAttribution: geerlingguy commented@bojanz - is that another re-roll of dww's patch above, or is it simply a re-upload?
Comment #64
bojanz CreditAttribution: bojanz commentedAs I said, I changed "they creates" to "they create" in one of the comments. So it's *almost* a reupload :)
Comment #65
webchickI'd really like to see tests for this, but as I understand it that's not possible with update manager and all the various testbot environments. Thanks to bendly and 1V for doing an awesome walkthrough of everything they tried. :)
Committed to HEAD. Thanks!
Comment #67
Alan D. CreditAttribution: Alan D. commentedI was just trying to use these functions for the first time and noticed a logical error:
If the file does not exist locally, the system will download it, otherwise it will use the existing file. There is nothing returned by update_delete_file_if_stale(). So there is potential for a stale file to be deleted and update_manager_file_get() returning the local non-existent path.
The patch add a return value for update_delete_file_if_stale().
For D7, to avoid an API change, just move the function call before the if statement.
Note: unlink() should flush the internal PHP cache, so file_exists() shouldn't be using cached values and clearstatcache() shouldn't be needed, unless there is a core PHP bug hanging around.
I'm guessing that either the internal poor-man-cron runs enough to avoid this use case or that other internal error checking is preventing this from creating issues.
Comment #68
Leeteq CreditAttribution: Leeteq commented(Coming from #613230: Allow upgrading -dev with update manager, comment #9.)
For the sake of perspective:
Is D7 still a long way away from being able to update modules form older -dev versions to the newest -dev version of contrib. modules and themes using D7 core's Update Manager?
AFAICT, this is still not possible in the latest D7 release(s), or is there a way to make this possible that I am not aware of? (this question is about the GUI/Update Manager, not about Drush)
[FYI: User name changed from DanielTheViking, Feb. 2012.]
Comment #69
kscheirer#67: allow-use-of-cached-non-stale-file-605318-66.patch queued for re-testing.
Comment #70
kscheirerRetesting against latest HEAD since it has been over a year.
Comment #85
dwwRe: #67: In the intervening years since your patch, this bug was independently rediscovered at #2303323: update_delete_file_if_stale() must return a value or update_manager_file_get() will always fetch remote files. The latest patch over there includes test coverage. This issue has been open way too long as it is, so let's re-close this and focus on #2303323 instead.
Thanks,
-Derek
Comment #86
dww