Re: Store_url_rewrite for squid 3+
On 9/6/2012 4:37 AM, Amos Jeffries wrote: On 06.09.2012 13:23, Eliezer Croitoru wrote: OK, it seems we are getting to somewhere. i know how to patch using command but what are the proper one to get a patch file to be run later? will look into it. If you are using diff: diff -u orig_code/ new_code/ >output_file.patch If you are using bzr: bzr diff >output_file.patch Amos Thanks, and I hoped to sleep but i'm not tired :( anyway I will post couple patches later with the basic thing. i'm now mangling my working version into the ridrect.cc file and moving on. -- Eliezer Croitoru https://www1.ngtech.co.il IT consulting for Nonprofit organizations eliezer ngtech.co.il
Re: Store_url_rewrite for squid 3+
On 06.09.2012 13:23, Eliezer Croitoru wrote: OK, it seems we are getting to somewhere. i know how to patch using command but what are the proper one to get a patch file to be run later? will look into it. If you are using diff: diff -u orig_code/ new_code/ >output_file.patch If you are using bzr: bzr diff >output_file.patch Amos
Re: Store_url_rewrite for squid 3+
OK, it seems we are getting to somewhere. i know how to patch using command but what are the proper one to get a patch file to be run later? will look into it. Eliezer On 9/6/2012 3:58 AM, Amos Jeffries wrote: On 06.09.2012 11:58, Eliezer Croitoru wrote: On 9/5/2012 9:56 AM, Eliezer Croitoru wrote: any leads,? Well there is a nice progress. I reviewed the 2.7 store_url_rewrite and I divided the task into more detailed smaller tasks. FTR: squid-2.7 ports are exempted as suitable in most cases for back-porting to stable despite our "no new features" policy. I am happy for this to be done as a series of patches instead of a singular change. It can be assembled in trunk and back-ported as a singular later. 1. Research the url_rewrite interface code and Introduce a modified version of url_rewrite as url_store_rewrite_program. - this task is kind of done(passed compiling and running on 3.2.1) by now and I want to get some ideas on naming conventions for the code to fit the project amazing code looks. list of changed files and code: create a mimic file of redirect.cc in ./store_rewrite.cc and change No need. What I meant earlier about src/redirect.cc being usable is that most of the code is an exact duplicate. You should only need to: * write a new start() function ie storeurlRewriteStart() * add new storeurl_rewriters global * adapt redirectRegisterWithCacheManager() to register a new "storeurl_rewrite" report (when that part is written) * adapt redirectInit() to setup both url_rewrite and storeurl-rewrite helpers. * adapt redirectShutdown() to cleanup both url_rewrite and storeurl-rewrite helpers. fair The new storeurlRewriteStart() to be used by store code for this re-writing and sets up the redirect interface using the new store helpers and whatever callback the changed URL is to be sent to. The entire rest of the code for helper management is identical. all the methods and variables to fit store_rewrite. strip all the url_rewrite data manipulating actions. change the debugging info. (after the store related planning tasks get back here to redo) ./structs.h adding the proper variables for: helper naming, bypass(on\off), acl_access namespace, child configs the ??_rewrites_host of url_rewrite dosnt belong for store_rewrite process at all. ./cache_cf.cc state the default configuration for the helper What do you mean by this? doConfigure() post-configuration checking? I don't think there is anything which needs new cache_cf.cc code. The parsing side if things is identical for url_rewrite_*. The different defaults and locations are all coded in cf.data.pre ... ok so later we will see how ot optimize the conf file. but there are coupe arguments there that are crucial for compilation and parsing the config file ./cf.data.pre stating all config directives for the the helper (copy and modify from url_rewrite_program) Okay. However, if merging the stages to trunk separately this will need to be the final step, since it makes the directives publicly visible. We want to make these changes and doc/release-notes/release-3.*.sgml at the same time when it is suitable for public use. Also, remove the storeurl_* entries at the top marking them as "obsolete"/unavilable type. ./ClientRequestContext.h adding int for state adding bool for done ./client_side_request.h stating the start method as squidexternal something Just "extern" will do. We are killing the SQUIDCEXTERN mess. ./client_side_request.cc adding calls and callouts ./protos.h stating the start init and shutdown methods. We are in the process of killing protos.h. Please create a store_urlrewrite.h header for these definitions instead. will be done However, see the comments above about redirect.cc. There should not need to be any new files created for this. Which removes the protos.h, main.cc and Makefile.am changes. ./main.cc: calling init and shutdown methods at start/reconfigure etc.. ./Makefile.am && ./Makefile.in adding the source ?.cc file into the commands Other than the notes above. Okay. If you have a patch for that please submit for audit :-) We can pause there for the infrastructure to look fine before moving on to the store details. I've been waiting on assistance from Henrik or Alex on that for a while. They are the ones who know the answers to your questions below AFAIK. 2. Research the workflow of storing objects in memory and store and introduce psudo for a new workflow of storing objects to avoid bad effects on cache objects usage in any form that can be. - I do know that squid uses some hash look-up and I have seen in the things about it. - as far I understood from the code: client_request builds the request of the http object. creates a mem-object and on the way creates a checksum. a transfer from of the mem-object to a "store" happens. if a store rebuild happens it takes all of the data from the file in the store. ? question how cachemgr gets the list of urls in memory
Re: Store_url_rewrite for squid 3+
On 06.09.2012 11:58, Eliezer Croitoru wrote: On 9/5/2012 9:56 AM, Eliezer Croitoru wrote: any leads,? Well there is a nice progress. I reviewed the 2.7 store_url_rewrite and I divided the task into more detailed smaller tasks. FTR: squid-2.7 ports are exempted as suitable in most cases for back-porting to stable despite our "no new features" policy. I am happy for this to be done as a series of patches instead of a singular change. It can be assembled in trunk and back-ported as a singular later. 1. Research the url_rewrite interface code and Introduce a modified version of url_rewrite as url_store_rewrite_program. - this task is kind of done(passed compiling and running on 3.2.1) by now and I want to get some ideas on naming conventions for the code to fit the project amazing code looks. list of changed files and code: create a mimic file of redirect.cc in ./store_rewrite.cc and change No need. What I meant earlier about src/redirect.cc being usable is that most of the code is an exact duplicate. You should only need to: * write a new start() function ie storeurlRewriteStart() * add new storeurl_rewriters global * adapt redirectRegisterWithCacheManager() to register a new "storeurl_rewrite" report (when that part is written) * adapt redirectInit() to setup both url_rewrite and storeurl-rewrite helpers. * adapt redirectShutdown() to cleanup both url_rewrite and storeurl-rewrite helpers. The new storeurlRewriteStart() to be used by store code for this re-writing and sets up the redirect interface using the new store helpers and whatever callback the changed URL is to be sent to. The entire rest of the code for helper management is identical. all the methods and variables to fit store_rewrite. strip all the url_rewrite data manipulating actions. change the debugging info. (after the store related planning tasks get back here to redo) ./structs.h adding the proper variables for: helper naming, bypass(on\off), acl_access namespace, child configs the ??_rewrites_host of url_rewrite dosnt belong for store_rewrite process at all. ./cache_cf.cc state the default configuration for the helper What do you mean by this? doConfigure() post-configuration checking? I don't think there is anything which needs new cache_cf.cc code. The parsing side if things is identical for url_rewrite_*. The different defaults and locations are all coded in cf.data.pre ... ./cf.data.pre stating all config directives for the the helper (copy and modify from url_rewrite_program) Okay. However, if merging the stages to trunk separately this will need to be the final step, since it makes the directives publicly visible. We want to make these changes and doc/release-notes/release-3.*.sgml at the same time when it is suitable for public use. Also, remove the storeurl_* entries at the top marking them as "obsolete"/unavilable type. ./ClientRequestContext.h adding int for state adding bool for done ./client_side_request.h stating the start method as squidexternal something Just "extern" will do. We are killing the SQUIDCEXTERN mess. ./client_side_request.cc adding calls and callouts ./protos.h stating the start init and shutdown methods. We are in the process of killing protos.h. Please create a store_urlrewrite.h header for these definitions instead. However, see the comments above about redirect.cc. There should not need to be any new files created for this. Which removes the protos.h, main.cc and Makefile.am changes. ./main.cc: calling init and shutdown methods at start/reconfigure etc.. ./Makefile.am && ./Makefile.in adding the source ?.cc file into the commands Other than the notes above. Okay. If you have a patch for that please submit for audit :-) We can pause there for the infrastructure to look fine before moving on to the store details. I've been waiting on assistance from Henrik or Alex on that for a while. They are the ones who know the answers to your questions below AFAIK. 2. Research the workflow of storing objects in memory and store and introduce psudo for a new workflow of storing objects to avoid bad effects on cache objects usage in any form that can be. - I do know that squid uses some hash look-up and I have seen in the things about it. - as far I understood from the code: client_request builds the request of the http object. creates a mem-object and on the way creates a checksum. a transfer from of the mem-object to a "store" happens. if a store rebuild happens it takes all of the data from the file in the store. ? question how cachemgr gets the list of urls in memory? so probable points of failure: using the wrong url to fetch the object. wrong arguments for checksum. storing with wrong arguments\url leading to faulty rebuild. I do remember that when I looked at a stored old store_url_rewrite cahce file long time ago there were two urls in the file what leads me (it's a bit fogy) to think that the stored file was the mem
Re: Store_url_rewrite for squid 3+
On 9/5/2012 9:56 AM, Eliezer Croitoru wrote: any leads,? Well there is a nice progress. I reviewed the 2.7 store_url_rewrite and I divided the task into more detailed smaller tasks. 1. Research the url_rewrite interface code and Introduce a modified version of url_rewrite as url_store_rewrite_program. - this task is kind of done(passed compiling and running on 3.2.1) by now and I want to get some ideas on naming conventions for the code to fit the project amazing code looks. list of changed files and code: create a mimic file of redirect.cc in ./store_rewrite.cc and change all the methods and variables to fit store_rewrite. strip all the url_rewrite data manipulating actions. change the debugging info. (after the store related planning tasks get back here to redo) ./structs.h adding the proper variables for: helper naming, bypass(on\off), acl_access namespace, child configs the ??_rewrites_host of url_rewrite dosnt belong for store_rewrite process at all. ./cache_cf.cc state the default configuration for the helper ./cf.data.pre stating all config directives for the the helper (copy and modify from url_rewrite_program) ./ClientRequestContext.h adding int for state adding bool for done ./client_side_request.h stating the start method as squidexternal something ./client_side_request.cc adding calls and callouts ./protos.h stating the start init and shutdown methods. ./main.cc: calling init and shutdown methods at start/reconfigure etc.. ./Makefile.am && ./Makefile.in adding the source ?.cc file into the commands 2. Research the workflow of storing objects in memory and store and introduce psudo for a new workflow of storing objects to avoid bad effects on cache objects usage in any form that can be. - I do know that squid uses some hash look-up and I have seen in the things about it. - as far I understood from the code: client_request builds the request of the http object. creates a mem-object and on the way creates a checksum. a transfer from of the mem-object to a "store" happens. if a store rebuild happens it takes all of the data from the file in the store. ? question how cachemgr gets the list of urls in memory? so probable points of failure: using the wrong url to fetch the object. wrong arguments for checksum. storing with wrong arguments\url leading to faulty rebuild. I do remember that when I looked at a stored old store_url_rewrite cahce file long time ago there were two urls in the file what leads me (it's a bit fogy) to think that the stored file was the memobject cache rather then a set of arguments such as refresh time related info,method,url,request,response. I will look at it later but if someone have solid knowledge on how the store routing was or implemented before i'm rushing into the code every piece of info will help me when looking into it. Eliezer -- Eliezer Croitoru https://www1.ngtech.co.il IT consulting for Nonprofit organizations eliezer ngtech.co.il
Re: ICAP connections under heavy loads
On 06.09.2012 02:30, Alex Rousskov wrote: On 09/05/2012 03:32 AM, Alexander Komyagin wrote: On Tue, 2012-09-04 at 09:16 -0600, Alex Rousskov wrote: Again, I hope that this trick is not needed to solve your problem, and I am worried that it will cause more/different problems elsewhere. I would recommend fixing CommOpener instead. If _that_ is not sufficient, we can discuss appropriate low-level enhancements. Both things shall be fixed, IMHO. If double-connect is not needed, we should not introduce it. AFAICT, double-connect is an attempt to cope with a bug in higher-level code. We should fix that bug and see if that is sufficient. If it is not sufficient, we should evaluate why and only then add appropriate low-level code if needed. It's there to fix an async race condition of Squid vs TCP: 1) the timeout occurs ... timeout AsyncCall is scheduled. 2) the socket TCP completes ... completed AsyncCall is scheduled. 3) timeout call is handled: a) if FD is ready use it, terminate the job early as a success (side effect: cancel the pending completed Call) b) else, terminate the job early as a fail. 4) connection completed call is dropped is handled. The 3a conditional test is what comm_connect_addr() is doing on timeout. The switch case in ConnOpener::connect() is handling the multi-state output of the 3a check. If you can find a cleaner way to rewrite the ConnOpener::connect() and comm_connect_addr() call chain for the timeout case it should be duplicated and polished into the timeout() handler. Amos
Re: PEER_MULTICAST_SIBLINGS
On 06.09.2012 00:10, Kinkie wrote: Hi guys, there's a question on PEER_MULTICAST_SIBLINGS: it is unconditionally defined in structs.h, and then there's a few #if(def)s elsewhere. Is there any reason why we shouldn't simply remove the #define and #ifdef clauses? It would seem there is no problem removing it. Amos
Re: BZR repository upgrade re-proposal.
On 05.09.2012 20:23, Kinkie wrote: On Wed, Sep 5, 2012 at 9:07 AM, Henrik Nordström wrote: ons 2012-09-05 klockan 09:03 +0200 skrev Kinkie: I'm glad that a consensus is forming. The next question would then be who will perform the upgrade, and when - assuming noone speaks up against it. This friday? +1 :) +1. Amos
Re: ICAP connections under heavy loads
On 09/05/2012 09:27 AM, Alexander Komyagin wrote: > So you think that it's ok for comm_coonect_addr() to return COMM_OK if > it was called before the appropriate select() notification. Am I right? Hard to say for sure since comm_coonect_addr() lacks an API description, and there are at least three similar but different ways the function is being used by Squid. One natural way to define this API is to say that it should return COMM_OK if and only if the socket is connected (making any select notifications irrelevant). However, this definition may be too CPU-expensive and/or too unportable to support. And this level of certainty may not actually be needed for current Squid needs! I would not be surprised if there is some gray area where we cannot really tell for sure (without too much additional overhead or portability risk) whether the async socket is connected. The Stevens book seem to imply that much. Inside that gray area, the function should probably return COMM_OK so that the rest of the code works: If we guessed wrong, we will get failures during I/O, but the code has to deal with those anyway. In other words, if you want to work on this, consider defining the API based on current Squid needs and then make sure we support those minimum requirements, keeping overheads and portability in mind. However, I would _start_ by fixing the calling code first (as it may affect the minimum requirements) -- your ConnOpener patch was a step in that direction. HTH, Alex. > On Wed, 2012-09-05 at 08:30 -0600, Alex Rousskov wrote: >> On 09/05/2012 03:32 AM, Alexander Komyagin wrote: >>> On Tue, 2012-09-04 at 09:16 -0600, Alex Rousskov wrote: >> Again, I hope that this trick is not needed to solve your problem, and I am worried that it will cause more/different problems elsewhere. I would recommend fixing CommOpener instead. If _that_ is not sufficient, we can discuss appropriate low-level enhancements. >>> >>> Both things shall be fixed, IMHO. >> >> If double-connect is not needed, we should not introduce it. AFAICT, >> double-connect is an attempt to cope with a bug in higher-level code. We >> should fix that bug and see if that is sufficient. If it is not >> sufficient, we should evaluate why and only then add appropriate >> low-level code if needed. >> >> The primary goal here is to fix the underlying issue, not just to find a >> workaround (which you have already provided). >> >> >> Thank you, >> >> Alex. >> >
Re: ICAP connections under heavy loads
So you think that it's ok for comm_coonect_addr() to return COMM_OK if it was called before the appropriate select() notification. Am I right? On Wed, 2012-09-05 at 08:30 -0600, Alex Rousskov wrote: > On 09/05/2012 03:32 AM, Alexander Komyagin wrote: > > On Tue, 2012-09-04 at 09:16 -0600, Alex Rousskov wrote: > > >> Again, I hope that this trick is not needed to solve your problem, and I > >> am worried that it will cause more/different problems elsewhere. I would > >> recommend fixing CommOpener instead. If _that_ is not sufficient, we can > >> discuss appropriate low-level enhancements. > > > > Both things shall be fixed, IMHO. > > If double-connect is not needed, we should not introduce it. AFAICT, > double-connect is an attempt to cope with a bug in higher-level code. We > should fix that bug and see if that is sufficient. If it is not > sufficient, we should evaluate why and only then add appropriate > low-level code if needed. > > The primary goal here is to fix the underlying issue, not just to find a > workaround (which you have already provided). > > > Thank you, > > Alex. > -- Best wishes, Alexander Komyagin
Re: [PATCH] cert based client authentication
If there is not any objection I will commit this patch to trunk. Regards, Christos On 08/31/2012 12:04 PM, Tsantilas Christos wrote: > ping :-) > > On 08/09/2012 06:55 PM, Tsantilas Christos wrote: >> TLS/SSL Options does not apply to the dynamically generated certificates >> >> The TLS/SSL options configured with http_port configuration parameter >> does not used to generate SSL_CTX context objects used to establish SSL >> connections. This is means that certificate based authentication, or SSL >> version selection and other SSL/TLS http_port options does not work for >> ssl-bumped connection. >> >> This patch fixes this problem. >> >> This is a Measurement Factory project >> > >
Re: ICAP connections under heavy loads
On 09/05/2012 03:32 AM, Alexander Komyagin wrote: > On Tue, 2012-09-04 at 09:16 -0600, Alex Rousskov wrote: >> Again, I hope that this trick is not needed to solve your problem, and I >> am worried that it will cause more/different problems elsewhere. I would >> recommend fixing CommOpener instead. If _that_ is not sufficient, we can >> discuss appropriate low-level enhancements. > > Both things shall be fixed, IMHO. If double-connect is not needed, we should not introduce it. AFAICT, double-connect is an attempt to cope with a bug in higher-level code. We should fix that bug and see if that is sufficient. If it is not sufficient, we should evaluate why and only then add appropriate low-level code if needed. The primary goal here is to fix the underlying issue, not just to find a workaround (which you have already provided). Thank you, Alex.
PEER_MULTICAST_SIBLINGS
Hi guys, there's a question on PEER_MULTICAST_SIBLINGS: it is unconditionally defined in structs.h, and then there's a few #if(def)s elsewhere. Is there any reason why we shouldn't simply remove the #define and #ifdef clauses? -- /kinkie
Re: ICAP connections under heavy loads
On Tue, 2012-09-04 at 09:16 -0600, Alex Rousskov wrote: > On 09/04/2012 03:10 AM, Alexander Komyagin wrote: > > On Fri, 2012-08-31 at 11:05 -0600, Alex Rousskov wrote: > >> On 08/31/2012 09:01 AM, Alexander Komyagin wrote: > >>> Alex, I figured it out, finally! The bug was in comm_connect_addr() > >>> function (I suppose it is kernel-dependent though). > >>> > >>> Consider following call trace: > >>> 1) Xaction starts ConnOpener in order to create new connection to ICAP > >>> 2) ConnOpener calls comm_connect_addr() > >>> 3) comm_connect_addr() initiates connection through connect() and > >>> returns COMM_INPROGRESS > >>> ...since our ICAP service is too busy connection will eventually timeout > >>> (connectTimeout_), so... > >>> 4) Comm::ConnOpener::timeout() is called > >>> 5) Comm::ConnOpener::timeout calls connect() > >>> 6) connect() calls comm_connect_addr() > >>> 7) comm_connect_addr() calls getsockopt(SOL_SOCKET, SO_ERROR) to check > >>> current connection status > >>> 8) getsockopt(..) returns errno 0 <--- BUG > >> > >> Actually, the bug is in step #5. If we timed out, we should not try to > >> connect again. > > > > In steps (5) and (6) it's not the OS connect() call, but > > Comm::ConnOpener::connect(), > > Yes, I know. > > > so technically it's OK to call it on > > timeout as long as we are sure that it is OK. > > Not sure what "technically" quantifier means in this context, but IMO, > it is not correct and is a significant source of bugs and mess in Squid: > The original author knows what the code is supposed to be doing but uses > an unfortunate method name. As the time passes, right code is added to > the wrong method and eventually nobody can tell what the method is > supposed to be doing. > > The connect() method should try to connect. We should not try to connect > after a timeout. Thus, we should not call the connect() method after a > timeout. > > > > I completely agree with you Alex, that logically ConnOpener::timeout() > > shall better not call ConnOpener::connect(), though it's just a bad > > design and not the actual source of the problem. Problem is that current > > comm subsystem design allows upper-level code to successfully perform > > comm_connect_addr() calls without waiting for Select() notification. > > > I do not want to argue whether "bad design" is "the source of the > problem" in this case. If fixing that "bad design" would fix the > problem, then we should do that rather than trying to recover from an > incorrect Comm calls order sequence. The latter has been proven very > difficult in the past, and I doubt this case is an exception. > > > >> Step #8 seems correct -- from the OS point of view, internal Squid > >> timeout is not an error. If you fix step #5, step #8 will not happen > >> though. > >> > >> > >>> 9) comm_connect_addr() returns COMM_OK so ConnOpener would think that > >>> connection was successfully established and pass the connection back to > >>> Xaction object, then we would have noteCommRead and noteCommWrote > >>> exceptions and so on... > >>> > >>> According to `man connect`, when connect() returns errno EINPROGRESS: > >>> EINPROGRESS > >>> The socket is nonblocking and the connection cannot be completed > >>> immediately. It is possible to select(2) or poll(2) for > >>> completion by selecting the socket for writing. After select(2) > >>> indicates writability, use getsockopt(2) to read the SO_ERROR > >>> option at level SOL_SOCKET to determine whether connect() > >>> completed successfully (SO_ERROR is zero) or unsuccessfully > >>> (SO_ERROR is one of the usual error codes listed here, > >>> explaining the reason for the failure). > >>> > >>> Actually ConnOpener uses SetSelect(..) in order to wait before calling > >>> comm_connect_addr() (and connect() ) again, but timeout in fact breaks > >>> the rule calling getsockopt() right after connect(). > >> > >> The above man page excerpt says to call getsockopt() after successful > >> select() notification. I think that is what we do now. > > > > Nope. On timeout it turns out that we're doing getsockopt() without > > waiting for select() notification. That's the thing that shall be fixed > > (maybe tricky) or correctly handled (as I did). > > I misinterpreted your "the rule calling getsockopt() right after > connect()" as your definition of the rule and just re-stated the correct > rule. Sorry. > > I do not take from-timeout-handler calls into account here because, as > discussed above, the properly fixed code will not have them. I agree > that the current code may violate the calling sequence during timeouts. > > > > >> * For the conn-open-timeout patch: > >> > >> Overall, I think this patch is fighting the side-effects of other bugs. > >> Most importantly, the timeout handler should abort the ConnOpener job on > >> the spot rather than go through one more select() try. I will comment on > >> specific changes below, just in
Re: BZR repository upgrade re-proposal.
On Wed, Sep 5, 2012 at 9:07 AM, Henrik Nordström wrote: > ons 2012-09-05 klockan 09:03 +0200 skrev Kinkie: >> I'm glad that a consensus is forming. >> The next question would then be who will perform the upgrade, and when >> - assuming noone speaks up against it. > > This friday? +1 :) -- /kinkie
Re: BZR repository upgrade re-proposal.
ons 2012-09-05 klockan 09:03 +0200 skrev Kinkie: > I'm glad that a consensus is forming. > The next question would then be who will perform the upgrade, and when > - assuming noone speaks up against it. This friday? Regards Henrik
Re: BZR repository upgrade re-proposal.
Okay, I'm glad that a consensus is forming. The next question would then be who will perform the upgrade, and when - assuming noone speaks up against it. Thanks -- /kinkie
