|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |