Re: [Xen-devel] [RFC] Shutdown event script for xl toolstack

[Joseph Glanville <joseph.glanville@xxxxxxxxxxxxxx>]

On 1 April 2012 09:13, Joseph Glanville <joseph.glanville@xxxxxxxxxxxxxx> wrote:
> On 31 March 2012 05:08, Joseph Glanville
> <joseph.glanville@xxxxxxxxxxxxxx> wrote:
>> On 31 March 2012 01:03, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:
>>> Joseph Glanville writes ("[RFC] Shutdown event script for xl toolstack"):
>>>> Previously I had a series of out of tree patches to xend that added a
>>>> shutdown event callback on domain death that would call a script with
>>>> the reason why the domain shutdown.
>>>> Basically analagous to this:
>>>> /etc/xen/scripts/shutdown-event $event $domname $domid
>>>
>>> I think this is a good idea, although it shouldn't be enabled by
>>> default and I don't see the need to provide an example script.
>>
>> Yep fair enough, agree on both points.
>>
>>>
>>>> My general idea is to add a config option for a path to an event
>>>> handler script:
>>>> on_shutdown_event = "/etc/xen/scripts/shutdown-event"
>>>
>>> Yes, this is good.  Shouldn't the argument be a string list so that
>>> you can specify all the arguments to exec() ?  Will the script inherit
>>> the domid or name in an environment variable ?
>>>
>>> You need to document this fully I think, in docs/man/xl.cfg.pod.5.
>>
>> I didn't think of this but you make a very valid point, especially
>> considering we can use environment vars to provide the domain data as
>> you suggested.
>>
>>>
>>> It would also be good for the script to be able to give instructions
>>> to the daemonic xl, for example to cause the daemonic xl to neither
>>> reboot nor preserve the domain.  So if you wanted your domain to get a
>>> different config when it reboots, you write a hook script which
>>> destroys the previous domain and starts the new one by hand.
>>
>> Hmm this is interesting and something I hadn't really thought of 
>> implementing.
>> Something that uses the return code of the script I think would be 
>> appropriate.
>>
>>>
>>>> Create the event during handle_domain_death in xl_cmdimpl.c and fire
>>>> the script once shutdown reason and action has been parsed.
>>>
>>> When you say "create the event" what do you mean ?
>>
>> I think "create" was most definitely incorrect terminology. :)
>> "catch" probably would have been a much better way of terming it now I
>> look at it.
>>
>>>
>>>> I implemented a hacky version to illustrate my point but I would like
>>>> some advice on how to do this properly and what tools/framework within
>>>> libxl I could leverage to make it cleaner.
>>>
>>> This is going in the right direction.  I'll comment in more detail
>>> below.
>>>
>>>> A quick overview of the following would help immensely:
>>>> Where to add in a new config option and what steps it goes through to
>>>> get to libxl_domain_config.
>>>
>>> This hook script functionality should be part of xl, not part of
>>> libxl, so arguably you shouldn't add it to libxl_domain_config.
>>>
>>> Indeed I think it's arguably a mistake that the on_{poweroff,...}
>>> items are in libxl_domain_config rather than in something provided in
>>> xl.
>>>
>>> The config parsing is done in parse_config_data.
>>
>> Cheers will take a look.
>>
>>>
>>>> How to exec an external script safely, can I use usual fork/exec or
>>>> should I be using libxl__exec or libxl_spawn*?
>>>
>>> In xl I think you should use fork/exec.  You may not use any functions
>>> libxl__* as they are private for libxl.
>>
>> Thanks for clarifying, I also read the naming convention docs this
>> morning which cleared this up too.
>>
>>>
>>>> Best place/way to get the event data, atm handle_domain_death looks
>>>> like an easy target but there might be more elegant ways..
>>>
>>> handle_domain_death is called in only one place, the event loop in the
>>> daemonic xl.  And yes, it seems like the right place to do this.
>>>
>>>> diff --git a/tools/hotplug/Linux/shutdown-event
>>>> b/tools/hotplug/Linux/shutdown-event
>>>
>>> As I say I don't think we need an example like this.
>>>
>>> Also do we think asking the user to handle this with a switch in their
>>> event script is really better than providing four separate config
>>> options for separate commands ?  Doing the latter will make the
>>> scripts simpler, which is good because they'll be very annoying to
>>> debug.
>>
>> Agreed, that is a much better approach.
>>
>>>
>>>> +    char event_name[10];
>>>> +    char event_cmd[100];
>>>>
>>>>     switch (info->shutdown_reason) {
>>>>     case SHUTDOWN_poweroff:
>>>>         action = d_config->on_poweroff;
>>>> +        strcpy(event_name,"poweroff");
>>>
>>> Surely event_name should be
>>>  const char *event_name;
>>> and then you can do
>>>  event_name = "poweroff";
>>>
>>> But this goes away if you have four separate hook script config
>>> options.
>>>
>>>> +    snprintf(event_cmd, 100, SHUTDOWN_EVENT_SCRIPT, event_name,
>>>> d_config->c_info.name, domid);
>>>> +    ret = system(event_cmd);
>>>
>>> This part needs serious work.  We need firstly to define an
>>> interface.  And, I don't really like system().  You should be using
>>> fork/exec.
>>
>> Yep, now that has been clarified I will work on something cleaner.
>> I think taking the array of string approach in the config file and
>> then parsing that array verbatim as the args to exec is a great idea.
>>
>>>
>>> Thanks,
>>> Ian.
>>
>> I will refactor this into a v1 patch after I work out the config file
>> stuff, sensible script interface and return values etc.
>>
>> Thanks for all of your help!
>>
>> Joseph.
>>
>> --
>> Founder | Director | VP Research
>> Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56
>> 99 52 | Mobile: 0428 754 846
>
> Hi Ian,
>
> I have a few more questions.
>
> As I have become more acquainted with the codebase I can see the line
> between libxl and xl quite clearly.
> However the config file territory seems somewhat ambiguous.
> Domain configuration is stored in libxl_domain_config which is a part
> of libxl but there isn't currently a structure for data that is
> private to xl, that is xl daemon or utility specific configuration.
> Is it considered bad form to add a configuration variable to
> libxl_domain_config that only xl will benefit from? if so what is the
> best course of action here?
> I could create another structure for private data but I feel seeking
> guidance on this as prudent.
>
> In the meantime I had added it to libxl_domain_config but I intend to
> have found/made a cleaner solution before submitting the patch for
> inclusion.
>
> In terms of interface I have come up with what I think is inherently
> simple and reliable.
> The shutdown_event_handler is executed with DOM_ID, DOM_NAME, ACTION
> and REASON in it's environment.
> The return code stipulates what action xl will then take, in
> correspondence with the LIBXL_ACTION_ON_SHUTDOWN* counterparts.
>
> I am yet to document this in the xl.cfg.pod file, I will do so in the
> next revision along with adding support for arguments - once we have
> where we are going to store them sorted out at least.
> Patch is only compile tested at this time, I am away from my testing
> environment atm.
>
> Thanks in advance and kind regards,
> Joseph.
>
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 6b69030..b3e5fb1 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -370,6 +370,8 @@ typedef struct {
>     libxl_action_on_shutdown on_reboot;
>     libxl_action_on_shutdown on_watchdog;
>     libxl_action_on_shutdown on_crash;
> +
> +    char *shutdown_event_handler;
>  } libxl_domain_config;
>  char *libxl_domain_config_to_json(libxl_ctx *ctx, libxl_domain_config *p);
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 1d59b89..1096f23 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -664,6 +664,10 @@ static void parse_config_data(const char
> *configfile_filename_report,
>         exit(1);
>     }
>
> +    if (xlu_cfg_replace_string (config, "shutdown_event_handler",
> +                            &d_config->shutdown_event_handler, 0))
> +        d_config->shutdown_event_handler = NULL;
> +
>     /* libxl_get_required_shadow_memory() must be called after final values
>      * (default or specified) for vcpus and memory are set, because the
>      * calculation depends on those values. */
> @@ -1206,6 +1210,63 @@ skip_vfb:
>     xlu_cfg_destroy(config);
>  }
>
> +static int xl_exec(const char *arg0, char **args, char **envp,
> +                   useconds_t timeout)
> +{
> +    int status;
> +    useconds_t sleep_time = 10, wait_time = 0;
> +    pid_t child_pid, wpid;
> +
> +    child_pid = fork();
> +    if (child_pid < 0) {
> +        LOG("Failed to fork in xl_exec");
> +        exit(-1);
> +    } else if (child_pid == 0) {
> +        execvpe(arg0,args,envp);
> +    } else {
> +        do {
> +            wpid = waitpid(child_pid, &status, WNOHANG);
> +            if (wpid == 0) {
> +                if (wait_time < timeout) {
> +                    usleep(sleep_time);
> +                    wait_time += sleep_time;
> +                } else {
> +                    kill(child_pid, SIGKILL);
> +                }
> +            }
> +        } while (wpid == 0 && wait_time <= timeout);
> +
> +        if (WIFSIGNALED(status)) {
> +            return WEXITSTATUS(status);
> +        }
> +    }
> +    return WTERMSIG(status);
> +}

Ahh I fumbled it. Obviously WTERMSIG and WEXITSTATUS should be
reversed here. No idea how I missed this on my last glance over.

> +
> +static int user_shutdown_action(libxl_domain_config *d_config, uint32_t 
> domid,
> +                         int action, unsigned shutdown_reason)
> +{
> +    int ret;
> +    const char* arg0 = d_config->shutdown_event_handler;
> +    char* argv[] = {NULL};
> +    char* envp[4];
> +    useconds_t timeout = 1000;
> +
> +    asprintf(&envp[0],"DOM_ID=%d", domid);
> +    asprintf(&envp[1],"DOM_NAME=%s", d_config->c_info.name);
> +    asprintf(&envp[2],"ACTION=%d", action);
> +    asprintf(&envp[3],"REASON=%u", shutdown_reason);
> +    envp[4] = NULL;
> +
> +    ret = xl_exec(arg0,argv,envp,timeout);
> +
> +    if ((ret < 0) || ( ret > 6)) {
> +        LOG("Invalid shutdown action requested: %d, ignoring",ret);
> +        return action;
> +    }
> +    return ret;
> +}
> +
>  /* Returns 1 if domain should be restarted,
>  * 2 if domain should be renamed then restarted, or 0 */
>  static int handle_domain_death(libxl_ctx *ctx, uint32_t domid,
> @@ -1237,6 +1298,10 @@ static int handle_domain_death(libxl_ctx *ctx,
> uint32_t domid,
>         action = LIBXL_ACTION_ON_SHUTDOWN_DESTROY;
>     }
>
> +       if (d_config->shutdown_event_handler)
> +        action = user_shutdown_action(d_config, domid, action,
> +
> event->u.domain_shutdown.shutdown_reason);
> +
>     LOG("Action for shutdown reason code %d is %s",
>         event->u.domain_shutdown.shutdown_reason,
>         action_on_shutdown_names[action])
>
> --
> Founder | Director | VP Research
> Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56
> 99 52 | Mobile: 0428 754 846



-- 
Founder | Director | VP Research
Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56
99 52 | Mobile: 0428 754 846

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel