7:03 p.m.
New subject: [PATCH 1 of 3] [RFC] Add optional check parameter field to migration schema
# HG changeset patch
# User Dan Smith <danms(a)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(a)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[];
};
7:03 p.m.
New subject: [PATCH 2 of 3] [RFC] Add configure support for migration check settings
# HG changeset patch
# User Dan Smith <danms(a)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(a)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])
7:03 p.m.
New subject: [PATCH 3 of 3] [RFC] Add external check functionality to CheckIsVSMigratable()
# HG changeset patch
# User Dan Smith <danms(a)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(a)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;
}
7:56 p.m.
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
8:36 p.m.
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(a)us.ibm.com
12:05 p.m.
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
6:14 p.m.
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(a)us.ibm.com
6237
days inactive
6239
days old
7 comments
2 participants
participants (2)
-
Dan Smith -
Heidi Eckhart