Backend connection queue #4030

walid-git · 2023-12-14T11:17:03Z

This patch allows a task to be queued when a backend reaches its max_connections. The task will queue on the backend and wait for a connection to become available, rather than immediately failing. This capability is off by default and must be enabled with new parameters.

The following parameters have been added:
backend_wait_timeout: the amount of time a task will wait (default 0.0).
backend_wait_limit: the maximum number of tasks that can wait (default 0).

The two parameters can also be overriden for individual backends in vcl:

    backend foo {
        .host = "bar.com";
        .wait_timeout = 3s;
        .wait_limit = 10;
    }

Authored by: @drodden (with minor rearrangements)

walid-git · 2023-12-14T13:35:04Z

With this change, it seems that the default value for thread_pool_stack on 32 bit systems is no more sufficient (reason why e00029.vtc is failing on ubuntu_bionic).

bsdphk · 2023-12-18T12:02:47Z

As an initial matter I would prefer queue_length and queue_timeout which I think are almost self-explanatory.

I'm not sure I think it is a good idea however, but default-off mitigates that.

dridi · 2023-12-18T13:27:24Z

FWIW this is a partial implementation of what we agreed on for VIP 31.

https://github.com/varnishcache/varnish-cache/wiki/VIP31%3A-Backend-connection-queue

There are two loose points not covered, disembarking and health status. Disembarking fetch tasks is a large project, one we can disregard or move to its own VIP. The saturation of both .max_connections and .wait_limit/backend_wait_limit is probably reasonable to address before closing VIP 31.

nigoroll

In general, I am 👍🏽

bin/varnishd/cache/cache.h

bin/varnishd/cache/cache_backend.c

include/vrt.h

nigoroll · 2024-01-15T15:39:35Z

Thank you, this looks good to me overall.

I would still have some suggestions, but would also be OK to polish after merge, if you agree:

struct backend_cw should have a magic value which we check for access from other threads. While I did suggest to move it to the stack, I am well aware of the risk of smashing foreign stacks, and magic checks can help raise a flag if that happens.
the magic would be initialized with INIT_OBJ(), which would also initialize the list pointers for clarity. The code is still correct, IIUC, even with uninitialized list pointers, but I think it helps debugging a lot if they are zeroed.
cw_state should be moved into struct backend_cw. This allows for additional assertions (e.g. in vbe_connwait_signal_locked, we can assert that the state is CW_QUEUED.
when we dequeue, we should change the state to a new state, e.g. CW_DEQUEUED.
We should add a backend_cw_fini function which asserts that the state is != CW_QUEUED before destroying the condition variable.
Regarding the inlining of the dequeue, I think I would actually prefer a vbe_connwait_dequeue_locked for clarity, because that would now also change the state.

nigoroll

see top level comment

bin/varnishd/cache/cache_backend.c

bin/varnishd/cache/cache_backend.h

walid-git · 2024-02-05T11:20:18Z

Rebased and addressed all review comments. Ready for a (hopefully) last review.

bin/varnishd/cache/cache_backend.c

Nitpick noticed during review of varnishcache#4030

nigoroll

I always feel bad when it looks like I was holding things up, so I would like to apologize for not having spotted some issues earlier.

bin/varnishd/cache/cache_backend.c

nigoroll · 2024-02-19T16:26:33Z

bin/varnishd/cache/cache_backend.c

@@ -149,6 +246,12 @@ vbe_dir_getfd(VRT_CTX, struct worker *wrk, VCL_BACKEND dir, struct backend *bp,
 	if (bo->htc == NULL) {
 		VSLb(bo->vsl, SLT_FetchError, "out of workspace");
 		/* XXX: counter ? */
+		if (cw->cw_state == CW_QUEUED) {
+			Lck_Lock(bp->director->mtx);
+			vbe_connwait_dequeue_locked(bp, cw);


Should we not move this whole htc alloc block to the top, even before the cw init?

Reason: the ws does not change during waiting, so if it overflows, it does so right from the start.

Why would we allocate workspace before we are sure that we can get a backend connection ?

If we wouldn't be able to allocate the htc in the first place, why wait at all?

On the other hand, if we are effectively not connecting and workspace would have been too tight, then we fail for the wrong reason, and we don't visit vcl_backend_error at all.

Since the default values for the parameters make this an opt-in feature, may I suggest adding an XXX comment for now to take the proper time later to see how to best approach this? (snapshot/reset for certain paths for example)

I have added an XXX comment as suggested

On the other hand, if we are effectively not connecting and workspace would have been too tight, then we fail for the wrong reason, and we don't visit vcl_backend_error at all.

I disagree on "fail for the wrong reason". This code only runs because we do want to connect and having enough workspace is a precondition for the connect to succeed. Potentially running into the connection or wait limit does not make it a "wrong reason" to fail for insufficient workspace.

walid-git · 2024-02-26T11:24:32Z

I have addressed most of the last review items, and mentioned the potential drawbacks of this feature in the docs as requested during last bugwash.

bin/varnishd/cache/cache.h

dridi · 2024-02-27T08:54:12Z

bin/varnishd/cache/cache_backend.c

@@ -149,6 +246,12 @@ vbe_dir_getfd(VRT_CTX, struct worker *wrk, VCL_BACKEND dir, struct backend *bp,
 	if (bo->htc == NULL) {
 		VSLb(bo->vsl, SLT_FetchError, "out of workspace");
 		/* XXX: counter ? */
+		if (cw->cw_state == CW_QUEUED) {
+			Lck_Lock(bp->director->mtx);
+			vbe_connwait_dequeue_locked(bp, cw);


If we wouldn't be able to allocate the htc in the first place, why wait at all?

On the other hand, if we are effectively not connecting and workspace would have been too tight, then we fail for the wrong reason, and we don't visit vcl_backend_error at all.

Since the default values for the parameters make this an opt-in feature, may I suggest adding an XXX comment for now to take the proper time later to see how to best approach this? (snapshot/reset for certain paths for example)

bin/varnishd/cache/cache_busyobj.c

nigoroll

I disagree on one detail still, but I do not want to hold this PR up any longer.

nigoroll · 2024-02-28T15:06:57Z

bin/varnishd/cache/cache_backend.c

@@ -149,6 +246,12 @@ vbe_dir_getfd(VRT_CTX, struct worker *wrk, VCL_BACKEND dir, struct backend *bp,
 	if (bo->htc == NULL) {
 		VSLb(bo->vsl, SLT_FetchError, "out of workspace");
 		/* XXX: counter ? */
+		if (cw->cw_state == CW_QUEUED) {
+			Lck_Lock(bp->director->mtx);
+			vbe_connwait_dequeue_locked(bp, cw);


On the other hand, if we are effectively not connecting and workspace would have been too tight, then we fail for the wrong reason, and we don't visit vcl_backend_error at all.

I disagree on "fail for the wrong reason". This code only runs because we do want to connect and having enough workspace is a precondition for the connect to succeed. Potentially running into the connection or wait limit does not make it a "wrong reason" to fail for insufficient workspace.

This patch allows a task to be queued when a backend reaches its max_connections. The task will queue on the backend and wait for a connection to become availble, rather than immediately failing. This initial commit just adds the basic functionality. It temporarily uses the connect_timeout as the queue wait time, until new parameters are added in followup effort.

The following parameters have been added: the amount of time a task will wait. the maximum number of tasks that can wait. - global parameters: backend_wait_timeout (default 0.0) backend_wait_limit (default 0) - those parameters can be overridden in the backend: backend foo { .host = "bar.com"; .wait_timeout = 3s; .wait_limit = 10; } The backend wait queue capability is off by default and must be enabled by setting both of the new parameters defined above. Note that this makes an ABI breaking change.

These counters were added to main: backend_wait - count of tasks that waited in queue for a connection. backend_wait_fail - count of tasks that waited in queue but did not get a connection (timed out).

As suggested by Nils

This makes sure that we won't abort a backend connection attempt if the backend can take it. It covers for any potential missing connwait_signal call.

nigoroll · 2024-03-01T15:21:48Z

bugwash:

proposed (re)name:

global parameters:

backend_queue_limit
backend_queue_timeout

VCL:

backend foo {
  .queue_limit = 42;
  .queue_timeout 1m;
}
sub vcl_backend_fetch {
  set bereq.queue_limit = 42;
  set bereq.queue_timeout = 1m;
}

gquintard · 2024-05-15T19:52:57Z

hi all! I'd really like this to get into the next release, and from what I'm reading, it's only a naming exercise from now on.

As a refresher, the current PR offers backend_wait_timeout/backend_wait_limit and the bugwash proposed backend_queue_timeout/backend_queue_limit. I feel like this isn't a big enough contention point to block a merge.

Any chance to get the original names in? I hate to bring it up and I'll probably get slapped for it but: we have customers using the feature and consistency is pretty important, I don't want to have to translate parameter names depending on the platform people are running.

nigoroll requested changes Dec 18, 2023

View reviewed changes

bin/varnishd/cache/cache.h Outdated Show resolved Hide resolved

bin/varnishd/cache/cache_backend.c Outdated Show resolved Hide resolved

include/vrt.h Show resolved Hide resolved

walid-git force-pushed the upstream_backend_queue branch 2 times, most recently from 478c1ef to 65bebcb Compare January 15, 2024 13:37

nigoroll approved these changes Jan 15, 2024

View reviewed changes

walid-git force-pushed the upstream_backend_queue branch from 65bebcb to 8fa9f61 Compare January 15, 2024 18:08

dridi requested changes Jan 15, 2024

View reviewed changes

bin/varnishd/cache/cache_backend.c Outdated Show resolved Hide resolved

bin/varnishd/cache/cache_backend.c Outdated Show resolved Hide resolved

bin/varnishd/cache/cache_backend.c Outdated Show resolved Hide resolved

walid-git force-pushed the upstream_backend_queue branch from 8fa9f61 to bc95536 Compare January 16, 2024 09:59

dridi requested changes Jan 16, 2024

View reviewed changes

bin/varnishd/cache/cache_backend.h Outdated Show resolved Hide resolved

walid-git force-pushed the upstream_backend_queue branch from bc95536 to f88295d Compare January 22, 2024 14:05

walid-git force-pushed the upstream_backend_queue branch 2 times, most recently from 7fb8ce2 to f82cb60 Compare February 5, 2024 11:18

dridi reviewed Feb 5, 2024

View reviewed changes

bin/varnishd/cache/cache_backend.c Show resolved Hide resolved

dridi approved these changes Feb 15, 2024

View reviewed changes

bin/varnishd/cache/cache_backend.c Outdated Show resolved Hide resolved

walid-git force-pushed the upstream_backend_queue branch from f82cb60 to 5eed80f Compare February 19, 2024 12:11

nigoroll self-requested a review February 19, 2024 14:52

nigoroll added a commit to nigoroll/varnish-cache that referenced this pull request Feb 19, 2024

pthread_cond_{timed,}wait shall not return an error code of [EINTR].

1bf00f9

Nitpick noticed during review of varnishcache#4030

nigoroll mentioned this pull request Feb 19, 2024

pthread_cond_{timed,}wait shall not return an error code of [EINTR]. #4058

Closed

nigoroll requested changes Feb 19, 2024

View reviewed changes

dridi approved these changes Feb 27, 2024

View reviewed changes

dridi added b=enhancement r=trunk c=varnishd a=ActionBeforeRelease a=Bugwash Today labels Feb 27, 2024

walid-git force-pushed the upstream_backend_queue branch from 07b07aa to 346de32 Compare February 28, 2024 10:43

nigoroll approved these changes Feb 28, 2024

View reviewed changes

drodden and others added 11 commits February 29, 2024 10:32

backend: add main counters for backend queue

37e338a

These counters were added to main: backend_wait - count of tasks that waited in queue for a connection. backend_wait_fail - count of tasks that waited in queue but did not get a connection (timed out).

backend: add test cases for the backend wait queue

e86c703

vrt: Register VBE connection queue changes

730775b

Add changelog

1f03de3

backend: Move cw_list and cw_cond to stack

448c8f8

As suggested by Nils

backend: Extract lock from vbe_connwait_dequeue

55567ae

backend: Add vbe_connwait_fini

7d68df2

vcl-backend: Document new connection queue attributes

fd5e6fc

conn-queue: Document potential drawbacks

54090b5

walid-git force-pushed the upstream_backend_queue branch from 346de32 to 07e0295 Compare February 29, 2024 09:34

walid-git added 2 commits February 29, 2024 10:35

cache_backend: Always initialize timeout vars (flexlint)

9986084

cache_backend: Re-check that BE is still busy after wakeup

d604088

This makes sure that we won't abort a backend connection attempt if the backend can take it. It covers for any potential missing connwait_signal call.

walid-git force-pushed the upstream_backend_queue branch from 07e0295 to d604088 Compare February 29, 2024 09:36

dridi removed a=ActionBeforeRelease a=Bugwash Today labels Mar 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backend connection queue #4030

Backend connection queue #4030

walid-git commented Dec 14, 2023

walid-git commented Dec 14, 2023

bsdphk commented Dec 18, 2023

dridi commented Dec 18, 2023

nigoroll left a comment

nigoroll commented Jan 15, 2024 •

edited

nigoroll left a comment

walid-git commented Feb 5, 2024

nigoroll left a comment

nigoroll Feb 19, 2024

walid-git Feb 20, 2024

dridi Feb 27, 2024

walid-git Feb 28, 2024

nigoroll Feb 28, 2024

walid-git commented Feb 26, 2024

dridi Feb 27, 2024

nigoroll left a comment

nigoroll Feb 28, 2024

nigoroll commented Mar 1, 2024

gquintard commented May 15, 2024

Backend connection queue #4030

Are you sure you want to change the base?

Backend connection queue #4030

Conversation

walid-git commented Dec 14, 2023

walid-git commented Dec 14, 2023

bsdphk commented Dec 18, 2023

dridi commented Dec 18, 2023

nigoroll left a comment

Choose a reason for hiding this comment

nigoroll commented Jan 15, 2024 • edited

nigoroll left a comment

Choose a reason for hiding this comment

walid-git commented Feb 5, 2024

nigoroll left a comment

Choose a reason for hiding this comment

nigoroll Feb 19, 2024

Choose a reason for hiding this comment

walid-git Feb 20, 2024

Choose a reason for hiding this comment

dridi Feb 27, 2024

Choose a reason for hiding this comment

walid-git Feb 28, 2024

Choose a reason for hiding this comment

nigoroll Feb 28, 2024

Choose a reason for hiding this comment

walid-git commented Feb 26, 2024

dridi Feb 27, 2024

Choose a reason for hiding this comment

nigoroll left a comment

Choose a reason for hiding this comment

nigoroll Feb 28, 2024

Choose a reason for hiding this comment

nigoroll commented Mar 1, 2024

gquintard commented May 15, 2024

nigoroll commented Jan 15, 2024 •

edited