Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New vcl_backend_refresh method #3994

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

New vcl_backend_refresh method #3994

wants to merge 6 commits into from

Conversation

walid-git
Copy link
Member

This PR introduces the new vcl_backend_refresh method discussed during VDD23Q3.
See: #3102 (comment) for more context.

@dridi
Copy link
Member

dridi commented Oct 9, 2023

You need to update doc/graphviz/cache_*.dot and rebuild doc/graphviz/cache_*.svg as part of this change.

@dridi
Copy link
Member

dridi commented Oct 9, 2023

This is also missing the obj_stale variable in vcl_backend_refresh. See how obj is handled for vcl_hit in doc/sphinx/reference/vcl_var.rst.

dridi
dridi reviewed Oct 10, 2023
View reviewed changes
bin/varnishd/builtin.vcl Outdated
Comment on lines 212 to 251
sub vcl_backend_refresh {
return (merge);
}
Copy link
Member

@dridi dridi Oct 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having second thoughts here, and I'm wondering whether we should generalize this subroutine to whenever there is a stale object involved.

sub vcl_backend_refresh {
        if (obj_stale.status == 200 && beresp.status == 304) {
                return (merge);
        }
        if (beresp.status == 304) {
                return (error); # until we refine error statuses
        }
        return (beresp);
}

Of course, with a pinch of splitting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dridi can you please motivate this suggestion? What else would you do with the stale object besides delivering it as-is (for grace mode) or use it for a refresh?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bugwash: This makes sense.

@walid-git walid-git force-pushed the vcl_backend_refresh branch 3 times, most recently from 5722162 to 68dda70 Compare October 16, 2023 11:59
@walid-git walid-git marked this pull request as ready for review October 23, 2023 12:06
dridi
dridi reviewed Nov 20, 2023
View reviewed changes
bin/varnishd/cache/cache_fetch.c Outdated
Comment on lines 499 to 491
case VCL_RET_ERROR:
/* FALLTHROUGH */
case VCL_RET_ABANDON:
/* FALLTHROUGH */
case VCL_RET_FAIL:
/* FALLTHROUGH */
case VCL_RET_RETRY:
skip_vbr = 1;
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply do this:

case VCL_RET_ERROR:
case VCL_RET_ABANDON:
case VCL_RET_FAIL:
case VCL_RET_RETRY:
	skip_vbr = 1;
	break;

It shouldn't trigger a warning.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember doing that on another PR and getting CCI failing compilation because of -Wimplicit-fallthrough

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that only happens if you got statements in-between without a break.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, I can test it then

dridi
dridi reviewed Nov 20, 2023
View reviewed changes
bin/varnishd/cache/cache_vrt.c Outdated
Comment on lines 260 to 272
case HDR_OBJ_STALE:
/* FALLTHROUGH */
case HDR_OBJ:
hp = NULL;
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

	case HDR_OBJ:
	case HDR_OBJ_STALE:
		hp = NULL;
		break;

dridi
dridi reviewed Nov 20, 2023
View reviewed changes
bin/varnishd/builtin.vcl Outdated
Comment on lines 214 to 215
if (!obj_stale.http.Last-Modified &&
!obj_stale.http.ETag) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are using obj_stale before it exists.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to understand, we only get into vcl_backend_refresh when we already have a stale_oc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable is introduced in VCL in the last commit of the current series.

@nigoroll
Copy link
Member

nigoroll commented Dec 4, 2023 •
edited
Loading

bugwash: #4026 should be addressed first

@dridi dridi marked this pull request as draft December 6, 2023 09:58
@dridi dridi mentioned this pull request Dec 6, 2023
Open
dridi added a commit to dridi/varnish-cache that referenced this pull request Mar 19, 2024
@dridi
builtin: Retry failed 304s by default
a0b6a7e 
Named after vcl_backend_refresh from varnishcache#3994.
dridi added a commit to dridi/varnish-cache that referenced this pull request Mar 19, 2024
@dridi
builtin: Retry failed 304s by default
541cd7a 
Named after vcl_backend_refresh from varnishcache#3994.
dridi added a commit to dridi/varnish-cache that referenced this pull request Jun 18, 2024
@dridi
builtin: Retry failed 304s by default
2c12ab4 
Named after vcl_backend_refresh from varnishcache#3994.
@dridi dridi mentioned this pull request Jul 8, 2024
Draft
@walid-git
VCL: introduce sub vcl_backend_refresh
5978808 
This commit introduces a new sub vcl_backend_refresh method intended
to allow allow a more flexible approach on object revalidation.
@walid-git
vrt_var: Generalize obj.* variables bindings
b0c3ccb
@walid-git
VCL: Introduce new obj_stale variable
b4b91fa 
obj_stale variable gives access to the stale object we had in cache
when doing a 304 revalidation. It is only readable from vcl_backend_refresh
subroutine.
@walid-git
VCL: Introduce "return retry(fetch)"
1a24fe5
@walid-git walid-git marked this pull request as ready for review August 26, 2024 12:07
@walid-git
Copy link
Member Author

PR updated according to @dridi's comment with slightly modified order in the builtin VCL (swapped vcl_refresh_conditions and vcl_refresh_status to allow using a custom return (merge/beresp/stale) while still going through the checks).

Anyway, as far as this ticket is concerned, I think we are fine with our current 304 handling and we can close it.

As @dridi concluded that we are correctly handling 304, I have now undrafted this PR making it ready for reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asadsa92 asadsa92 asadsa92 left review comments

@nigoroll nigoroll nigoroll left review comments

@dridi dridi dridi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@walid-git @dridi @nigoroll @asadsa92