PATCH: Fix name uniqueness check

PATCH: Fix name uniqueness check

[Daniel P. Berrange]

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

Changeset  15124:f5459c358575 altered check_name() in XendDomainInfo so that
it compares domain IDs instead of UUIDs. This breaks a number of things

 - You can no longer use 'xm new' to define a persistent config file for
   a running guest. This breaks the key OS provisioning scenario where
   you boot a kenrel+initrd for the installer, and at the same time define
   a permanent config with pygrub.

 - It lets you define multiple inactive guests with different UUIDs, but  
   the same name because all inactive guests have a domid of None. So you
   can now end up with multiple guests with same name, which is contrary
   to the goal implied by the patch which was name uniqueness.

It is unclear from the original commit logs just what scenario it was trying
to protect against, but the original checking of uniqueness based on UUID 
was correct & is what was used in previous releases XenD.

Thus the attached patch reverts 15124

   Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

[-- Attachment #2: xen-name-check.patch --]
[-- Type: text/plain, Size: 738 bytes --]

diff -rup xen-3.1-testing.hg/tools/python/xen/xend/XendDomainInfo.py xen-3.1-testing-15432/tools/python/xen/xend/XendDomainInfo.py
--- xen-3.1-testing.hg/tools/python/xen/xend/XendDomainInfo.py	2007-09-27 11:54:38.000000000 -0400
+++ xen-3.1-testing-15432/tools/python/xen/xend/XendDomainInfo.py	2007-09-27 12:43:20.000000000 -0400
@@ -2150,7 +2150,7 @@ class XendDomainInfo:
             raise VmError('Invalid VM Name')
 
         dom =  XendDomain.instance().domain_lookup_nr(name)
-        if dom and dom.domid != self.domid:
+        if dom and dom.info['uuid'] != self.info['uuid']:
             raise VmError("VM name '%s' already exists%s" %
                           (name,
                            dom.domid is not None and

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: PATCH: Fix name uniqueness check

[Jim Fehlig]

Daniel P. Berrange wrote:
> Changeset  15124:f5459c358575 altered check_name() in XendDomainInfo so that
> it compares domain IDs instead of UUIDs. This breaks a number of things
>
>  - You can no longer use 'xm new' to define a persistent config file for
>    a running guest. This breaks the key OS provisioning scenario where
>    you boot a kenrel+initrd for the installer, and at the same time define
>    a permanent config with pygrub.
>
>  - It lets you define multiple inactive guests with different UUIDs, but  
>    the same name because all inactive guests have a domid of None. So you
>    can now end up with multiple guests with same name, which is contrary
>    to the goal implied by the patch which was name uniqueness.
>
> It is unclear from the original commit logs just what scenario it was trying
> to protect against, but the original checking of uniqueness based on UUID 
> was correct & is what was used in previous releases XenD.
>   

Yes, I was not sure what this patch was attempting to fix either.  There
was some discussion about the patch in this thread

http://lists.xensource.com/archives/html/xen-devel/2007-05/msg00887.html

At any rate, the patch was giving me some problems with reboot so I
added to the misery with this

http://xenbits2.xensource.com/xen-unstable.hg?rev/9f4f70233041

Regards,
Jim

Re: PATCH: Fix name uniqueness check

[Daniel P. Berrange]

On Thu, Sep 27, 2007 at 11:16:27AM -0600, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
> > Changeset  15124:f5459c358575 altered check_name() in XendDomainInfo so that
> > it compares domain IDs instead of UUIDs. This breaks a number of things
> >
> >  - You can no longer use 'xm new' to define a persistent config file for
> >    a running guest. This breaks the key OS provisioning scenario where
> >    you boot a kenrel+initrd for the installer, and at the same time define
> >    a permanent config with pygrub.
> >
> >  - It lets you define multiple inactive guests with different UUIDs, but  
> >    the same name because all inactive guests have a domid of None. So you
> >    can now end up with multiple guests with same name, which is contrary
> >    to the goal implied by the patch which was name uniqueness.
> >
> > It is unclear from the original commit logs just what scenario it was trying
> > to protect against, but the original checking of uniqueness based on UUID 
> > was correct & is what was used in previous releases XenD.
> >   
> 
> Yes, I was not sure what this patch was attempting to fix either.  There
> was some discussion about the patch in this thread
> 
> http://lists.xensource.com/archives/html/xen-devel/2007-05/msg00887.html

Ok, so if I follow that correctly, the crux of the issue is that it was 
possible to start 2 unmanaged domains with same name and same uuid. So
I think we can probably address that by checking for UUID, and the only
if both are running, also check for domid match. So really a combo of
both the original & current code.

> At any rate, the patch was giving me some problems with reboot so I
> added to the misery with this
> 
> http://xenbits2.xensource.com/xen-unstable.hg?rev/9f4f70233041


Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: PATCH: Fix name uniqueness check

[Jim Fehlig]

Daniel P. Berrange wrote:
> On Thu, Sep 27, 2007 at 11:16:27AM -0600, Jim Fehlig wrote:
>   
>> Daniel P. Berrange wrote:
>>     
>>> Changeset  15124:f5459c358575 altered check_name() in XendDomainInfo so that
>>> it compares domain IDs instead of UUIDs. This breaks a number of things
>>>
>>>  - You can no longer use 'xm new' to define a persistent config file for
>>>    a running guest. This breaks the key OS provisioning scenario where
>>>    you boot a kenrel+initrd for the installer, and at the same time define
>>>    a permanent config with pygrub.
>>>
>>>  - It lets you define multiple inactive guests with different UUIDs, but  
>>>    the same name because all inactive guests have a domid of None. So you
>>>    can now end up with multiple guests with same name, which is contrary
>>>    to the goal implied by the patch which was name uniqueness.
>>>
>>> It is unclear from the original commit logs just what scenario it was trying
>>> to protect against, but the original checking of uniqueness based on UUID 
>>> was correct & is what was used in previous releases XenD.
>>>   
>>>       
>> Yes, I was not sure what this patch was attempting to fix either.  There
>> was some discussion about the patch in this thread
>>
>> http://lists.xensource.com/archives/html/xen-devel/2007-05/msg00887.html
>>     
>
> Ok, so if I follow that correctly, the crux of the issue is that it was 
> possible to start 2 unmanaged domains with same name and same uuid. So
> I think we can probably address that by checking for UUID, and the only
> if both are running, also check for domid match. So really a combo of
> both the original & current code.
>   

Unstable, but not 3.1.1, also has

  http://xenbits2.xensource.com/xen-unstable.hg?rev/207582c8d88b

I did a little testing on a 3.1-based system that includes the above c/s
and your reversion of c/s 15124.  No problems noticed testing create,
new, reboot, save, restore.  Did not test migration or hvm guests.  So
perhaps reverting 15124 is fine for unstable but not sure about 3.1.1
*without* c/s 15642.

Regards,
Jim

Re: PATCH: Fix name uniqueness check

[Daniel P. Berrange]

On Thu, Sep 27, 2007 at 01:42:10PM -0600, Jim Fehlig wrote:
> Daniel P. Berrange wrote:
> > On Thu, Sep 27, 2007 at 11:16:27AM -0600, Jim Fehlig wrote:
> >> Yes, I was not sure what this patch was attempting to fix either.  There
> >> was some discussion about the patch in this thread
> >>
> >> http://lists.xensource.com/archives/html/xen-devel/2007-05/msg00887.html
> >>     
> >
> > Ok, so if I follow that correctly, the crux of the issue is that it was 
> > possible to start 2 unmanaged domains with same name and same uuid. So
> > I think we can probably address that by checking for UUID, and the only
> > if both are running, also check for domid match. So really a combo of
> > both the original & current code.
> >   
> 
> Unstable, but not 3.1.1, also has
> 
>   http://xenbits2.xensource.com/xen-unstable.hg?rev/207582c8d88b
> 
> I did a little testing on a 3.1-based system that includes the above c/s
> and your reversion of c/s 15124.  No problems noticed testing create,
> new, reboot, save, restore.  Did not test migration or hvm guests.  So
> perhaps reverting 15124 is fine for unstable but not sure about 3.1.1
> *without* c/s 15642.

Yep, I just tested 3.1.1 with 15124 reverted, and 15642 applied and it copes
with the use cases I have. So I'd reckon on making those two changes to the
3.1-testing tree would be sufficient. 

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: PATCH: Fix name uniqueness check

[Keir Fraser]

On 27/9/07 21:40, "Daniel P. Berrange" <berrange@redhat.com> wrote:

>> Unstable, but not 3.1.1, also has
>> 
>>   http://xenbits2.xensource.com/xen-unstable.hg?rev/207582c8d88b
>> 
>> I did a little testing on a 3.1-based system that includes the above c/s
>> and your reversion of c/s 15124.  No problems noticed testing create,
>> new, reboot, save, restore.  Did not test migration or hvm guests.  So
>> perhaps reverting 15124 is fine for unstable but not sure about 3.1.1
>> *without* c/s 15642.
> 
> Yep, I just tested 3.1.1 with 15124 reverted, and 15642 applied and it copes
> with the use cases I have. So I'd reckon on making those two changes to the
> 3.1-testing tree would be sufficient.

Is this still wanted? Masaki Kanno's response seems to indicate there are
still issues.

 -- Keir

Re: PATCH: Fix name uniqueness check

[Daniel P. Berrange]

On Mon, Oct 01, 2007 at 06:36:17AM +0100, Keir Fraser wrote:
> On 27/9/07 21:40, "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> >> Unstable, but not 3.1.1, also has
> >> 
> >>   http://xenbits2.xensource.com/xen-unstable.hg?rev/207582c8d88b
> >> 
> >> I did a little testing on a 3.1-based system that includes the above c/s
> >> and your reversion of c/s 15124.  No problems noticed testing create,
> >> new, reboot, save, restore.  Did not test migration or hvm guests.  So
> >> perhaps reverting 15124 is fine for unstable but not sure about 3.1.1
> >> *without* c/s 15642.
> > 
> > Yep, I just tested 3.1.1 with 15124 reverted, and 15642 applied and it copes
> > with the use cases I have. So I'd reckon on making those two changes to the
> > 3.1-testing tree would be sufficient.
> 
> Is this still wanted? Masaki Kanno's response seems to indicate there are
> still issues.

I need to try and reproduce Masaki's results & figure out what's missing.
We definitely need some form of patch, because as it stands two important
use cases are broken for xen-3.1-testing - provisioning of new VMs wit the
virt-install tool being one of them. I'd rather reduce the strictness of
name uniqueness checks to be too leniant than be too strict and break things

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: PATCH: Fix name uniqueness check

[Daniel P. Berrange]

On Mon, Oct 01, 2007 at 02:57:18PM +0100, Daniel P. Berrange wrote:
> On Mon, Oct 01, 2007 at 06:36:17AM +0100, Keir Fraser wrote:
> > On 27/9/07 21:40, "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > 
> > >> Unstable, but not 3.1.1, also has
> > >> 
> > >>   http://xenbits2.xensource.com/xen-unstable.hg?rev/207582c8d88b
> > >> 
> > >> I did a little testing on a 3.1-based system that includes the above c/s
> > >> and your reversion of c/s 15124.  No problems noticed testing create,
> > >> new, reboot, save, restore.  Did not test migration or hvm guests.  So
> > >> perhaps reverting 15124 is fine for unstable but not sure about 3.1.1
> > >> *without* c/s 15642.
> > > 
> > > Yep, I just tested 3.1.1 with 15124 reverted, and 15642 applied and it copes
> > > with the use cases I have. So I'd reckon on making those two changes to the
> > > 3.1-testing tree would be sufficient.
> > 
> > Is this still wanted? Masaki Kanno's response seems to indicate there are
> > still issues.
> 
> I need to try and reproduce Masaki's results & figure out what's missing.
> We definitely need some form of patch, because as it stands two important
> use cases are broken for xen-3.1-testing - provisioning of new VMs wit the
> virt-install tool being one of them. I'd rather reduce the strictness of
> name uniqueness checks to be too leniant than be too strict and break things

There are 6 basic test cases:

   a. same name + same UUID:
   b. diff name + same UUID:
   c. same name + diff UUID:
   d. diff name + diff UUID:
   e. same name + no UUID:
   f. diff name + no UUID:

These need to be tested with 'xm create' and 'xm new', and all tests need to
be done with the pre-existing VM both inactive, and active.

As it stands now, 3.1-testing  incorrectly rejects several of the test
cases. It also incorrectly allows several others. 

Adding in http://xenbits2.xensource.com/xen-unstable.hg?rev/207582c8d88b
from xen-unstable causes it to incorrectly allow sevaral more, and does
not fix the false rejects.

Reverting 15124:f5459c358575  removes the false rejects, but doesn't
deal with the false allows.

So no combination of patches from either 3.1-testing, or -unstable is
showing completely correct behaviour & both trees need further fixes.
Hopefully have something more later today...

Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: PATCH: Fix name uniqueness check

[Daniel P. Berrange]

[-- Attachment #1: Type: text/plain, Size: 4407 bytes --]

On Mon, Oct 01, 2007 at 07:18:11PM +0100, Daniel P. Berrange wrote:
> There are 6 basic test cases:
> 
>    a. same name + same UUID:
>    b. diff name + same UUID:
>    c. same name + diff UUID:
>    d. diff name + diff UUID:
>    e. same name + no UUID:
>    f. diff name + no UUID:
> 
> These need to be tested with 'xm create' and 'xm new', and all tests need to
> be done with the pre-existing VM both inactive, and active.

So, to 6 tests under 4 scenarios, gives 24 combinations. NB in the list
above from my previous mail I flipped some letters compared to my actual
testsuite - so pay attention to the letters used below, not the ones above.

UUID is the primary unique key for VMs. Name is the secondary unique
key. The former takes priority, eg redefining with existing UUID, but
different name causes a rename in the config stored.

  - Scenario 1: xm new + existing inactive vm

      a. same name + same UUID:  ALLOW  (redefine)
      b. same name + diff UUID:  REJECT (clashing name)
      c. diff name + same UUID:  ALLOW  (redefine+rename)
      d. diff name + diff UUID:  ALLOW  (define)
      e. same name + no UUID:    REJECT (clashing name)
      f. diff name + no UUID:    ALLOW  (define)

  - Scenario 2: xm create + existing inactive vm

      a. same name + same UUID:  ALLOW  (create)
      b. same name + diff UUID:  REJECT (clashing name)
      c. diff name + same UUID:  ALLOW  (create)
      d. diff name + diff UUID:  ALLOW  (create)
      e. same name + no UUID:    REJECT (clashing name)
      f. diff name + no UUID:    ALLOW  (create)

  - Scenario 3: xm new + existing active vm

      a. same name + same UUID:  ALLOW  (redefine)
      b. same name + diff UUID:  REJECT (clashing name)
      c. diff name + same UUID:  ALLOW  (redefine+rename)
      d. diff name + diff UUID:  ALLOW  (define)
      e. same name + no UUID:    REJECT (clashing name)
      f. diff name + no UUID:    ALLOW  (define)

  - Scenario 4: xm create + existing active vm

      a. same name + same UUID:  REJECT (clashing name/uuid)
      b. same name + diff UUID:  REJECT (clashing name)
      c. diff name + same UUID:  REJECT (clashing uuid)
      d. diff name + diff UUID:  REJECT (disk image in use)
      e. same name + no UUID:    REJECT (clashing name)
      f. diff name + no UUID:    REJECT (disk image in use)

Basically the allow/reject rules at the same for the first 3 scenarios.
The reject scnarios are dealt with by the _checkName() function in the
XendDomainInfo class. Only in the last one do we have to reject all combos
more aggressively. This is because if you 'xm create' the same VM with 
identical details it wouldn't be stopped by the hotplug disk checks in 
all cases. This is not possible to validate from _checkName() since it 
does not have enough context to know that the new VM is about to be started.
So we have to make extra checks in the 'create()' function in XendDomainInfo

I am attaching a file 'test.sh' which creates a test VM & configs and runs
through all these 24 scenarios. Feel free to add this to XenD's regression
test suite if practical.

So the actual patches needed...

In 3.1-testing

 - Revert 15168:a717cb2fac90  (aka pull in 15973:8817a53c030f from 
   xen-unstable.hg). This addresses scenarios 1-3 above

 - Apply the 'xen-create-name-uniqueness.patch'  which adds a check
   to XendDomainInfo.create() to deal with special needs of scenario 4

In xen-unstable

 - Revert 15642:207582c8d88b (this patch is now bogus since 15168:a717cb2fac90
   is already reverted by 15973:8817a53c030f). It only covered 'xm new'
   where as proposed patches cover 'xm new' and 'xm create' together

 - Apply the 'xen-create-name-uniqueness.patch'  which adds a check
   to XendDomainInfo.create() to deal with special needs of scenario 4

I don't have a xen-unstable.hg box currently, so my testing of these patches
has been against the 3.1-testing.hg tree. The 'test.sh' script should be able
to validate them for unstable. BTW, run test.sh with /etc/xen as CWD.

  Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

[-- Attachment #2: xen-create-name-uniqueness.patch --]
[-- Type: text/plain, Size: 1066 bytes --]

diff -r 687ce253a0f4 tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Wed Sep 26 10:05:02 2007 +0100
+++ b/tools/python/xen/xend/XendDomainInfo.py	Mon Oct 01 17:07:43 2007 -0400
@@ -74,9 +74,15 @@ def create(config):
     @return: An up and running XendDomainInfo instance
     @raise VmError: Invalid configuration or failure to start.
     """
-
+    from xen.xend import XendDomain
+    domconfig = XendConfig.XendConfig(sxp_obj = config)
+    othervm = XendDomain.instance().domain_lookup_nr(domconfig["name_label"])
+    if othervm is None or othervm.domid is None:
+        othervm = XendDomain.instance().domain_lookup_nr(domconfig["uuid"])
+    if othervm is not None and othervm.domid is not None:
+        raise VmError("Domain '%s' already exists with ID '%d'" % (domconfig["name_label"], othervm.domid))
     log.debug("XendDomainInfo.create(%s)", scrub_password(config))
-    vm = XendDomainInfo(XendConfig.XendConfig(sxp_obj = config))
+    vm = XendDomainInfo(domconfig)
     try:
         vm.start()
     except:

[-- Attachment #3: test.sh --]
[-- Type: application/x-sh, Size: 3905 bytes --]

[-- Attachment #4: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: PATCH: Fix name uniqueness check

[Masaki Kanno]

Hi Dan,

Thanks for your effort and your patch. 
I think that the allow/reject rules are wonderful.  But, I have a few 
comments. 


I agree the rule of the following case. 
But, the behavior is (redefine+rename+create), isn't it?

>  - Scenario 2: xm create + existing inactive vm
>      c. diff name + same UUID:  ALLOW  (create)

# xm new /xen/vm1.conf uuid="11111111-2222-3333-4444-555555555555"
Using config file "/xen/vm1.conf".
# xm list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   941     2     r-----     85.0
vm1                                              256     1                 0.0
# xm create /xen/vm2.conf uuid="11111111-2222-3333-4444-555555555555"
Using config file "/xen/vm2.conf".
Started domain vm2
# xm list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   743     2     r-----     88.9
vm2                                          1   256     1     r-----      2.2
# cat /var/lib/xend/domains/11111111-2222-3333-4444-555555555555/config.sxp | grep name
    (name_label vm2)
    (name vm2)
            (uname file:/xen/root-vm2.img)
# xm shutdown vm2
# xm list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   743     2     r-----     95.9
vm2                                              256     1                32.0


When I tested the following case, the result was as follows. 
I think that we should reject xm new command if same UUID vm is active. 

>  - Scenario 3: xm new + existing active vm
>      c. diff name + same UUID:  ALLOW  (redefine+rename)

# xm create /xen/vm1.conf uuid="11111111-2222-3333-4444-555555555555"
Using config file "/xen/vm1.conf".
Started domain vm1
# xm list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   743     2     r-----    105.6
vm1                                          2   256     1     -b----      1.4
# xm new /xen/vm2.conf uuid="11111111-2222-3333-4444-555555555555"
Using config file "/xen/vm2.conf".
# xm list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   743     2     r-----    110.7
vm1                                          2   256     1     -b----     26.5
# xm start vm2
# xm list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   491     2     r-----    114.8
vm1                                          2   256     1     -b----     26.6
vm2                                          3   256     1     r-----     20.5
# xm list --long vm1 vm2 | grep uuid
    (uuid 11111111-2222-3333-4444-555555555555)
            (uuid ed6136f4-f140-fbdb-49db-861499cd54ad)
            (uuid 632ba398-1cfe-831f-cfdd-a5e1b92ae7c6)
            (uuid 028f4c5d-5850-0eda-e0b5-cde0f252be13)
    (uuid 11111111-2222-3333-4444-555555555555)
            (uuid 12427bbe-6120-a864-bb5b-56ed5f8d14c5)
            (uuid 56ba82ca-4a62-42e7-3eb7-b15578bf828b)

Best regards,
 Kan

Re: PATCH: Fix name uniqueness check

[Daniel P. Berrange]

On Tue, Oct 02, 2007 at 10:11:01PM +0900, Masaki Kanno wrote:
> Hi Dan,
> 
> Thanks for your effort and your patch. 
> I think that the allow/reject rules are wonderful.  But, I have a few 
> comments. 
> 
> 
> I agree the rule of the following case. 
> But, the behavior is (redefine+rename+create), isn't it?

Yes, that is actually what it ends up doing, replacing the config for the 
matching UUID causes a rename.

> When I tested the following case, the result was as follows. 
> I think that we should reject xm new command if same UUID vm is active. 

I hadn't noticed that, but its easy to special case this particular
case / scenario to be rejected. Or we could fix it to correctly rename
the existing running VM which might be more user friendly. 

Either option is a small add-on patch to my previous submissions.

Regards,
Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

Re: PATCH: Fix name uniqueness check

[Masaki Kanno]

Tue, 2 Oct 2007 15:47:12 +0100, "Daniel P. Berrange" wrote:

>On Tue, Oct 02, 2007 at 10:11:01PM +0900, Masaki Kanno wrote:
>> Hi Dan,
>> 
>> Thanks for your effort and your patch. 
>> I think that the allow/reject rules are wonderful.  But, I have a few 
>> comments. 
>> 
>> 
>> I agree the rule of the following case. 
>> But, the behavior is (redefine+rename+create), isn't it?
>
>Yes, that is actually what it ends up doing, replacing the config for the 
>matching UUID causes a rename.
>
>> When I tested the following case, the result was as follows. 
>> I think that we should reject xm new command if same UUID vm is active. 
>
>I hadn't noticed that, but its easy to special case this particular
>case / scenario to be rejected. Or we could fix it to correctly rename
>the existing running VM which might be more user friendly. 
>
>Either option is a small add-on patch to my previous submissions.

Sorry for delay with replies to your message. 

I am worried about changing the configuration of existing running VM. 
But, my worry is vague, and does not have great grounds. 
If UUID is same, maybe we will become possible to change the name of 
the VM and all the configuration of the VM by xm new command, I guess. 

If only the config.sxp of the VM is changed by xm new command, and if 
the definition of the config.sxp is reflected after xm shutdown command, 
my worry will be resolved. 

Best regards,
 Kan

Re: PATCH: Fix name uniqueness check

[Daniel P. Berrange]

[-- Attachment #1: Type: text/plain, Size: 2106 bytes --]

On Fri, Oct 05, 2007 at 02:19:16PM +0900, Masaki Kanno wrote:
> Tue, 2 Oct 2007 15:47:12 +0100, "Daniel P. Berrange" wrote:
> 
> >On Tue, Oct 02, 2007 at 10:11:01PM +0900, Masaki Kanno wrote:
> >> Hi Dan,
> >> 
> >> Thanks for your effort and your patch. 
> >> I think that the allow/reject rules are wonderful.  But, I have a few 
> >> comments. 
> >> 
> >> 
> >> I agree the rule of the following case. 
> >> But, the behavior is (redefine+rename+create), isn't it?
> >
> >Yes, that is actually what it ends up doing, replacing the config for the 
> >matching UUID causes a rename.
> >
> >> When I tested the following case, the result was as follows. 
> >> I think that we should reject xm new command if same UUID vm is active. 
> >
> >I hadn't noticed that, but its easy to special case this particular
> >case / scenario to be rejected. Or we could fix it to correctly rename
> >the existing running VM which might be more user friendly. 
> >
> >Either option is a small add-on patch to my previous submissions.
> 
> Sorry for delay with replies to your message. 
> 
> I am worried about changing the configuration of existing running VM. 
> But, my worry is vague, and does not have great grounds. 
> If UUID is same, maybe we will become possible to change the name of 
> the VM and all the configuration of the VM by xm new command, I guess. 
> 
> If only the config.sxp of the VM is changed by xm new command, and if 
> the definition of the config.sxp is reflected after xm shutdown command, 
> my worry will be resolved. 

Kan, give this patch a try which simply updates the name_label field for
the exsting VM. With this minimal rename it avoids the risk of the same VM
being started twice as you demonstrated 

  Signed-off-by: Daniel P. Berrange <berrange@redhat.com


Dan.
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

[-- Attachment #2: xen-active-rename.patch --]
[-- Type: text/plain, Size: 949 bytes --]

diff -r 5f95b278cdbd tools/python/xen/xend/XendDomainInfo.py
--- a/tools/python/xen/xend/XendDomainInfo.py	Wed Oct 03 18:15:30 2007 -0400
+++ b/tools/python/xen/xend/XendDomainInfo.py	Fri Oct 05 09:33:22 2007 -0400
@@ -229,12 +235,16 @@ def createDormant(domconfig):
     @return: A up and running XendDomainInfo instance
     @raise XendError: Errors with configuration.    
     """
-    
+    from xen.xend import XendDomain
+    othervm = XendDomain.instance().domain_lookup_nr(domconfig["uuid"])
     log.debug("XendDomainInfo.createDormant(%s)", scrub_password(domconfig))
     
     # domid does not make sense for non-running domains.
     domconfig.pop('domid', None)
     vm = XendDomainInfo(domconfig)
+    # If there was an existing VM with UUID make sure it is renamed
+    if othervm is not None and othervm.domid is not None:
+        othervm.info["name_label"] = vm.info["name_label"]
     return vm    
 
 def domain_by_name(name):

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: PATCH: Fix name uniqueness check

[Masaki Kanno]

Fri, 5 Oct 2007 14:35:16 +0100, "Daniel P. Berrange" wrote:

>On Fri, Oct 05, 2007 at 02:19:16PM +0900, Masaki Kanno wrote:
>> Tue, 2 Oct 2007 15:47:12 +0100, "Daniel P. Berrange" wrote:
>> 
>> >On Tue, Oct 02, 2007 at 10:11:01PM +0900, Masaki Kanno wrote:
>> >> Hi Dan,
>> >> 
>> >> Thanks for your effort and your patch. 
>> >> I think that the allow/reject rules are wonderful.  But, I have a few 
>> >> comments. 
>> >> 
>> >> 
>> >> I agree the rule of the following case. 
>> >> But, the behavior is (redefine+rename+create), isn't it?
>> >
>> >Yes, that is actually what it ends up doing, replacing the config for the 
>> >matching UUID causes a rename.
>> >
>> >> When I tested the following case, the result was as follows. 
>> >> I think that we should reject xm new command if same UUID vm is active. 
>> >
>> >I hadn't noticed that, but its easy to special case this particular
>> >case / scenario to be rejected. Or we could fix it to correctly rename
>> >the existing running VM which might be more user friendly. 
>> >
>> >Either option is a small add-on patch to my previous submissions.
>> 
>> Sorry for delay with replies to your message. 
>> 
>> I am worried about changing the configuration of existing running VM. 
>> But, my worry is vague, and does not have great grounds. 
>> If UUID is same, maybe we will become possible to change the name of 
>> the VM and all the configuration of the VM by xm new command, I guess. 
>> 
>> If only the config.sxp of the VM is changed by xm new command, and if 
>> the definition of the config.sxp is reflected after xm shutdown command, 
>> my worry will be resolved. 
>
>Kan, give this patch a try which simply updates the name_label field for
>the exsting VM. With this minimal rename it avoids the risk of the same VM
>being started twice as you demonstrated 
>

Hi Dan,

Thank you.  The problem that I found first was resolved. 

# xm create /xen/vm1.conf uuid="11111111-2222-3333-4444-555555555555"
Using config file "/xen/vm1.conf".
Started domain vm1
# xm list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   747     2     r-----    389.8
vm1                                          2   256     1     -b----      1.3
# xm new /xen/vm2.conf uuid="11111111-2222-3333-4444-555555555555"
Using config file "/xen/vm2.conf".
# xm list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   747     2     r-----    392.5
vm2                                          2   256     1     ------     18.2


But, I found another problem...  When I rebooted the VM, the name of the VM 
was restored to 'vm1'.  Could you fix the problem too? 

# xm new /xen/vm1.conf uuid="11111111-2222-3333-4444-555555555555"
Using config file "/xen/vm1.conf".
# xm start vm1
# xm list
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   747     2     r-----    410.3
vm1                                          3   256     1     -b----     26.2
# xm new /xen/vm2.conf uuid="11111111-2222-3333-4444-555555555555"
Using config file "/xen/vm2.conf".
# xm reboot vm2
# xm list 
Name                                        ID   Mem VCPUs      State   Time(s)
Domain-0                                     0   747     2     r-----    420.4
vm1                                          4   256     1     -b----     25.8


Best regards,
 Kan

Re: PATCH: Fix name uniqueness check

[Masaki Kanno]

Hi,

I tested xen-unstable 15967 and staging 15973.

[xm create]
 if an UUID is specified:
     if a VM with same name exists:
         if the VM is same UUID too:
             ==> xm create succeed, but...
             # xm list
             Name     ID   Mem VCPUs      State   Time(s)
             Domain-0  0   491     2     r-----    109.3
             vm1       2   256     1     r-----   3636.3
             vm1       4   256     1     --p---      0.0
             
         else:
             ==> xm create failed
             Error: VM name 'vm1' already exists as domain 2
             
     else:
         if a VM with same UUID exists:
             ==> xm create succeed... Right?
             
         else:
             ==> xm create succeed
             
 else:
     if a VM with same name exists:
         ==> xm create failed
         Error: VM name 'vm1' already exists as domain 2
         
     else:
         ==> xm create succeed
         

[xm new]
 if an UUID is specified:
     if a VM with same name exists:
         if the VM is same UUID too:
             ==> xm new failed?  but an error message was not shown.
             # xm new /xen/vm2.conf name="vm1"
             Using config file "/xen/vm2.conf".
             # xm list
             Name     ID   Mem VCPUs      State   Time(s)
             Domain-0  0   491     2     r-----    243.9
             vm1       2   256     1     r-----  10003.1
             
         else:
             ==> xm new failed
             Error: Domain name 'vm1' is already used.
             
     else:
         if a VM with same UUID exists:
             ==> xm new failed
             Error: Domain UUID '3ef95bbe-57a9-1caf-9a15-27ab27eb66b3' 
                    is already used.
         else:
             ==> xm new succeed
             
 else:
     if a VM with same name exists:
         ==> xm new failed?  but an error message was not shown.
         
     else:
         ==> xm new succeed
         

Best regards,
 Kan

Thu, 27 Sep 2007 17:50:51 +0100, "Daniel P. Berrange" wrote:

>Changeset  15124:f5459c358575 altered check_name() in XendDomainInfo so that
>it compares domain IDs instead of UUIDs. This breaks a number of things
>
> - You can no longer use 'xm new' to define a persistent config file for
>   a running guest. This breaks the key OS provisioning scenario where
>   you boot a kenrel+initrd for the installer, and at the same time define
>   a permanent config with pygrub.
>
> - It lets you define multiple inactive guests with different UUIDs, but  
>   the same name because all inactive guests have a domid of None. So you
>   can now end up with multiple guests with same name, which is contrary
>   to the goal implied by the patch which was name uniqueness.
>
>It is unclear from the original commit logs just what scenario it was trying
>to protect against, but the original checking of uniqueness based on UUID 
>was correct & is what was used in previous releases XenD.
>
>Thus the attached patch reverts 15124
>
>   Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>
>Regards,
>Dan.
>-- 
>|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
>|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
>|=-               Projects: http://freshmeat.net/~danielpb/               -=|
>|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -
>|=| 
>
>-------------------------------text/plain-------------------------------
>_______________________________________________
>Xen-devel mailing list
>Xen-devel@lists.xensource.com
>http://lists.xensource.com/xen-devel

----
菅野 大樹/Masaki Kanno,TEL 055-924-7349(7551-5366),富士通(株)沼津工場B4F
サーバ本)Linux技術開発統括部 kanno.masaki@jp.fujitsu.com

Re: PATCH: Fix name uniqueness check

[Masaki Kanno]

Hi,

Next, I tested xen-unstable 15967 without staging 15973.
I think that new problems occur by staging 15973.  I will investigate 
causes to solve new problems.

[xm create]
 if an UUID is specified:
     if a VM with same name exists:
         if the VM is same UUID too:
             ==> xm create failed
             Error: VM name 'vm1' already exists as domain 1
             
         else:
             ==> xm create failed
             Error: VM name 'vm1' already exists as domain 1
             
     else:
         if a VM with same UUID exists:
             ==> xm create succeed... Right?
             
         else:
             ==> xm create succeed
             
 else:
     if a VM with same name exists:
         ==> xm create failed
         Error: VM name 'vm1' already exists as domain 1
         
     else:
         ==> xm create succeed
         

[xm new]
 if an UUID is specified:
     if a VM with same name exists:
         if the VM is same UUID too:
             ==> xm new failed
             Error: VM name 'vm1' already exists as domain 1
             
         else:
             ==> xm new failed
             Error: Domain name 'vm1' is already used.
             
     else:
         if a VM with same UUID exists:
             ==> xm new failed
             Error: Domain UUID '191a8164-aaff-c9fe-63b4-493eba17021a' 
                    is already used.
         else:
             ==> xm new succeed
             
 else:
     if a VM with same name exists:
         ==> xm new failed
         Error: VM name 'vm1' already exists as domain 1
         
     else:
         ==> xm new succeed
         

Best regards,
 Kan

Fri, 28 Sep 2007 12:59:27 +0900, Masaki Kanno wrote:

>Hi,
>
>I tested xen-unstable 15967 and staging 15973.
>
>[xm create]
> if an UUID is specified:
>     if a VM with same name exists:
>         if the VM is same UUID too:
>             ==> xm create succeed, but...
>             # xm list
>             Name     ID   Mem VCPUs      State   Time(s)
>             Domain-0  0   491     2     r-----    109.3
>             vm1       2   256     1     r-----   3636.3
>             vm1       4   256     1     --p---      0.0
>             
>         else:
>             ==> xm create failed
>             Error: VM name 'vm1' already exists as domain 2
>             
>     else:
>         if a VM with same UUID exists:
>             ==> xm create succeed... Right?
>             
>         else:
>             ==> xm create succeed
>             
> else:
>     if a VM with same name exists:
>         ==> xm create failed
>         Error: VM name 'vm1' already exists as domain 2
>         
>     else:
>         ==> xm create succeed
>         
>
>[xm new]
> if an UUID is specified:
>     if a VM with same name exists:
>         if the VM is same UUID too:
>             ==> xm new failed?  but an error message was not shown.
>             # xm new /xen/vm2.conf name="vm1"
>             Using config file "/xen/vm2.conf".
>             # xm list
>             Name     ID   Mem VCPUs      State   Time(s)
>             Domain-0  0   491     2     r-----    243.9
>             vm1       2   256     1     r-----  10003.1
>             
>         else:
>             ==> xm new failed
>             Error: Domain name 'vm1' is already used.
>             
>     else:
>         if a VM with same UUID exists:
>             ==> xm new failed
>             Error: Domain UUID '3ef95bbe-57a9-1caf-9a15-27ab27eb66b3' 
>                    is already used.
>         else:
>             ==> xm new succeed
>             
> else:
>     if a VM with same name exists:
>         ==> xm new failed?  but an error message was not shown.
>         
>     else:
>         ==> xm new succeed
>         
>
>Best regards,
> Kan
>
>Thu, 27 Sep 2007 17:50:51 +0100, "Daniel P. Berrange" wrote:
>
>>Changeset  15124:f5459c358575 altered check_name() in XendDomainInfo so that
>>it compares domain IDs instead of UUIDs. This breaks a number of things
>>
>> - You can no longer use 'xm new' to define a persistent config file for
>>   a running guest. This breaks the key OS provisioning scenario where
>>   you boot a kenrel+initrd for the installer, and at the same time define
>>   a permanent config with pygrub.
>>
>> - It lets you define multiple inactive guests with different UUIDs, but  
>>   the same name because all inactive guests have a domid of None. So you
>>   can now end up with multiple guests with same name, which is contrary
>>   to the goal implied by the patch which was name uniqueness.
>>
>>It is unclear from the original commit logs just what scenario it was trying
>>to protect against, but the original checking of uniqueness based on UUID 
>>was correct & is what was used in previous releases XenD.
>>
>>Thus the attached patch reverts 15124
>>
>>   Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>>
>>Regards,
>>Dan.
>>-- 
>>|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 
>>|-=|
>>|=-           Perl modules: http://search.cpan.org/~danberr/              
>>|-=|
>>|=-               Projects: http://freshmeat.net/~danielpb/               
>>|-=|
>>|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -
>>|=| 
>>