-
Notifications
You must be signed in to change notification settings - Fork 387
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.
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
vdire: plug the remaining vcl temperature change races #4259
base: master
Are you sure you want to change the base?
Conversation
689613b
to
0e115d9
Compare
The context for this change is varnishcache#4142, but while it builds upon it, it is still causally unrelated. To motivate this change, let's look at vcl_set_state() from before, and consider some non-issues and issues: transition to COLD ------------------ Lck_Lock(&vcl_mtx); vcl->temp = VTAILQ_EMPTY(&vcl->ref_list) ? VCL_TEMP_COLD : VCL_TEMP_COOLING; Lck_Unlock(&vcl_mtx); // *A* vcl_BackendEvent(vcl, VCL_EVENT_COLD); non-issues: At point *A*, backends could get added. For VCL_TEMP_COOLING, this fails (see VRT_AddDirector()). For VCL_TEMP_COLD, the state is consistent, because backends get created cold and that's it. At point *A*, backends could get removed. vdire_resign() from varnishcache#4142 ensures that the temperature passed to vcldir_retire() is consistent with the events the backends have received. transition to WARM (success) ---------------------------- Lck_Lock(&vcl_mtx); vcl->temp = VCL_TEMP_WARM; Lck_Unlock(&vcl_mtx); // *B* ... i = vcl_send_event(vcl, VCL_EVENT_WARM, msg); if (i == 0) { // ... *B* vcl_BackendEvent(vcl, VCL_EVENT_WARM); break; } issues: (1) In region *B*, backends could get removed. They will not have received a WARM event, but will be called with a WARM temperature, so they will receive a bogus COLD event. (2) Also in region *B*, backends could get added. They will implicitly receive a WARM event (see VDI_event() in VRT_AddDirector()), and then another from vcl_BackendEvent() transition to WARM (failed) --------------------------- AZ(vcl_send_event(vcl, VCL_EVENT_COLD, &nomsg)); AZ(nomsg); vcl->temp = VCL_TEMP_COLD; issues: (3) Backends added in region *B* will have received the implicit WARM event, and thus need a COLD event for the "cancel". To solve these issues, we need to do two things: There needs to be some kind of transaction which begins with the temperature change and ends when all backends have been notified appropriately. Backends can not get deleted while the transaction is in progress. We need a notion of "backends from before the temperature change" and "backends from after". The first part is already delivered by varnishcache#4142: The vdire facility already provides the transaction during which backends do not actually get deleted and it ensures that the right temperature gets passed when the deletion is carried out. So for this part, we only need to use vdire. But issues (2) and (3) are not yet covered. For these, we add a checkpoint, such that we know which directors from the list are the "base" from before the temperature change and which are the "delta" added after it. That's this patch. vdire_start_event() atomically (under the vcl_mtx) sets the checkpoint and the new temperature. vdire_end_event() just uses the existing vdire_end_iter() and clears the checkpoint. vcl_BackendEvent() gets split into two: backend_event_base() notifies backends from before the checkpoint. backend_event_delta() atomically sets a new temperature and notifies backends from after the checkpoint (but not from after its temperature change). Fixes varnishcache#4199
0e115d9
to
dd8da8b
Compare
This has been discussed during bugwash, mostly focused around the layering of dynamic backends use case. We concluded that this was not for the next release. Other than that I welcome input on possible alternative solutions. |
One addition to the bugwash discussion: Dynamic backend's temperature events are, by design, not tied to VCL temperature events. When they are created on a warm vcl, they receive a warm event, and, conversely, a cold event when they are removed. |
The context for this change is #4142, but while it builds upon it, it is still causally unrelated. This PR replaces #4205
Problem
To motivate this change, let's look at
vcl_set_state()
as of before the patch, and consider some non-issues and issues:transition to COLD
non-issues:
At point A, backends could get added. For
VCL_TEMP_COOLING
, this fails (seeVRT_AddDirector()
). ForVCL_TEMP_COLD
, the state is consistent, because backends get created cold and that's it.At point A, backends could get removed.
vdire_resign()
from #4142 ensures that the temperature passed tovcldir_retire()
is consistent with the events the backends have received.transition to WARM (success)
issues:
(1) In region B, backends could get removed. They will not have received a WARM event, but will be called with a WARM temperature, so they will receive a bogus COLD event.
(2) Also in region B, backends could get added. They will implicitly receive a WARM event (see
VDI_event()
inVRT_AddDirector()
), and then another fromvcl_BackendEvent()
transition to WARM (failed)
issues:
(3) Backends added in region B will have received the implicit WARM event, and thus need a COLD event for the "cancel".
Solution
To solve these issues, we need to do two things:
There needs to be some kind of transaction which begins with the temperature change and ends when all backends have been notified appropriately. Backends can not get deleted while the transaction is in progress.
We need a notion of "backends from before the temperature change" and "backends from after".
The first part is already delivered by #4142: The vdire facility already provides the transaction during which backends do not actually get deleted and it ensures that the right temperature gets passed when the deletion is carried out. So for this part, we only need to use vdire.
But issues (2) and (3) are not yet covered. For these, we add a checkpoint, such that we know which directors from the list are the "base" from before the temperature change and which are the "delta" added after it.
That's this patch.
vdire_start_event()
atomically (under thevcl_mtx
) sets the checkpoint and the new temperature.vdire_end_event()
just uses the existingvdire_end_iter()
and clears the checkpoint.vcl_BackendEvent()
gets split into two:backend_event_base()
notifies backends from before the checkpoint.backend_event_delta()
atomically sets a new temperature and notifies backends from after the checkpoint (but not from after its temperature change).Fixes #4199