10:51 a.m.
Federico Simoncelli has uploaded a new change for review.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Avoid to activate parent volume on cloning
The benefit is that we can now create new snapshot volumes without
looking at the parent. This is indispensable for live snapshots since
the parent volume is in use by the VM.
In this patch:
* Do not activate the parent volume on cloning (it could be in use
by the VM), the only information needed is the size and it can be
provided as additional parameter in qemu-img
* Do not change the volume permission to read-only when setting the
parent to "internal" (it could be in use by the VM)
Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
---
M vdsm/storage/volume.py
1 file changed, 11 insertions(+), 13 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/70/570/1
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
8:15 a.m.
Dan Kenigsberg has submitted this change and it was merged.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Avoid to activate parent volume on cloning
The benefit is that we can now create new snapshot volumes without
looking at the parent. This is indispensable for live snapshots since
the parent volume is in use by the VM.
WARNING: if you run an SPM host with this change you need all the
HSM hosts to have: I4b5cb55a868cb9bf5a720610273af139ee858a1a (Don't
lock the image when preparing the VM path) and you won't be able use
the domain from old SPMs/HSMs anymore.
In this patch:
* Do not activate the parent volume on cloning (it could be in use
by the VM), the only information needed is the size and it can be
provided as additional parameter in qemu-img
* Do not change the volume permission to read-only when setting the
parent to "internal" (it could be in use by the VM)
Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
---
M vdsm/storage/volume.py
1 file changed, 15 insertions(+), 13 deletions(-)
Approvals:
Dan Kenigsberg: Verified
Igor Lvovsky: Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
8:15 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 10: Verified
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
7:43 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 10: Looks good to me, approved
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
11:20 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 8: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
8:41 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 7: (1 inline comment)
....................................................
File vdsm/storage/volume.py
Line 456: # volume.
hmmm, removing this would crate havoc if the Vm is later run by a host without
http://gerrit.ovirt.org/797
(Fix HSM flows should not change rw permissions)
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
8:09 a.m.
Dan Kenigsberg has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 7: (1 inline comment)
....................................................
File vdsm/storage/volume.py
Line 872: # Temprary fix for qemu-img creation problem
maybe not, but should not be fixed in this patch..
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
8:06 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 6: Looks good to me, but someone else must approve
(2 inline comments)
Only minor things
....................................................
File vdsm/storage/volume.py
Line 870: # cwd = os.path.split(os.path.split(volume)[0])[0]
Why this dead code?
I know that it not your, but if you already touch this...
Line 872: # Temprary fix for qemu-img creation problem
Is comment still relevant?
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
7:54 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 7: Looks good to me, but someone else must approve
(2 inline comments)
Two minor comments
....................................................
File vdsm/storage/volume.py
Line 870: # cwd = os.path.split(os.path.split(volume)[0])[0]
Why this dead code?
Line 872: # Temprary fix for qemu-img creation problem
Is this still relevant?
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
1:19 p.m.
Igor Lvovsky has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 9: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
6:52 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 10: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi(a)redhat.com>
6:22 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 4: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
4:43 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 3: Looks good to me, but someone else must approve
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
8:18 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 2: (1 inline comment)
....................................................
File vdsm/storage/volume.py
Line 876: size = int(size)
You right, the better way is always use size in bytes, but
I think that if you will look for createVolume in the code (especially in image.py ) you
will figure out things like :
tmpSize = 20480 # in sectors (10M)
destDom.createVolume(...., size=tmpSize, .....)
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
12:55 p.m.
Federico Simoncelli has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 2: (2 inline comments)
....................................................
File vdsm/storage/sp.py
Line 1879: # Acquiring the lock on the image only if it's not a snapshot
This all is removed in the newer version.
....................................................
File vdsm/storage/volume.py
Line 876: size = int(size)
I am aware of that but "sometimes" is not much descriptive. Are you suggesting
that somewhere we call createVolume using sectors and we never noticed it because we
didn't hit the "else" section? I didn't find any trace of that, the
value used here comes from the manager so I can't guarantee that they'll be always
using the same unit (even if they should).
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
12:40 p.m.
Igor Lvovsky has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(4 inline comments)
....................................................
File vdsm/storage/sp.py
Line 1879: # Acquiring the lock on the image only if it's not a snapshot
It's not very clear.
Do you mean 'Acquiring the lock on the image only if created volume not a
snapshot' ?
Line 1880: if (imgUUID != srcImgUUID) or (volFormat != volume.COW_FORMAT):
why (imgUUID != srcImgUUID) ? Actually it's OK to get srcImgUUID as zero-UUID for
snapshot, it's mean 'Create snapshot on the same iamge'.
If you want to do this you need to check (volUUID != srcVolUUID).
Line 1899: return dict(uuid=uuid)
Gennerally, I am not really happy with this change. If you decided create snapshots
without locking, why you need locking at all? Only for base volumes ?
Anyway, why this related to 'clone volume' patch?
....................................................
File vdsm/storage/volume.py
Line 876: size = int(size)
Do you aware that sometimes we pass 'size' parameter in sectors? Did you checked
that in all appearances of volume.createVolume() you have right 'size' ?
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
1:18 a.m.
Igor Lvovsky has posted comments on this change.
Change subject: Avoid to activate parent volume on cloning
......................................................................
Patch Set 1:
In generally it looks good, I have only one question:
Do you aware that sometimes we pass 'size' parameter in sectors? Did you checked
that in all appearances of volume.createVolume() you have right 'size' ?
--
To view, visit http://gerrit.ovirt.org/570
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a7d488da5ab0c27fc2e083976d4bc44e6f810e3
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky(a)redhat.com>
4471
days inactive
4512
days old
vdsm-patches@lists.fedorahosted.org
16 comments
3 participants
participants (3)
-
Dan Kenigsberg -
Federico Simoncelli -
Igor Lvovsky