Arik Hadas has uploaded a new change for review.
Change subject: do not use OOP for padding snapshot's memory volume ......................................................................
do not use OOP for padding snapshot's memory volume
Change-Id: I2a94354e188019f3afd209633979ec5a5b35293b Signed-off-by: Arik Hadas ahadas@redhat.com --- M vdsm/virt/vm.py 1 file changed, 3 insertions(+), 9 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/38/26538/1
diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py index 6711bc6..b8ce533 100644 --- a/vdsm/virt/vm.py +++ b/vdsm/virt/vm.py @@ -3543,15 +3543,9 @@ '_srcDomXML': self._dom.XMLDesc(0), 'elapsedTimeOffset': time.time() - self._startTime}
- def _padMemoryVolume(memoryVolPath, sdUUID): - sdType = sd.name2type( - self.cif.irs.getStorageDomainInfo(sdUUID)['info']['type']) - if sdType in sd.FILE_DOMAIN_TYPES: - if sdType == sd.NFS_DOMAIN: - oop.getProcessPool(sdUUID).fileUtils. \ - padToBlockSize(memoryVolPath) - else: - fileUtils.padToBlockSize(memoryVolPath) + def _padMemoryVolume(memoryVolPath): + if not utils.isBlockDevice(memoryVolPath): + fileUtils.padToBlockSize(memoryVolPath)
snap = xml.dom.minidom.Element('domainsnapshot') disks = xml.dom.minidom.Element('disks')
oVirt Jenkins CI Server has posted comments on this change.
Change subject: do not use OOP for padding snapshot's memory volume ......................................................................
Patch Set 1:
Build Successful
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7902/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/7112/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/8014/ : SUCCESS
Federico Simoncelli has posted comments on this change.
Change subject: do not use OOP for padding snapshot's memory volume ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/26538/1//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2014-04-08 13:58:13 +0300 Line 4: Commit: Arik Hadas ahadas@redhat.com Line 5: CommitDate: 2014-04-08 13:58:13 +0300 Line 6: Line 7: do not use OOP for padding snapshot's memory volume Why? Line 8: Line 9: Change-Id: I2a94354e188019f3afd209633979ec5a5b35293b
Michal Skrivanek has posted comments on this change.
Change subject: do not use OOP for padding snapshot's memory volume ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/26538/1//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2014-04-08 13:58:13 +0300 Line 4: Commit: Arik Hadas ahadas@redhat.com Line 5: CommitDate: 2014-04-08 13:58:13 +0300 Line 6: Line 7: do not use OOP for padding snapshot's memory volume
Why?
I'd say it's not needed since the QEMU just finished writing all the RAM content to that file…so it's quite likely it's accessible since we get so far. Other alternative would be to use OOP for all types…but I'm not really sure it's needed Line 8: Line 9: Change-Id: I2a94354e188019f3afd209633979ec5a5b35293b
Dan Kenigsberg has posted comments on this change.
Change subject: do not use OOP for padding snapshot's memory volume ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
http://gerrit.ovirt.org/#/c/26538/1//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2014-04-08 13:58:13 +0300 Line 4: Commit: Arik Hadas ahadas@redhat.com Line 5: CommitDate: 2014-04-08 13:58:13 +0300 Line 6: Line 7: do not use OOP for padding snapshot's memory volume
I'd say it's not needed since the QEMU just finished writing all the RAM co
Without oop we have a chance that a single disappearing NFS export moves all of Vdsm into D state. This must be maintained. Line 8: Line 9: Change-Id: I2a94354e188019f3afd209633979ec5a5b35293b
Michal Skrivanek has posted comments on this change.
Change subject: do not use OOP for padding snapshot's memory volume ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/26538/1//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2014-04-08 13:58:13 +0300 Line 4: Commit: Arik Hadas ahadas@redhat.com Line 5: CommitDate: 2014-04-08 13:58:13 +0300 Line 6: Line 7: do not use OOP for padding snapshot's memory volume
Without oop we have a chance that a single disappearing NFS export moves al
How could it disappear when QEMU just finished writing data to it?
We're not protecting the libvirt call either, btw.
Anyway, if we do want to cover ourselves in this case as well, OOP would always be better, not just for NFS. Line 8: Line 9: Change-Id: I2a94354e188019f3afd209633979ec5a5b35293b
Arik Hadas has posted comments on this change.
Change subject: do not use OOP for padding snapshot's memory volume ......................................................................
Patch Set 1:
(1 comment)
http://gerrit.ovirt.org/#/c/26538/1//COMMIT_MSG Commit Message:
Line 3: AuthorDate: 2014-04-08 13:58:13 +0300 Line 4: Commit: Arik Hadas ahadas@redhat.com Line 5: CommitDate: 2014-04-08 13:58:13 +0300 Line 6: Line 7: do not use OOP for padding snapshot's memory volume
How could it disappear when QEMU just finished writing data to it?
I added an explanation to the commit msg, Federico & Dan - do you agree? Line 8: Line 9: Change-Id: I2a94354e188019f3afd209633979ec5a5b35293b
oVirt Jenkins CI Server has posted comments on this change.
Change subject: do not use OOP for padding snapshot's memory volume ......................................................................
Patch Set 2:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/8985/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9126/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8197/ : SUCCESS
Dan Kenigsberg has posted comments on this change.
Change subject: do not use OOP for padding snapshot's memory volume ......................................................................
Patch Set 2: Code-Review-2
(1 comment)
http://gerrit.ovirt.org/#/c/26538/2//COMMIT_MSG Commit Message:
Line 8: Line 9: The padding is made right after libvirt dumps the memory content into Line 10: the volume, so the volumes is accessible and writable - doing the Line 11: padding in different process looks like an expensive operation which is Line 12: not required in that case. Thus, all the padding will now be made As I said, this is wrong. All you know is that the volume WAS accessible and writable. It may no longer be in that state.
All writes to NFS must happen from another process. Otherwise, if the storage disappear, the whole of Vdsm would freeze in D state. It does not matter that qemu just finished writing to there - the race condition is small but it exists. It does not matter that libvirt may have a similar bug. (well, it does matter, but we should fix it there, not reinroduce it here).
I do not know enough about the kernel implementation of other file storage domains; they may well have similar bugs, but nowhere is Vdsm do we handle these bugs. In NFS we saw the bugs and do not want to see it again. Line 13: without OOP. Line 14: Line 15: Change-Id: I2a94354e188019f3afd209633979ec5a5b35293b
Arik Hadas has posted comments on this change.
Change subject: Always use OOP when padding snapshot's memory volume ......................................................................
Patch Set 2:
(1 comment)
http://gerrit.ovirt.org/#/c/26538/2//COMMIT_MSG Commit Message:
Line 8: Line 9: The padding is made right after libvirt dumps the memory content into Line 10: the volume, so the volumes is accessible and writable - doing the Line 11: padding in different process looks like an expensive operation which is Line 12: not required in that case. Thus, all the padding will now be made
As I said, this is wrong. All you know is that the volume WAS accessible an
yeah, I see. so how about to use OOP for all file-based storage types? I tried to convince in the commit message of the next patch-set :) Line 13: without OOP. Line 14: Line 15: Change-Id: I2a94354e188019f3afd209633979ec5a5b35293b
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Always use OOP when padding snapshot's memory volume ......................................................................
Patch Set 3:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8541/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9475/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9329/ : SUCCESS
Michal Skrivanek has posted comments on this change.
Change subject: Always use OOP when padding snapshot's memory volume ......................................................................
Patch Set 4:
Then I'd even argue that we should use OOP for all storage types as even the block storage is accessing NAS vulnerable to connectivity issues, timeouts, all kind of issues similar to NFS problems
oVirt Jenkins CI Server has posted comments on this change.
Change subject: Always use OOP when padding snapshot's memory volume ......................................................................
Patch Set 4:
Build Failed
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/8542/ : SUCCESS
http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit/9476/ : FAILURE
http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/9330/ : SUCCESS
Dan Kenigsberg has posted comments on this change.
Change subject: Always use OOP when padding snapshot's memory volume ......................................................................
Patch Set 4: Code-Review-1
(2 comments)
http://gerrit.ovirt.org/#/c/26538/4//COMMIT_MSG Commit Message:
Line 7: Always use OOP when padding snapshot's memory volume Line 8: Line 9: We used to use OOP when padding snapshot's memory volume only if the Line 10: storage type was NFS. In order to determine if the storage type is NFS Line 11: we had to call the costly (and possibly failing) getStorageDomainInfo when is it failing? why do we want to continue with the padding if it has failed? Line 12: method. Since other file based storage types can theoretically suffer Line 13: from the same problems as the ones NFS suffer from, it seems safer to Line 14: use OOP for all of them and not perform that costly call. Line 15:
http://gerrit.ovirt.org/#/c/26538/4/vdsm/virt/vm.py File vdsm/virt/vm.py:
Line 3725: '_srcDomXML': self._dom.XMLDesc(0), Line 3726: 'elapsedTimeOffset': time.time() - self._startTime} Line 3727: Line 3728: def _padMemoryVolume(memoryVolPath, sdUUID): Line 3729: if not utils.isBlockDevice(memoryVolPath): isBlockDevice() does os.stats() which may hang forever on top of NFS. Line 3730: oop.getProcessPool(sdUUID).fileUtils. \ Line 3731: padToBlockSize(memoryVolPath) Line 3732: Line 3733: snap = xml.dom.minidom.Element('domainsnapshot')
Itamar Heim has posted comments on this change.
Change subject: Always use OOP when padding snapshot's memory volume ......................................................................
Patch Set 4:
ping
Jenkins CI RO has posted comments on this change.
Change subject: Always use OOP when padding snapshot's memory volume ......................................................................
Patch Set 4:
Abandoned due to no activity - please restore if still relevant
Jenkins CI RO has abandoned this change.
Change subject: Always use OOP when padding snapshot's memory volume ......................................................................
Abandoned
Abandoned due to no activity - please restore if still relevant
automation@ovirt.org has posted comments on this change.
Change subject: Always use OOP when padding snapshot's memory volume ......................................................................
Patch Set 4:
* Update tracker::IGNORE, no Bug-Url found
vdsm-patches@lists.fedorahosted.org