Fix order of COLD events for directors vs. VCL (and VMODs, implicitly)

[nigoroll]

@delthas reported an interesting issue with vmod_dynamic: When the vmod receives a cold event, it might have some directors left to delete, and when it deletes them, they do not receive a cold event, and a follow up assertion triggers because a warm director is deleted.

I believe the root cause is actually a wrong ordering in Varnish-Cache:

vcldir_retire() only sends a VCL_EVENT_COLD when the vcl is warm. In other words, it (rightly, IMHO) asserts that a COLD event (if any) has alrady been posted when a director is deleted on a COLD vcl.

Yet, when the director is deleted upon a COLD vmod event, this assertion was wrong, because the COLD events for directors were only posted after vmod events.

Given that vmods do things like deleting directors, it appears (more) correct to post VDI COLD events before VMOD COLD events.

[dridi]

On paper I think this change looks good and it's certainly something we could quickly merge to solve the problem this addresses.

I thought this was going to be short review, but revisiting the VCL event logic I'm wondering whether the semantics should be clarified again. Before this change it looks like we are marking the VCL as cold and then propagating the cold events further down and I think that in general that's a sane behavior.

On the other hand it looks like we do the same for warm events: we make the VCL warm and then propagate the warm signal. I feel like it should be the other way around: once everything is warm, "commit" the warmup transaction at the VCL level. Of course if the VMODs and VBEs can't warm up without a warm VCL, then it is inappropriate.

Of course this is not something we can just do without making sure that we don't break the loose contract we currently have with VMODs. Maybe vcl_send_event() should be in charge of the dispatch.

We could start by encoding the desired behavior with types:

static const struct vcl_event {
        const char      *name;
        int             forward; /* before < 0, never == 0, after > 0 */
        unsigned        method;
        unsigned        have_msg;
        // ...
} vcl_events[] = {{
        // ...
}};

static const struct vcl_event *
vcl_event(enum vcl_event_e ev)
{
        // assert
        return (&vcl_events[ev]);
}

And with something like this we could nail down the forward logic and semantics in one central place.

[nigoroll]

I feel like it should be the other way around: once everything is warm, "commit" the warmup transaction at the VCL level.

I looked into a similar question before adding the PR, and in general I agree that the current logic looks inconsistent: We mark the VCL warm before it is, and we mark it cold before it is.

But running git grep -C 5 -E -- '->is_(warm|cold)' shows that, besides assertions, the warm/cold attribute is used for exactly two purposes: VSC hiding and the VDI_Event() discussed here. And I think that for both purposes the current implementation is suitable, except for the ordering issue raised herein.

I think that any proposal to complicate this logic should be motivated by relevant use cases. For this reason and my expectation of canned worms, I have not thought about your proposal in more detail yet.

[bsdphk]

I think the ordering should be reversed, but it's obviously something which is badly underdocumented.

[nigoroll]

FTR, OKed by bugwash