Backend mode in varnishncsa #1905

hermunn · 2016-04-14T15:12:41Z

A new mode, backend mode, in varnishncsa is introduced. It lets you
log trafic from varnish to the backends. Typically you will run this
in addition to varnishncsa in normal mode.

gquintard · 2016-04-14T15:14:24Z

bin/varnishncsa/varnishncsa.c

-		if (t->type != VSL_t_req)
-			/* Only look at client requests */
+		/* In client mode, only look at client requests.
+		   In bacend mode, only look at backend requests. */


missing 'k'

fgsch · 2016-04-14T15:35:34Z

I like this but I think we should add support for both -b and -c, and let the user decide without forcing them to run 2 separate processes. This is inline with the rest of the tools.

Having to run multiple varnishncsa instances is not ideal from the admin POV.
If they really want to have separate logs, they can specifying the correct parameter, but they will have to deal with the management headache.

For this to make more sense we'll need to add a format to log whether this is "b"ackend or "c"lient, much as we do in varnishlog for example.

fgsch · 2016-04-16T16:20:46Z

bin/varnishncsa/varnishncsa.c

@@ -134,8 +135,8 @@ struct ctx {

 	/* State */
 	struct watch_head	watch_vcl_log;
-	struct watch_head	watch_reqhdr;
-	struct watch_head	watch_resphdr;
+	struct watch_head	watch_reqhdr; /* also breqhdr */


typo. should be bereqhdr

fgsch · 2016-04-18T09:53:28Z

2 wrongs don't make one right. I'd take code duplication any time over unreadable code.
My comment does not apply to that particular case either, but all of them.

Also, why there are these if .. break when you have the:

+       if ((t->type != VSL_t_req && !CTX.b_opt)
+           || (t->type != VSL_t_bereq && CTX.b_opt))

hermunn · 2016-04-18T11:07:32Z

If I were not to safeguard against bad behavior, then all the if() break; statements would go away. I made a choice, and If the consensus is that it was the wrong one, then I will change it. I guess what I am trying to say is that someone (probably @mbgrydeland) should just decide how it should be, so that I can implement it and we can move on to something else.

hermunn · 2016-04-19T07:10:13Z

I have thought a lot about this, and think I have found a way to make everyone happy when it comes to readability, maintainability and function. I will update the pull request "soon", and then everyone can look at it again.

However, I need to go bug hunting a bit first, on a different part of the code.

hermunn · 2016-04-20T09:54:34Z

I have now slightly rewritten the most problematic code, and implemented mixed mode (both client and backend requests in the same log). I hope the code is easier to read, easier to maintain, and that the behavior of the old varnishncsa is kept exactly.

The switch construction is a bit hackish, but I hope you will like it anyway.

fgsch · 2016-04-20T10:28:06Z

bin/varnishncsa/varnishncsa.c

+		} else if (t->type == VSL_t_bereq && CTX.b_opt) {
+			CTX.side = "backend";
+			be_mark = BACKEND_MARKER;
+		} else continue;


Why not simply?

if ((t->type == VSL_t_req && !CTX.c_opt) || (t->type == VSL_t_bereq && !CTX.b_opt)) continue;

Also perhaps use "c" and "b" to be in sync with other tools, e.g. varnishlog?

"c" and "b" is probably better, yes. Thanks.

The if test

if ((t->type == VSL_t_req && !CTX.c_opt) || (t->type == VSL_t_bereq && !CTX.b_opt)) continue;

does not work when both b_opt and c_opt are set. Also, similar constructions would remove safety against "impossible" combinations like SLT_PipeAcct when t->type == VSL_t_bereq . In the current version this bad behavior from varnish would be ignored, and I want to keep the current behavior in the new version.

In varnishncsa, the variable CTX already had an a_opt member, so for the two other options, b_opt and c_opt was chosen.

Not sure what you meant.

does not work when both b_opt and c_opt are set. Also, similar constructions would remove safety against "impossible" combinations like SLT_PipeAcct when t->type == VSL_t_bereq . In the current version this bad behavior from varnish would be ignored, and I want to keep the current behavior in the new version.

Ugh? How come it will not work?
I don't think the impossible scenarios justify the extra code. If you are concerned about SLT_PipeAcct you can check that particular one. We have different records (e.g. ReqHeader vs BereqHeader) for that specific purpose.
The code does not handle backend records in client requests either.

fgsch · 2016-04-20T11:16:41Z

In varnishncsa, the variable CTX already had an a_opt member, so for the two other options, b_opt and c_opt was chosen.

Not sure what you meant.

does not work when both b_opt and c_opt are set. Also, similar constructions would remove safety against "impossible" combinations like SLT_PipeAcct when t->type == VSL_t_bereq . In the current version this bad behavior from varnish would be ignored, and I want to keep the current behavior in the new version.

Ugh? How come it will not work?
I don't think the impossible scenarios justify the extra code. If you are concerned about SLT_PipeAcct you can check that particular one. We have different records (e.g. ReqHeader vs BereqHeader) for that specific purpose.

hermunn · 2016-04-20T11:44:44Z

Not sure what you meant.

I misunderstood, but Dag cleared it up for me, so I changed the reply. You were right all along about "c" and "b" being better than "client" and "backend".

Ugh? How come it will not work?

if t->type is neither VSL_t_req nor VSL_t_bereq, then test should evaluate to true, but that did not seem to be the case in the quoted code.

The code does not handle backend records in client requests either.

If a client request is coming in, then be_mark will be 0, and then tag == SLT_BereqHeader will be ignored since there is no case SLT_BereqHeader, only case (SLT_BereqHeader + BACKEND_MARK). But maybe I misunderstood again?

I just looked at the code immediately below the switch, and realized that

            if ((tag == SLT_ReqHeader && CTX.c_opt)
                || (tag == SLT_BereqHeader && CTX.b_opt))
                process_hdr(&CTX.watch_reqhdr, b, e);
            if ((tag == SLT_RespHeader && CTX.c_opt)
                || (tag == SLT_BerespHeader && CTX.b_opt))
                process_hdr(&CTX.watch_resphdr, b, e);

is wrong, and I should consider t->type instead of CTX.c_opt and CTX.b_opt.

fgsch · 2016-04-20T12:16:44Z

if t->type is neither VSL_t_req nor VSL_t_bereq, then test should evaluate to true, but that did not seem to be the case in the quoted code.

varnishncsa only support vxid or request grouping mode. No other records are possible IIRC.

mbgrydeland · 2016-04-22T10:55:55Z

bin/varnishncsa/varnishncsa.c

 	(void)vsl;
 	(void)priv;

+#define BACKEND_MARKER VSL_t__MAX


I liked this better when BACKEND_MARKER was waaay higher than VSL_t__MAX. If the varnishlog and varnishd binaries are out of sync (not completely unlikely if e.g. one considers a log sent to support), then there can be strangeness observed.

hermunn · 2016-04-22T12:55:36Z

Pushing again, after editing according to @mbgrydeland's wishes...

This log record is a counterpart to the SLT_ReqStart for backends, and shows the socket endpoint we are talking to at the start of backend requests. Today this information is only logged on opening a backend connection (SLT_BackendOpen), and is thus not available on the backend log transaction when on a reused backend connection. We need this information to complete the backend view varnishncsa extension.

A new mode, backend mode, in varnishncsa is introduced. It lets you log trafic from varnish to the backends. The normal (default) mode is now named "client mode" and can be explicitly selected using -c. You can run varnishncsa in backend mode in addition to varnishncsa in client mode, but it is also possible to run in mixed mode by specifying both -b and -c. New formatter added: %{Varnish:side}x will be either "b" or "c", depending on where the request was made.

hermunn · 2016-04-25T10:48:21Z

Now @mbgrydeland has made a patch that adds logging in varnishd, necessary to log the IP of the backend correctly, and this is now part of this pull request (first commit). The second commit is the backend mode in varnishncsa.

hermunn · 2016-04-25T14:16:29Z

Merged into master and 4.1 manually (fast forward).

lkarsten · 2016-05-30T12:01:20Z

Backport review: In 4.1 as 5a46b8c and 3b07876. Will be in next VC4.1 release.

This is new functionality and as such not for 4.0.

Ref: #1905

gquintard reviewed Apr 14, 2016
View reviewed changes

hermunn force-pushed the master-backend-mode-varnishncsa branch from 534d56a to 317bbc6 Compare April 14, 2016 15:39

fgsch reviewed Apr 16, 2016
View reviewed changes

hermunn force-pushed the master-backend-mode-varnishncsa branch from 317bbc6 to a8c7e26 Compare April 18, 2016 08:12

hermunn force-pushed the master-backend-mode-varnishncsa branch from a8c7e26 to c8f5e6a Compare April 20, 2016 09:50

hermunn force-pushed the master-backend-mode-varnishncsa branch from c8f5e6a to 245f93e Compare April 20, 2016 09:57

fgsch reviewed Apr 20, 2016
View reviewed changes

hermunn force-pushed the master-backend-mode-varnishncsa branch from 245f93e to 154fda0 Compare April 20, 2016 11:12

mbgrydeland reviewed Apr 22, 2016
View reviewed changes

hermunn force-pushed the master-backend-mode-varnishncsa branch from 154fda0 to 8c6086c Compare April 22, 2016 12:56

mbgrydeland and others added 2 commits April 25, 2016 11:39

hermunn force-pushed the master-backend-mode-varnishncsa branch from 8c6086c to eb6ed80 Compare April 25, 2016 10:44

hermunn closed this Apr 25, 2016

lkarsten added backport-review-4.0 and removed backport-review-4.0 labels May 24, 2016

lkarsten removed the backport-review-4.1 label May 30, 2016

lkarsten pushed a commit that referenced this pull request May 30, 2016

Add varnishncsa backend mode to changelog.

ccf8775

Ref: #1905

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backend mode in varnishncsa #1905

Backend mode in varnishncsa #1905

hermunn commented Apr 14, 2016

gquintard Apr 14, 2016

fgsch commented Apr 14, 2016

fgsch Apr 16, 2016

fgsch commented Apr 18, 2016

hermunn commented Apr 18, 2016 •

edited

hermunn commented Apr 19, 2016

hermunn commented Apr 20, 2016

fgsch Apr 20, 2016

fgsch Apr 20, 2016

hermunn Apr 20, 2016 •

edited

hermunn Apr 20, 2016

fgsch Apr 20, 2016

fgsch commented Apr 20, 2016 •

edited

hermunn commented Apr 20, 2016

fgsch commented Apr 20, 2016 •

edited

mbgrydeland Apr 22, 2016

hermunn commented Apr 22, 2016

hermunn commented Apr 25, 2016

hermunn commented Apr 25, 2016

lkarsten commented May 30, 2016

Backend mode in varnishncsa #1905

Backend mode in varnishncsa #1905

Conversation

hermunn commented Apr 14, 2016

gquintard Apr 14, 2016

Choose a reason for hiding this comment

fgsch commented Apr 14, 2016

fgsch Apr 16, 2016

Choose a reason for hiding this comment

fgsch commented Apr 18, 2016

hermunn commented Apr 18, 2016 • edited

hermunn commented Apr 19, 2016

hermunn commented Apr 20, 2016

fgsch Apr 20, 2016

Choose a reason for hiding this comment

fgsch Apr 20, 2016

Choose a reason for hiding this comment

hermunn Apr 20, 2016 • edited

Choose a reason for hiding this comment

hermunn Apr 20, 2016

Choose a reason for hiding this comment

fgsch Apr 20, 2016

Choose a reason for hiding this comment

fgsch commented Apr 20, 2016 • edited

hermunn commented Apr 20, 2016

fgsch commented Apr 20, 2016 • edited

mbgrydeland Apr 22, 2016

Choose a reason for hiding this comment

hermunn commented Apr 22, 2016

hermunn commented Apr 25, 2016

hermunn commented Apr 25, 2016

lkarsten commented May 30, 2016

hermunn commented Apr 18, 2016 •

edited

hermunn Apr 20, 2016 •

edited

fgsch commented Apr 20, 2016 •

edited

fgsch commented Apr 20, 2016 •

edited