Royce Lv has uploaded a new change for review.
Change subject: fix getVmList 0 sdUUID error ......................................................................
fix getVmList 0 sdUUID error
BZ#807719:getVmsList get broken when running without sdUUID if sdUUID specified,enumerate vm lists of given sdUUID if sdUUID not specified,enumerate vm lists of pool's master domain we should get shared lock of master sdUUID when sdUUID = 0/None instead of getting lock of storage.none/storage.0
Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Signed-off-by: Royce Lv lvroyce@linux.vnet.ibm.com --- M vdsm/storage/hsm.py 1 file changed, 3 insertions(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/60/3560/1 -- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com
Dan Kenigsberg has posted comments on this change.
Change subject: fix getVmList 0 sdUUID error ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(1 inline comment)
Could you move the Bugzilla bug to POST and a link to this patch? It should help other BZ readers to understand where this bug stands.
The code itself seems fine to me, but I do not understand the commit message at all.
.................................................... Commit Message Line 13: getting lock of storage.none/storage.0 I do not understand the commit message. Could you use multiple, short sentences?
-- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com
Igor Lvovsky has posted comments on this change.
Change subject: fix getVmList 0 sdUUID error ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(1 inline comment)
.................................................... File vdsm/storage/hsm.py Line 1136: self.validateSdUUID(sdUUID) Hmmm, I think it should be:
# Only backup domains are allowed in this path
self.validateBackupDom(sdUUID)
-- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com
Ayal Baron has posted comments on this change.
Change subject: fix check None sdUUID for getVmsList and getVmsInfo ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(1 inline comment)
.................................................... File vdsm/storage/hsm.py Line 1136: self.validateBackupDom(sdUUID) This will break the moment we start storing OVFs on all the domains (we would like to do this because then there is no dependency on master domain and we could import existing domains and run VMs from vdsm directly without manager even if master domain is not available etc).
So I think we should remove this validation. If the user passed a domain which doesn't have any OVFs on it we should just make sure to fail gracefully.
-- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com
Royce Lv has posted comments on this change.
Change subject: fix check None sdUUID for getVmsList and getVmsInfo ......................................................................
Patch Set 2: (1 inline comment)
.................................................... File vdsm/storage/hsm.py Line 1136: self.validateBackupDom(sdUUID) so if we store OVFs on all domains, when we call getVmsList with no sdUUID specified, it means we should enumerate all storage domains of the storage pool to see whether it has OVFs on it, right? Now we just enumerate master domain in this case. At this moment, shall I change it or leave it what it is? Thank you for suggestion: )
-- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com
Igor Lvovsky has posted comments on this change.
Change subject: fix check None sdUUID for getVmsList and getVmsInfo ......................................................................
Patch Set 2: Looks good to me, but someone else must approve
(2 inline comments)
.................................................... File vdsm/storage/hsm.py Line 1136: self.validateBackupDom(sdUUID) Ayal, you have this behavior on several places in the code. So, If you want to support future OVF spreading among different domains let's do it in separate patch in ALL places. This patch fix very particular BZ, let's keep it like this
Line 1159: if sdUUID and sdUUID != sd.BLANK_UUID: Ah, I saw it but I didn't want to bothering you with this one too, because this not related to your BZ. But if you already touched it and only if you will send another patch for this. Please fix it to: if sdUUID != sd.BLANK_UUID: the sdUUID can't be None here or put default None to function call
-- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com
Ayal Baron has posted comments on this change.
Change subject: fix check None sdUUID for getVmsList and getVmsInfo ......................................................................
Patch Set 2: (1 inline comment)
.................................................... File vdsm/storage/hsm.py Line 1136: self.validateBackupDom(sdUUID) There is a difference between adding support for something and preventing it from happening. The "validate" line should not be here, that's it, no need to start supporting searching OVFs and the like.
-- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com
Royce Lv has posted comments on this change.
Change subject: fix check None sdUUID for getVmsList and getVmsInfo ......................................................................
Patch Set 2: (1 inline comment)
.................................................... File vdsm/storage/hsm.py Line 1159: if sdUUID and sdUUID != sd.BLANK_UUID: I guess sdUUID still can be 0 if I run: vdsClient 0 getVmsInfo <spuuid> 0 , so in this case, same error will still be raised
-- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com
Igor Lvovsky has posted comments on this change.
Change subject: fix check None sdUUID for getVmsList and getVmsInfo ......................................................................
Patch Set 2: (2 inline comments)
.................................................... File vdsm/storage/hsm.py Line 1136: self.validateBackupDom(sdUUID) Ayal, I still not agree with you, but if you preferred previous behavior... Royce, sorry for all this noise. Please go with Ayal's decision and leave it as previously
Line 1159: if sdUUID and sdUUID != sd.BLANK_UUID: OK, leave it
-- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com
Igor Lvovsky has posted comments on this change.
Change subject: fix check None sdUUID for getVmsList and getVmsInfo ......................................................................
Patch Set 3: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com
Ayal Baron has posted comments on this change.
Change subject: fix check None sdUUID for getVmsList and getVmsInfo ......................................................................
Patch Set 3: Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com
Royce Lv has posted comments on this change.
Change subject: fix check None sdUUID for getVmsList and getVmsInfo ......................................................................
Patch Set 3: Verified
-- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com
Igor Lvovsky has submitted this change and it was merged.
Change subject: fix check None sdUUID for getVmsList and getVmsInfo ......................................................................
fix check None sdUUID for getVmsList and getVmsInfo
BZ#807719:getVmsList get broken when running without sdUUID when sdUUID given, validate domain uuid, when sdUUID is None, enumerate Vms from master domain of the pool v3>v2, change according to Ayal's comments,clear validate backup domain
Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Signed-off-by: Royce Lv lvroyce@linux.vnet.ibm.com --- M vdsm/storage/hsm.py 1 file changed, 6 insertions(+), 2 deletions(-)
Approvals: Ayal Baron: Looks good to me, approved Royce Lv: Verified Igor Lvovsky: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/3560 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: I27fe906e6738320279bc44f9736acaa97131d7ee Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Royce Lv lvroyce@linux.vnet.ibm.com Gerrit-Reviewer: Adam Litke agl@us.ibm.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Royce Lv lvroyce@linux.vnet.ibm.com
vdsm-patches@lists.fedorahosted.org