[libvirt] [PATCH] qemudDomainCreate check if domain is already active
Hi, currently virsh create foo.xml overwrites running domains. In case of qemu this leaves detached qemu processes around and the domain creation fails later on being unable to start other domains afterwards - not nice. Attached patch checks if we already have a running domain by that name and in this case refuses to create a new domain from xml by that name. Probably this check needs to be pushed further upward since this might affect other hypervisors too, haven't checked that though. Cheers, -- Guido
On Thu, Jul 24, 2008 at 12:49:13AM -0400, Guido Günther wrote:
Hi, currently virsh create foo.xml overwrites running domains. In case of qemu this leaves detached qemu processes around and the domain creation fails later on being unable to start other domains afterwards - not nice. Attached patch checks if we already have a running domain by that name and in this case refuses to create a new domain from xml by that name.
okay looks fine to me
Probably this check needs to be pushed further upward since this might affect other hypervisors too, haven't checked that though.
Well for Xen that check is delegated to xend, and in general it's better to keep it that way to avoid potential races. I guess the problem is more likely for hypervisors where libvirt(d) runs a process to control the domain and where there is no central checking done. OpenVZ and LXC should be checked for that, yes. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Thu, Jul 24, 2008 at 12:49:13AM -0400, Guido G?nther wrote:
Hi, currently virsh create foo.xml overwrites running domains. In case of qemu this leaves detached qemu processes around and the domain creation fails later on being unable to start other domains afterwards - not nice. Attached patch checks if we already have a running domain by that name and in this case refuses to create a new domain from xml by that name.
Probably this check needs to be pushed further upward since this might affect other hypervisors too, haven't checked that though.
Actually, in Xen case, XenD itself will do the checking.
[PATCH] qemudDomainCreate: check if domain is already active
otherwise we overwrite a running domain which lets libvirtd disconnect from the running qemu. --- src/qemu_driver.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index b84bdf4..0ad72ae 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1978,6 +1978,15 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (!(def = virDomainDefParseString(conn, driver->caps, xml))) return NULL;
+ vm = virDomainFindByName(driver->domains, def->name);
You need to check for UUID clash too. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Jul 24, 2008 at 10:01:32AM +0100, Daniel P. Berrange wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index b84bdf4..0ad72ae 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1978,6 +1978,15 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (!(def = virDomainDefParseString(conn, driver->caps, xml))) return NULL;
+ vm = virDomainFindByName(driver->domains, def->name);
You need to check for UUID clash too. Indeed. But before fixing this I wonder what the exact semantics of domainCreateLinux are. Is it correct that we don't call virDomainSaveConfiguration? If so we probably don't even need to check if the domain is running. The mere existance of a domain with the same name/uuid should probably throw an error already since having another domain by the same name/uuid known to libvirt already can be a source of confusion for the user. -- Guido
On Thu, Jul 24, 2008 at 10:41:31AM -0400, Guido G?nther wrote:
On Thu, Jul 24, 2008 at 10:01:32AM +0100, Daniel P. Berrange wrote:
diff --git a/src/qemu_driver.c b/src/qemu_driver.c index b84bdf4..0ad72ae 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -1978,6 +1978,15 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (!(def = virDomainDefParseString(conn, driver->caps, xml))) return NULL;
+ vm = virDomainFindByName(driver->domains, def->name);
You need to check for UUID clash too. Indeed. But before fixing this I wonder what the exact semantics of domainCreateLinux are. Is it correct that we don't call virDomainSaveConfiguration?
Yes, that is correct. virDomainCreateLinux() starts a virtual machine with no config file. All trace of it will disappear when it shuts down - a so called 'transient' VM. Alternatively you can define the config first with virDomainDefineXML() and then start it based on this definition with virDomainCreate(). This gives you a persistent VM. Now, while a transient VM is running you can explicitly give it a config file by called virDomainDefineXML with the same uuid, thus turning it in to a persistent VM. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Thu, Jul 24, 2008 at 04:12:05PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 24, 2008 at 10:41:31AM -0400, Guido G?nther wrote:
On Thu, Jul 24, 2008 at 10:01:32AM +0100, Daniel P. Berrange wrote: [..snip..]
You need to check for UUID clash too. Indeed. But before fixing this I wonder what the exact semantics of domainCreateLinux are. Is it correct that we don't call virDomainSaveConfiguration?
Yes, that is correct.
virDomainCreateLinux() starts a virtual machine with no config file. All trace of it will disappear when it shuts down - a so called 'transient' VM.
Alternatively you can define the config first with virDomainDefineXML() and then start it based on this definition with virDomainCreate(). This gives you a persistent VM.
Now, while a transient VM is running you can explicitly give it a config file by called virDomainDefineXML with the same uuid, thus turning it in to a persistent VM. The attache patch also checks the uuid. -- Guido
It appears that this patch was applied (in commit 45616162db2d1e807dbe70e60c67cb701cbd06d8) with the virDomainIsActive(vm) checks removed from qemudDomainCreate, such that we fail out with "domain [...] is already defined and running" even if the domain is only defined but not running. The attached (completely trivial) patch (created against 301cbb70aa52db2d8c42bc9f9441366385f0a9c4) resolves this.
On Wed, Jul 30, 2008 at 12:05:56AM -0500, Charles Duffy wrote:
It appears that this patch was applied (in commit 45616162db2d1e807dbe70e60c67cb701cbd06d8) with the virDomainIsActive(vm) checks removed from qemudDomainCreate, such that we fail out with "domain [...] is already defined and running" even if the domain is only defined but not running.
The attached (completely trivial) patch (created against 301cbb70aa52db2d8c42bc9f9441366385f0a9c4) resolves this.
I agree that the message is wrong. But the idea that you can't create a temporary domain if there is one already defined with the same name or UUID seems sound to me. Are you disagreeing with the message (which your patch doesn't fix) or with the semantic of the check (and then why allow to create a domain reusing the UUID of another defined but not running domain, I can only see confusion or security problems in doing so) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard wrote:
Are you disagreeing with the message (which your patch doesn't fix) or with the semantic of the check (and then why allow to create a domain reusing the UUID of another defined but not running domain, I can only see confusion or security problems in doing so)
The act of creating a domain reusing the UUID of another defined but not running domain is presumably an action taken with intent to rename that other domain (and thus should undefine the prior domain holding that UUID in the process... which, admittedly, the given patch didn't explicitly do). Anyhow, two patches attached; one revises the error messages, while the other improves behavior with the semantics I assumed, to explicitly undefine any duplicates found during a create; I'm happy with either.
Blerg; the more complex patch I provided was dangerously wrong. Just applying the one that corrects the message WORKSFORME.
On Wed, Jul 30, 2008 at 02:06:29PM -0500, Charles Duffy wrote:
Blerg; the more complex patch I provided was dangerously wrong.
Just applying the one that corrects the message WORKSFORME.
Okay, done, thanks ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Jul 30, 2008 at 01:49:50PM -0500, Charles Duffy wrote:
Daniel Veillard wrote:
Are you disagreeing with the message (which your patch doesn't fix) or with the semantic of the check (and then why allow to create a domain reusing the UUID of another defined but not running domain, I can only see confusion or security problems in doing so)
The act of creating a domain reusing the UUID of another defined but not running domain is presumably an action taken with intent to rename that other domain (and thus should undefine the prior domain holding that UUID in the process... which, admittedly, the given patch didn't explicitly do).
Anyhow, two patches attached; one revises the error messages, while the other improves behavior with the semantics I assumed, to explicitly undefine any duplicates found during a create; I'm happy with either.
We shouldn't have 'create' automatically undefine the duplicate - we have a explicit undefine method apps can already use if they want that to be done, without needing to encode this policy in the create method. I'm fine with us applying the 2nd patch to fix the error message. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
It appears that this patch was applied (in commit 45616162db2d1e807dbe70e60c67cb701cbd06d8) with the virDomainIsActive(vm) checks removed from qemudDomainCreate, such that we fail out with "domain [...] is already defined and running" even if the domain is only defined but not running. The error message is confusing. I missed to correct that. Shall I send a
On Wed, Jul 30, 2008 at 12:05:56AM -0500, Charles Duffy wrote: patch? I thing doing more than that is simply too confusing for users. -- Guido
On Thu, Jul 31, 2008 at 07:08:33AM -0400, Guido Günther wrote:
It appears that this patch was applied (in commit 45616162db2d1e807dbe70e60c67cb701cbd06d8) with the virDomainIsActive(vm) checks removed from qemudDomainCreate, such that we fail out with "domain [...] is already defined and running" even if the domain is only defined but not running. The error message is confusing. I missed to correct that. Shall I send a
On Wed, Jul 30, 2008 at 12:05:56AM -0500, Charles Duffy wrote: patch? I thing doing more than that is simply too confusing for users.
No, no problem, Charles was being confused :-) all set now ! Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (4)
-
Charles Duffy -
Daniel P. Berrange -
Daniel Veillard -
Guido Günther