[PATCH 0 of 2] MigrationIndications
These two patches together are dependent on some libcmpiutil changes I'll be sending out shortly; one for dup_instance, and another for raise functionality. The four patches together give us Migration Indications. Any time VSMigrationService is used to trigger a migration, the appropriate indications will be fired. The big caveat for these patches is that migration is still a bit flaky on my system, so while I have tested that the "started" and "failed" indications work I haven't had a chance to see "succeeded" fire. I have no reason to believe it won't, because of the way the raising happens, but it would be good to confirm.
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1201882409 18000 # Node ID a17039d1b29cd5275ecd126782bb1757a415a0c0 # Parent 6d86529bbdce9eb68b26a7fc5339fd66a1f2fdd2 Add calls into VSMigrationService to utilize new MigrationIndication provider. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r 6d86529bbdce -r a17039d1b29c src/Virt_VSMigrationService.c --- a/src/Virt_VSMigrationService.c Fri Feb 01 11:13:26 2008 -0500 +++ b/src/Virt_VSMigrationService.c Fri Feb 01 11:13:29 2008 -0500 @@ -34,6 +34,7 @@ #include "misc_util.h" #include <libcmpiutil/std_instance.h> #include <libcmpiutil/std_invokemethod.h> +#include <std_indication.h> #include "Virt_VSMigrationService.h" #include "Virt_HostSystem.h" @@ -264,11 +265,32 @@ static CMPIStatus vs_migratable_system(C return vs_migratable(ref, name, dname, results, argsout); } +static bool raise_indication(const CMPIContext *context, + const char *base_type, + const char *ns, + const CMPIInstance *ind) +{ + char *type; + CMPIStatus s; + + /* Seems like this shouldn't be hardcoded. */ + type = get_typed_class("Xen", base_type); + + s = stdi_raise_indication(_BROKER, context, type, ns, ind); + + free(type); + + return s.rc == CMPI_RC_OK; +} + static void migrate_job_set_state(struct migration_job *job, uint16_t state, const char *status) { CMPIInstance *inst; + CMPIInstance *prev_inst; + CMPIInstance *ind; + bool rc; CMPIStatus s; CMPIObjectPath *op; @@ -297,12 +319,50 @@ static void migrate_job_set_state(struct CMSetProperty(inst, "Status", (CMPIValue *)status, CMPI_chars); + CU_DEBUG("Creating indication."); + /* Prefix needs to be dynamic */ + ind = get_typed_instance(_BROKER, + "Xen", + "ComputerSystemMigrationIndication", + job->ref_ns); + /* Prefix needs to be dynamic */ + if (ind == NULL) { + CU_DEBUG("Failed to create ind, type '%s:%s_%s'", + job->ref_ns, + "Xen", + "ComputerSystemMigrationIndication"); + } + + /* Need to copy job inst before attaching as PreviousInstance because + otherwise the changes we are about to make to job inst are made + to PreviousInstance as well. */ + s = cu_dup_instance(_BROKER, inst, &prev_inst); + if (s.rc != CMPI_RC_OK || prev_inst == NULL) { + CU_DEBUG("dup_instance failed (%i:%s)", s.rc, s.msg); + return; + } + + CU_DEBUG("Setting PreviousInstance"); + CMSetProperty(ind, "PreviousInstance", + (CMPIValue *)&prev_inst, CMPI_instance); + CU_DEBUG("Modifying job %s (%i:%s)", job->uuid, state, status); s = CBModifyInstance(_BROKER, job->context, op, inst, NULL); if (s.rc != CMPI_RC_OK) CU_DEBUG("Failed to update job instance: %s", CMGetCharPtr(s.msg)); + + CU_DEBUG("Setting SourceInstance"); + CMSetProperty(ind, "SourceInstance", + (CMPIValue *)&inst, CMPI_instance); + + rc = raise_indication(job->context, + "ComputerSystemMigrationIndication", + job->ref_ns, + ind); + if (!rc) + CU_DEBUG("Failed to raise indication"); } static CMPIStatus migrate_vs(struct migration_job *job)
Jay Gagnon wrote:
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1201882409 18000 # Node ID a17039d1b29cd5275ecd126782bb1757a415a0c0 # Parent 6d86529bbdce9eb68b26a7fc5339fd66a1f2fdd2 Add calls into VSMigrationService to utilize new MigrationIndication provider.
static void migrate_job_set_state(struct migration_job *job, uint16_t state, const char *status) { CMPIInstance *inst; + CMPIInstance *prev_inst; + CMPIInstance *ind; + bool rc; CMPIStatus s; CMPIObjectPath *op;
@@ -297,12 +319,50 @@ static void migrate_job_set_state(struct CMSetProperty(inst, "Status", (CMPIValue *)status, CMPI_chars);
+ CU_DEBUG("Creating indication."); + /* Prefix needs to be dynamic */ + ind = get_typed_instance(_BROKER, + "Xen", + "ComputerSystemMigrationIndication", + job->ref_ns); + /* Prefix needs to be dynamic */
I'm showing my indication ignorance here.. Would it be possible to get the prefix from the ref in migrate_do? You could then add a prefix value to the migration job struct. You already do something similar with the namespace. However, I'm not entirely sure the ref has the proper prefix you need. -- Kaitlin Rupert IBM Linux Technology Center kaitlin@linux.vnet.ibm.com
JG> +static bool raise_indication(const CMPIContext *context, JG> + const char *base_type, JG> + const char *ns, JG> + const CMPIInstance *ind) JG> +{ JG> + char *type; JG> + CMPIStatus s; JG> + JG> + /* Seems like this shouldn't be hardcoded. */ JG> + type = get_typed_class("Xen", base_type); JG> + JG> + s = stdi_raise_indication(_BROKER, context, type, ns, ind); JG> + JG> + free(type); JG> + JG> + return s.rc == CMPI_RC_OK; JG> +} So, this actually brings up an interesting point which I think we glazed over when we put the migration provider in the first time. I left the migration job classname as Virt_MigrationJob, thus not having a specific platform prefix on it. I did that because it seemed to me that clients could potentially use the job as an entry point, needing to see what guests were leaving a host at any point, and then following the ref's to platform-specific details. However, as Jay points out, I think that the indication should be typed per-platform as usual. So, should we type the job to match, since we'll need to persist the information for typing the indication as well? Heidi, maybe you have some input here? JG> @@ -297,12 +319,50 @@ static void migrate_job_set_state(struct JG> CMSetProperty(inst, "Status", JG> (CMPIValue *)status, CMPI_chars); JG> + CU_DEBUG("Creating indication."); JG> + /* Prefix needs to be dynamic */ JG> + ind = get_typed_instance(_BROKER, JG> + "Xen", JG> + "ComputerSystemMigrationIndication", JG> + job->ref_ns); JG> + /* Prefix needs to be dynamic */ JG> + if (ind == NULL) { JG> + CU_DEBUG("Failed to create ind, type '%s:%s_%s'", JG> + job->ref_ns, JG> + "Xen", JG> + "ComputerSystemMigrationIndication"); JG> + } JG> + JG> + /* Need to copy job inst before attaching as PreviousInstance because JG> + otherwise the changes we are about to make to job inst are made JG> + to PreviousInstance as well. */ JG> + s = cu_dup_instance(_BROKER, inst, &prev_inst); I didn't quite understand from the other patch, but I don't think this is what you want to do. You create an instance with a set of properties set (like CCN) with the get_typed_instance() call, but then just overwrite the ObjectPath of it here. I think the semantics of the cu_dup_instance() call should be that it creates you a new instance. Something like this maybe: new_inst = cu_dup_instance(_BROKER, src_inst, &s); JG> + if (s.rc != CMPI_RC_OK || prev_inst == NULL) { JG> + CU_DEBUG("dup_instance failed (%i:%s)", s.rc, s.msg); JG> + return; JG> + } JG> + JG> + CU_DEBUG("Setting PreviousInstance"); JG> + CMSetProperty(ind, "PreviousInstance", JG> + (CMPIValue *)&prev_inst, CMPI_instance); JG> + JG> CU_DEBUG("Modifying job %s (%i:%s)", job->uuid, state, status); JG> s = CBModifyInstance(_BROKER, job->context, op, inst, NULL); JG> if (s.rc != CMPI_RC_OK) JG> CU_DEBUG("Failed to update job instance: %s", JG> CMGetCharPtr(s.msg)); JG> + JG> + CU_DEBUG("Setting SourceInstance"); JG> + CMSetProperty(ind, "SourceInstance", JG> + (CMPIValue *)&inst, CMPI_instance); JG> + JG> + rc = raise_indication(job->context, JG> + "ComputerSystemMigrationIndication", JG> + job->ref_ns, JG> + ind); JG> + if (!rc) JG> + CU_DEBUG("Failed to raise indication"); JG> } These changes make the migrate_job_set_state() function quite a bit longer. Can we break out these out into a separate function? If it just returns an indication instance, then maybe something like this could work: ind = prepare_indication(job_inst); /* Existing job_inst update code */ raise_indication(ind, job_inst); The prepare call would dup the old instance and create the indication instance, and the raise call would add the updated version and do the actual raise. What do you think? -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
However, as Jay points out, I think that the indication should be typed per-platform as usual. So, should we type the job to match, since we'll need to persist the information for typing the indication as well?
I'm no Heidi, but it seems like we should be prefixing this. We prefix every other class we create, and if somebody doesn't want to deal with the Xen/KVM distinction they are welcome to subscribe to/filter on/whatever they want the CIM super class and get both, right? As to where the prefix comes from, I think Kaitlin was on to something with the ref that comes in from migrate_do, although I think it can be even easier than she said. We don't actually need to attach the prefix to the migration_job structure; we're already attaching the classname, so all we need is a call to class_prefix_name and we've got a prefix.
JG> @@ -297,12 +319,50 @@ static void migrate_job_set_state(struct JG> CMSetProperty(inst, "Status", JG> (CMPIValue *)status, CMPI_chars);
JG> + CU_DEBUG("Creating indication."); JG> + /* Prefix needs to be dynamic */ JG> + ind = get_typed_instance(_BROKER, JG> + "Xen", JG> + "ComputerSystemMigrationIndication", JG> + job->ref_ns); JG> + /* Prefix needs to be dynamic */ JG> + if (ind == NULL) { JG> + CU_DEBUG("Failed to create ind, type '%s:%s_%s'", JG> + job->ref_ns, JG> + "Xen", JG> + "ComputerSystemMigrationIndication"); JG> + } JG> + JG> + /* Need to copy job inst before attaching as PreviousInstance because JG> + otherwise the changes we are about to make to job inst are made JG> + to PreviousInstance as well. */ JG> + s = cu_dup_instance(_BROKER, inst, &prev_inst);
I didn't quite understand from the other patch, but I don't think this is what you want to do. You create an instance with a set of properties set (like CCN) with the get_typed_instance() call, but then just overwrite the ObjectPath of it here.
I think the semantics of the cu_dup_instance() call should be that it creates you a new instance. Something like this maybe:
new_inst = cu_dup_instance(_BROKER, src_inst, &s);
As was already brought to light by my first run at cu_dup_instance, I get easily turned around when I start working with actual instances and such, so I'd like to make sure I follow this. Let's say I fix up cu_dup_instance so that it creates a new instance using the existing instances ObjectPath, then copies all the properties correctly. Then I wouldn't have to call get_typed_instance in the first place, because everything that it does is already taken care of, right?
JG> + if (s.rc != CMPI_RC_OK || prev_inst == NULL) { JG> + CU_DEBUG("dup_instance failed (%i:%s)", s.rc, s.msg); JG> + return; JG> + } JG> + JG> + CU_DEBUG("Setting PreviousInstance"); JG> + CMSetProperty(ind, "PreviousInstance", JG> + (CMPIValue *)&prev_inst, CMPI_instance); JG> + JG> CU_DEBUG("Modifying job %s (%i:%s)", job->uuid, state, status);
JG> s = CBModifyInstance(_BROKER, job->context, op, inst, NULL); JG> if (s.rc != CMPI_RC_OK) JG> CU_DEBUG("Failed to update job instance: %s", JG> CMGetCharPtr(s.msg)); JG> + JG> + CU_DEBUG("Setting SourceInstance"); JG> + CMSetProperty(ind, "SourceInstance", JG> + (CMPIValue *)&inst, CMPI_instance); JG> + JG> + rc = raise_indication(job->context, JG> + "ComputerSystemMigrationIndication", JG> + job->ref_ns, JG> + ind); JG> + if (!rc) JG> + CU_DEBUG("Failed to raise indication"); JG> }
These changes make the migrate_job_set_state() function quite a bit longer. Can we break out these out into a separate function? If it just returns an indication instance, then maybe something like this could work:
ind = prepare_indication(job_inst);
/* Existing job_inst update code */
raise_indication(ind, job_inst);
The prepare call would dup the old instance and create the indication instance, and the raise call would add the updated version and do the actual raise. What do you think?
Sure that's fine. I didn't think too much about code structure when I was in "wait is that gonna work?" mode; should have cleaned up a little more before I sent it out. -- -Jay
JG> We prefix every other class we create, and if somebody doesn't JG> want to deal with the Xen/KVM distinction they are welcome to JG> subscribe to/filter on/whatever they want the CIM super class and JG> get both, right? Yeah, okay, then the plan is to type the job. I guess we can leave the hard-coded "Xen" in place for now. JG> Let's say I fix up cu_dup_instance so that it creates a new JG> instance using the existing instances ObjectPath, then copies all JG> the properties correctly. Then I wouldn't have to call JG> get_typed_instance in the first place, because everything that it JG> does is already taken care of, right? Yep. -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
However, as Jay points out, I think that the indication should be typed per-platform as usual. So, should we type the job to match, since we'll need to persist the information for typing the indication as well?
I'm no Heidi, but it seems like we should be prefixing this. We prefix every other class we create, and if somebody doesn't want to deal with the Xen/KVM distinction they are welcome to subscribe to/filter on/whatever they want the CIM super class and get both, right? Sorry for my late jump into this discussion. I know, its already decided and only wanted to add my thought too. I also agree to prefixing the job with either Xen or KVM. This also means to update the mof files to
Jay Gagnon wrote: directly derive these two classes from CIM_ConcreteJob - or maybe better - rename Virt_MigrationJob to CIM_MigrationJob as placeholder for the upcoming migration profile and derive from CIM_MigrationJob.
As to where the prefix comes from, I think Kaitlin was on to something with the ref that comes in from migrate_do, although I think it can be even easier than she said. We don't actually need to attach the prefix to the migration_job structure; we're already attaching the classname, so all we need is a call to class_prefix_name and we've got a prefix.
Straightforward idea :). -- Regards Heidi Eckhart Software Engineer IBM Linux Technology Center - Open Hypervisor
# HG changeset patch # User Jay Gagnon <grendel@linux.vnet.ibm.com> # Date 1201882406 18000 # Node ID 6d86529bbdce9eb68b26a7fc5339fd66a1f2fdd2 # Parent 2920e184226248f7885dd87a28e30eb1a90005c9 ComputerSystemMigrationIndication Indication to keep clients up to date on CS migrations, along with relevant makefile and schema work. Signed-off-by: Jay Gagnon <grendel@linux.vnet.ibm.com> diff -r 2920e1842262 -r 6d86529bbdce Makefile.am --- a/Makefile.am Wed Jan 30 10:52:58 2008 -0800 +++ b/Makefile.am Fri Feb 01 11:13:26 2008 -0500 @@ -24,6 +24,7 @@ MOFS = \ schema/RegisteredProfile.mof \ schema/ElementConformsToProfile.mof \ schema/ComputerSystemIndication.mof \ + schema/ComputerSystemMigrationIndication.mof \ schema/ResourceAllocationSettingData.mof \ schema/ResourcePoolConfigurationService.mof \ schema/ResourcePoolConfigurationCapabilities.mof \ @@ -65,6 +66,7 @@ REGS = \ schema/DiskPool.registration \ schema/HostedResourcePool.registration \ schema/ComputerSystemIndication.registration \ + schema/ComputerSystemMigrationIndication.registration \ schema/ResourceAllocationSettingData.registration \ schema/ResourcePoolConfigurationService.registration \ schema/ResourcePoolConfigurationCapabilities.registration \ @@ -109,4 +111,4 @@ clean-local: clean-local: rm -f $(find . -name "*.orig") rm -f $(find . -name "*.rej") - rm -f $(find . -name "*~") \ No newline at end of file + rm -f $(find . -name "*~") diff -r 2920e1842262 -r 6d86529bbdce schema/ComputerSystemMigrationIndication.mof --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/schema/ComputerSystemMigrationIndication.mof Fri Feb 01 11:13:26 2008 -0500 @@ -0,0 +1,15 @@ +// Copyright IBM Corp. 2007 + +[Description ("Xen_ComputerSystem migration status"), + Provider("cmpi::Virt_ComputerSystemModificationIndication") +] +class Xen_ComputerSystemMigrationIndication : CIM_InstModification +{ +}; + +[Description ("KVM_ComputerSystem migration status"), + Provider("cmpi::Virt_ComputerSystemModificationIndication") +] +class KVM_ComputerSystemMigrationIndication : CIM_InstModification +{ +}; diff -r 2920e1842262 -r 6d86529bbdce schema/ComputerSystemMigrationIndication.registration --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/schema/ComputerSystemMigrationIndication.registration Fri Feb 01 11:13:26 2008 -0500 @@ -0,0 +1,4 @@ +# Copyright IBM Corp. 2007 +# Classname Namespace ProviderName ProviderModule ProviderTypes +Xen_ComputerSystemMigrationIndication root/virt Virt_ComputerSystemMigrationIndication Virt_ComputerSystemMigrationIndication indication method +KVM_ComputerSystemMigrationIndication root/virt Virt_ComputerSystemMigrationIndication Virt_ComputerSystemMigrationIndication indication method diff -r 2920e1842262 -r 6d86529bbdce src/Makefile.am --- a/src/Makefile.am Wed Jan 30 10:52:58 2008 -0800 +++ b/src/Makefile.am Fri Feb 01 11:13:26 2008 -0500 @@ -30,6 +30,7 @@ provider_LTLIBRARIES = libVirt_ComputerS libVirt_Device.la \ libVirt_SystemDevice.la \ libVirt_ComputerSystemIndication.la \ + libVirt_ComputerSystemMigrationIndication.la \ libVirt_RASD.la \ libVirt_HostSystem.la \ libVirt_VirtualSystemManagementService.la \ @@ -61,6 +62,10 @@ libVirt_ComputerSystemIndication_la_DEPE libVirt_ComputerSystemIndication_la_DEPENDENCIES = libVirt_ComputerSystem.la libVirt_ComputerSystemIndication_la_SOURCES = Virt_ComputerSystemIndication.c libVirt_ComputerSystemIndication_la_LIBADD = -lVirt_ComputerSystem -lpthread -lrt + +libVirt_ComputerSystemMigrationIndication_la_DEPENDENCIES = libVirt_ComputerSystem.la +libVirt_ComputerSystemMigrationIndication_la_SOURCES = Virt_ComputerSystemMigrationIndication.c +libVirt_ComputerSystemMigrationIndication_la_LIBADD = -lVirt_ComputerSystem libVirt_VirtualSystemManagementService_la_DEPENDENCIES = libVirt_ComputerSystem.la libVirt_ComputerSystemIndication.la libVirt_RASD.la libVirt_HostSystem.la libVirt_VirtualSystemManagementService_la_SOURCES = Virt_VirtualSystemManagementService.c diff -r 2920e1842262 -r 6d86529bbdce src/Virt_ComputerSystemMigrationIndication.c --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/src/Virt_ComputerSystemMigrationIndication.c Fri Feb 01 11:13:26 2008 -0500 @@ -0,0 +1,120 @@ +/* + * Copyright IBM Corp. 2007 + * + * Authors: + * Dan Smith <danms@us.ibm.com> + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ +#include <unistd.h> +#include <stdio.h> +#include <fcntl.h> +#include <string.h> +#include <stdlib.h> +#include <stdbool.h> + +#include <cmpidt.h> +#include <cmpift.h> +#include <cmpimacs.h> + +#include <libvirt/libvirt.h> + +#include <libcmpiutil/libcmpiutil.h> +#include <misc_util.h> +#include <libcmpiutil/std_indication.h> +#include <cs_util.h> + +#include "config.h" + +#include "Virt_ComputerSystem.h" + +static const CMPIBroker *_BROKER; + +#ifdef CMPI_EI_VOID +# define _EI_RTYPE void +# define _EI_RET() return +#else +# define _EI_RTYPE CMPIStatus +# define _EI_RET() return (CMPIStatus){CMPI_RC_OK, NULL} +#endif + +static CMPIStatus ActivateFilter(CMPIIndicationMI* mi, + const CMPIContext* ctx, + const CMPISelectExp* se, + const char *ns, + const CMPIObjectPath* op, + CMPIBoolean first) +{ + CMPIStatus s = {CMPI_RC_OK, NULL}; + + return s; +} + +static CMPIStatus DeActivateFilter(CMPIIndicationMI* mi, + const CMPIContext* ctx, + const CMPISelectExp* se, + const char *ns, + const CMPIObjectPath* op, + CMPIBoolean last) +{ + return (CMPIStatus){CMPI_RC_OK, NULL}; +} + +static _EI_RTYPE EnableIndications(CMPIIndicationMI* mi, + const CMPIContext *ctx) +{ + struct std_indication_ctx *_ctx; + _ctx = (struct std_indication_ctx *)mi->hdl; + _ctx->enabled = true; + CU_DEBUG("ComputerSystemModifiedIndication enabled"); + + _EI_RET(); +} + +static _EI_RTYPE DisableIndications(CMPIIndicationMI* mi, + const CMPIContext *ctx) +{ + struct std_indication_ctx *_ctx; + _ctx = (struct std_indication_ctx *)mi->hdl; + _ctx->enabled = false; + CU_DEBUG("ComputerSystemModifiedIndication disabled"); + + _EI_RET(); +} + +static struct std_indication_handler csi = { + .raise_fn = NULL, + .trigger_fn = NULL, +}; + +DEFAULT_IND_CLEANUP(); +DEFAULT_AF(); +DEFAULT_MP(); + +STDI_IndicationMIStub(, + Virt_ComputerSystemMigrationIndication, + _BROKER, + libvirt_cim_init(), + &csi); + +/* + * Local Variables: + * mode: C + * c-set-style: "K&R" + * tab-width: 8 + * c-basic-offset: 8 + * indent-tabs-mode: nil + * End: + */
JG> + * Dan Smith <danms@us.ibm.com> While I'd love to take credit, I think you better fix this. You also need to add your name to CSI, as it resembles less of my original work than yours, I'm sure. Otherwise the provider looks good (not much to complain about). The default_raise() idea was obviously killer :) -- Dan Smith IBM Linux Technology Center Open Hypervisor Team email: danms@us.ibm.com
Dan Smith wrote:
JG> + * Dan Smith <danms@us.ibm.com>
While I'd love to take credit, I think you better fix this.
Heh. I noticed it once but was in the middle of something that required too much attention, so I promptly forgot it. -- -Jay
participants (4)
-
Dan Smith -
Heidi Eckhart -
Jay Gagnon -
Kaitlin Rupert