nexcess / magento-turpentine

A Varnish extension for Magento.
GNU General Public License v2.0
520 stars 252 forks source link

Varnish 4.0 Support #498

Closed aheadley closed 9 years ago

aheadley commented 10 years ago

Varnish 4.0 has been released, need to add support for it.

https://www.varnish-cache.org/docs/trunk/whats-new/upgrading.html

eth8505 commented 10 years ago

Are you actively working on this? I was going to fiddle around with varnish 4 anyway now that its been released and could give the basic implementation a try...

aheadley commented 10 years ago

Not right now, I wasn't planning on looking into it til next week so go ahead if you want. The changes needed look to be pretty minimal, the only part that might present a challenge is determining whether the Varnish server is 3.x or 4.0 for the Automatic version selection.

eth8505 commented 10 years ago

Yeah, the automatic version selection may be challenging. I'll try to get a VM running with magento and varnish4 asap though to try that out.

eth8505 commented 10 years ago

I guess we could do version detection by parsing the banner. Varnish 3+ seems to output the version number in there, varnish 2.1 does not. That approach might actually even be faster than what turpentine currently does, since there is no need for running a help command on the socket...

eth8505 commented 10 years ago

After two days of fiddling around with the VCL and finding the things missing from the upgrade docs I managed to get it running on a default magento installation. You can check it out here: https://github.com/eth8505/magento-turpentine/tree/varnish4-compat

I also created a vagrant environment that allows to automatically install varnish 2.1, 3.0 or 4.0 depending on the config https://github.com/eth8505/magento-vagrant-puppet-varnish Feel free to have a go at it. I'm pretty certain that it's not at 100% yet, but I'm getting there.

I haven't submitted a pull request yet though, since I'm not sure that it's fully working.

csdougliss commented 10 years ago

:+1:

jdub commented 10 years ago

Nice!

csdougliss commented 10 years ago

@eth8505 How are you getting on?

eth8505 commented 10 years ago

I didn't have much time in the last two weeks. I'm hoping to put in some time for testing and fixing this week. Anyone is welcome to try this out and give me some feedback though :-)

eth8505 commented 10 years ago

I'm sorry it took me so long, but I finally got around to do some more testing today. I just pushed a few fixes to my fork https://github.com/eth8505/magento-turpentine/tree/varnish4-compat which should make it a little more stable now. The main change on the branch is the refactored varnish version detection no longer requiring to actually send a command to varnish when detecting the version, but now solely relies on the banner that is delivered by varnish as of 2.1. I tested the new version detection on 2.1, 3.0 and 4.0 and it works well. Getting the VCL to run was actually more tricky as I had anticipated, since the upgrade guide provided by the Lasse and Poul wasn't complete and I had to test around a lot. Also, the inline C was broken, since the API for VRT_SetHdr changed in Varnish 4 and I had to dig around the Varnich repo to find out how to mod the old code.

I would classify this as a beta version now and would greatly appreciate if you folks could put in some time to test it. Please note that in order to get varnish4 to work with turpentine you need to change the startup parameters a little more than with varnish 3, since inline C is fully disabled by default. Also, the bitmask for disabling the syntax checks that was previously called "esi_syntax" has now been moved to a feature parameter. So you need to set -p feature=+esi_ignore_other_elements as well as -p vcc_allow_inline_c=on.

Edit: I just added the required varnish startup params to their respective sections in the technical notes.

csdougliss commented 10 years ago

Great work, I am trying it out now.

Hi, I am not sure if it just me but the generated VCL has a blank (where it should be false or true) here:

if ( || client.ip ~ debug_acl) {
        # debugging is on, give some extra info
        set resp.http.X-Varnish-Hits = obj.hits;
        set resp.http.X-Varnish-Esi-Method = req.http.X-Varnish-Esi-Method;
        set resp.http.X-Varnish-Esi-Access = req.http.X-Varnish-Esi-Access;
        set resp.http.X-Varnish-Currency = req.http.X-Varnish-Currency;
        set resp.http.X-Varnish-Store = req.http.X-Varnish-Store;
    } else {

Also another one - but I think this is because I have strip whitespace off

error: invalid preprocessing directive #@
     # @source app/code/community/Nexcessnet/Turpentine/misc/uuid.c
eth8505 commented 10 years ago

I don't see how that can happen. Can you debug what happens in \Nexcessnet_Turpentine_Model_Varnish_Configurator_Abstract::_formatTemplate ?

eth8505 commented 10 years ago

That looks more like a problem with the whitespace trimming to be honest. Do you have that option in there? If so, please set the respective option to "always". It appears this is a result of the comment within the C code block, since C does not support hash style comments. Is a fix for a separate branch though. Please keep your whitespace trimming to always for testing :-)

csdougliss commented 10 years ago

I reported the strip whitespace option separately, https://github.com/nexcess/magento-turpentine/issues/535

The issue with the missing false/true seems to have gone away :)

csdougliss commented 10 years ago

I have it working (finally)!

I've had to disable a bit of custom code (newrelic) and syslog due to changed functions - VRT_SetHdr (the headers has changed, and it's not mentioned in the list of changes (on the varnish cache website):

newrelic:

sub vcl_recv {
# This is not working in Varnish 4.0
#    Add X-Request-Start header so we can track queue times in New Relic RPM beginning at Varnish.
#    if (req.restarts == 0) {
#        C{
#                struct timeval detail_time;
#                gettimeofday(&detail_time,NULL);
#                char start[20];
#                sprintf(start, "t=%lu%06lu", detail_time.tv_sec, detail_time.tv_usec);
#                VRT_SetHdr(sp, HDR_REQ, "\020X-Request-Start:", start, vrt_magic_string_end);
#        }C
#    }
}

syslog - sp is undefined, VRT_r_obj_response no longer exists

if(fp != NULL) {
                fprintf(fp, "%s: Error (%s) (%s) (%s)\n",
                ft, VRT_r_req_url(sp), VRT_r_obj_response(sp), VRT_r_req_xid(sp));

                fclose(fp);
            } else {
                syslog(LOG_INFO, "Error (%s) (%s) (%s)",
                VRT_r_req_url(sp), VRT_r_obj_response(sp), VRT_r_req_xid(sp));
            }
eth8505 commented 10 years ago

Yep, the header stuff changed quite a lot in varnish4, i modified the default code in version-4.vcl though to get the VRT_SetHdr stuff working. You can check there on how to get your code ported.

Edit: https://github.com/eth8505/magento-turpentine/blob/varnish4-compat/app/code/community/Nexcessnet/Turpentine/misc/version-4.vcl#L58

csdougliss commented 10 years ago

Thanks for newrelic I've followed your example and modified it so:

if (req.restarts == 0) {
        C{
            struct timeval detail_time;
            gettimeofday(&detail_time,NULL);
            char start[20];
            sprintf(start, "t=%lu%06lu", detail_time.tv_sec, detail_time.tv_usec);
            static const struct gethdr_s VGC_HDR_REQ_VARNISH_FAKED_SESSION = { HDR_REQ, "\030X-Varnish-Faked-Session:"};
            VRT_SetHdr(ctx, &VGC_HDR_REQ_VARNISH_FAKED_SESSION, "\020X-Request-Start:", start, vrt_magic_string_end);
        }C
    }

Is it necessary to have a vcl_backend_error? What happens when you don't? What error message gets displayed?

eth8505 commented 10 years ago

That's a good question, I'd have to find out what the default handling is there. But since turpentine didn't have vcl_error in 3.0 i don't think we need vcl_backend_error unless the handling changed dramatically.

csdougliss commented 10 years ago

Could you make a PR for this? I think it's ready to be merged into devel.

eth8505 commented 10 years ago

I'm not so sure, since it's only been tested by two people so far ...

jdub commented 10 years ago

For anyone using Ubuntu or Debian, upstream Varnish 4.0 packages are available for easy testing. Replace "precise" with the code name of your release:

deb http://repo.varnish-cache.org/ubuntu/ precise varnish-4.0

Install the repo key before you apt-get update:

sudo apt-key adv --keyserver keyserver.ubuntu.com --recv-keys 60E7C096C4DEFFEB

I'll be back with testing results! :smiley:

eth8505 commented 10 years ago

Great :-) Let me know if you find any bugs and I'll fix them asap.

csdougliss commented 10 years ago

If anyone is using 14.04 (trusty) the packages are not available just yet - I recompiled from source.

csdougliss commented 10 years ago

I will try the new varnish 4.0 branch on varnish 3.0 for backwards compatibility hopefully today

eth8505 commented 10 years ago

@craigcarnell great, thanks for that. I hope the updated version detection works as intended.

csdougliss commented 10 years ago

@eth8505 I have Varnish 3.0 installed locally. I've tried the Varnish 4.0 branch with the config settings set to auto and it seems to generate the correct VCL for Varnish 3.0 just fine.

eth8505 commented 10 years ago

I just submitted a pull request for the varnish4 branch :-)

csdougliss commented 10 years ago

@eth8505 Thanks. I have a question regarding the change noted in the Varnish 4.0 change log:

Background (re)fetch of expired objects. On a cache miss where a stale copy is available, serve the client the stale copy while fetching an updated copy from the backend in the background.

I assume, therefore, when I hit an expired page in magento (such as a category page) - I will see the varnish(ed) page (at varnish speed, and not at nginx/php speed) and the next visitor will see a refreshed varnish(ed) page - is this correct? Thanks

eth8505 commented 10 years ago

@craigcarnell That's what it's supposed to do, yes. That's also the reason why vcl_fetch has been renamed to vcl_backend_response. It's one of the big advantages of varnish 4, since the user generating a new cache entry no longer has to wait for the backend to generate the response, but gets the stale copy immediately. I am unsure if the VCL works for that though, I simply converted it from 3.0 to 4.0 VCL, but the upgrade docs never said anything else about changed grace config.

csdougliss commented 10 years ago

@eth8505 Not sure if this is related or not, but using the varnish 4.0 branch on a varnish 3.0.5 instance:

a:5:{i:0;s:59:"Call to a member function xpath() on a non-object (boolean)";i:1;s:1252:"#0 /var/www/vhosts/magento-capistrano/releases/20140625071020/app/code/community/Nexcessnet/Turpentine/controllers/EsiController.php(209): Mage_Core_Model_Layout->generateXml()
#1 /var/www/vhosts/magento-capistrano/releases/20140625071020/app/code/community/Nexcessnet/Turpentine/controllers/EsiController.php(101): Nexcessnet_Turpentine_EsiController->_getEsiBlock()
#2 /var/www/vhosts/magento-capistrano/releases/20140625071020/app/code/core/Mage/Core/Controller/Varien/Action.php(418): Nexcessnet_Turpentine_EsiController->getBlockAction()
#3 /var/www/vhosts/magento-capistrano/releases/20140625071020/app/code/core/Mage/Core/Controller/Varien/Router/Standard.php(250): Mage_Core_Controller_Varien_Action->dispatch()
#4 /var/www/vhosts/magento-capistrano/releases/20140625071020/app/code/core/Mage/Core/Controller/Varien/Front.php(172): Mage_Core_Controller_Varien_Router_Standard->match()
#5 /var/www/vhosts/magento-capistrano/releases/20140625071020/app/code/core/Mage/Core/Model/App.php(354): Mage_Core_Controller_Varien_Front->dispatch()
#6 /var/www/vhosts/magento-capistrano/releases/20140625071020/app/Mage.php(684): Mage_Core_Model_App->run()
#7 /var/www/vhosts/magento-capistrano/releases/20140625071020/index.php(113): Mage::run()
#8 {main}";s:3:"url";s:498:"/turpentine/esi/getBlock/method/ajax/access/private/ttl/0/hmac/e4b2f60159a0534171b530c7cfeffe911b4beac1383e68dfb96c12cc3bdd7798/data/.06RvkbfpkSbspKV33S35tJQQ9apcIpddFqdjm1FqcQhRpRssAzQCZaC3na3nWvxI1kmXH6-oASZVvynbZjJ6sjkvT6RO5eVjIxqmgTpB2x5B5M9LPoDY5bi4bFke14ZqWUFfx5XY3By-OJhLiSzgPLABJ3IbeZAurtDLiqbiWvs3wkRNFfgjiW3q4O0xS9b5pAYCx5zLNCmLGECSfjaxHAdOY.NRpitubkTzN.Z2orxG9VytfszWJClC4pxC8CyTdDABCDVNtQhbkx-4CeZjdw8fjYJchujFKCwWxT.1IcJPe8KPc5OXF3VkGvGoADGT8BXjoZKhAZeBcucHDJYiEU2izIL4bJjU.VkHIzjHOg=/";s:11:"script_name";s:10:"/index.php";s:4:"skin";s:13:"vax_yard_sale";}

This is in var/report

eth8505 commented 10 years ago

I don't see how it could be, since I didn't actually change anything in the layout processing.

jesavro commented 10 years ago

Hi, Any news on varnish 4 support?

csdougliss commented 10 years ago

@eth8505 I think your right, I've not seen that error message again

csdougliss commented 10 years ago

Wondering if there are changes needed for Varnish 4.0.1?

electrawn commented 10 years ago

Greets, I have had time to performance test eth8505's fork against stock EE 1.13. There seems to be a bug in the ESI product category/header code, but turning the ESI block cache off works fine. Anything needed to help merge this to devel?

eth8505 commented 10 years ago

I haven't yet had the time to actually properly rebase the varnish4 compat branch onto devel. Also, I haven't tested anything against 4.0.1 and most likely I won't get around to it before september either.

Were you able to determine what breaks with the category/header code? I don't have an enterprise version to test against...

electrawn commented 10 years ago

I'll leave this up for 24 hrs:

http://54.164.73.224/index.php/category-1-3/category-1.html

        <!-- ESI START DUMMY COMMENT [] -->

esi:removeESI processing not enabled/esi:remove

        <ul class="links">
                    <li class="first" ><a href="http://54.164.73.224/index.php/customer/account/" title="My Account" >My Account</a></li>
                            <li class=" last" ><a href="http://54.164.73.224/index.php/wishlist/" title="My Wishlist" >My Wishlist</a></li>
        </ul>
eth8505 commented 10 years ago

Looks alright to me, the esi processing not enabled messages are rather weird though. Can you check your logs? Most of the time esi processing not enabled errors result from some sort of exeption generated somewhere...

awald commented 10 years ago

@eth8505 any timeline for when Varnish 4 support will be merged?

eth8505 commented 10 years ago

@awald you'd have to ask @aheadley about that. I guess he doesn't have any time for it. If it doesn't happen until we migrate to varnish4 with our systems I'll create a full fork myself though.

csdougliss commented 10 years ago

@aheadley Are you still out there? :)

aheadley commented 10 years ago

I am. Haven't had time to work on Turpentine as of late, but should be pushing out the devel branch sometime this week, then I'll look at merging the 4.0 support.

csdougliss commented 10 years ago

@aheadley Looking forward to it :)

eth8505 commented 10 years ago

@aheadley are you planning to merge any more of the PRs before getting a new version out there? If so, I would be able to get my varnish 4.0 PR rebased onto the new devel alongside with some testing on varnish 4.0.1. Festival season is over now, and I will finally have some time on weekends for programming :)

csdougliss commented 10 years ago

@aheadley I take it you didn't quite have the time to do it? :)

srmobile commented 10 years ago

Any news? :)

Krapulat commented 10 years ago

In Varnish 4 I have the same error as @electrawn .

I get " " in the source code.

I think esi:removeESI is not working.

electrawn commented 10 years ago

I created a patch to fix the issue for enterprise. I patched against the varnish4-compat fork. Default Enterprise magento has a few things in the header that need categories enabled just right. The app/design/frontend/enterprise/default/layout/turpentine_esi.xml will override the settings posed by the default one in app/design/frontend/default/layout/turpentine_esi.xml . Surprised @aheadley enterprise support is an easy fix.

csdougliss commented 10 years ago

@aheadley Updates on this? Really would like to see this!!

zewolf commented 9 years ago

I have Varnish4 and turpentine varnishcompat4 tree, but I still have "Failed to apply the VCL to 127.0.0.1:6082: Varnish data to write over length limit by 210 characters" error.

I already have options in my Daemons options :

         -p cli_buffer=16384 \
         -p vcc_allow_inline_c=on \
         -p feature=+esi_ignore_other_elements \

Any idea ?