Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2016-01-06 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 11:

* Update tracker: IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2016-01-06 Thread nsoffer 
Nir Soffer has submitted this change and it was merged.

Change subject: v2v: extract specific classes for libvirt and ova
..


v2v: extract specific classes for libvirt and ova

We are handling a lot of inputs for ImportVM (libvirt VM, ova) and
adding Xen and Kvm which make ImportVM bloated with a lot of conditions.

Introduction a V2VCommand that each implementation will create its own
command and will have execution context.

Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Signed-off-by: Shahar Havivi 
Reviewed-on: https://gerrit.ovirt.org/49951
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer 
Tested-by: Shahar Havivi 
Reviewed-by: Francesco Romani 
---
M vdsm/v2v.py
1 file changed, 185 insertions(+), 178 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, but someone else must approve
  Shahar Havivi: Verified
  Jenkins CI: Passed CI tests
  Francesco Romani: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2016-01-05 Thread fromani 
Francesco Romani has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 10: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-30 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 10: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-30 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 10: -Verified

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-27 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 10:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-27 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 9:

(3 comments)

https://gerrit.ovirt.org/#/c/49951/9/vdsm/v2v.py
File vdsm/v2v.py:

Line 360
Line 361
Line 362
Line 363
Line 364
> Job must implement this.
Done


Line 278: 
Line 279: 
Line 280: class V2VCommand(object):
Line 281: def __init__(self, uri, vminfo, vmid, irs):
Line 282: self._uri = uri
> uri does not belong here it should be parameter of LibvirtCommand.
Done
Line 283: self._vminfo = vminfo
Line 284: self._vmid = vmid
Line 285: self._irs = irs
Line 286: self._prepared_volumes = []


Line 416:   self._vmid, self._passwd_file)
Line 417: 
Line 418: 
Line 419: class OvaCommand(V2VCommand):
Line 420: def __init__(self, ova_path, vminfo, vmid, irs):
> Keep the order of arguments same as the super class, adding the specific ar
no its not the V2VCommand ignore the uri.
I am removing it from V2vCommand.
Line 421: super(self.__class__, self).__init__(ova_path, vminfo, vmid, 
irs)
Line 422: self._ova_path = ova_path
Line 423: 
Line 424: def _command(self):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-27 Thread nsoffer 
Nir Soffer has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 10: Code-Review+1

Looks good to me, waiting for Francesco review.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-27 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 10: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik 
Gerrit-Reviewer: Milan Zamazal 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: Vinzenz Feenstra 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-24 Thread nsoffer 
Nir Soffer has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 9: Code-Review-1

(8 comments)

Generally look good

https://gerrit.ovirt.org/#/c/49951/9/vdsm/v2v.py
File vdsm/v2v.py:

Line 360
Line 361
Line 362
Line 363
Line 364
Job must implement this.


Line 404
Line 405
Line 406
Line 407
Line 408
This looks more like _run_command now.


Line 278: 
Line 279: 
Line 280: class V2VCommand(object):
Line 281: def __init__(self, uri, vminfo, vmid, irs):
Line 282: self._uri = uri
uri does not belong here it should be parameter of LibvirtCommand.
Line 283: self._vminfo = vminfo
Line 284: self._vmid = vmid
Line 285: self._irs = irs
Line 286: self._prepared_volumes = []


Line 358: we need storage domain absolute path so we go up 3 levels
Line 359: '''
Line 360: return path.rsplit(os.sep, 3)[0]
Line 361: 
Line 362: def _environments(self):
This common name for this is "environment"
Line 363: env = {'LIBGUESTFS_BACKEND': 'direct'}
Line 364: if 'virtio_iso_path' in self._vminfo:
Line 365: env['VIRTIO_WIN'] = self._vminfo['virtio_iso_path']
Line 366: return env


Line 366: return env
Line 367: 
Line 368: 
Line 369: class LibvirtCommand(V2VCommand):
Line 370: def __init__(self, uri, username, password, vminfo, vmid, irs):
Keep the order of arguments same as the super class, adding the specific 
arguments in the end.

uri, vminfo, vmid, irs, username, password
Line 371: super(self.__class__, self).__init__(uri, vminfo, vmid, irs)
Line 372: self._username = username
Line 373: self._password = password
Line 374: 


Line 367: 
Line 368: 
Line 369: class LibvirtCommand(V2VCommand):
Line 370: def __init__(self, uri, username, password, vminfo, vmid, irs):
Line 371: super(self.__class__, self).__init__(uri, vminfo, vmid, irs)
This works for the current class, but is not the common idiom. Please use the 
standard way:

super(LibvirtCommand, self)...
Line 372: self._username = username
Line 373: self._password = password
Line 374: 
Line 375: self._passwd_file = os.path.join(_V2V_DIR, "%s.tmp" % vmid)


Line 416:   self._vmid, self._passwd_file)
Line 417: 
Line 418: 
Line 419: class OvaCommand(V2VCommand):
Line 420: def __init__(self, ova_path, vminfo, vmid, irs):
Keep the order of arguments same as the super class, adding the specific 
arguments in the end.

uri, vminfo, vmid, irs

In this case ova_path is used as uri when you call the super class, is this 
correct?

Maybe uri belong only to LibvirtCommand?
Line 421: super(self.__class__, self).__init__(ova_path, vminfo, vmid, 
irs)
Line 422: self._ova_path = ova_path
Line 423: 
Line 424: def _command(self):


Line 417: 
Line 418: 
Line 419: class OvaCommand(V2VCommand):
Line 420: def __init__(self, ova_path, vminfo, vmid, irs):
Line 421: super(self.__class__, self).__init__(ova_path, vminfo, vmid, 
irs)
Same, use class name, not __class__
Line 422: self._ova_path = ova_path
Line 423: 
Line 424: def _command(self):
Line 425: cmd = [_VIRT_V2V.cmd,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-24 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 9:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-24 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 7:

(3 comments)

https://gerrit.ovirt.org/#/c/49951/7/vdsm/v2v.py
File vdsm/v2v.py:

Line 400:sync=False,
Line 401:deathSignal=signal.SIGTERM,
Line 402:nice=NICENESS.HIGH,
Line 403:ioclass=IOCLASS.IDLE,
Line 404:env=self._environments())
> This block is indentical in both classes, so better move it to super class,
Done
Line 405: yield proc
Line 406: 
Line 407: @contextmanager
Line 408: def _password_file(self):


Line 550: if isinstance(event, ImportProgress):
Line 551: self._status = STATUS.COPYING_DISK
Line 552: logging.info("Job %r copying disk %d/%d",
Line 553:  self._id, event.current_disk,
Line 554:  event.disk_count)
> Unrelated change
Done
Line 555: self._disk_progress = 0
Line 556: self._current_disk = event.current_disk
Line 557: self._disk_count = event.disk_count
Line 558: self._description = event.description


Line 560: self._disk_progress = event.progress
Line 561: if event.progress % 10 == 0:
Line 562: logging.info("Job %r copy disk %d progress 
%d/100",
Line 563:  self._id, self._current_disk,
Line 564:  event.progress)
> Unrelated change
Done
Line 565: else:
Line 566: raise RuntimeError("Job %r got unexpected parser 
event: %s" %
Line 567:(self._id, event))
Line 568: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-23 Thread nsoffer 
Nir Soffer has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 8:

Please check and reply to all comments in the previous version.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-23 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 8:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-23 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 7:

(5 comments)

https://gerrit.ovirt.org/#/c/49951/7/vdsm/v2v.py
File vdsm/v2v.py:

Line 284: self._uri = uri
Line 285: self._vminfo = vminfo
Line 286: self._vmid = vmid
Line 287: self._irs = irs
Line 288: 
> Remove the blank line
Done
Line 289: self._prepared_volumes = []
Line 290: 
Line 291: def execute(self):
Line 292: """


Line 290: 
Line 291: def execute(self):
Line 292: """
Line 293: implement in subclasses
Line 294: """
> To make it clear that this is abstract class, and subclass must implement t
Done
Line 295: 
Line 296: def _get_disk_format(self):
Line 297: fmt = self._vminfo.get('format', 'raw').lower()
Line 298: if fmt == 'cow':


Line 296: def _get_disk_format(self):
Line 297: fmt = self._vminfo.get('format', 'raw').lower()
Line 298: if fmt == 'cow':
Line 299: return 'qcow2'
Line 300: return fmt
> Can simplified to:
Done
Line 301: 
Line 302: def _generate_disk_parameters(self):
Line 303: parameters = []
Line 304: for disk in self._vminfo['disks']:


Line 298: if fmt == 'cow':
Line 299: return 'qcow2'
Line 300: return fmt
Line 301: 
Line 302: def _generate_disk_parameters(self):
> I would call it _disk_parameters()
Done
Line 303: parameters = []
Line 304: for disk in self._vminfo['disks']:
Line 305: try:
Line 306: parameters.append('--vdsm-image-uuid')


Line 312: % (self._vmid, e))
Line 313: return parameters
Line 314: 
Line 315: @contextmanager
Line 316: def _volumes_handler(self):
> Why _handler? Why not call it _volumes()?
Done
Line 317: self._prepare_volumes()
Line 318: try:
Line 319: yield
Line 320: finally:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-17 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 7:

Verified importing ova file and VM from esxi 5.5

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-17 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 7: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-17 Thread nsoffer 
Nir Soffer has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 7:

(8 comments)

I like this, just few tweaks and it is ready.

https://gerrit.ovirt.org/#/c/49951/7/vdsm/v2v.py
File vdsm/v2v.py:

Line 284: self._uri = uri
Line 285: self._vminfo = vminfo
Line 286: self._vmid = vmid
Line 287: self._irs = irs
Line 288: 
Remove the blank line
Line 289: self._prepared_volumes = []
Line 290: 
Line 291: def execute(self):
Line 292: """


Line 290: 
Line 291: def execute(self):
Line 292: """
Line 293: implement in subclasses
Line 294: """
To make it clear that this is abstract class, and subclass must implement this, 
do:

def execute(self):
raise NotImplementedError("Subclass must implement this")
Line 295: 
Line 296: def _get_disk_format(self):
Line 297: fmt = self._vminfo.get('format', 'raw').lower()
Line 298: if fmt == 'cow':


Line 296: def _get_disk_format(self):
Line 297: fmt = self._vminfo.get('format', 'raw').lower()
Line 298: if fmt == 'cow':
Line 299: return 'qcow2'
Line 300: return fmt
Can simplified to:

fmt = self._vminfo.get('format', 'raw').lower()
return "qcow2" if fmt == "cow" else fmt
Line 301: 
Line 302: def _generate_disk_parameters(self):
Line 303: parameters = []
Line 304: for disk in self._vminfo['disks']:


Line 298: if fmt == 'cow':
Line 299: return 'qcow2'
Line 300: return fmt
Line 301: 
Line 302: def _generate_disk_parameters(self):
I would call it _disk_parameters()
Line 303: parameters = []
Line 304: for disk in self._vminfo['disks']:
Line 305: try:
Line 306: parameters.append('--vdsm-image-uuid')


Line 312: % (self._vmid, e))
Line 313: return parameters
Line 314: 
Line 315: @contextmanager
Line 316: def _volumes_handler(self):
Why _handler? Why not call it _volumes()?
Line 317: self._prepare_volumes()
Line 318: try:
Line 319: yield
Line 320: finally:


Line 400:sync=False,
Line 401:deathSignal=signal.SIGTERM,
Line 402:nice=NICENESS.HIGH,
Line 403:ioclass=IOCLASS.IDLE,
Line 404:env=self._environments())
This block is indentical in both classes, so better move it to super class, 
mayb call it _start_virt_v2v()

Then this command will do:

with self._volumes_handler(), self._password_file():
yield self._start_virt_v2v()

And the ova command:

with self._volumes_handler():
yield self._start_virt_v2v()

The xen commnd will probably do:

with self._volumes_handler(), self._ssh_agent():
yield self._start_virt_v2v()
Line 405: yield proc
Line 406: 
Line 407: @contextmanager
Line 408: def _password_file(self):


Line 550: if isinstance(event, ImportProgress):
Line 551: self._status = STATUS.COPYING_DISK
Line 552: logging.info("Job %r copying disk %d/%d",
Line 553:  self._id, event.current_disk,
Line 554:  event.disk_count)
Unrelated change
Line 555: self._disk_progress = 0
Line 556: self._current_disk = event.current_disk
Line 557: self._disk_count = event.disk_count
Line 558: self._description = event.description


Line 560: self._disk_progress = event.progress
Line 561: if event.progress % 10 == 0:
Line 562: logging.info("Job %r copy disk %d progress 
%d/100",
Line 563:  self._id, self._current_disk,
Line 564:  event.progress)
Unrelated change
Line 565: else:
Line 566: raise RuntimeError("Job %r got unexpected parser 
event: %s" %
Line 567:(self._id, event))
Line 568: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-16 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 7:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-15 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 5:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-15 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 4:

(6 comments)

https://gerrit.ovirt.org/#/c/49951/4/vdsm/v2v.py
File vdsm/v2v.py:

Line 292: self._prepared_volumes = []
Line 293: 
Line 294: @property
Line 295: def id(self):
Line 296: return self._id
> question: should `id' be tied to the ImportVm object (old code) or to the C
You right,
I will be in both - 
in the command its the vm id
in importVm its the job id
Line 297: 
Line 298: @property
Line 299: def proc(self):
Line 300: return self._proc


Line 309:  nice=NICENESS.HIGH,
Line 310:  ioclass=IOCLASS.IDLE,
Line 311:  env=self.environments())
Line 312: 
Line 313: yield
> Sure, ImportVm should have proc member, since it manage this process.
Done
Line 314: finally:
Line 315: self._teardown_volumes()
Line 316: 
Line 317: def _get_disk_format(self):


Line 325: try:
Line 326: self._disk_parameters.append('--vdsm-image-uuid')
Line 327: self._disk_parameters.append(disk['imageID'])
Line 328: self._disk_parameters.append('--vdsm-vol-uuid')
Line 329: self._disk_parameters.append(disk['volumeID'])
> Why keep disk_parameters instead of returning them? This is more fragile, a
Done
Line 330: except KeyError as e:
Line 331: raise InvalidInputError('Job %r missing required 
property: %s'
Line 332: % (self._id, e))
Line 333: 


Line 448: cmd.extend(self._disk_parameters)
Line 449: return cmd
Line 450: 
Line 451: @contextmanager
Line 452: def execute(self):
> this just invoked the parent's execute(), thus feels redundant and could be
Done
Line 453: with super(self.__class__, self).execute():
Line 454: yield
Line 455: 
Line 456: 


Line 460: 
Line 461: def __init__(self, command):
Line 462: '''
Line 463: do not use directly, use a factory method instead!
Line 464: '''
> stale comment, you killed the factory methods.
Done
Line 465: self._command = command
Line 466: self._thread = None
Line 467: 
Line 468: self._status = STATUS.STARTING


Line 536: logging.info('Job %r finished import successfully',
Line 537:  self._command.id)
Line 538: 
Line 539: def _wait_for_process(self):
Line 540: if self._command.proc.returncode is not None:
> This works, but I think this that it would be nicer if the Command classes 
Done
Line 541: return
Line 542: logging.debug("Job %r waiting for virt-v2v process", 
self._command.id)
Line 543: if not 
self._command.proc.wait(timeout=self.PROC_WAIT_TIMEOUT):
Line 544: raise V2VProcessError("Job %r timeout waiting for process 
pid=%s",


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-15 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 6:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-14 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/49951/4/tests/v2vTests.py
File tests/v2vTests.py:

Line 354: self.assertEquals(network['macAddr'], 
_mac_from_uuid(spec.vmid))
Line 355: self.assertEquals(network['bridge'], 'VM Network')
Line 356: 
Line 357: @MonkeyPatch(v2v, '_VIRT_V2V', FAKE_VIRT_V2V)
Line 358: @MonkeyPatch(v2v, '_V2V_DIR', '/tmp/')
> please check if elsewhere we use NamedTemporaryDir(). If so, please use it 
Done
Line 359: def testSuccessfulImport(self):
Line 360: vminfo = {'vmName': self.vm_name,
Line 361:   'poolID': self.pool_id,
Line 362:   'domainID': self.domain_id,


https://gerrit.ovirt.org/#/c/49951/4/vdsm/v2v.py
File vdsm/v2v.py:

Line 159: 
Line 160: 
Line 161: def convert_external_vm(uri, username, password, vminfo, job_id, irs):
Line 162: command = LibvirtCommand(uri, username, password, vminfo, job_id, 
irs)
Line 163: job = ImportVm(command)
> job_id belongs to the job, not to the LibvirtCommand.
both need it.
The job id represent the job id in our v2v.py 
and its actually the VM id which needed by the command
the parameter --vdsm-vm-uuid.
I will add it as a member to ImportVM,
We can add it to the BaseCommmand as well or we can pass it via execute.
Line 164: job.start()
Line 165: _add_job(job_id, job)
Line 166: return {'status': doneCode}
Line 167: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-14 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/49951/4/vdsm/v2v.py
File vdsm/v2v.py:

Line 303: def execute(self):
Line 304: self._generate_disk_parameters()
Line 305: self._prepare_volumes()
Line 306: try:
Line 307: self._proc = execCmd(self._command(), sync=False,
> VERY minor nit
Done
Line 308:  deathSignal=signal.SIGTERM,
Line 309:  nice=NICENESS.HIGH,
Line 310:  ioclass=IOCLASS.IDLE,
Line 311:  env=self.environments())


Line 309:  nice=NICENESS.HIGH,
Line 310:  ioclass=IOCLASS.IDLE,
Line 311:  env=self.environments())
Line 312: 
Line 313: yield
> Why not:
The proc is accessible from other methods such as abort().
If we yeild the proc we need a proc member in the importVM.
Line 314: finally:
Line 315: self._teardown_volumes()
Line 316: 
Line 317: def _get_disk_format(self):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-14 Thread nsoffer 
Nir Soffer has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/49951/4/vdsm/v2v.py
File vdsm/v2v.py:

Line 309:  nice=NICENESS.HIGH,
Line 310:  ioclass=IOCLASS.IDLE,
Line 311:  env=self.environments())
Line 312: 
Line 313: yield
> The proc is accessible from other methods such as abort().
Sure, ImportVm should have proc member, since it manage this process.

The Command classes do not need proc member, since they only create the process 
and do not manage it. If ImportVm manage something.
Line 314: finally:
Line 315: self._teardown_volumes()
Line 316: 
Line 317: def _get_disk_format(self):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-11 Thread nsoffer 
Nir Soffer has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 4:

(7 comments)

https://gerrit.ovirt.org/#/c/49951/4/vdsm/v2v.py
File vdsm/v2v.py:

Line 159: 
Line 160: 
Line 161: def convert_external_vm(uri, username, password, vminfo, job_id, irs):
Line 162: command = LibvirtCommand(uri, username, password, vminfo, job_id, 
irs)
Line 163: job = ImportVm(command)
job_id belongs to the job, not to the LibvirtCommand.
Line 164: job.start()
Line 165: _add_job(job_id, job)
Line 166: return {'status': doneCode}
Line 167: 


Line 167: 
Line 168: 
Line 169: def convert_ova(ova_path, vminfo, job_id, irs):
Line 170: command = OvaCommand(ova_path, vminfo, job_id, irs)
Line 171: job = ImportVm(command)
job_id belongs to the job, not to the OvaCommand.
Line 172: job.start()
Line 173: _add_job(job_id, job)
Line 174: return response.success()
Line 175: 


Line 299: def proc(self):
Line 300: return self._proc
Line 301: 
Line 302: @contextmanager
Line 303: def execute(self):
You sould provide instead a context manager for preparing volumes, like the 
password_file context manager:

def prepared_volumes(self):
self._generate_disk_parameters()
self._prepare_volumes()
try:
yield
finally:
self._teardown_volumes()

Subclasses can combine this, for example, LIbvirtCommand.execute:

def execute(self):
with self._prepare_volumes(), self._password_file():
proc = execCmd(...)
yield proc

OvaCommand.execute needs only the volumes, so:

def execute(self):
with self._prepare_volumes():
proc = execCmd(...)
yield proc

If the execCmd(...) is common, it can also moved up:

def execute(self):
with self._prepare_volumes():
yield self._start_v2v_process()
Line 304: self._generate_disk_parameters()
Line 305: self._prepare_volumes()
Line 306: try:
Line 307: self._proc = execCmd(self._command(), sync=False,


Line 309:  nice=NICENESS.HIGH,
Line 310:  ioclass=IOCLASS.IDLE,
Line 311:  env=self.environments())
Line 312: 
Line 313: yield
Why not:

yield proc

And you don't need the the proc() method now.
Line 314: finally:
Line 315: self._teardown_volumes()
Line 316: 
Line 317: def _get_disk_format(self):


Line 325: try:
Line 326: self._disk_parameters.append('--vdsm-image-uuid')
Line 327: self._disk_parameters.append(disk['imageID'])
Line 328: self._disk_parameters.append('--vdsm-vol-uuid')
Line 329: self._disk_parameters.append(disk['volumeID'])
Why keep disk_parameters instead of returning them? This is more fragile, as 
calling this twice is now wrong, was ok before.
Line 330: except KeyError as e:
Line 331: raise InvalidInputError('Job %r missing required 
property: %s'
Line 332: % (self._id, e))
Line 333: 


Line 448: cmd.extend(self._disk_parameters)
Line 449: return cmd
Line 450: 
Line 451: @contextmanager
Line 452: def execute(self):
> this just invoked the parent's execute(), thus feels redundant and could be
I agree, but it wold be nicer if subclasses implement their execute(), and the 
parent only provides the helper methods to do this.
Line 453: with super(self.__class__, self).execute():
Line 454: yield
Line 455: 
Line 456: 


Line 536: logging.info('Job %r finished import successfully',
Line 537:  self._command.id)
Line 538: 
Line 539: def _wait_for_process(self):
Line 540: if self._command.proc.returncode is not None:
This works, but I think this that it would be nicer if the Command classes 
generate a process object, and this class keeps this object, instead of going 
through the command object.

The command object gives no services regarding its proc object, so it should 
not keep it.
Line 541: return
Line 542: logging.debug("Job %r waiting for virt-v2v process", 
self._command.id)
Line 543: if not 
self._command.proc.wait(timeout=self.PROC_WAIT_TIMEOUT):
Line 544: raise V2VProcessError("Job %r timeout waiting for process 
pid=%s",


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer:

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-10 Thread fromani 
Francesco Romani has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 4:

(5 comments)

partial review, overall looks OK and less scary than the size suggests.
I'll have another look later to be sure I'm not missing something important. 
Meanwhile, some initial comments.

https://gerrit.ovirt.org/#/c/49951/4/tests/v2vTests.py
File tests/v2vTests.py:

Line 354: self.assertEquals(network['macAddr'], 
_mac_from_uuid(spec.vmid))
Line 355: self.assertEquals(network['bridge'], 'VM Network')
Line 356: 
Line 357: @MonkeyPatch(v2v, '_VIRT_V2V', FAKE_VIRT_V2V)
Line 358: @MonkeyPatch(v2v, '_V2V_DIR', '/tmp/')
please check if elsewhere we use NamedTemporaryDir(). If so, please use it as 
well, is possible (if not, please add a comment explaining why)
Line 359: def testSuccessfulImport(self):
Line 360: vminfo = {'vmName': self.vm_name,
Line 361:   'poolID': self.pool_id,
Line 362:   'domainID': self.domain_id,


https://gerrit.ovirt.org/#/c/49951/4/vdsm/v2v.py
File vdsm/v2v.py:

Line 292: self._prepared_volumes = []
Line 293: 
Line 294: @property
Line 295: def id(self):
Line 296: return self._id
question: should `id' be tied to the ImportVm object (old code) or to the 
Command (new code)?
It seems to me that it belongs more on the ImportVm object, but maybe I'm 
missing something here.
Line 297: 
Line 298: @property
Line 299: def proc(self):
Line 300: return self._proc


Line 303: def execute(self):
Line 304: self._generate_disk_parameters()
Line 305: self._prepare_volumes()
Line 306: try:
Line 307: self._proc = execCmd(self._command(), sync=False,
VERY minor nit

If we need to put the majority of arguments on separate line, I'd bite the 
bullet and put all of them on its line:

  self._proc = execCmd(self._command(),
   sync=False,
   deathSignal=signal.SIGTERM,
   ...)

but please don't resubmit for such minor thing!
Line 308:  deathSignal=signal.SIGTERM,
Line 309:  nice=NICENESS.HIGH,
Line 310:  ioclass=IOCLASS.IDLE,
Line 311:  env=self.environments())


Line 448: cmd.extend(self._disk_parameters)
Line 449: return cmd
Line 450: 
Line 451: @contextmanager
Line 452: def execute(self):
this just invoked the parent's execute(), thus feels redundant and could be 
dropped
Line 453: with super(self.__class__, self).execute():
Line 454: yield
Line 455: 
Line 456: 


Line 460: 
Line 461: def __init__(self, command):
Line 462: '''
Line 463: do not use directly, use a factory method instead!
Line 464: '''
stale comment, you killed the factory methods.
Line 465: self._command = command
Line 466: self._thread = None
Line 467: 
Line 468: self._status = STATUS.STARTING


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-09 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 4:

> yes, looking at the tests seems much nicer. It will take me some
 > time to process the large changes in v2v.py. Please consider to
 > split it in smaller patches if it makes sense (I just glanced at
 > the code, I don't know for sure)
Don't think its possible since we are removing logic from methods and creating 
classes for each importer.
Basically the patch remove the creating of the command and the processes to 
each class as well as each class execute method have its own contextmanager 
which enable each class to perform actions before and after the import process

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-08 Thread fromani 
Francesco Romani has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 4:

yes, looking at the tests seems much nicer. It will take me some time to 
process the large changes in v2v.py. Please consider to split it in smaller 
patches if it makes sense (I just glanced at the code, I don't know for sure)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-07 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 4:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-06 Thread shavivi 
Shahar Havivi has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 2:

(8 comments)

https://gerrit.ovirt.org/#/c/49951/2/tests/v2vTests.py
File tests/v2vTests.py:

Line 364
Line 365
Line 366
Line 367
Line 368
> Do we still need this?
no...


https://gerrit.ovirt.org/#/c/49951/2/vdsm/v2v.py
File vdsm/v2v.py:

Line 360: super(self.__class__, self).__init__(uri, vminfo, job_id, irs)
Line 361: self._username = username
Line 362: self._password = password
Line 363: 
Line 364: self._disk_parameters = []
> Move up to V2VCommand
Done
Line 365: self._passwd_file = os.path.join(_V2V_DIR, "%s.tmp" % job_id)
Line 366: 
Line 367: def command(self):
Line 368: cmd = [_VIRT_V2V.cmd,


Line 385: return cmd
Line 386: 
Line 387: @contextmanager
Line 388: def execute(self):
Line 389: self._generate_disk_parameters()
> This is common for both, so maybe implement in V2VCommand?
Done
Line 390: self._prepare_volumes()
Line 391: fd = os.open(self._passwd_file, os.O_WRONLY | os.O_CREAT, 
0o600)
Line 392: try:
Line 393: os.write(fd, self._password)


Line 386: 
Line 387: @contextmanager
Line 388: def execute(self):
Line 389: self._generate_disk_parameters()
Line 390: self._prepare_volumes()
> preparing and tearing down volumes is common for all imports, so better kee
Done
Line 391: fd = os.open(self._passwd_file, os.O_WRONLY | os.O_CREAT, 
0o600)
Line 392: try:
Line 393: os.write(fd, self._password)
Line 394: finally:


Line 387: @contextmanager
Line 388: def execute(self):
Line 389: self._generate_disk_parameters()
Line 390: self._prepare_volumes()
Line 391: fd = os.open(self._passwd_file, os.O_WRONLY | os.O_CREAT, 
0o600)
> It is better to keep the password context manager separate. You can nest mu
Done
Line 392: try:
Line 393: os.write(fd, self._password)
Line 394: finally:
Line 395: os.close(fd)


Line 395: os.close(fd)
Line 396: try:
Line 397: yield
Line 398: finally:
Line 399: self._teardown_volumes()
> This is unsafe now, if it raises, we don't clear the password file.
Done
Line 400: try:
Line 401: os.remove(self._passwd_file)
Line 402: except Exception:
Line 403: logging.exception("Job %r error removing passwd file: 
%s",


Line 426: cmd.extend(self._generate_disk_parameters())
Line 427: return cmd
Line 428: 
Line 429: @contextmanager
Line 430: def execute(self):
> This is can make sense if you actually execute the command here, and return
Done
Line 431: self._generate_disk_parameters()
Line 432: self._prepare_volumes()
Line 433: try:
Line 434: yield


Line 507: self._proc = execCmd(self._command.command(), sync=False,
Line 508:  deathSignal=signal.SIGTERM,
Line 509:  nice=NICENESS.HIGH,
Line 510:  ioclass=IOCLASS.IDLE,
Line 511:  env=self._command.environments())
> This can work if execCmd is done in self._command.execute():
Done
Line 512: 
Line 513: self._proc.blocking = True
Line 514: self._watch_process_output()
Line 515: self._wait_for_process()


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Shahar Havivi 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: v2v: extract specific classes for libvirt and ova

2015-12-06 Thread automation 
gerrit-hooks has posted comments on this change.

Change subject: v2v: extract specific classes for libvirt and ova
..


Patch Set 3:

* Update tracker: IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' 
and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a9ecd4a2cde6f379188da647c3a6f8874c41abd
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi 
Gerrit-Reviewer: Francesco Romani 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches