IBX-3014: Varnish 7 support #175
Conversation
| @@ -0,0 +1,335 @@ | |||
| // Varnish VCL for: | |||
| // - Varnish 7.1 or higher | |||
There was a problem hiding this comment.
It would be good if you in the first commit only copied varnsh5.vcl as-is. So that in the this PR I could acutally see changed in the varnish6 vs varnish7...
As it is not, I have to diff them locally in order to see the difference
There was a problem hiding this comment.
@vidarl I can force-push this change to branch, however, in PR you will still see a new file added, not a modification of the previous one.
There was a problem hiding this comment.
yes. but if you first added the file as-is (a carbon copy of varnsish5.vcl, I could see the changes per commit.. The PR overview would still show it as a new file, correct
|
There's a difference in behaviour with the new VCL (on Varnish 7) compared to the old one when a Content Item is in grace. Setup:
When the Content Item is edited in the backend the cache is purged. When we request the Content Item on the frontend on Varnish 6:
On Varnish 7 with current VCL:
I believe the new VCL doesn't have logic correspodning to this part: https://github.com/ezsystems/ezplatform-http-cache/blob/2.3/docs/varnish/vcl/varnish5.vcl#L91-L94 in the old VCL. I thought about this a bit and I think that it's ok - if we were 100% serious about not letting logged in users see the old Content on the frontend then we wouldn't use soft-purging. It does not make a difference for Admin panel, because HTTP cache is disabled there. If you agree then I will see if we can adapt the failing test for Varnish 7 (or maybe disable it if it's not possible) |
|
Hi
I am surprised by "1." The code you refer to should disable the background fetch. Also, when I test myself, I see the behavior I expect, not the one you are observing. I agree with you. I think the new behaviour is anyway acceptable. I also had a look at the Fastly implementation and it does not have the varnish6 behavior where stale-cache is not served users with cookie. Implementing this behavior in varnish 7 would complicate things quite a bit, it would require a new restart as |
There was a problem hiding this comment.
Hi
The vcl changes looks good to me.
But changes in parameters.vcl should be reverted IMO
While testing it, I found however this problem : https://issues.ibexa.co/browse/IBX-3715
But that is not related this PR directly ( both varnish6 and fastly is affected by the same problem )
docs/varnish/vcl/parameters.vcl
Outdated
| @@ -5,7 +5,7 @@ | |||
| // https://github.com/ezsystems/ezplatform/blob/master/doc/docker/entrypoint/varnish/parameters.vcl | |||
|
|
|||
| backend ezplatform { | |||
| .host = "127.0.0.1"; | |||
| .host = "web"; // Replace with hostname/ip of the application server | |||
There was a problem hiding this comment.
I don't think these changes are relevant. They look docker specific. So I would recommend leaving them as-is
The paramters.yml file used by docker containers are anyway this one https://github.com/ezsystems/ezplatform/blob/master/doc/docker/entrypoint/varnish/parameters.vcl
There was a problem hiding this comment.
Hi Vidar,
As for those changes, they are required to make tests pass properly.
Specifically in https://github.com/ibexa/docker/pull/7/files
However, we should simply refresh that PR to use the file mentioned above, and we should be good to go.
|
@vidarl thank you for your review. @ibexa-yuna we must look into it as soon as you are back. This is top priority. |
|
Kudos, SonarCloud Quality Gate passed!
|
TODO:
$ composer test)$ composer fix-cs).