|
Bugzilla – Full Text Bug Listing |
| Summary: | zypper request flood | ||
|---|---|---|---|
| Product: | [openSUSE] openSUSE 10.3 | Reporter: | Peter Poeml <poeml> |
| Component: | libzypp | Assignee: | Jan Kupec <jkupec> |
| Status: | RESOLVED DUPLICATE | QA Contact: | Stanislav Visnovsky <visnov> |
| Severity: | Critical | ||
| Priority: | P5 - None | CC: | coolo, dmacvicar, jsrain, locilka, ma, mmarek, mt |
| Version: | Beta 2 | Flags: | coolo:
SHIP_STOPPER-
|
| Target Milestone: | --- | ||
| Hardware: | Other | ||
| OS: | Other | ||
| Whiteboard: | |||
| Found By: | --- | Services Priority: | |
| Business Priority: | Blocker: | --- | |
| Marketing QA Status: | --- | IT Deployment: | --- |
| Attachments: |
CURL_TIMECOND_IFMODSINCE for doProvideFileCopy()
corrected patch |
||
|
Description
Peter Poeml
2007-09-03 07:57:05 UTC
So its just the .key and .asc files which are downloaded more than once. Ugly, but not a blocker imho. Yeah, I agree, just see this as part of the counter-measures I'm taking against the real blocker, that I see sneaking up... Still, a critical bug from my perspective, not major. (In reply to comment #0 from Peter Poeml) > > To increase the survival chance of our download infrastructure, this > should be fixed before 10.3 release. yes otherwise if this stuf makes too much (redundant request * number of users) the redirector will die in the battle field :( Let's get this fixed for 10.3 GM! I've played a bit with the debugger: ** check if we need to refresh ** 84.44.247.44 - - [03/Sep/2007:09:44:12 +0200] "GET /repositories/home:/poeml/openSUSE_Factory/repodata/repomd.xml HTTP/1.1" 200 1198 "-" "Novell ZYPP Installer" "-" ** start refresh ** ** does the file exist? ** (MediaCurl::getDoesFileExist) 84.44.247.44 - - [03/Sep/2007:09:44:13 +0200] "GET /repositories/home:/poeml/openSUSE_Factory/repodata/repomd.xml HTTP/1.1" 200 1198 "-" "Novell ZYPP Installer" "-" ** does the file exist? ** (MediaCurl::getDoesFileExist) 84.44.247.44 - - [03/Sep/2007:09:44:13 +0200] "GET /repositories/home:/poeml/openSUSE_Factory/repodata/repomd.xml.key HTTP/1.1" 206 2 "-" "Novell ZYPP Installer" "-" ** does the file exist? ** (MediaCurl::getDoesFileExist) 84.44.247.44 - - [03/Sep/2007:09:44:13 +0200] "GET /repositories/home:/poeml/openSUSE_Factory/repodata/repomd.xml.asc HTTP/1.1" 206 2 "-" "Novell ZYPP Installer" "-" ** get the files ** (called from refreshMedata) 84.44.247.44 - - [03/Sep/2007:09:44:13 +0200] "GET /repositories/home:/poeml/openSUSE_Factory/repodata/repomd.xml.key HTTP/1.1" 200 893 "-" "Novell ZYPP Installer" "-" 84.44.247.44 - - [03/Sep/2007:09:44:13 +0200] "GET /repositories/home:/poeml/openSUSE_Factory/repodata/repomd.xml.asc HTTP/1.1" 200 189 "-" "Novell ZYPP Installer" "-" 84.44.247.44 - - [03/Sep/2007:09:44:14 +0200] "GET /repositories/home:/poeml/openSUSE_Factory/repodata/repomd.xml.key HTTP/1.1" 200 893 "-" "Novell ZYPP Installer" "-" 84.44.247.44 - - [03/Sep/2007:09:44:14 +0200] "GET /repositories/home:/poeml/openSUSE_Factory/repodata/repomd.xml.asc HTTP/1.1" 200 189 "-" "Novell ZYPP Installer" "-" 84.44.247.44 - - [03/Sep/2007:09:44:14 +0200] "GET /repositories/home:/poeml/openSUSE_Factory/repodata/repomd.xml HTTP/1.1" 200 1198 "-" "Novell ZYPP Installer" "-" 84.44.247.44 - - [03/Sep/2007:09:44:15 +0200] "GET /repositories/home:/poeml/openSUSE_Factory/repodata/filelists.xml.gz HTTP/1.1" 200 1862 "-" "Novell ZYPP Installer" "-" 84.44.247.44 - - [03/Sep/2007:09:44:15 +0200] "GET /repositories/home:/poeml/openSUSE_Factory/repodata/primary.xml.gz HTTP/1.1" 200 5294 "-" "Novell ZYPP Installer" "-" 84.44.247.44 - - [03/Sep/2007:09:44:15 +0200] "GET /repositories/home:/poeml/openSUSE_Factory/repodata/patterns.test2.xml.gz HTTP/1.1" 200 99 "-" "Novell ZYPP Installer" "-" Still, even the refresh on its own tries to download files multiple times - seems to be related to the signing check. (In reply to comment #6 from Stanislav Visnovsky) > I've played a bit with the debugger: > > ** check if we need to refresh ** > > 84.44.247.44 - - [03/Sep/2007:09:44:12 +0200] "GET > /repositories/home:/poeml/openSUSE_Factory/repodata/repomd.xml HTTP/1.1" 200 > 1198 "-" "Novell ZYPP Installer" "-" > > ** start refresh ** > > ** does the file exist? ** (MediaCurl::getDoesFileExist) > > 84.44.247.44 - - [03/Sep/2007:09:44:13 +0200] "GET > /repositories/home:/poeml/openSUSE_Factory/repodata/repomd.xml HTTP/1.1" 200 > 1198 "-" "Novell ZYPP Installer" "-" > > huh ? I am missing something ? why does it donwloads the file @_O ? a ¿does the file exist? check sounds like HTTP HEAD request NOT HTTP GET !! if HTTP HEAD returns 200 then the file exists, for local/NFS repos. stat()..., via ftp ..MTDM will return the filemtime in case the file exists or error in case it does not.. that's weird. (In reply to comment #7 from Cristian Rodriguez) > > huh ? I am missing something ? I have read the source , it fortunately downloads only a tiny bit of data. ;) The duplicate requests are because Meda backend uses a 1 byte download for checking if a optional file exists. If we want to have this fixed, then doesFileExists has to be reimplemented using a more advanced technique. The problem, ftp does not have "HEAD". Don't look before you leap...!
HEAD request? Why check at all? Is it not useful in this scenario to do
two requests, if you can do one. If the request fails, you'll get a 404.
If it succeeds, you have the file.
And if you _need_ to check for the existance of a _large_ file which you
_don't_ want to download:
HTTP:
a HEAD request is preferable to a 2-byte partial GET.
FTP:
FTP is ineffective as protocol for us anyway because it doesn't have
any cache control, which renders the protocol unusable for
buildservice repositories. Secondly, we don't run an FTP server
ourselves. So I care less. But FTP has commands like
LIST <file> # return information about file, if it exists
MDTM <file> # return last-modified time of a file
SIZE <file> # return size
MDTM must be announced in the FTP server features, but LIST is always
there.
But normally, don't look before you leap.
Two notes:
1) ...checking Last-modified and size is principally a good idea, before
re-downloading a file. However, repomd.xml is a special case because it
is only a kilobyte and already contains hashes for all other potentially
needed files. Thus, it makes the check on the other files and is a real
saver.
2) if you ever need to re-download for a file, do it the right way and
make it a conditional GET, so you need only one request as well -- with
If-Modified-Since header (rfc2616 14.25).
With libcurl, you have the most capable library on-hand :-)
curl knows well how to do that: poeml@batavia510 ~ % curl -Iv ftp://aust.suse.de/pub/packages/README * About to connect() to aust.suse.de port 21 [...] > MDTM README < 213 20040224153912 Last-Modified: Tue, 24 Feb 2004 15:39:12 GMT [...] > SIZE README < 213 622 Content-Length: 622 Sorry Cristian, I overlooked your hint to MDTM, which you previously gave :-) (In reply to comment #11 from Peter Poeml) > curl knows well how to do that: Exactly that was my point, curl knows exactlt what to do , if you issue a HEAD request to an FTP server, it will give you an error if the file does not exists, or will show you the last modified time, it is handled transparently. >2) if you ever need to re-download for a file, do it the right way and m>ake it a conditional GET, so you need only one request as well -- with >If-Modified-Since header gotcha Peter, that 's the exact point. the server will return 304 "Not Modified" in case you already have your file up to date, once again, Im pretty sure curl knows how to do this the right way. Basically the right thing to do is what the CURL "-z" option does. curl -O -v -z README ftp://aust.suse.de/pub/packages/README I guess the library has an option to do exactly that , and will work with FTP (and probably with any of the supported protocols) just try the above example two times. curl_easy_setopt(_curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); curl_easy_setopt(_curl, CURLOPT_TIMEVALUE, timestamp); where timestamp is either the timestamp stored in the last refresh or the timestamp of the last version stored in disk(if any) being the curl options that matches the correct friendly behaviuor suggested by both Peter and me. Looks promising, but i'm not sure we can afford to use this without much testing. I would prefer to have either a 100% safe fix or an online update. I see the load problem, but it doesn't appear _soo_ severe to risk not getting a 10.3 out in time Adding also libcurl's maintainer: mmarek Mmarek: could you, please, tell us more about the possibility mentioned in comment #14? For more information as directly jano. Thanks. (In reply to comment #14 from Cristian Rodriguez) > curl_easy_setopt(_curl, CURLOPT_TIMECONDITION, CURL_TIMECOND_IFMODSINCE); > curl_easy_setopt(_curl, CURLOPT_TIMEVALUE, timestamp); > > where timestamp is either the timestamp stored in the last refresh or the > timestamp of the last version stored in disk(if any) being the curl options > that matches the correct friendly behaviuor suggested by both Peter and me. Right, just one note in case it's not obvious: If you're going to use this feature, you need to defer opening of the local file to the first call of the CURLOPT_WRITEFUNCTION callback, otherwise you'd truncate it. curl_easy_getinfo(_curl, CURLINFO_RESPONSE_CODE, pointer_to_long) will then tell you the status code (200 if the file was downloaded, 304 if not). (In reply to comment #19 from Michal Marek) > Right, just one note in case it's not obvious: If you're going to use this > feature, you need to defer opening of the local file to the first call of the > CURLOPT_WRITEFUNCTION callback, otherwise you'd truncate it. Actually libzypp downloads to a tmp file, which is then moved to the target destination, so this is not an issue. Adapting summary (In reply to comment #20 from Michal Marek) > (In reply to comment #19 from Michal Marek) > > Right, just one note in case it's not obvious: If you're going to use this > > feature, you need to defer opening of the local file to the first call of the > > CURLOPT_WRITEFUNCTION callback, otherwise you'd truncate it. > > Actually libzypp downloads to a tmp file, which is then moved to the target > destination, so this is not an issue. It seems it is at last - the tmp file is 0 lenght if not modified and thus the target is overwritten with an empty file on copy. Just one more thing: is the timestamp of the file the only thing curl checks when CURL_TIMECOND_IFMODSINCE is used? So if i DO modify the file and reset the timestamp to the original, will curl still think it has not been changed? If so, is there a way to check for such situation? Created attachment 175753 [details]
CURL_TIMECOND_IFMODSINCE for doProvideFileCopy()
This is a patch for doProvideFileCopy() which prevents downloading the same file multiple times during a single attach session (which is quite some ATM).
(In reply to comment #22 from Jan Kupec) > (In reply to comment #20 from Michal Marek) > > Actually libzypp downloads to a tmp file, which is then moved to the > > target destination, so this is not an issue. > > It seems it is at last - the tmp file is 0 lenght if not modified and > thus the target is overwritten with an empty file on copy. Yeah, but at that time (after the transfer) you already have the response code, so you of course move only if it's not 304 (as you do in the patch) (In reply to comment #23 from Jan Kupec) > Just one more thing: is the timestamp of the file the only thing curl > checks when CURL_TIMECOND_IFMODSINCE is used? So if i DO modify the > file and reset the timestamp to the original, will curl still think it > has not been changed? If so, is there a way to check for such > situation? You pass the timestamp (not some filename) stored in a long. The curl library doesn't care where you got the timestamp from and it assumes it's valid. (In reply to comment #24 from Jan Kupec) > Created an attachment (id=175753) [details] > CURL_TIMECOND_IFMODSINCE for doProvideFileCopy() > > + if (modified || infoRet != CURLE_OK) > + { > + // apply umask > + if ( ::fchmod( ::fileno(file), filesystem::applyUmaskTo( 0644 ) ) ) > + { > + ERR << "Failed to chmod file " << destNew << endl; > + } > + ::fclose( file ); > + > + // move the temp file into dest > + if ( rename( destNew, dest ) != 0 ) { > + ERR << "Rename failed" << endl; > + ZYPP_THROW(MediaWriteException(dest)); > + } > } > + > DBG << "done: " << PathInfo(dest) << endl; Shouldn't the fclose() call be unconditional? Otherwise you could run out of filedescriptors. Also, in the not-modified case you can remove the zero-length tmp file. (In reply to comment #27 from Michal Marek) > Shouldn't the fclose() call be unconditional? Otherwise you could run out of Sure. Thanx. In the future, i will wrap the fopen and fclose into some object's c-tor and d-tor, so that i don't have to care. > filedescriptors. Also, in the not-modified case you can remove the zero-length > tmp file. Yes, done. Created attachment 175794 [details]
corrected patch
(In reply to comment #28 from Ján Kupec) > Sure. Thanx. In the future, i will wrap the fopen and fclose into some > object's c-tor and d-tor, so that i don't have to care. It's easier as you may think (see zypp/AutoDispose.h) Replace FILE *file = ::fopen( "/dev/null", "w" ); with AutoDispose<FILE*> file( ::fopen( "/dev/null", "w" ) ); if ( file ) file.setDispose( ::fclose ); // Or longer: // AutoDispose<FILE*> file( NULL ); // file = ::fopen( "/dev/null", "w" ); // if ( file ) // file.setDispose( ::fclose ); // Or shorter, but will perform fclose(NULL) if fopen failed // AutoDispose<FILE*> file( ::fopen( "foofile", "w" ), ::fclose ); and you can remove the explicit ::fclose calls. An automatic fclose is performed when files goes out of scope. Michael, thanx for the hint, i'll use it in the 11.0 code. As for the patch, i tested it and it at least gives some 304 response codes when refreshing yum repos: 10.20.1.178 - - [24/Oct/2007:10:42:52 +0200] "GET /rpmmd/repodata/repomd.xml HTTP/1.1" 206 2 "-" "Novell ZYPP Installer" 10.20.1.178 - - [24/Oct/2007:10:43:09 +0200] "GET /rpmmd/repodata/repomd.xml HTTP/1.1" 200 1231 "-" "Novell ZYPP Installer" 10.20.1.178 - - [24/Oct/2007:10:43:09 +0200] "GET /rpmmd/repodata/repomd.xml HTTP/1.1" 200 1231 "-" "Novell ZYPP Installer" 10.20.1.178 - - [24/Oct/2007:10:43:09 +0200] "GET /rpmmd/repodata/repomd.xml.key HTTP/1.1" 206 2 "-" "Novell ZYPP Installer" 10.20.1.178 - - [24/Oct/2007:10:43:09 +0200] "GET /rpmmd/repodata/repomd.xml.asc HTTP/1.1" 206 2 "-" "Novell ZYPP Installer" 10.20.1.178 - - [24/Oct/2007:10:43:09 +0200] "GET /rpmmd/repodata/repomd.xml.key HTTP/1.1" 200 2173 "-" "Novell ZYPP Installer" 10.20.1.178 - - [24/Oct/2007:10:43:09 +0200] "GET /rpmmd/repodata/repomd.xml.asc HTTP/1.1" 200 189 "-" "Novell ZYPP Installer" 10.20.1.178 - - [24/Oct/2007:10:43:09 +0200] "GET /rpmmd/repodata/repomd.xml.key HTTP/1.1" 304 - "-" "Novell ZYPP Installer" 10.20.1.178 - - [24/Oct/2007:10:43:09 +0200] "GET /rpmmd/repodata/repomd.xml.asc HTTP/1.1" 304 - "-" "Novell ZYPP Installer" 10.20.1.178 - - [24/Oct/2007:10:43:09 +0200] "GET /rpmmd/repodata/repomd.xml HTTP/1.1" 200 1231 "-" "Novell ZYPP Installer" 10.20.1.178 - - [24/Oct/2007:10:43:09 +0200] "GET /rpmmd/repodata/patches.xml HTTP/1.1" 200 6506 "-" "Novell ZYPP Installer" 10.20.1.178 - - [24/Oct/2007:10:43:09 +0200] "GET /rpmmd/repodata/patches.xml HTTP/1.1" 304 - "-" "Novell ZYPP Installer" It's not much of an improvement but at least some. Peter, Coolo, please decide if we should make it available through the online update. Further improvements (making use of the raw meta-data cache and maybe rewriting the getDoesFileExist() method) will require quite some work. (In reply to comment #31 from Ján Kupec) > Michael, thanx for the hint, i'll use it in the 11.0 code. > > As for the patch, i tested it and it at least gives some 304 response codes > when refreshing yum repos: > Just in case, remember that FTP will NOT return 304 error code, but success,d oes ftp works ok after this patch ? > It's not much of an improvement but at least some. from the server point of view, it is an excellent step forward. > maybe rewriting > the getDoesFileExist() hrmm..in case that work will be done for 11, AFAICS: 1. file_exists should only be used in cases where the precence of the file is critical to take other decisions different to download or update. 2. It should not download content. 3. for dowload(or update) a file, conditional requests should be done always, this saves significant work in the server side and makes zypp a good netizen. This is of course a sugeesttion purely from the server side point of view, note that Im not expert on the zypp and your mileage may vary. and remember you can always ask Peter, darix or me in case of doubt. we are good enough on this topic ;) (In reply to comment #32 from Cristian Rodriguez) > Just in case, remember that FTP will NOT return 304 error code, but success,d > oes ftp works ok after this patch ? The question is whether the file gets truncated when CURL_TIMECOND_IFMODSINCE is set. I'll check. > and remember you can always ask Peter, darix or me in case of doubt. we are > good enough on this topic ;) thanx :O) I don't have a problem with either a seperate release or the patch being part of an agreggate patch in case you have more to fix. If the fix is good enough, Peter has to say. (In reply to comment #32 from Cristian Rodriguez) > Just in case, remember that FTP will NOT return 304 error code, but success,d > oes ftp works ok after this patch ? Just checked, works OK, but of course with the overhead of downloading the same files multiple times. This doesn't look like a significant improvement to me. I'd say, if it affects only tiny files like repomd.xml*, it doesn't really make a difference if a conditional or a normal request is done, because the size of the file about matches a 304 reply. If, however, the fix applies to _larger_ files as well (I don't know if it does!), then it would be valuable. (In reply to comment #37 from Peter Poeml) > If, however, the fix applies to _larger_ files as well (I don't know if > it does!), then it would be valuable. It doesn't. Major change is needed to make use of the raw metadata cache in order to make a significant difference. *** This bug has been marked as a duplicate of bug 348050 *** |