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

Add director notfiy method and use it for admin health changes #4186

Closed
wants to merge 3 commits into from
Closed

Add director notfiy method and use it for admin health changes #4186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
17063ac
Add a notify method for directors and implement it for VBE
nigoroll Sep 17, 2024
69ab043
Use VRT_Notify() when the admin health changes
nigoroll Sep 17, 2024
280450a
SQUASHME: A newly created backend does not have a director yet
nigoroll Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 21 additions & 12 deletions bin/varnishd/cache/cache_backend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,16 +139,8 @@ VBE_Connect_Error(struct VSC_vbe *vsc, int err)

/*--------------------------------------------------------------------*/

int
VBE_is_ah_auto(const struct backend *bp)
{

CHECK_OBJ_NOTNULL(bp, BACKEND_MAGIC);
return (bp->director->vdir->admin_health == VDI_AH_AUTO);
}

void
VBE_connwait_signal_all(const struct backend *bp)
static void
vbe_connwait_signal_all(const struct backend *bp)
Comment on lines +142 to +143
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: vbe_connwait_broadcast()

{
struct connwait *cw;

Expand Down Expand Up @@ -661,6 +653,21 @@ vbe_healthy(VRT_CTX, VCL_BACKEND d, VCL_TIME *t)
return (!bp->sick);
}

static VCL_VOID v_matchproto_(vdi_notify_f)
vbe_notify(VCL_BACKEND d)
{
const struct vdi_ahealth *ah;
struct backend *bp;

CHECK_OBJ_NOTNULL(d, DIRECTOR_MAGIC);
CAST_OBJ_NOTNULL(bp, d->priv, BACKEND_MAGIC);
CHECK_OBJ_NOTNULL(d->vdir, VCLDIR_MAGIC);
ah = d->vdir->admin_health;

// sick == 0 for _noprobe
if (ah == VDI_AH_SICK || (ah == VDI_AH_AUTO && bp->sick))
vbe_connwait_signal_all(bp);
}

/*--------------------------------------------------------------------
*/
Expand All @@ -676,7 +683,8 @@ static const struct vdi_methods vbe_methods[1] = {{
.destroy = vbe_destroy,
.panic = vbe_panic,
.list = vbe_list,
.healthy = vbe_healthy
.healthy = vbe_healthy,
.notify = vbe_notify
Comment on lines 683 to +687
Copy link
Member

Choose a reason for hiding this comment

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

I was bothered by the notify method because this is a breaking change as we established earlier and I wasn't satisfied with such a resolution on the 7.6 branch where we'd rather not break VRT if we can help it.

It then occurred to me that in spirit the new notify method is very close to the event method and I was wondering whether we'd agree to the creation of a new going-sick event even though that was not the original scope of the event enum.

I don't personally see a strong reason against widening the scope beyond the existing set of events.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that yes, no or something else, @dridi ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, phk also brought up using the .event method, is this what you mean? I can look at it, but it did not seem natural to me.

Copy link
Member

Choose a reason for hiding this comment

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

I was suggesting adding a new enum entry for the event callback, one that would be dedicated to backend/directors, instead of adding a new notify callback.

Copy link
Member Author

Choose a reason for hiding this comment

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

FTR: I did consider using the .event() method, but it seemed confusing to me because it is tied to the VCL temperature / lifetime. But as both you and phk do not share this concern, I will implement this to have something to look at.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

}};

static const struct vdi_methods vbe_methods_noprobe[1] = {{
Expand All @@ -689,7 +697,8 @@ static const struct vdi_methods vbe_methods_noprobe[1] = {{
.event = vbe_dir_event,
.destroy = vbe_destroy,
.panic = vbe_panic,
.list = vbe_list
.list = vbe_list,
.notify = vbe_notify
}};

/*--------------------------------------------------------------------
Expand Down
6 changes: 2 additions & 4 deletions bin/varnishd/cache/cache_backend_probe.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,8 @@ VBP_Update_Backend(struct vbp_target *vt)
i = (vt->good < vt->threshold);
chg = (i != vt->backend->sick);
vt->backend->sick = i;
if (i && chg && (vt->backend->director != NULL &&
VBE_is_ah_auto(vt->backend))) {
VBE_connwait_signal_all(vt->backend);
}
if (i && chg && vt->backend->director != NULL)
VRT_Notify(vt->backend->director);

AN(vt->backend->vcl_name);
VSL(SLT_Backend_health, NO_VXID,
Expand Down
24 changes: 17 additions & 7 deletions bin/varnishd/cache/cache_director.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,22 @@ VRT_SetChanged(VCL_BACKEND d, VCL_TIME changed)
d->vdir->health_changed = changed;
}

/*--------------------------------------------------------------------
* Notify of change (admin_health for now) outside backend
*/

VCL_VOID
VRT_Notify(VCL_BACKEND d)
{

CHECK_OBJ_NOTNULL(d, DIRECTOR_MAGIC);
CHECK_OBJ_NOTNULL(d->vdir, VCLDIR_MAGIC);
CHECK_OBJ_NOTNULL(d->vdir->methods, VDI_METHODS_MAGIC);
if (d->vdir->methods->notify == NULL)
return;
d->vdir->methods->notify(d);
}

/* Send Event ----------------------------------------------------------
*/

Expand Down Expand Up @@ -477,7 +493,6 @@ static int v_matchproto_(vcl_be_func)
do_set_health(struct cli *cli, struct director *d, void *priv)
{
struct set_health *sh;
struct vrt_ctx *ctx;

(void)cli;
CHECK_OBJ_NOTNULL(d, DIRECTOR_MAGIC);
Expand All @@ -488,12 +503,7 @@ do_set_health(struct cli *cli, struct director *d, void *priv)
if (d->vdir->admin_health != sh->ah) {
d->vdir->health_changed = VTIM_real();
d->vdir->admin_health = sh->ah;
ctx = VCL_Get_CliCtx(0);
if (sh->ah == VDI_AH_SICK || (sh->ah == VDI_AH_AUTO &&
d->vdir->methods->healthy != NULL &&
!d->vdir->methods->healthy(ctx, d, NULL))) {
VBE_connwait_signal_all(d->priv);
}
VRT_Notify(d);
}
return (0);
}
Expand Down
4 changes: 0 additions & 4 deletions bin/varnishd/cache/cache_varnishd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,7 @@ void VCA_Init(void);
void VCA_Shutdown(void);

/* cache_backend.c */
struct backend;

void VBE_InitCfg(void);
void VBE_connwait_signal_all(const struct backend *bp);
int VBE_is_ah_auto(const struct backend *bp);

/* cache_ban.c */

Expand Down
10 changes: 9 additions & 1 deletion include/vrt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
# error "include vdef.h before vrt.h"
#endif

#define VRT_MAJOR_VERSION 20U
#define VRT_MAJOR_VERSION 21U

#define VRT_MINOR_VERSION 0U

Expand All @@ -58,6 +58,9 @@
* binary/load-time compatible, increment MAJOR version
*
* NEXT (2024-03-15)
* 21.0
* notify callback added to struct vdi_methods
* VRT_Notify() added
* 20.0 (2024-09-13)
* struct vrt_backend.backend_wait_timeout added
* struct vrt_backend.backend_wait_limit added
Expand Down Expand Up @@ -737,6 +740,7 @@ typedef void vdi_release_f(VCL_BACKEND);
typedef void vdi_destroy_f(VCL_BACKEND);
typedef void vdi_panic_f(VCL_BACKEND, struct vsb *);
typedef void vdi_list_f(VRT_CTX, VCL_BACKEND, struct vsb *, int, int);
typedef void vdi_notify_f(VCL_BACKEND);

struct vdi_methods {
unsigned magic;
Expand All @@ -755,6 +759,9 @@ struct vdi_methods {
vdi_destroy_f *destroy;
vdi_panic_f *panic;
vdi_list_f *list;
// when something changes outside the backend's control
// (for now, admin health)
vdi_notify_f *notify;
};

struct director {
Expand All @@ -767,6 +774,7 @@ struct director {
};

VCL_BOOL VRT_Healthy(VRT_CTX, VCL_BACKEND, VCL_TIME *);
VCL_VOID VRT_Notify(VCL_BACKEND);
VCL_VOID VRT_SetChanged(VCL_BACKEND, VCL_TIME);
VCL_BACKEND VRT_AddDirector(VRT_CTX, const struct vdi_methods *,
void *, const char *, ...) v_printflike_(4, 5);
Expand Down