Change in vdsm[master]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 2:

* 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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
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

Change in vdsm[master]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 2: Code-Review-1

(5 comments)

https://gerrit.ovirt.org/#/c/50221/2/vdsm/storage/sdm/api/sdm_job.py
File vdsm/storage/sdm/api/sdm_job.py:

Line 31: from .. import volume_artifacts
Line 32: 
Line 33: 
Line 34: class SdmJob(jobs.Job):
Line 35: log = logging.getLogger('Storage.SdmJob')
Lets use only lowercase logger names in new code: "storage.sdmjob", or 
"storage.sdm"
Line 36: 
Line 37: def __init__(self, job_id, desc, host_id):
Line 38: super(SdmJob, self).__init__(job_id, desc)
Line 39: self._error = None


Line 35: log = logging.getLogger('Storage.SdmJob')
Line 36: 
Line 37: def __init__(self, job_id, desc, host_id):
Line 38: super(SdmJob, self).__init__(job_id, desc)
Line 39: self._error = None
Why not move error up to Job?
Line 40: self._progress = None
Line 41: self.host_id = host_id
Line 42: 
Line 43: @property


Line 45: return self._progress
Line 46: 
Line 47: def info(self):
Line 48: sdm_info = super(SdmJob, self).info()
Line 49: sdm_info['extra_info'] = {'error': self._error}
Why not add error to standard job info?
Line 50: return sdm_info
Line 51: 
Line 52: def run(self):
Line 53: vars.job_id = self.id


Line 58:self.id, self.description)
Line 59: self._status = jobs.STATUS.FAILED
Line 60: self._error = e
Line 61: except Exception as e:
Line 62: self.log.exception("Job (id:%s desc:%s) failed",
We are using mostly (id=%s, desc=%s) in other places, and it is also the format 
used by Pyhton builtins such as namedtuple, so better keep this format.
Line 63:self.id, self.description)
Line 64: self._error = se.GENERAL_EXCEPTION(str(e))
Line 65: self._status = jobs.STATUS.FAILED
Line 66: else:


Line 60: self._error = e
Line 61: except Exception as e:
Line 62: self.log.exception("Job (id:%s desc:%s) failed",
Line 63:self.id, self.description)
Line 64: self._error = se.GENERAL_EXCEPTION(str(e))
This returns a tuple, but we want a GeneralExcpetion instance here.
If creating a GeneralException instance works with current code (it should), 
lets use it.

In the future, we should raise everywhere an error which has a response() 
method, and kill the error handling code in storage_exception.py.
Line 65: self._status = jobs.STATUS.FAILED
Line 66: else:
Line 67: self._status = jobs.STATUS.DONE


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 2:

(2 comments)

https://gerrit.ovirt.org/#/c/50221/2/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

Line 32: class Job(sdm_job.SdmJob):
Line 33: def __init__(self, job_id, host_id, dom_manifest, vol_params):
Line 34: super(Job, self).__init__(job_id, 'create_volume', host_id)
Line 35: self.dom_manifest = dom_manifest
Line 36: self.vol_params = _CreateVolumeParams(vol_params)
Why not _VolumeParameters?
Line 37: 
Line 38: def _run(self):
Line 39: self.dom_manifest.validateCreateVolumeParams(
Line 40: self.vol_params.vol_format, self.vol_params.parent_vol_id)


Line 38: def _run(self):
Line 39: self.dom_manifest.validateCreateVolumeParams(
Line 40: self.vol_params.vol_format, self.vol_params.parent_vol_id)
Line 41: 
Line 42: self.dom_manifest.acquireDomainLock(self.host_id)
We can use now with dom_manifest.domain_lock(self.host_id)
Line 43: try:
Line 44: image_res_ns = sd.getNamespace(self.dom_manifest.sdUUID,
Line 45:IMAGE_NAMESPACE)
Line 46: with rmanager.acquireResource(image_res_ns, 
self.vol_params.img_id,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: sdm: Add create_volume job

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 2:

(7 comments)

https://gerrit.ovirt.org/#/c/50221/2/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

Line 32: class Job(sdm_job.SdmJob):
Line 33: def __init__(self, job_id, host_id, dom_manifest, vol_params):
Line 34: super(Job, self).__init__(job_id, 'create_volume', host_id)
Line 35: self.dom_manifest = dom_manifest
Line 36: self.vol_params = _CreateVolumeParams(vol_params)
> Why not _VolumeParameters?
Changed to _CreateVolumeInfo for symmetry with the schema.
Line 37: 
Line 38: def _run(self):
Line 39: self.dom_manifest.validateCreateVolumeParams(
Line 40: self.vol_params.vol_format, self.vol_params.parent_vol_id)


Line 38: def _run(self):
Line 39: self.dom_manifest.validateCreateVolumeParams(
Line 40: self.vol_params.vol_format, self.vol_params.parent_vol_id)
Line 41: 
Line 42: self.dom_manifest.acquireDomainLock(self.host_id)
> We can use now with dom_manifest.domain_lock(self.host_id)
Done
Line 43: try:
Line 44: image_res_ns = sd.getNamespace(self.dom_manifest.sdUUID,
Line 45:IMAGE_NAMESPACE)
Line 46: with rmanager.acquireResource(image_res_ns, 
self.vol_params.img_id,


https://gerrit.ovirt.org/#/c/50221/2/vdsm/storage/sdm/api/sdm_job.py
File vdsm/storage/sdm/api/sdm_job.py:

Line 31: from .. import volume_artifacts
Line 32: 
Line 33: 
Line 34: class SdmJob(jobs.Job):
Line 35: log = logging.getLogger('Storage.SdmJob')
> Lets use only lowercase logger names in new code: "storage.sdmjob", or "sto
Done
Line 36: 
Line 37: def __init__(self, job_id, desc, host_id):
Line 38: super(SdmJob, self).__init__(job_id, desc)
Line 39: self._error = None


Line 35: log = logging.getLogger('Storage.SdmJob')
Line 36: 
Line 37: def __init__(self, job_id, desc, host_id):
Line 38: super(SdmJob, self).__init__(job_id, desc)
Line 39: self._error = None
> Why not move error up to Job?
Done
Line 40: self._progress = None
Line 41: self.host_id = host_id
Line 42: 
Line 43: @property


Line 45: return self._progress
Line 46: 
Line 47: def info(self):
Line 48: sdm_info = super(SdmJob, self).info()
Line 49: sdm_info['extra_info'] = {'error': self._error}
> Why not add error to standard job info?
Done
Line 50: return sdm_info
Line 51: 
Line 52: def run(self):
Line 53: vars.job_id = self.id


Line 58:self.id, self.description)
Line 59: self._status = jobs.STATUS.FAILED
Line 60: self._error = e
Line 61: except Exception as e:
Line 62: self.log.exception("Job (id:%s desc:%s) failed",
> We are using mostly (id=%s, desc=%s) in other places, and it is also the fo
Done
Line 63:self.id, self.description)
Line 64: self._error = se.GENERAL_EXCEPTION(str(e))
Line 65: self._status = jobs.STATUS.FAILED
Line 66: else:


Line 60: self._error = e
Line 61: except Exception as e:
Line 62: self.log.exception("Job (id:%s desc:%s) failed",
Line 63:self.id, self.description)
Line 64: self._error = se.GENERAL_EXCEPTION(str(e))
> This returns a tuple, but we want a GeneralExcpetion instance here.
Now using utils.GeneralException
Line 65: self._status = jobs.STATUS.FAILED
Line 66: else:
Line 67: self._status = jobs.STATUS.DONE


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: sdm: Add create_volume job

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 1:

(17 comments)

https://gerrit.ovirt.org/#/c/50221/1/tests/sdm_verbs_test.py
File tests/sdm_verbs_test.py:

Line 38: self.sdUUID = sd_id
Line 39: 
Line 40: def validateCreateVolumeParams(self, *args):
Line 41: pass
Line 42: 
> Use @recorded and validate that the job acquired and released the lock?
Done
Line 43: def acquireDomainLock(self, *args):
Line 44: pass
Line 45: 
Line 46: def releaseDomainLock(self, *args):


Line 53: class FakeVolumeArtifacts(object):
Line 54: def __init__(self, img_id, vol_id):
Line 55: self.img_id = img_id
Line 56: self.vol_id = vol_id
Line 57: 
> Use @recorded and validate the the job created and committed?
Added TODO since I depend on the classmethod support in @recorded.
Line 58: def create(self, *args):
Line 59: pass
Line 60: 
Line 61: def commit(self):


Line 73: break
Line 74: 
Line 75: 
Line 76: @expandPermutations
Line 77: class CreateVolumeContainerTests(SdmVerbTests):
> Lets have separate tests for each verb, and put common helpers in sdmtestli
Done
Line 78: 
Line 79: def _get_args(self):
Line 80: dom_manifest = FakeDomainManifest(str(uuid.uuid4()))
Line 81: return dict(dom_manifest=dom_manifest, 
job_id=str(uuid.uuid4()),


https://gerrit.ovirt.org/#/c/50221/1/tests/storagefakelib.py
File tests/storagefakelib.py:

Line 218: return ''.join(random.choice(chars) for _ in range(size))
Line 219: return '-'.join(part(size) for size in [6, 4, 4, 4, 4, 6])
Line 220: 
Line 221: 
Line 222: class FakeResourceManager(object):
> Nice! - lets also @record operation so we can validate the locking?
I used a bit of custom recording logic to achieve this in the next version.
Line 223: @contextmanager
Line 224: def acquireResource(self, namespace, name, lockType, 
timeout=None):
Line 225: yield
Line 226: 


Line 224: def acquireResource(self, namespace, name, lockType, 
timeout=None):
Line 225: yield
Line 226: 
Line 227: def releaseResource(self, namespace, name):
Line 228: pass
> Split this into its own patch.
Done


https://gerrit.ovirt.org/#/c/50221/1/vdsm/storage/sdm/Makefile.am
File vdsm/storage/sdm/Makefile.am:

Line 17: #
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
Line 20: 
Line 21: SUBDIRS = jobs
> Good point, lets keep the package.
Done
Line 22: 
Line 23: include $(top_srcdir)/build-aux/Makefile.subs
Line 24: 
Line 25: vdsmsdmdir = $(vdsmdir)/storage/sdm


https://gerrit.ovirt.org/#/c/50221/1/vdsm/storage/sdm/jobs/Makefile.am
File vdsm/storage/sdm/jobs/Makefile.am:

Line 22: 
Line 23: vdsmsdmjobsdir = $(vdsmdir)/storage/sdm/jobs
Line 24: 
Line 25: dist_vdsmsdmjobs_PYTHON = \
Line 26:createVolumeContainer.py \
> While you rename it, lets use "create_volume.py"
Done
Line 27:$(NULL)


https://gerrit.ovirt.org/#/c/50221/1/vdsm/storage/sdm/jobs/createVolumeContainer.py
File vdsm/storage/sdm/jobs/createVolumeContainer.py:

Line 16: # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 
02110-1301 USA
Line 17: #
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
Line 20: 
> Add this in every new module.
Done
Line 21: from vdsm import jobs
Line 22: from storage import resourceManager as rm
Line 23: from storage import blockVolume, fileVolume, sd
Line 24: from storage.resourceFactories import IMAGE_NAMESPACE


Line 21: from vdsm import jobs
Line 22: from storage import resourceManager as rm
Line 23: from storage import blockVolume, fileVolume, sd
Line 24: from storage.resourceFactories import IMAGE_NAMESPACE
Line 25: from storage.sdm import volumeartifacts
> We can use relative imports to import stuff from storage and storage.sdm:
Done
Line 26: 
Line 27: import sdmJob
Line 28: 
Line 29: rmanager = rm.ResourceManager.getInstance()


Line 23: from storage import blockVolume, fileVolume, sd
Line 24: from storage.resourceFactories import IMAGE_NAMESPACE
Line 25: from storage.sdm import volumeartifacts
Line 26: 
Line 27: import sdmJob
> Use relative imports when importing stuff from same package:
Done
Line 28: 
Line 29: rmanager = rm.ResourceManager.getInstance()
Line 30: 
Line 31: 


Line 47: self._progress = 0
Line 48: 
Line 49: @property
Line 50: def progress(self):
Line 51: return self._progress
> Lets keep it, and return special progress value that engine will treat as i
Let's go with None.  It seems logical.
Line 52: 
Line 53: def _run(self):
Line 54: self.dom_manifest.validateCreateVolumeParams(self.vol_format,
Line 55:  
self.parent_vol_id)


Line 65: artifacts = self._get_artifacts_instance()
Line 66: artifacts.create(sel

Change in vdsm[master]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
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

Change in vdsm[master]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 3: Code-Review-1

(15 comments)

Partial review, I like this very much.

https://gerrit.ovirt.org/#/c/50221/3/tests/sdm_create_volume_test.py
File tests/sdm_create_volume_test.py:

Line 38: 
Line 39: class FakeDomainManifest(object):
Line 40: def __init__(self, sd_id):
Line 41: self.sdUUID = sd_id
Line 42: 
We can@record this an verify that a job verified the arguments.

Lets also check the case when this raises.
Line 43: def validateCreateVolumeParams(self, *args):
Line 44: pass
Line 45: 
Line 46: @recorded


Line 73: self.vol_id = vol_id
Line 74: 
Line 75: # TODO: record these calls and verify them in the tests.
Line 76: 
Line 77: def create(self, *args):
Lets test that we deal correctly with failures here (locks released, error 
reported)
Line 78: pass
Line 79: 
Line 80: def commit(self):
Line 81: pass


Line 76: 
Line 77: def create(self, *args):
Line 78: pass
Line 79: 
Line 80: def commit(self):
Same
Line 81: pass
Line 82: 
Line 83: 
Line 84: class CreateVolumeTests(VdsmTestCase):


Line 82: 
Line 83: 
Line 84: class CreateVolumeTests(VdsmTestCase):
Line 85: 
Line 86: def _get_args(self):
Better call this setUp, and set the needed test state in self.
Line 87: job_id = str(uuid.uuid4())
Line 88: host_id = 1
Line 89: dom_manifest = FakeDomainManifest(str(uuid.uuid4()))
Line 90: vol_info = dict(img_id=str(uuid.uuid4()), 
vol_id=str(uuid.uuid4()),


Line 101: self.rm)]):
Line 102: yield
Line 103: 
Line 104: def test_create_volume(self):
Line 105: args = self._get_args()
You are re-inventing setUp()
Line 106: job = storage.sdm.api.create_volume.Job(**args)
Line 107: 
Line 108: with self._fake_env():
Line 109: job.run()


Line 127: def test_create_volume_domainlock_contended(self):
Line 128: def error(*args):
Line 129: raise se.AcquireLockFailure('id', 'rc', 'out', 'err')
Line 130: 
Line 131: args = self._get_args()
You are re-inventing setUp()
Line 132: args['dom_manifest'].acquireDomainLock = error
Line 133: job = storage.sdm.api.create_volume.Job(**args)
Line 134: job.run()
Line 135: wait_for_job(job)


https://gerrit.ovirt.org/#/c/50221/3/vdsm/storage/sdm/api/__init__.py
File vdsm/storage/sdm/api/__init__.py:

Line 17: #
Line 18: # Refer to the README and COPYING files for full details of the license
Line 19: #
Line 20: 
Line 21: __all__ = [
Please avoid this, we don't want code that does "from sdm import *", adding 20 
names to the global namespace.
Line 22: 'create_volume',


https://gerrit.ovirt.org/#/c/50221/3/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

Line 28: 
Line 29: rmanager = rm.ResourceManager.getInstance()
Line 30: 
Line 31: 
Line 32: class Job(sdm_job.SdmJob):
Please keep empty line after between class and __init__.
Line 33: def __init__(self, job_id, host_id, dom_manifest, vol_info):
Line 34: super(Job, self).__init__(job_id, 'create_volume', host_id)
Line 35: self.dom_manifest = dom_manifest
Line 36: self.vol_info = _CreateVolumeInfo(vol_info)


Line 36: self.vol_info = _CreateVolumeInfo(vol_info)
Line 37: 
Line 38: def _run(self):
Line 39: self.dom_manifest.validateCreateVolumeParams(
Line 40: self.vol_info.vol_format, self.vol_info.parent_vol_id)
Since we validate vol_info in __init__, we should also validate these 
parameters in __init__. I would copy the logic from dom_manifest into 
CreateVolumeInfo, so we don't have to depend on this class method, or if you 
want to avoid the duplication, send the manifest to  CreateVolumeInfo.__init__.
Line 41: 
Line 42: with self.dom_manifest.domain_lock(self.host_id):
Line 43: image_res_ns = sd.getNamespace(self.dom_manifest.sdUUID,
Line 44:IMAGE_NAMESPACE)


Line 50:  self.vol_info.vol_id)
Line 51: artifacts.create(
Line 52: self.vol_info.size, self.vol_info.vol_format,
Line 53: self.vol_info.disk_type, self.vol_info.desc,
Line 54: self.vol_info.parent_vol_id)
Wny not pass vol_info to artifacts.create?
Line 55: artifacts.commit()
Line 56: 
Line 57: # TODO: Adopt the properties framework for managing complex verb 
parameters
Line 58: 


Line 56: 
Line 57: # TODO: Adopt the properties framework for managing complex verb 
parameters
Line 58: 
Line 59: 
Line 60: class _CreateVolumeInfo(object):
We don't need to keep this private. In the future, it would be nice if the 
bridge creates this class (based on the type name in 

Change in vdsm[master]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
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

Change in vdsm[master]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
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

Change in vdsm[master]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 6
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
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

Change in vdsm[master]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 7
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 8: Code-Review-1

(13 comments)

Partial review

https://gerrit.ovirt.org/#/c/50221/8/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3688: their meaning.
Line 3689: """
Line 3690: return {'domains': self.domainMonitor.getHostStatus(domains)}
Line 3691: 
Line 3692: def sdm_schedule(self, name, job):
No need for name parameter, use the job.id or add job.name.
Line 3693: """
Line 3694: SDM jobs currently run using the old TaskManager thread pool 
but none
Line 3695: of the other old Task features (ie. rollbacks, persistence) 
are
Line 3696: supported.  SDM tasks are managed using the Host Jobs API in 
jobs.py.


Line 3695: of the other old Task features (ie. rollbacks, persistence) 
are
Line 3696: supported.  SDM tasks are managed using the Host Jobs API in 
jobs.py.
Line 3697: """
Line 3698: def start_job(*args, **kwargs):
Line 3699: pass
Leftover?
Line 3700: 
Line 3701: jobs.add(job)
Line 3702: self.taskMng.scheduleJob("sdm", None, vars.task, name, 
job.run)
Line 3703: 


Line 3701: jobs.add(job)
Line 3702: self.taskMng.scheduleJob("sdm", None, vars.task, name, 
job.run)
Line 3703: 
Line 3704: @public
Line 3705: def sdm_create_volume(self, job_id, vol_info_params):
We don't need _params - we get a vol_info dict, and wrap it in 
CreateVolumeInfo, replacing vol_info so we cannot touch it.

vol_info = Wapper(vol_info)

This is good pattern when you do this in the start of the function.

In the future, the bridge will wrap the vol_info dict for us, based on the 
schema type.
Line 3706: vol_info = 
sdm.api.create_volume.CreateVolumeInfo(vol_info_params)
Line 3707: dom_manifest = sdCache.produce(vol_info.sd_id).manifest
Line 3708: host_id = self.domainMonitor.getHostId(vol_info.sd_id)
Line 3709: job = sdm.api.create_volume.Job(job_id, host_id, 
dom_manifest,


https://gerrit.ovirt.org/#/c/50221/8/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

Line 31: rmanager = rm.ResourceManager.getInstance()
Line 32: 
Line 33: 
Line 34: class Job(sdm_job.SdmJob):
Line 35: def __init__(self, job_id, host_id, dom_manifest, vol_info):
Lets use "sd" instead of "dom".
Line 36: super(Job, self).__init__(job_id, 'create_volume', host_id)
Line 37: self.dom_manifest = dom_manifest
Line 38: self.vol_info = vol_info
Line 39: 


Line 37: self.dom_manifest = dom_manifest
Line 38: self.vol_info = vol_info
Line 39: 
Line 40: def _run(self):
Line 41: vol_format = volume.name2type(self.vol_info.vol_format)
vol_info.vol_format can be the format enum - the api must use only strings, the 
code can use enums. If we use this system, we don't have to do this kind of 
translation by hand.
Line 42: self.dom_manifest.validateCreateVolumeParams(
Line 43: vol_format, self.vol_info.parent_vol_id)
Line 44: 
Line 45: with self.dom_manifest.domain_lock(self.host_id):


Line 54: try:
Line 55: artifacts.create(
Line 56: self.vol_info.virtual_size, vol_format,
Line 57: self.vol_info.disk_type, 
self.vol_info.description,
Line 58: self.vol_info.parent_vol_id)
Too many parameter, can we pass vol_info as is?
Line 59: artifacts.commit()
Line 60: except (volume_artifacts.CannotCreateVolumeArtifacts,
Line 61: volume_artifacts.VolumeArtifactsNotFound):
Line 62: self.log.exception("Error creating volume 
artifacts")


Line 59: artifacts.commit()
Line 60: except (volume_artifacts.CannotCreateVolumeArtifacts,
Line 61: volume_artifacts.VolumeArtifactsNotFound):
Line 62: self.log.exception("Error creating volume 
artifacts")
Line 63: raise se.VolumeCreationError()
This error translation is bad, and hiding the original error. The concept of 
raising specific error in vdsm for each verb is usless, and we should stop it 
now.

We should have set of common errors, and raise any of them from any verb, based 
the error (e.g OutOfSpace error is good for create volume or snapshot or most 
operations).

Engine has all the context to show translated error about creating a volume, it 
does not need CreateVolumeError from vdsm. If can use more specific errors from 
vdsm like OutOfSpace or PermisionDenied, or NoSuchFile to show useful error 
message.

For example:

Cannot create volume "Foo"

There is no available space on storage domain "Bar".

The first part of the error is based on what engine knows. Th

Change in vdsm[master]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 9: Code-Review-1

(3 comments)

Nice

https://gerrit.ovirt.org/#/c/50221/9/vdsm/storage/sdm/api/Makefile.am
File vdsm/storage/sdm/api/Makefile.am:

Line 24: 
Line 25: dist_vdsmsdmapi_PYTHON = \
Line 26:__init__.py \
Line 27:create_volume.py \
Line 28:sdm_job.py \
sdm_job.Job looks like the base class for all sdm.api..Job, so maybe call 
it base.py:

# sdm/api/delete_image.py
from . import base

class Job(base.Job):
...


https://gerrit.ovirt.org/#/c/50221/9/vdsm/storage/sdm/api/sdm_job.py
File vdsm/storage/sdm/api/sdm_job.py:

Line 37: self._status = jobs.STATUS.PENDING
Line 38: self.host_id = host_id
Line 39: 
Line 40: def run(self):
Line 41: vars.job_id = self.id
You missed my comments from previous versions:
- Move vars.job_id = self.id just before the try:
- Add finally: to clean it up
Line 42: self._status = jobs.STATUS.RUNNING
Line 43: try:
Line 44: self._run()
Line 45: except se.StorageException as e:


Line 42: self._status = jobs.STATUS.RUNNING
Line 43: try:
Line 44: self._run()
Line 45: except se.StorageException as e:
Line 46: self.log.exception("Job (id:%s desc:%s) failed",
Use (id=xxx desc=yyy)
Line 47:self.id, self.description)
Line 48: self._status = jobs.STATUS.FAILED
Line 49: self._error = e
Line 50: except Exception as e:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 10: Verified+1

Verified with 'make check' and by doing LSM+Live Merge.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 10: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/50221/10/vdsm/storage/sdm/api/base.py
File vdsm/storage/sdm/api/base.py:

Line 50: except Exception as e:
Line 51: self.log.exception("Job (id=%s desc=%s) failed",
Line 52:self.id, self.description)
Line 53: self._error = utils.GeneralException(str(e))
Line 54: self._status = jobs.STATUS.FAILED
The only difference between the exception handlers is the wrapping of the 
exeption, so we can do this instead:

except Exception as e:
self.log.exception(...)
if not isinstance(e, utils.GeneralException):
e = utils.GeneralException(str(e))
self._error = e
...
Line 55: else:
Line 56: self._status = jobs.STATUS.DONE
Line 57: finally:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 10:

(1 comment)

https://gerrit.ovirt.org/#/c/50221/10/vdsm/storage/sdm/api/base.py
File vdsm/storage/sdm/api/base.py:

Line 50: except Exception as e:
Line 51: self.log.exception("Job (id=%s desc=%s) failed",
Line 52:self.id, self.description)
Line 53: self._error = utils.GeneralException(str(e))
Line 54: self._status = jobs.STATUS.FAILED
> The only difference between the exception handlers is the wrapping of the e
Done
Line 55: else:
Line 56: self._status = jobs.STATUS.DONE
Line 57: finally:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 11:

* 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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 11: Code-Review-1

(2 comments)

partial review

https://gerrit.ovirt.org/#/c/50221/11/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

Line 77: volume.BLANK_UUID)
Line 78: self.initial_size = _get_param_default(params, 'initial_size', 
0)
Line 79: 
Line 80: 
Line 81: def _get_required(params, name):
_get_xxx is ugly. How about _required() and _enum()?
Line 82: if name not in params:
Line 83: raise ValueError("Missing required property %r" % name)
Line 84: return params[name]
Line 85: 


Line 91:  (value, name, values))
Line 92: return value
Line 93: 
Line 94: 
Line 95: def _get_param_default(params, name, default):
We can drop this, and use instead:

self.foo = params.get("key", "default")
Line 96: try:
Line 97: return params[name]
Line 98: except KeyError:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 12:

* 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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 11:

(2 comments)

https://gerrit.ovirt.org/#/c/50221/11/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

Line 77: volume.BLANK_UUID)
Line 78: self.initial_size = _get_param_default(params, 'initial_size', 
0)
Line 79: 
Line 80: 
Line 81: def _get_required(params, name):
> _get_xxx is ugly. How about _required() and _enum()?
Done
Line 82: if name not in params:
Line 83: raise ValueError("Missing required property %r" % name)
Line 84: return params[name]
Line 85: 


Line 91:  (value, name, values))
Line 92: return value
Line 93: 
Line 94: 
Line 95: def _get_param_default(params, name, default):
> We can drop this, and use instead:
Done
Line 96: try:
Line 97: return params[name]
Line 98: except KeyError:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 11
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 9:

(2 comments)

https://gerrit.ovirt.org/#/c/50221/9/vdsm/storage/sdm/api/sdm_job.py
File vdsm/storage/sdm/api/sdm_job.py:

Line 37: self._status = jobs.STATUS.PENDING
Line 38: self.host_id = host_id
Line 39: 
Line 40: def run(self):
Line 41: vars.job_id = self.id
> You missed my comments from previous versions:
Done
Line 42: self._status = jobs.STATUS.RUNNING
Line 43: try:
Line 44: self._run()
Line 45: except se.StorageException as e:


Line 42: self._status = jobs.STATUS.RUNNING
Line 43: try:
Line 44: self._run()
Line 45: except se.StorageException as e:
Line 46: self.log.exception("Job (id:%s desc:%s) failed",
> Use (id=xxx desc=yyy)
Done
Line 47:self.id, self.description)
Line 48: self._status = jobs.STATUS.FAILED
Line 49: self._error = e
Line 50: except Exception as e:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 9
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 12:

(1 comment)

https://gerrit.ovirt.org/#/c/50221/12/vdsm/storage/sdm/api/base.py
File vdsm/storage/sdm/api/base.py:

Line 44: self._run()
Line 45: except Exception as e:
Line 46: self.log.exception("Job (id=%s desc=%s) failed",
Line 47:self.id, self.description)
Line 48: if not isinstance(e, utils.GeneralException):
Needs a rebase, this exceptions is in lib/vdsm/exception.py
Line 49: e = utils.GeneralException(str(e))
Line 50: self._error = e
Line 51: self._status = jobs.STATUS.FAILED
Line 52: else:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 13:

* 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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 13
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 14:

* 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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 14
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 15:

* 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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 15:

(8 comments)

https://gerrit.ovirt.org/#/c/50221/8/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 3688
Line 3689
Line 3690
Line 3691
Line 3692
> No need for name parameter, use the job.id or add job.name.
Done


Line 3695
Line 3696
Line 3697
Line 3698
Line 3699
> Leftover?
yep, removed,


Line 3701
Line 3702
Line 3703
Line 3704
Line 3705
> We don't need _params - we get a vol_info dict, and wrap it in CreateVolume
Done


https://gerrit.ovirt.org/#/c/50221/8/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

Line 31: 
Line 32: class Job(base.Job):
Line 33: def __init__(self, job_id, host_id, sd_manifest, vol_info):
Line 34: super(Job, self).__init__(job_id, 'create_volume', host_id)
Line 35: self.sd_manifest = sd_manifest
> Lets use "sd" instead of "dom".
Done
Line 36: self.vol_info = vol_info
Line 37: 
Line 38: def _run(self):
Line 39: vol_format = volume.name2type(self.vol_info.vol_format)


Line 54: artifacts.commit()
Line 55: 
Line 56: 
Line 57: # TODO: Adopt the properties framework for managing complex verb 
parameters
Line 58: 
> Too many parameter, can we pass vol_info as is?
Let's worry about this for a future patch.
Line 59: 
Line 60: class CreateVolumeInfo(object):
Line 61: def __init__(self, params):
Line 62: self.sd_id = _required(params, 'sd_id')


Line 59: 
Line 60: class CreateVolumeInfo(object):
Line 61: def __init__(self, params):
Line 62: self.sd_id = _required(params, 'sd_id')
Line 63: self.img_id = _required(params, 'img_id')
> This error translation is bad, and hiding the original error. The concept o
Done
Line 64: self.vol_id = _required(params, 'vol_id')
Line 65: self.virtual_size = _required(params, 'virtual_size')
Line 66: vol_types = [volume.VOLUME_TYPES[vt]
Line 67:  for vt in (volume.RAW_FORMAT, volume.COW_FORMAT)]


Line 73: self.initial_size = params.get('initial_size', 0)
Line 74: 
Line 75: 
Line 76: def _required(params, name):
Line 77: if name not in params:
> This must a string in new code - no more magic integers in the api, but the
Agreed.  This evaluates to ['COW', 'RAW']
Line 78: raise ValueError("Missing required property %r" % name)
Line 79: return params[name]
Line 80: 
Line 81: 


Line 75: 
Line 76: def _required(params, name):
Line 77: if name not in params:
Line 78: raise ValueError("Missing required property %r" % name)
Line 79: return params[name]
> This must a string in new code - no more magic integers in the api, but the
Agreed.  This evaluates to ['UNKNOWN', 'SYSTEM', 'DATA', 'SHARED', 'SWAP', 
'TEMP']
Line 80: 
Line 81: 
Line 82: def _enum(params, name, values):
Line 83: value = _required(params, name)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 15: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 15: Code-Review-1

(5 comments)

https://gerrit.ovirt.org/#/c/50221/15/vdsm/storage/sdm/api/base.py
File vdsm/storage/sdm/api/base.py:

Line 22: 
Line 23: import logging
Line 24: 
Line 25: from vdsm import jobs
Line 26: from vdsm import exception
This is now in vdsm.storage
Line 27: 
Line 28: from storage.threadLocal import vars
Line 29: 
Line 30: 


Line 34: 
Line 35: def __init__(self, job_id, desc, host_id):
Line 36: super(Job, self).__init__(job_id, desc)
Line 37: self._status = jobs.STATUS.PENDING
Line 38: self.host_id = host_id
This works now, but we would like to support different id per storage domain. 
Better get the host id when you take the domain(s) lock.

If you don't find an easy way we can work on this later.
Line 39: 
Line 40: def run(self):
Line 41: self._status = jobs.STATUS.RUNNING
Line 42: vars.job_id = self.id


https://gerrit.ovirt.org/#/c/50221/15/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

Line 39: vol_format = volume.name2type(self.vol_info.vol_format)
Line 40: self.sd_manifest.validateCreateVolumeParams(
Line 41: vol_format, self.vol_info.parent_vol_id)
Line 42: 
Line 43: with self.sd_manifest.domain_lock(self.host_id):
We should make the domain host id accessible to the manifest object, so we 
don't have to pass the host id. We want to support different host id per domain.

If you don't find an easy way, we can work on this later.
Line 44: image_res_ns = sd.getNamespace(self.sd_manifest.sdUUID,
Line 45:IMAGE_NAMESPACE)
Line 46: with rmanager.acquireResource(image_res_ns, 
self.vol_info.img_id,
Line 47:   rm.LockType.exclusive):


Line 74: 
Line 75: 
Line 76: def _required(params, name):
Line 77: if name not in params:
Line 78: raise ValueError("Missing required property %r" % name)
Why not use se.InvalidParameterException?
Line 79: return params[name]
Line 80: 
Line 81: 
Line 82: def _enum(params, name, values):


Line 82: def _enum(params, name, values):
Line 83: value = _required(params, name)
Line 84: if value not in values:
Line 85: raise ValueError("Invalid value %r for %r, expecting one of 
%s" %
Line 86:  (value, name, values))
Same


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 15:

(5 comments)

https://gerrit.ovirt.org/#/c/50221/15/vdsm/storage/sdm/api/base.py
File vdsm/storage/sdm/api/base.py:

Line 22: 
Line 23: import logging
Line 24: 
Line 25: from vdsm import jobs
Line 26: from vdsm import exception
> This is now in vdsm.storage
Nope.  Using base exceptions from vdsm only
Line 27: 
Line 28: from storage.threadLocal import vars
Line 29: 
Line 30: 


Line 34: 
Line 35: def __init__(self, job_id, desc, host_id):
Line 36: super(Job, self).__init__(job_id, desc)
Line 37: self._status = jobs.STATUS.PENDING
Line 38: self.host_id = host_id
> This works now, but we would like to support different id per storage domai
Right now the only way to get the host ID is to look up the domainMonitor from 
the cif singleton and get it from there.  For one, that is not clean at all and 
we are going to be making some changes in that area as part of the sdm pool 
refactoring later.  I prefer to keep this simple for now and we can always drop 
the host id parameter later if there ends up being a better way to look it up.
Line 39: 
Line 40: def run(self):
Line 41: self._status = jobs.STATUS.RUNNING
Line 42: vars.job_id = self.id


https://gerrit.ovirt.org/#/c/50221/15/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

Line 39: vol_format = volume.name2type(self.vol_info.vol_format)
Line 40: self.sd_manifest.validateCreateVolumeParams(
Line 41: vol_format, self.vol_info.parent_vol_id)
Line 42: 
Line 43: with self.sd_manifest.domain_lock(self.host_id):
> We should make the domain host id accessible to the manifest object, so we 
As stated in another response, let's leave this change until after the pool 
refactoring.
Line 44: image_res_ns = sd.getNamespace(self.sd_manifest.sdUUID,
Line 45:IMAGE_NAMESPACE)
Line 46: with rmanager.acquireResource(image_res_ns, 
self.vol_info.img_id,
Line 47:   rm.LockType.exclusive):


Line 74: 
Line 75: 
Line 76: def _required(params, name):
Line 77: if name not in params:
Line 78: raise ValueError("Missing required property %r" % name)
> Why not use se.InvalidParameterException?
Because InvalidParameterException is for reporting an invalid value, not a 
missing value.  I can switch to exception.MissingParameter instead.
Line 79: return params[name]
Line 80: 
Line 81: 
Line 82: def _enum(params, name, values):


Line 82: def _enum(params, name, values):
Line 83: value = _required(params, name)
Line 84: if value not in values:
Line 85: raise ValueError("Invalid value %r for %r, expecting one of 
%s" %
Line 86:  (value, name, values))
> Same
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 16:

* 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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 16: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 15:

(4 comments)

https://gerrit.ovirt.org/#/c/50221/15/vdsm/storage/sdm/api/base.py
File vdsm/storage/sdm/api/base.py:

Line 22: 
Line 23: import logging
Line 24: 
Line 25: from vdsm import jobs
Line 26: from vdsm import exception
> Nope.  Using base exceptions from vdsm only
OK
Line 27: 
Line 28: from storage.threadLocal import vars
Line 29: 
Line 30: 


Line 34: 
Line 35: def __init__(self, job_id, desc, host_id):
Line 36: super(Job, self).__init__(job_id, desc)
Line 37: self._status = jobs.STATUS.PENDING
Line 38: self.host_id = host_id
> Right now the only way to get the host ID is to look up the domainMonitor f
OK
Line 39: 
Line 40: def run(self):
Line 41: self._status = jobs.STATUS.RUNNING
Line 42: vars.job_id = self.id


https://gerrit.ovirt.org/#/c/50221/15/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

Line 39: vol_format = volume.name2type(self.vol_info.vol_format)
Line 40: self.sd_manifest.validateCreateVolumeParams(
Line 41: vol_format, self.vol_info.parent_vol_id)
Line 42: 
Line 43: with self.sd_manifest.domain_lock(self.host_id):
> As stated in another response, let's leave this change until after the pool
OK
Line 44: image_res_ns = sd.getNamespace(self.sd_manifest.sdUUID,
Line 45:IMAGE_NAMESPACE)
Line 46: with rmanager.acquireResource(image_res_ns, 
self.vol_info.img_id,
Line 47:   rm.LockType.exclusive):


Line 74: 
Line 75: 
Line 76: def _required(params, name):
Line 77: if name not in params:
Line 78: raise ValueError("Missing required property %r" % name)
> Because InvalidParameterException is for reporting an invalid value, not a 
MissingParameter sounds good.
Line 79: return params[name]
Line 80: 
Line 81: 
Line 82: def _enum(params, name, values):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 15
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 16: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 16:

Looks good, but lets have another review.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[frolland]

Freddy Rolland has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 16: Code-Review-1

(2 comments)

Please remove vdsm/storage/sdm/jobs/__init__.py

https://gerrit.ovirt.org/#/c/50221/16/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

PS16, Line 79: def _required(params, name):
I guess we will need '_required' and '_enum' in a lot of other places.
Maybe we should have it in some common util module?


https://gerrit.ovirt.org/#/c/50221/16/vdsm/storage/sdm/jobs/__init__.py
File vdsm/storage/sdm/jobs/__init__.py:

This file is not needed


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 17:

* 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/50221
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 16:

(2 comments)

https://gerrit.ovirt.org/#/c/50221/16/vdsm/storage/sdm/api/create_volume.py
File vdsm/storage/sdm/api/create_volume.py:

Line 75: self.parent_vol_id = params.get('parent_vol_id', 
volume.BLANK_UUID)
Line 76: self.initial_size = params.get('initial_size', 0)
Line 77: 
Line 78: 
Line 79: def _required(params, name):
> I guess we will need '_required' and '_enum' in a lot of other places.
Yes, we talked about this.  Nir has a series that will address this but I don't 
want to hold up sdm work on it.  For now, we'll use sdm-specific validation 
routines but switch to the shared version once it hits the repo.  

https://gerrit.ovirt.org/#/q/status:open+project:vdsm+branch:master+topic:properties
Line 80: if name not in params:
Line 81: raise exception.MissingParameter()
Line 82: return params[name]
Line 83: 


https://gerrit.ovirt.org/#/c/50221/16/vdsm/storage/sdm/jobs/__init__.py
File vdsm/storage/sdm/jobs/__init__.py:

> This file is not needed
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[alitke]

Adam Litke has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 17: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[frolland]

Freddy Rolland has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 17: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has submitted this change and it was merged.

Change subject: sdm: Add create_volume job
..


sdm: Add create_volume job

This is the first SDM verb to be wired up using all of the
infrastructure (jobs and getHostJobs API, VolumeArtifacts, etc.).  HSM
creates a create_volume.Job and schedules it using the existing
TaskManager thread pool.  Eventually we can switch to using the executor
framework but that can be for a later patch.

Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Signed-off-by: Adam Litke 
Reviewed-on: https://gerrit.ovirt.org/50221
Reviewed-by: Freddy Rolland 
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer 
---
M configure.ac
M debian/vdsm.install
M tests/Makefile.am
A tests/sdmtestlib.py
A tests/storage_sdm_api_test.py
A tests/storage_sdm_create_volume_test.py
M vdsm.spec.in
M vdsm/storage/hsm.py
M vdsm/storage/sdm/Makefile.am
A vdsm/storage/sdm/api/Makefile.am
A vdsm/storage/sdm/api/__init__.py
A vdsm/storage/sdm/api/base.py
A vdsm/storage/sdm/api/create_volume.py
13 files changed, 480 insertions(+), 1 deletion(-)

Approvals:
  Nir Soffer: Looks good to me, approved
  Adam Litke: Verified
  Jenkins CI: Passed CI tests
  Freddy Rolland: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 17: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
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]: sdm: Add create_volume job

[automation]

gerrit-hooks has posted comments on this change.

Change subject: sdm: Add create_volume job
..


Patch Set 18:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia614059f52c9625da7841ea9fbca2b2f2375cd75
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Allon Mureinik 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 
Gerrit-Reviewer: Maor Lipchuk 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: Tal Nisan 
Gerrit-Reviewer: Vered Volansky 
Gerrit-Reviewer: gerrit-hooks 
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches