Tomas Jelinek has posted comments on this change.

Change subject: migration: wait properly for migration to begin
......................................................................


Patch Set 3:

(1 comment)

https://gerrit.ovirt.org/#/c/60073/3/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 766:             stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_PROCESSED, -1),
Line 767:             stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_REMAINING, -1),
Line 768:             stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_BPS, -1),
Line 769:             stats.get(libvirt.VIR_DOMAIN_JOB_MEMORY_CONSTANT, -1),
Line 770:             stats.get(libvirt.VIR_DOMAIN_JOB_COMPRESSION_BYTES, -1),
> I think it is better to raise an exception here, or signal somehow the call
well, in order to make the call on 658 work, we need this not to fail in 
from_job_stats().

An alternative approach would be to do something like this on line 658:

if self._vm._dom.jobStats()['type'] == libvirt.VIR_DOMAIN_JOB_NONE:
    continue

An other option I see would be to change the ongoing() to something like:

@classmethod
def ongoing(cls, stats):
    return stats['type'] != libvirt.VIR_DOMAIN_JOB_NONE

so we could avoid the parsing of all the params if we need only to find out if 
it is active or not.

What you think?
Line 771:             # available since libvirt 1.3
Line 772:             stats.get('memory_dirty_rate', -1),
Line 773:             # available since libvirt 1.3
Line 774:             stats.get('memory_iteration', -1),


-- 
To view, visit https://gerrit.ovirt.org/60073
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If1ba369a0992c2473acf1395fad0b0c260c250ba
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: Francesco Romani <[email protected]>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Milan Zamazal <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/admin/lists/[email protected]

Reply via email to