[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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);
+}
+
+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

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.