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.

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 tell/ear interface for CLI access to vmod objects #3729

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nigoroll
Copy link
Member

@nigoroll nigoroll commented Nov 3, 2021

Runtime modification of vmod object properties has been a long standing item on our wishlist. For example, #3652 is about a use case to change a custom director property, which is not covered by the director health state. Another simple example is to instruct a vmod object to emit log messages for tracing only when needed.

This commit implements a basic interface for CLI access to vmod objects:

VMOD objects now can have ears, and the CLI gets a command to tell messages to ears.

vmod ears

VMOD object classes gain a special method type $Ear, which is almost identical to $Method, except that only one ear is supported per class, and only the specific signature

$Ear STRANDS earname(STRANDS)

is supported.

The ear receives input via the single STRANDS argument and can either return NULL for error, or a response as a STRANDS.

For the error case, the ear can write details to ctx->msg.

cli tell command

The tell command takes an optional vcl name, object name and message to send. Individual message arguments are passed as constituents of the STRANDS argument to the ear.

demo

A new test case demos the functionality: The debug.obj class has gained an ear which just returns the instance name followed by the original message:

varnish> help tell
200
tell [<vcl>.]<object> <msg> ...
Tell <msg> to <object> from the given <vcl> or the active vcl
varnish> tell obj0 is there anybody out there?
200
obj0: is there anybody out there?
varnish> tell whoisit hello?
300
No object named whoisit found
varnish> tell obj0 fail
300
You asked me to fail

@nigoroll
Copy link
Member Author

nigoroll commented Nov 8, 2021

force push: moved from VRT to VPI

@nigoroll
Copy link
Member Author

nigoroll commented Nov 8, 2021

TODOs based on feedback from @dridi and @bsdphk :

  • boringification-rename $Ear -> $Cli
  • So the ear is going to be renamed to "cli method"
  • change cli method return value to integer
  • return string in vsb argument both for success and failure

@nigoroll
Copy link
Member Author

nigoroll commented Nov 9, 2021

I have implemented the TODOs and force pushed.
The commit message of 0f53a0b reflects the new naming:

  • vmod_cli_f for the cli method type
  • SYM_CLI_METHOD for the vcc symbol
  • $Cli for the vcc file stanza

RST generation in vmodtool is still todo once we have agreement on the current PR.

Copy link
Member

@dridi dridi left a comment

Choose a reason for hiding this comment

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

I like this iteration much better already, and I have a few comments.

Comment on lines +1010 to +1040
if (objname) {
n = strndup(av[2], objname - av[2]);
objname++;
Copy link
Member

Choose a reason for hiding this comment

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

AN(n);

Comment on lines +91 to +107
if (s->n > 0 && ! strcmp(s->p[0], "fail")) {
VSB_cat(ctx->msg, "You asked me to fail");
return (300);
}
VSB_cat(ctx->msg, o->vcl_name);
VSB_putc(ctx->msg, ':');

for (i = 0; i < s->n; i++) {
VSB_putc(ctx->msg, ' ');
VSB_cat(ctx->msg, s->p[i]);
}

return (200);
Copy link
Member

Choose a reason for hiding this comment

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

Should we expose (or even restrict to) a subset of the VCLI status constants to VMODs?

Comment on lines +811 to +839
if self.cli is not None:
self.cli.json(ll)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe make $CLI unconditional like $INIT and $FINI to simplify the libvcc side.

Comment on lines +190 to +313
/*--------------------------------------------------------------------
* tell interface.
*
* we all agree it does not quite fit the purpose of VPI, but it fits here
* better than anywhere else
*
* XXX: Replace instance info with a tree indexed by name
*/
Copy link
Member

Choose a reason for hiding this comment

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

It probably belongs to cache_vcl.c, just like sending events happens from there, alongside vcl_cli_tell().

@bsdphk
Copy link
Contributor

bsdphk commented Nov 22, 2021

I think this is fundamentally a good idea.

I think this PR is a competent prototype that shows it is possible.

I am also pretty certain this is not how the final solution will look.

For one thing, object instances are not the only things we want to tell things:

vcl.tell blavcl.vmod_im_debugging debug_level 7

People have also been wanting to frob probes and backends without reloading VCL's from day one.

So I suggest the next step is to write the proposed user documentation for this feature, including examples, because that is a great way to find weaknesses in the design.

@nigoroll
Copy link
Member Author

nigoroll commented Nov 22, 2021

The existing documentation is here and in the commit message. I did not write documentation for vmod authors yet because the interface should be settled first.

My understanding from pow wow is that we should have proposed documentation for how this feature would look like when it has evolved further. My suggestion to that effect would be this (reflecting the change of the command to vcl.tell):

vcl.tell [<vcl>.]<thing> <msg> ...
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  Tell <msg> to <thing> from the given <vcl> or the active vcl

  <thing> can be any named object known to <vcl> like probes, backends, vmod 
  objects or vmods.

  Both <vcl> and <thing> can contain wildcards. When wildcards are used, status 
  codes >= 300 are ignored and the otherwise highest status code from any invoked 
  method is returned.

  If no match is found for <vcl> or <thing>, an error with status 300 is returned.

  Not all things are required to implement the tell interface. If they do, it is entirely up 
  to the respective implementation to interpret <msg>, implement any actions and 
  generate a response. Likewise, documentation of the supported <msg> will be 
  found with the documentation of <thing>.

  By convention, implementations of tell interfaces should:

  * support the "ping" command and respond with "pong"
  * interpret the first element of <msg> as a command
  * prefix supported command names by their vmod name
  * generate an error message for commands which are not understood

Examples:
---------

varnish> vcl.tell obj0 is there anybody out there?
200
obj0: is there anybody out there?

varnish> vcl.tell whoisit hello?
300
No object named whoisit found

varnish> vcl.tell obj0 fail
300
You asked me to fail

varnish> vcl.tell dir_fallback_* fallback.reset
200
dir_fallback_one: reset succeeded
dir_fallback_two: reset succeeded
dir_fallback_three: already on primary backend

varnish> vcl.tell *.* ping
200
vclA.probe1: pong
vclA.backend1: pong
vclA.obj1: pong

@bsdphk
Copy link
Contributor

bsdphk commented Nov 23, 2021

The more I think about this, the more I feel tempted to extend the vcli all the way out there.

For instance, what about help vcl.tell ?

That needs to do more than just say "Say something to something"

help vcl.tell some_vcl should at the very least explain who is listening.

help vcl.tell some_vcl.fooobj should explain what that object is willing to hear

@bsdphk
Copy link
Contributor

bsdphk commented Nov 23, 2021

I've looked at vcli today, with an eye to the "help" question, and I think we can swing that by allowing help functions to emit their own help (marking these with a flag letter).

@dridi
Copy link
Member

dridi commented Jul 10, 2023

Should we put this on hold until the security model question from #3906 is resolved? This ticket should definitely serve as input, I will mention it there as well.

Since you touch on flags and the help command, we have a downstream change that cleans certain things up and streamlines others in this area that we should be able to submit soon-ish.

Runtime modification of vmod object properties has been a long
standing item on our wishlist. For example, varnishcache#3652 is about a use case
to change a custom director property, which is not covered by the
director health state. Another simple example is to instruct a vmod
object to emit log messages for tracing only when needed.

This commit implements a basic interface for CLI access to vmod
objects:

VMOD objects now can have a single $Cli method, and the CLI gets a
command to tell messages by invoking that method.

vmod $Cli method
----------------

VMOD object classes gain a special method type $Cli, which is almost
identical to $Method, except that only one method is supported per
class, and only the specific signature

	$Cli INT cli_method(STRANDS)

is supported.

The cli method receives input via the single STRANDS arguments. It is
expected to write output to ctx->msg and return the CLI status.

cli tell command
----------------

The tell command takes an optional vcl name, object name and message
to send. Individual message arguments are passed as constituents of
the STRANDS argument to the object's cli method.

demo
----

A new test case demos the functionality: The debug.obj class has
gained a cli method which just returns the instance name followed by
the original message:

varnish> help tell
200
tell [<vcl>.]<object> <msg> ...
Tell <msg> to <object> from the given <vcl> or the active vcl

varnish> tell obj0 is there anybody out there?
200
obj0: is there anybody out there?

varnish> tell whoisit hello?
300
No object named whoisit found

varnish> tell obj0 fail
300
You asked me to fail
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants