[PATCH 0 of 3] (#3) [RFC] Add external migration checks
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1205337335 25200 # Node ID b2355adc325da20f0a613341fa2877a939b1afd5 # Parent 25d3e0aa63f04a2e0231a7b0c7b93ddb5dca6b6e [RFC] Add optional check parameter field to migration schema Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r 25d3e0aa63f0 -r b2355adc325d schema/VSMigrationSettingData.mof --- a/schema/VSMigrationSettingData.mof Wed Mar 12 08:55:33 2008 -0700 +++ b/schema/VSMigrationSettingData.mof Wed Mar 12 08:55:35 2008 -0700 @@ -11,6 +11,7 @@ class Xen_VirtualSystemMigrationSettingD [ ValueMap {"0","1","2","3","4","5","6"}, Values { "Unknown", "Other", "SSH", "TLS", "TLS Strict", "TCP", "UNIX" }] uint16 TransportType; + string CheckParameters[]; }; [Provider("cmpi::Virt_VSMigrationSettingData")] @@ -18,4 +19,5 @@ class KVM_VirtualSystemMigrationSettingD [ ValueMap {"0","1","2","3","4","5","6"}, Values { "Unknown", "Other", "SSH", "TLS", "TLS Strict", "TCP", "UNIX" }] uint16 TransportType; + string CheckParameters[]; };
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1205337336 25200 # Node ID 79615aba657b1860f17573d1cac1729a63eb835e # Parent b2355adc325da20f0a613341fa2877a939b1afd5 [RFC] Add configure support for migration check settings Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r b2355adc325d -r 79615aba657b configure.ac --- a/configure.ac Wed Mar 12 08:55:35 2008 -0700 +++ b/configure.ac Wed Mar 12 08:55:36 2008 -0700 @@ -72,6 +72,20 @@ AC_ARG_WITH(html-dir, [test "x$withval" != "x" && HTML_DIR="$withval"], [HTML_DIR='$(datadir)/doc/$(PACKAGE)-$(VERSION)/html']) AC_SUBST(HTML_DIR) + +AC_ARG_WITH([migrate_check_timeout], + [ --with-migrate_check_timeout=SECS Max runtime allowed for external migration checks], + [test "x$withval" != "x" && MIG_CHECKS_TIMEOUT="$withval"], + [MIG_CHECKS_TIMEOUT=10]) +AC_SUBST(MIG_CHECKS_TIMEOUT) +AC_DEFINE_UNQUOTED(MIG_CHECKS_TIMEOUT, $MIG_CHECKS_TIMEOUT, [External migration check timeout]) + +AC_ARG_WITH([migrate_check_dir], + [ --with-migrate_check_dir=dir Location of external migration checks], + [test "x$withval" != "x" && MIG_CHECKS_DIR="$withval"], + [MIG_CHECKS_DIR=/usr/libexec/extchecks]) +AC_SUBST(MIG_CHECKS_DIR) +AC_DEFINE_UNQUOTED(MIG_CHECKS_DIR, "$MIG_CHECKS_DIR", [External migration check timeout]) # Autogenerate the autoconf header file to store build settings AC_CONFIG_HEADER([config.h])
# HG changeset patch # User Dan Smith <danms@us.ibm.com> # Date 1205337366 25200 # Node ID 39a6d63d6c89d887b68a529a6d7f369c65e6e38b # Parent 79615aba657b1860f17573d1cac1729a63eb835e [RFC] Add external check functionality to CheckIsVSMigratable() Changes: - Fetch the array out of the MSD ourselves to avoid adding a special case to get_msd_values() Signed-off-by: Dan Smith <danms@us.ibm.com> diff -r 79615aba657b -r 39a6d63d6c89 src/Virt_VSMigrationService.c --- a/src/Virt_VSMigrationService.c Wed Mar 12 08:55:36 2008 -0700 +++ b/src/Virt_VSMigrationService.c Wed Mar 12 08:56:06 2008 -0700 @@ -21,6 +21,13 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <sys/types.h> +#include <sys/wait.h> +#include <sys/stat.h> +#include <signal.h> +#include <unistd.h> +#include <dirent.h> +#include <errno.h> #include <uuid/uuid.h> @@ -39,6 +46,8 @@ #include "Virt_VSMigrationService.h" #include "Virt_HostSystem.h" #include "Virt_VSMigrationSettingData.h" + +#include "config.h" #define CIM_JOBSTATE_STARTING 3 #define CIM_JOBSTATE_RUNNING 4 @@ -262,6 +271,258 @@ static CMPIStatus check_hver(virConnectP return s; } +static bool is_valid_check(const char *path) +{ + struct stat s; + + if (stat(path, &s) != 0) + return false; + + if (!S_ISREG(s.st_mode)) + return false; + + if ((s.st_mode & S_IXUSR) || + (s.st_mode & S_IXGRP) || + (s.st_mode & S_IXOTH)) + return true; + else + return false; +} + +static void free_list(char **list, int count) +{ + int i; + + if (list == NULL) + return; + + for (i = 0; i < count; i++) + free(list[i]); + + free(list); +} + +static char **list_migration_checks(int *count) +{ + DIR *dir; + struct dirent *de; + char **list = NULL; + int len = 0; + + *count = 0; + + dir = opendir(MIG_CHECKS_DIR); + if (dir == NULL) { + CU_DEBUG("Unable to open migration checks dir: %s (%s)", + MIG_CHECKS_DIR, + strerror(errno)); + *count = -1; + return NULL; + } + + while ((de = readdir(dir)) != NULL) { + int ret; + char *path = NULL; + + if (de->d_name[0] == '.') + continue; + + if (*count == len) { + char **tmp; + + len = (len * 2) + 1; + tmp = realloc(list, sizeof(char *) * len); + if (tmp == NULL) { + CU_DEBUG("Failed to alloc check list"); + goto error; + } + + list = tmp; + } + + ret = asprintf(&path, + "%s/%s", + MIG_CHECKS_DIR, + de->d_name); + if (ret == -1) { + CU_DEBUG("Failed to alloc path for check"); + goto error; + } + + if (is_valid_check(path)) { + list[*count] = path; + (*count) += 1; + } else { + CU_DEBUG("Invalid check program: %s", path); + free(path); + } + } + + closedir(dir); + + return list; + error: + closedir(dir); + + free_list(list, *count); + *count = 0; + + return NULL; +} + +static CMPIStatus _call_check(virDomainPtr dom, + const char *prog, + const char *param_path) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + pid_t pid; + int i; + int rc = -1; + + pid = fork(); + if (pid == 0) { + virConnectPtr conn = virDomainGetConnect(dom); + const char *name = virDomainGetName(dom); + const char *uri = virConnectGetURI(conn); + + if (setpgrp() == -1) + perror("setpgrp"); + + execl(prog, prog, name, uri, param_path, NULL); + CU_DEBUG("exec(%s) failed: %s", prog, strerror(errno)); + _exit(1); + } + + for (i = 0; i < (MIG_CHECKS_TIMEOUT * 4); i++) { + int status; + if (waitpid(pid, &status, WNOHANG) != pid) { + usleep(250000); + } else { + rc = WEXITSTATUS(status); + break; + } + } + + if (rc == -1) { + CU_DEBUG("Killing off stale child %i", pid); + killpg(pid, SIGKILL); + waitpid(pid, NULL, WNOHANG); + } + + if (rc != 0) { + char *name = strdup(prog); + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Migration check `%s' failed", + basename(name)); + free(name); + } + + return s; +} + +static CMPIStatus call_external_checks(virDomainPtr dom, + const char *param_path) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + char **list = NULL; + int count = 0; + int i; + + list = list_migration_checks(&count); + if (count < 0) { + cu_statusf(_BROKER, &s, + CMPI_RC_ERR_FAILED, + "Unable to execute migration checks"); + goto out; + } else if (list == NULL) { + goto out; + } + + for (i = 0; i < count; i++) { + CU_DEBUG("Calling migration check: %s", list[i]); + s = _call_check(dom, list[i], param_path); + if (s.rc != CMPI_RC_OK) { + CU_DEBUG("...Failed"); + break; + } else + CU_DEBUG("...OK"); + } + out: + free_list(list, count); + + return s; +} + +static char *write_params(CMPIArray *array) +{ + int i; + int fd; + char *filename = strdup("/tmp/libvirtcim_mig.XXXXXX"); + FILE *file = NULL; + + if (filename == NULL) { + CU_DEBUG("Unable to get temporary file"); + return NULL; + } + + fd = mkstemp(filename); + if (fd < 0) { + CU_DEBUG("Unable to get temporary file: %s", strerror(errno)); + free(filename); + filename = NULL; + goto out; + } + + file = fdopen(fd, "w"); + if (file == NULL) { + CU_DEBUG("Unable to open temporary file: %s", strerror(errno)); + free(filename); + filename = NULL; + goto out; + } + + for (i = 0; i < CMGetArrayCount(array, NULL); i++) { + CMPIData d; + CMPIStatus s; + + d = CMGetArrayElementAt(array, i, &s); + if ((s.rc != CMPI_RC_OK) || CMIsNullValue(d)) { + CU_DEBUG("Unable to get array[%i]: %s", + i, + CMGetCharPtr(s.msg)); + continue; + } + + fprintf(file, "%s\n", CMGetCharPtr(d.value.string)); + } + + out: + if (file != NULL) + fclose(file); + + close(fd); + + return filename; +} + +static char *get_parms_file(const CMPIObjectPath *ref, + const CMPIArgs *argsin) +{ + CMPIStatus s; + CMPIArray *array; + CMPIInstance *msd; + + s = get_msd(ref, argsin, &msd); + if (s.rc != CMPI_RC_OK) + return NULL; + + if (cu_get_array_prop(msd, "CheckParameters", &array) == CMPI_RC_OK) + return write_params(array); + else + return NULL; +} + static CMPIStatus vs_migratable(const CMPIObjectPath *ref, const char *domain, const char *destination, @@ -276,6 +537,7 @@ static CMPIStatus vs_migratable(const CM CMPIBoolean isMigratable = 0; uint16_t type; virDomainPtr dom = NULL; + char *path = NULL; s = get_msd_values(ref, destination, argsin, &type, &dconn); if (s.rc != CMPI_RC_OK) @@ -298,6 +560,11 @@ static CMPIStatus vs_migratable(const CM } s = check_caps(conn, dconn); + if (s.rc != CMPI_RC_OK) + goto out; + + path = get_parms_file(ref, argsin); + s = call_external_checks(dom, path); if (s.rc != CMPI_RC_OK) goto out; @@ -315,6 +582,10 @@ static CMPIStatus vs_migratable(const CM virDomainFree(dom); virConnectClose(conn); virConnectClose(dconn); + + if (path != NULL) + unlink(path); + free(path); return s; }
As Jay already said - the code is of that high quality that we are used to get from you :). So I have no concerns about the implementation of your approach. But I have concerns in regard to the approach how this feature is added to the providers. First: its not a (let me say) common CIM way to add a property array to a class and let the client use this array as de-facto command line interface. What you want to do is giving the client a possibility to enhance the providers build in functionality - without rewriting the provider. And this functionality should be configurable by the client. The idea is valid and the implementation is straightforward. But what the client gets is a very powerful access point to - not only the resource managed by this class, but also to - all other resources on the system. So please don't blame me for bringing up security issues, but you do not have under control what kind of scripts will be put into this directory. As you officially need to point clients to this directory to make clear how to use the CheckParamaters property, its well known. But deep in my heard I trust the system admins, that they do not copy some strange scripts in there ;). So I know that this argument can be very fast invalidated. On the other hand, all scripts ... also of different authors, because you can have more than one mgmt. app on one system ... have to make sure, that they can handle input parameters, that might follow absolutely no logic. But ok, this is the responsibility of the client to harden its scripts in that way. So these issues are bound to the implementation approach and something I could live with. What does not mean, that I like this time-bomb. As I said before, the way how this feature is added to the schema is definitely not CIM conform. Its a property misused as command line interface. That's not what a property is - yet by definition of the word itself ;). But I don't only want to vote against this patch set without having a proposal. Normally you start evaluating the necessary functionality and enhance the existing model. This could mean adding methods or properties. But we can not foresee what the client wants to do by its scripts. On the other hand I can imagine that the amount of checks is limited. First by the intention of CheckParameters - which is de-facto a replacement for the methods CheckIsVSMigratable...(). And second by the hypervisor itself. So I suggest to go the CIM way for implementing this functionality. What you want is enhancing CheckIsVSMigratable...() and also the executing methods MigrateVSTo...(), because these methods have to do the check again, as between the client's CheckIsVSMigratable...() result and the MigrateVSTo...() request is enough time, that the result of the check can became invalid in the meantime. I looked a bit through the Migration Profile and found a class that I suggest to use for this functionality. That's the description of this class: "CIM_VirtualSystemMigrationSettingData defines the parameters to control a virtual system migration implementation, as managed by a CIM_VirtualSystemMigrationService. It is expected that a migration implementation will subclass this class to add implementation-specific migration options." An instance of this class is transported to all CheckIsVSMigratable...() and MigrateVSTo...() via the MigrationSettingData parameter. So we stay absolutely conform to the Migration Profile. The Xen and KVM subclasses can check properties, that are well know to cause migration problems. E.g. if the migration depends on the architecture, this class can get a property of maybe TargetArchitecture, which needs to be checked by the provider. Your approach of letting the client parameterize scripts through this class might look like this: [Description("An array containing the scripts to be executed as part of the check, if the migration is possible.")] string ScriptsToExecute[]; [Description("An array containing the parameters given as input to the scripts defined by ScriptsToExecute. Each entry in this array is related to the same array entry in ScriptToExecute. A list of parameters for one script is divided by space characters.")] string ScriptsParameterList[]; What do you - and all the others in this list interested in this feature - think about this ? Thanks ... Heidi -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
HE> I looked a bit through the Migration Profile and found a class HE> that I suggest to use for this functionality. That's the HE> description of this class: "CIM_VirtualSystemMigrationSettingData HE> defines the parameters to control a virtual system migration HE> implementation, as managed by a HE> CIM_VirtualSystemMigrationService. It is expected that a migration HE> implementation will subclass this class to add HE> implementation-specific migration options." An instance of this HE> class is transported to all CheckIsVSMigratable...() and HE> MigrateVSTo...() via the MigrationSettingData parameter. So we HE> stay absolutely conform to the Migration Profile. The Xen and KVM HE> subclasses can check properties, that are well know to cause HE> migration problems. This is what I did. I extended Xen_MigrationSettingData to include the CheckParameters[] property, which is what I pass to the check program. There are no additional arguments to either of the CIM methods, and this extra property of the SettingData is optional. HE> Your approach of letting the client parameterize scripts through HE> this class might look like this: [Description("An array containing HE> the scripts to be executed as part of the check, if the migration HE> is possible.")] string ScriptsToExecute[]; [Description("An array HE> containing the parameters given as input to the scripts defined by HE> ScriptsToExecute. Each entry in this array is related to the same HE> array entry in ScriptToExecute. A list of parameters for one HE> script is divided by space characters.")] string HE> ScriptsParameterList[]; Choosing the external checks to run does two things: (1) It means that the client must know which checks are available on the host, and (2) it means that the client must change if the system administrator changes the name of the check on disk. Giving the client visibility and control over what we run and where seems to (1) be more of a security risk, and (2) be too specific. What I have now is less of "run command A with argument B" and more of "Here is some information I have that should help determine if a migration can succeed". I think that the latter is more generally applicable. However, if others feel strongly, we can change this to a list of programs and a list of arguments. Thanks Heidi! -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
HE> I looked a bit through the Migration Profile and found a class HE> that I suggest to use for this functionality. That's the HE> description of this class: "CIM_VirtualSystemMigrationSettingData HE> defines the parameters to control a virtual system migration HE> implementation, as managed by a HE> CIM_VirtualSystemMigrationService. It is expected that a migration HE> implementation will subclass this class to add HE> implementation-specific migration options." An instance of this HE> class is transported to all CheckIsVSMigratable...() and HE> MigrateVSTo...() via the MigrationSettingData parameter. So we HE> stay absolutely conform to the Migration Profile. The Xen and KVM HE> subclasses can check properties, that are well know to cause HE> migration problems.
This is what I did. I extended Xen_MigrationSettingData to include the CheckParameters[] property, which is what I pass to the check program. There are no additional arguments to either of the CIM methods, and this extra property of the SettingData is optional.
Sorry Dan, either I'm working too long or I should go to an optician and let me give "very strong" glasses. I was under the impression that MigrationService itself was extended by CheckParameters. So, I'm fine with your integration into the model. -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
HE> I was under the impression that MigrationService itself was HE> extended by CheckParameters. So, I'm fine with your integration HE> into the model. Okay, no problem :) Thanks for the review! -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
participants (2)
-
Dan Smith -
Heidi Eckhart