/** * virLockManager: * * A lock manager is a process that will supervise another * process. It will obtain & hold locks/leases for resources * the supervised process uses */ typedef struct virLockManager virLockManager; typedef virLockManager *virLockManagerPtr; typedef enum { VIR_LOCK_MANAGER_RESOURCE_READONLY = (1 << 0), VIR_LOCK_MANAGER_RESOURCE_SHARED = (1 << 1), } virLockManagerResourceFlags; enum { VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK = 0, } virLockManagerResourceType; enum { VIR_LOCK_MANAGER_NEW_DOMAIN = 0, } virLockManagerNewType; enum { VIR_LOCK_MANAGER_NEW_MIGRATE = (1 << 0), VIR_LOCK_MANAGER_NEW_ATTACH = (1 << 1), } virLockManagerNewFlags; enum { VIR_LOCK_MANAGER_MIGRATE_CANCEL = (1 << 0); } virLockManagerCompleteMigrateFlags; /** * virLockManagerNew: * @driver: the driver implementation to use * @type: the type of process to be supervised * @flags: one of virLockManagerNewFlags * * Create a new context to supervise a process, usually * a virtual machine. For a normal startup, flags can * be 0. For incoming migration, use VIR_LOCK_MANAGER_NEW_MIGRATE * * Returns a new lock manager context */ virLockManagerPtr virLockManagerNew(virLockManagerDriverPtr driver, unsigned int type, unsigned int flags); /** * virLockManagerSetParameter: * @manager: the lock manager context * @key: the parameter name * @value: the parameter value * * Set a configuration parameter for the managed process. * A process of VIR_LOCK_MANAGER_START_DOMAIN will be * given at least 3 parameters: * * - id: the domain unique id * - uuid: the domain uuid * - name: the domain name * * There may be other parameters specific to the lock manager * plugin that are provided for the managed process * * This should only be called prior to the supervised process * being started. Setting parameters may change the set of * argv returned by virLockManagerGetSupervisorArgs. */ int virLockManagerSetParameter(virLockManagerPtr manager, const char *key, const char *value); /** * virLockManagerRegisterResource: * @manager: the lock manager context * @type: the resource type virLockManagerResourceType * @name: the resource name * @flags: the resource access flags * * Register a resource that is required when the process * starts up. This may only be called prior to the process * being started. The format of @name varies according to * the resource @type. A VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK * will have a fully qualified file path. * * If no flags are given, the resource is assumed to be * used in exclusive, read-write mode. Access can be * relaxed to readonly, or shared read-write. */ int virLockManagerRegisterResource(virLockManagerPtr manager, unsigned int type, const gchar *name, unsigned int flags); /** * virLockManagerAcquireResource: * @manager: the lock manager context * @type: the resource type virLockManagerResourceType * @name: the resource name * @flags: the resource access flags * * Dynamically acquire a resource for a running process. * This may only be called once the process has been * started. The format of @name varies according to * the resource @type. A VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK * will have a fully qualified file path. * * If no flags are given, the resource is assumed to be * used in exclusive, read-write mode. Access can be * relaxed to readonly, or shared read-write. */ int virLockManagerAcquireResource(virLockManagerPtr manager, const gchar *file, unsigned int flags); /** * virLockManagerReleaseResource: * @manager: the lock manager context * @type: the resource type virLockManagerResourceType * @name: the resource name * @flags: the resource access flags * * Dynamically release a resource for a running process. * This may only be called after the process has been * started. The format of @name varies according to * the resource @type. A VIR_LOCK_MANAGER_RESOURCE_TYPE_DISK * will have a fully qualified file path. * * If no flags are given, the resource is assumed to be * used in exclusive, read-write mode. Access can be * relaxed to readonly, or shared read-write. */ int virLockManagerReleaseResource(virLockManagerPtr manager, const gchar *file, unsigned int flags); /** * virLockManager: * @manager: the lock manager context * * Retrieve the path of the supervisor binary */ char *virLockManagerGetSupervisorPath(virLockManagerPtr manager); /** * virLockManager: * @manager: the lock manager context * * Retrieve the security context of the supervisor binary */ char *virLockManagerGetSupervisorContext(virLockManagerPtr manager); /** * virLockManager: * @manager: the lock manager context * @argv: returned argument values * @argc: returned argument count * * Retrieve custom argv to pass to the supervisor binary * ahead of the normal process binary & argv. It is recommended * that the last argv be '--' to allow reliable determination * of the last supervisor arg. */ int virLockManagerGetSupervisorArgs(virLockManagerPtr manager, char ***argv, int *argc, unsigned int flags); /** * virLockManagerPrepareMigrate: * @manager: the lock manager context * @targetURI: destination of the migration * * Notify the supevisor that the process is about to be migrated * to another host at @targetURI */ int virLockManagerPrepareMigrate(virLockManagerPtr manager, const char *targetURI, unsigned int flags); /** * virLockManagerCompleteMigrateIn: * @manager: the lock manager context * @sourceURI: the origin of the migration * * Notify the supervisor that the process has completed * migration. If the migration is aborted, then the @flags * should be VIR_LOCK_MANAGER_MIGRATE_CANCEL */ int virLockManagerCompleteMigrateIn(virLockManagerPtr manager, const char *sourceURI, unsigned int flags); /** * virLockManagerCompleteMigrateOut: * @manager: the lock manager context * @targetURI: the destination of the migration * * Notify the supervisor that the process has completed * migration. If the migration is aborted, then the @flags * should be VIR_LOCK_MANAGER_MIGRATE_CANCEL */ int virLockManagerCompleteMigrateOut(virLockManagerPtr manager, const char *targetURI, unsigned int flags); /** * virLockManagerGetChild: * @manager: the lock manager context * * Obtain the PID of the managed process */ int virLockManagerGetChild(virLockManagerPtr manager, pid_t *pid, unsigned int flags); /** * virLockManager: * @manager: the lock manager context * * Inform the supervisor that the supervised process has * been, or can be stopped. */ int virLockManagerShutdown(virLockManagerPtr manager, unsigned int flags); /** * virLockManager: * @manager: the lock manager context * * Release resources. If the supervised process is still * running, it will be killed with prejudice */ void virLockManagerFree(virLockManagerPtr manager); -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Fri, Sep 10, 2010 at 01:50:56PM -0600, Eric Blake wrote: Yes, they're all folllowing that normal convention. I can't really think of a case when libvirt would need todo a non-blocking probe, so I left that out for simplicity. Do we need to guarentee that the lock is not obtained on failure ? I don't think that is neccessary to specify for safety, so I'd just leave it undefined. I think it is sufficient to leave it undefined, since that is still safe. Opps, virLockManagerShutdown The intention is that all locks will be released, but as above this is not critical from a safety POV. > > * @manager: the lock manager context > > * > > * Release resources. If the supervised process is still > > * running, it will be killed with prejudice > > Does this first attempt to automatically release any locks held by the > manager? I intend that it will call virLockManagerShutdown() so in that sense, yes. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Sun, Sep 12, 2010 at 02:22:04PM +0200, Saggi Mizrahi wrote: You still have to know at which point to release the lock on the source side & which point to re-acquire it on the destination. There are two choices there a. Release before migration starts + acquire when QEMU -incoming process starts b. Release after migration completes + acquire when QEMU resumes CPUs I could rename the driver APIs here so they didn't include the word 'migrate', but I don't see how we can remove any of these APIs - there are potential actions that a lock/lease manger may want to perform at each of these points in migration. The goal with the driver API is to try not to restrict the possible implementation strategies that a lock manager can use. Encoding a special verb for handover seems overly restrictive to me, since it is forcing a particular implementation strategy. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

typedef struct _virLockManagerDriver virLockManagerDriver; typedef virLockManagerDriver *virLockManagerDriverPtr; /* Which callbacks are supported */ typedef enum { VIR_LOCK_MANAGER_DRIVER_RESOURCES = (1 << 0), VIR_LOCK_MANAGER_DRIVER_MIGRATE = (1 << 0), VIR_LOCK_MANAGER_DRIVER_HOTPLUG = (1 << 1), }; struct _virLockManagerDriver { /** * @version: the newest implemented plugin ABI version * @flags: the implemented plugin optional extras */ unsigned int version; unsigned int flags; /** * load_drv: * @version: the libvirt requested plugin ABI version * @flags: the libvirt requested plugin optional extras * * Allow the plugin to validate the libvirt requested * plugin version / flags. This allows it to reject * use of versions which are too old. A plugin may * be loaded multiple times, for different libvirt * drivers * * Returns -1 if the requested version/flags were inadequate */ int (*load_drv)(unsigned int version, unsigned int flags); /** * unload_drv: * * Called to release any resources prior to the plugin * being unloaded from memory. Returns -1 to prevent * plugin unloading. */ int (*load_drv)(void); /* Mandatory callbacks */ virLockManagerPtr (*new_drv)(unsigned int type, unsigned int flags); int (*set_parameter_drv)(virLockManagerPtr man, const char *key, const char *value); char *(*get_supervisor_path_drv)(virLockManagerPtr man); char *(*get_supervisor_context_drv)(virLockManagerPtr man); int (*get_supervisor_args_drv)(virLockManagerPtr man, char ***argv, int *argc, unsigned int flags); int (*connect_drv)(virLockManagerPtr man, unsigned int flags); int (*startup_drv)(virLockManagerPtr man, pid_t *pid, unsigned int flags); int (*shutdown_drv)(virLockManagerPtr man, unsigned int flags); void (*free_drv)(virLockManagerPtr man); /* End of mandatory callbacks */ /* Only if VIR_LOCK_MANAGER_DRIVER_RESOURCES */ int (*set_file_drv)(virLockManagerPtr man, const char *file, unsigned int flags); /* Only if VIR_LOCK_MANAGER_DRIVER_HOTPLUG */ int (*add_file_drv)(virLockManagerPtr man, const char *file, unsigned int flags); int (*remove_file_drv)(virLockManagerPtr man, const char *file, unsigned int flags); /* Only if VIR_LOCK_MANAGER_DRIVER_MIGRATE */ int (*prepare_migrate_drv)(virLockManagerPtr man, const gchar *targetURI, unsigned int flags); int (*complete_migrate_out_drv)(virLockManagerPtr man, const gchar *targetURI, unsigned int flags); int (*complete_migrate_in_drv)(virLockManagerPtr man, const gchar *sourceURI, unsigned int flags); }; /** * virLockManagerPluginDriver: * * The signature of the function for obtaining a lock manager * driver from a dlopen()d plugin module. The plugin must contain * a symbol called "plugin_init" with this signature */ typedef virLockManagerDriverPtr (*virLockManagerPluginDriver)(void); /** * virLockManagerPluginLoad: * @name: the name of the plugin * * Attempt to load the plugin $(libdir)/libvirt/lock-manager/(a)name.so * The plugin driver entry point will be resolved & invoked to obtain * the lock manager driver */ virLockManagerPluginPtr virLockManagerPluginLoad(const char *name); int virLockManagerPluginUnload(virLockManagerPluginPtr); -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Fri, Sep 10, 2010 at 02:01:16PM -0600, Eric Blake wrote: We'll report errors via the normal libvirt error & logging APIs so that should still be diagnosable. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Wed, Nov 10, 2010 at 03:01:01PM +0000, Richard W.M. Jones wrote: The load_drv() method there is to allow the plugin implementation to block its use with a libvirtd that is too old. eg, say we add a new API 'foo', and the plugin implements that API. It wants to block its use in any libvirt which doesn't know about the 'foo' entry point, because such use could be dangerous to safety. The load_drv() method allows that to be handled. If load_drv() fails libvirt/libguestfs *must* refuse to run any VMs. In libvirt context they are loaded when the QEMU driver is initialized in libvirtd which is still single threaded. In libguestfs, you've no global "driver" state, so I'd suggest it is easiest to just call load_drv before starting the VM, and unload_drv after stopping it. The API contract requires that a plugin needs to cope with being loaded multiple times. You're thinking only about libvirtd checking what a plugin supports. The load_drv() method is about the *opposite*, allowing the plugin to check what libvirtd supports. You can't do that with dlsym, or __attribute__(). In addition to the reasons already mentioned for constructor not being able to replace load_drv, "destructor" can't replace the unload_drv method. The unload_drv method is there to allow the plugin to refuse to allow module unloading from the app. The load/unload API entry points aren't really intended for one time module state initialization. They are for doing compatibility and safety checks, which require parameters & return values that you can't do with constructors. If a plugin does want todo global state initialization then it can of course use __attribute__((constructor)) and __attribute__((destructor)), and leave unload_drv/load_Drv to be no-op Not locked down yet, but likely $LIBDIR/libvirt/plugins/lock_manager/$NAME.so At the point where libvirt drivers invoke the plugin, there is no virDomainPtr in available, only the internal objects virDomainDefPtr which aren't something we want to expose since they're not ABI stable. In addition from the context where the plugin executes, it likely won't be able to use the libvirt APIs safely without causing deadlock in libvirt drivers. It isn't passing the XML because we didn't want the lock manager implementations to all have to re-implement XML parsing, and again the libvirt driver code doesn't have the XML around at this point. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Wed, Nov 10, 2010 at 04:17:13PM +0000, Richard W.M. Jones wrote: Allowing read-only calls to continue isn't guarenteed to be safe. The lock manager plugin may wish to deny even readonly usage, to ensure that there aren't data consistency complication with NFS close-to-open behaviour during migration. If a lock manager is configured for use and it fails, not running any VMs is the only safe thing, until the admin has fixed the config problem. As mentioned below, these APIs are intended for doing global state management, instead they are for version/feature compatibility checks. There can be multiple lock manager plugin impls, with different requirements. Renaming the DSO impacts all implementations, where as the load_drv() callback is intended to allow individual impls to define their own minimum requirements. It isn't about the locking preventing unload. There are a variety of scenarios in which is it unsafe to unload *any* dlopen'd library. eg, if the module has set a thread local variable with a destructor, it is not safe to unload the module unless all threads have terminated, or all thread locals cleaned up. The module has no way to ensure this, so the only safe option is to never dlclose() it, otherwise you'll get SIGBUS or SIGSEGV. The unload_drv() method return code is to allow for this scenario to be dealt with safely. Unfortunately this is a no-win situation. Either the plugin impls have to parse the XML, or the plugin callers have to. On balance there are more plugin impls than callers, and one of the callers already knows how to parse the XML, so I put the burden of XML parsing on the caller rather than the plugin impl. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On Wed, Nov 10, 2010 at 05:29:28PM +0000, Richard W.M. Jones wrote: We can't predict the future, so I designed the plugin to give maximum flexibility to adapt to what it may bring. Making one API call after loading the plugin to check compatibility really isn't any burden and so its a worthwhile safety net for future change. You wouldn't expect this to ever fail in normal circumstances. RPM dependancies should ensure it doesn't get into a broken state. This is there as a safety net in case the admin does something stupid, so that we don't pose any risk to the safety of the disk images. This isn't about completely changing the locking scheme, it is about allowing extra APIs to be added to provide additional information. If a plugin wants to be able to rely on that additional info being present, it should be able to check that this is supported by the libvirtd instance upfront. This avoids a runtime failure at time of guest startup, disk hotplug, or worse, at guest migration where it is possibly already too late to cope. Unfortunately there are plenty of existing APIs out there which don't provide a way to cancel future callbacks. eg pthread_key_create has no way to guarantee that the destructor won't ever be called again, unless you can kill off all threads before unloading the module. Another example is the GObject type system which does not allow you to unregister modules. Thus to be safe, we need to allow plugins to indicate that they don't wish to be unloaded from memory. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

On 09/10/2010 10:01 AM, Daniel P. Berrange wrote: Can you load more than one lock manager at a time, or just one active lock manager? How does a user configure which lock manager(s) to load when libvirtd is started? Would it be better to first try virLockManagerShutdown? And rather than a direct kill(), shouldn't this be virLockManagerFree? So if there needs to be any relaxation of locks from exclusive to shared-write for the duration of the migration, that would be the responsibility of virLockManagerPrepareMigrate, and not done directly by libvirt? Wouldn't this be virLockManagerCompleteMigrateOut? Likewise? Overall, this looks workable to me. As proposed, this assumes a 1:1 relation between LockManager process and managed VMs. But I guess you can still have a central manager process that manages all the VMs, by having the lock manager plugin spawn a simple shim process that does all the communication with the central lock manager. -- Eric Blake eblake(a)redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On Mon, 2010-09-13 at 13:35 +0100, Daniel P. Berrange wrote: VM but rather having the sync_manager process register with a central daemon and exec into qemu (or anything else) so assuming there is a process per VM is essentially false. But the verb could be used for "unregistering" the current instance with the main manager so the verb does have it use. Further more, even if we decide to leave the current 'sync_manager process per child process' system as is for now. The general direction is a central deamon per host for managing all the leases and guarding all processes. So be sure to keep that in mind while assembling the API.

On Mon, 2010-09-13 at 14:29 +0100, Daniel P. Berrange wrote: My explanation might have been confusing or ill phrased. I'll try again. What the suggestion was: instead of libvirt running sync manager that will fork off and run qemu. libvirt would run sync_manager wrapper that will register with the central daemon wait for it to acquire leases and then exec to qemu (in process). From that moment the central daemon monitors the process and when it quits frees it's leases. This way we still keep all the context stuff from libvirt and have only 1 process managing the leases. But, as I said, this is only a suggestion and is still in very early stages. We might not implement in the initial version and leave the current forking method.

----- "Daniel P. Berrange" <berrange(a)redhat.com&gt; wrote: Correct me if I'm wrong, but the security limitations (selinux context) would only take effect after the "exec", no? so the process could still communicate with the daemon, open an FD and then exec. After exec, the VM would be locked down but the daemon could still wait on the FD to see whether VM has died.

On Mon, Sep 13, 2010 at 02:29:49PM +0100, Daniel P. Berrange wrote: I think there are two valid approaches people may want to take, both of which seem compatible with the requirements you describe. First, per the RFC (and current sync_manager design): Method 1: libvirt forks a supervisor process which: 1. acquires lock 2. forks qemu 3. waits for qemu exit 4. releases lock Method 2: without a supervisor process, libvirt: 1. calls plugin acquire_lock() 2. forks qemu 3. waits for qemu exit 4. calls plugin release_lock() (I should add 2.b. calls plugin set_pid() to tell lock manager the qemu pid.) In method 2, I expect the plugin functions would generally send ipc to a local lock manager daemon that holds and manages locks. (And may also wish to monitor the qemu pid itself.) Dave