Hello Lee Yarwood,
I'd like you to do a code review. Please visit
to review the following change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Use cp to copy sparse volumes when moving a vm
Previously all volumes were copied with dd, with sparse files this would result in the destination volume being fully allocated.
Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Signed-off-by: Lee Yarwood lyarwood@redhat.com Signed-off-by: Yeela Kaplan ykaplan@redhat.com --- M vdsm/storage/image.py M vdsm/storage/misc.py 2 files changed, 36 insertions(+), 2 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/43/10543/1
diff --git a/vdsm/storage/image.py b/vdsm/storage/image.py index 4364c38..a8af04f 100644 --- a/vdsm/storage/image.py +++ b/vdsm/storage/image.py @@ -25,6 +25,7 @@ from contextlib import contextmanager
import volume +import fileVolume from sdc import sdCache import sd import misc @@ -582,8 +583,16 @@ # Do the actual copy try: dstVol = destDom.produceVolume(imgUUID=imgUUID, volUUID=srcVol.volUUID) - srcSize = srcVol.getVolumeSize(bs=1) - misc.ddWatchCopy(srcVol.getVolumePath(), dstVol.getVolumePath(), vars.task.aborting, size=srcSize) + # Copy sparse files with cp. + if isinstance(dstVol, fileVolume.FileVolume): + misc.cpWatchCopy(srcVol.getVolumePath(), + dstVol.getVolumePath(), vars.task.aborting, + sparse=True) + else: + srcSize = srcVol.getVolumeSize(bs=1) + misc.ddWatchCopy(srcVol.getVolumePath(), + dstVol.getVolumePath(), vars.task.aborting, + size=srcSize) except se.ActionStopped: raise except se.StorageException: diff --git a/vdsm/storage/misc.py b/vdsm/storage/misc.py index 81cb9e7..3cdac72 100644 --- a/vdsm/storage/misc.py +++ b/vdsm/storage/misc.py @@ -397,6 +397,31 @@ return "".join(random.sample(string.letters, strLen))
+def cpWatchCopy(src, dst, stop, sparse=False, recoveryCallback=None): + """ + Copy src to dst using cp with stop abilities. + """ + cmd = [constants.EXT_CP, src, dst] + + if sparse: + cmd.append("--sparse=always") + + cmd = constants.CMD_LOWPRIO + cmd + + if not stop: + (rc, out, err) = execCmd(cmd, sudo=False, nice=NICENESS.LOW, + ioclass=IOCLASS.IDLE) + else: + (rc, out, err) = watchCmd(cmd, stop=stop, + recoveryCallback=recoveryCallback, + nice=NICENESS.LOW, ioclass=IOCLASS.IDLE) + + if rc: + raise se.MiscFileWriteException() + + return (rc, out, err) + + def ddWatchCopy(src, dst, stop, size, offset=0, recoveryCallback=None): """ Copy src to dst using dd command with stop abilities
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 1:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/543/ (2/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 1:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/508/ (1/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 2:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/544/ (2/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 2:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/509/ (1/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 2: I would prefer that you didn't submit this
Build Unstable
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/509/ : UNSTABLE
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/544/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 3:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/612/ (1/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 3:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/577/ (2/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 3: I would prefer that you didn't submit this
Build Unstable
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/612/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/577/ : UNSTABLE
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Yeela Kaplan has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 3: Verified
Created a vm with a snapshot. moved between: block -> file file -> block file -> file all stays thin provisioned.
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 4:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/613/ (2/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 4:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/578/ (1/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 4: I would prefer that you didn't submit this
Build Unstable
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/613/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/578/ : UNSTABLE
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 5:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/580/ (1/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 5:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/615/ (2/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 5:
Build Successful
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/615/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/580/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Yeela Kaplan has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 5: Verified
Created a vm with a snapshot. moved between: block -> file file -> block file -> file all stays thin provisioned.
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Lee Yarwood has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 5: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Shu Ming has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 5: I would prefer that you didn't submit this
(1 inline comment)
.................................................... File vdsm/storage/misc.py Line 411: ioclass=IOCLASS.IDLE) Line 412: else: Line 413: (rc, out, err) = watchCmd(cmd, stop=stop, Line 414: recoveryCallback=recoveryCallback, Line 415: nice=NICENESS.LOW, ioclass=IOCLASS.IDLE) Why not catch the exception threw out by watchCmd, like ActionStopped? Line 416: Line 417: if rc: Line 418: raise se.MiscFileWriteException() Line 419:
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Federico Simoncelli has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 5: I would prefer that you didn't submit this
(1 inline comment)
.................................................... File vdsm/storage/image.py Line 583: # Do the actual copy Line 584: try: Line 585: dstVol = destDom.produceVolume(imgUUID=imgUUID, volUUID=srcVol.volUUID) Line 586: # Copy sparse files with cp. Line 587: if isinstance(dstVol, fileVolume.FileVolume): This looks like it's something that should be implemented by the volume class (so for example other domain/volume types, as gluster, could define their own way to make the copy). Line 588: misc.cpWatchCopy(srcVol.getVolumePath(), Line 589: dstVol.getVolumePath(), Line 590: vars.task.aborting, sparse=True) Line 591: else:
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 6:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/639/ (1/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 6:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/604/ (2/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 6:
Build Successful
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/604/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/639/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Yeela Kaplan has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 6: Verified
Created a vm with a snapshot. moved between: block -> file file -> block file -> file all stays thin provisioned.
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Ayal Baron has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 6: I would prefer that you didn't submit this
(5 inline comments)
.................................................... File vdsm/storage/blockVolume.py Line 639: mtime = 0 Line 640: Line 641: return mtime Line 642: Line 643: def watchCopy(self, srcVol, stop, sparse=False): I don't think we need sparse in the API here Line 644: srcSize = srcVol.getVolumeSize(bs=1) Line 645: misc.ddWatchCopy(srcVol.getVolumePath(), Line 646: self.getVolumePath(), Line 647: stop, srcSize)
.................................................... File vdsm/storage/fileVolume.py Line 615: return self.getMetaParam(volume.MTIME) Line 616: except se.MetaDataKeyNotFoundError: Line 617: return self.oop.os.stat(volPath).st_mtime Line 618: Line 619: def watchCopy(self, srcVol, stop, sparse=False): why would we want sparse to ever be False here? as mentioned before, I think it should just be set to True when calling cpWatchCopy below and not part of the API here. Line 620: misc.cpWatchCopy(srcVol.getVolumePath(), Line 621: self.getVolumePath(),
.................................................... File vdsm/storage/image.py Line 581: for srcVol in chains['srcChain']: Line 582: # Do the actual copy Line 583: try: Line 584: dstVol = destDom.produceVolume(imgUUID=imgUUID, volUUID=srcVol.volUUID) Line 585: # Copy sparse files with cp. comment is misplaced (this code is agnostic to internal implementation) Line 586: dstVol.watchCopy(srcVol, vars.task.aborting, sparse=True) Line 587: except se.ActionStopped: Line 588: raise Line 589: except se.StorageException:
Line 582: # Do the actual copy Line 583: try: Line 584: dstVol = destDom.produceVolume(imgUUID=imgUUID, volUUID=srcVol.volUUID) Line 585: # Copy sparse files with cp. Line 586: dstVol.watchCopy(srcVol, vars.task.aborting, sparse=True) I don't understand why the sparse property belongs here and not inside fileVolume Line 587: except se.ActionStopped: Line 588: raise Line 589: except se.StorageException: Line 590: self.log.error("Unexpected error", exc_info=True)
.................................................... File vdsm/storage/volume.py Line 904: volParams['descr'] = self.getDescription() Line 905: volParams['legality'] = self.getLegality() Line 906: return volParams Line 907: Line 908: def watchCopy(self, srcVol, stop, sparse=False): redundant, and if Eduardo were here he would oppose this method :) Line 909: """ Line 910: Copy the volume contents Line 911: """ Line 912: pass
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 7:
Build Started http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/647/ (1/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 7:
Build Started http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/612/ (2/2)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 7:
Build Successful
http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/612/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_manual_gerrit/647/ : SUCCESS
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Federico Simoncelli has posted comments on this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 7: (1 inline comment)
.................................................... File vdsm/storage/image.py Line 581: for srcVol in chains['srcChain']: Line 582: # Do the actual copy Line 583: try: Line 584: dstVol = destDom.produceVolume(imgUUID=imgUUID, volUUID=srcVol.volUUID) Line 585: dstVol.watchCopy(srcVol, vars.task.aborting) What I don't like here is that we don't check the source and destination domain types, which means that we could end up using "cp" also from a block domain to a file domain. I think we already discussed this and we said that "cp" is able to handle the copy correctly, nonetheless it looks ugly to me (and it might even work only by accident). Either we check for the domains to be of the same kind eg:
if type(dstVol) == type(srcVol): dstVol.watchCopy(srcVol, vars.task.aborting) else: ...use the regular dd...
Or we just use qemu-img that is capable of generating sparseness on the fly (even from block devices to files). Line 586: except se.ActionStopped: Line 587: raise Line 588: except se.StorageException: Line 589: self.log.error("Unexpected error", exc_info=True)
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
Yeela Kaplan has abandoned this change.
Change subject: Use cp to copy sparse volumes when moving a vm ......................................................................
Patch Set 7: Abandoned
This patch was a bug fix now replaced by a different implementation: I9eedaa41a64502e548e52c10742335abd05301d8
-- To view, visit http://gerrit.ovirt.org/10543 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: abandon Gerrit-Change-Id: I0c4c693884a225c868c18bdcb4930c2c1042db2c Gerrit-PatchSet: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dafna Ron dron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimonce@redhat.com Gerrit-Reviewer: Igor Lvovsky ilvovsky@redhat.com Gerrit-Reviewer: Lee Yarwood lyarwood@redhat.com Gerrit-Reviewer: Shu Ming shuming@linux.vnet.ibm.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server
vdsm-patches@lists.fedorahosted.org