Change in vdsm[master]: storage: Add basic BlockVolumeArtifacts

[nsoffer]

Nir Soffer has submitted this change and it was merged.

Change subject: storage: Add basic BlockVolumeArtifacts
..


storage: Add basic BlockVolumeArtifacts

This patch adds the block specific support for the VolumeArtifacts
interface.  A BlockVolumeArtifact is first created by adding an LV with
a special tag.  It is comitted to a BlockVolume by removing that tag.

Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Signed-off-by: Adam Litke 
Reviewed-on: https://gerrit.ovirt.org/55987
Continuous-Integration: Jenkins CI
Reviewed-by: Freddy Rolland 
Reviewed-by: Nir Soffer 
---
M tests/storage_volume_artifacts_test.py
M tests/storagetestlib.py
M vdsm/storage/blockSD.py
M vdsm/storage/sdm/volume_artifacts.py
4 files changed, 339 insertions(+), 8 deletions(-)

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 22
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 
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]: storage: Add basic BlockVolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 21: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 21
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
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]: storage: Add basic BlockVolumeArtifacts

[frolland]

Freddy Rolland has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 21: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 21
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 21:

* 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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 21
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
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]: storage: Add basic BlockVolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 20: Code-Review+1

Waiting for another review.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
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]: storage: Add basic BlockVolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 20:

Please another review on this change.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
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]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 20: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 20:

* 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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 20
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
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]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 19:

(1 comment)

https://gerrit.ovirt.org/#/c/55987/19/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 377: slot_before = self.get_next_free_slot(env)
Line 378: artifacts = env.sd_manifest.get_volume_artifacts(
Line 379: self.img_id, self.vol_id)
Line 380: with MonkeyPatchScope([
Line 381: [blockVolume.BlockVolumeManifest, '_putMetadata', 
failure]
> Now changing BlockVolumeManifest private method will break unrelated tests 
Done
Line 382: ]):
Line 383: self.assertRaises(ExpectedFailure, artifacts.create,
Line 384:   *BASE_RAW_PARAMS)
Line 385: self.validate_invisibility(env, artifacts, 
is_garbage=True)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
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]: storage: Add basic BlockVolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 19: Code-Review-1

(1 comment)

https://gerrit.ovirt.org/#/c/55987/19/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 377: slot_before = self.get_next_free_slot(env)
Line 378: artifacts = env.sd_manifest.get_volume_artifacts(
Line 379: self.img_id, self.vol_id)
Line 380: with MonkeyPatchScope([
Line 381: [blockVolume.BlockVolumeManifest, '_putMetadata', 
failure]
Now changing BlockVolumeManifest private method will break unrelated tests :-)

Why not mock the public method used by BlockVolumeArtifacts?
Line 382: ]):
Line 383: self.assertRaises(ExpectedFailure, artifacts.create,
Line 384:   *BASE_RAW_PARAMS)
Line 385: self.validate_invisibility(env, artifacts, 
is_garbage=True)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 19:

* 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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 19
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
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]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 18:

(8 comments)

https://gerrit.ovirt.org/#/c/55987/18/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 331: # If we fail to create the LV then storage is clean and we 
can retry
Line 332: with self.fake_env() as env:
Line 333: artifacts = env.sd_manifest.get_volume_artifacts(
Line 334: self.img_id, self.vol_id)
Line 335: artifacts._create_lv_artifact = self.failure
> We don't want to know about private methods in the tests. We should mock ou
Yes, changed and it looks much nicer.
Line 336: self.assertRaises(ExpectedFailure, artifacts.create,
Line 337:   *BASE_RAW_PARAMS)
Line 338: self.validate_invisibility(env, artifacts, 
is_garbage=False)
Line 339: 


Line 341: artifacts = env.sd_manifest.get_volume_artifacts(
Line 342: self.img_id, self.vol_id)
Line 343: artifacts.create(*BASE_RAW_PARAMS)
Line 344: 
Line 345: def test_create_fail_acquiring_meta_slot(self):
> We need to mock the domain method for acquiring metadata slot.
Done
Line 346: # If we fail to acquire the meta_slot we have just a garbage 
LV
Line 347: with self.fake_env() as env:
Line 348: artifacts = env.sd_manifest.get_volume_artifacts(
Line 349: self.img_id, self.vol_id)


Line 352:   *BASE_RAW_PARAMS)
Line 353: self.validate_invisibility(env, artifacts, 
is_garbage=True)
Line 354: self.validate_domain_has_garbage(env.sd_manifest)
Line 355: 
Line 356: def test_create_fail_setting_meta_slot(self):
> Name too similar to previous test - maybe test_create_fail_to_set_mdtag
changed to test_create_fail_setting_metadata_lvtag
Line 357: # If we fail to set the meta_slot in the LV tags that slot 
remains
Line 358: # available for allocation (even without garbage collection)
Line 359: 
Line 360: with self.fake_env() as env:


Line 354: self.validate_domain_has_garbage(env.sd_manifest)
Line 355: 
Line 356: def test_create_fail_setting_meta_slot(self):
Line 357: # If we fail to set the meta_slot in the LV tags that slot 
remains
Line 358: # available for allocation (even without garbage collection)
> Nice!
Yeah, pretty slick.
Line 359: 
Line 360: with self.fake_env() as env:
Line 361: def patch_and_fail(f):
Line 362: @wraps(f)


Line 364: with MonkeyPatchScope([
Line 365: [env.lvm, 'changeLVTags', self.failure]
Line 366: ]):
Line 367: f(*args, **kwargs)
Line 368: return wrapper
> Too complicated.
Fixed with MonkeyPatch
Line 369: 
Line 370: slot_before = self.get_next_free_slot(env)
Line 371: artifacts = env.sd_manifest.get_volume_artifacts(
Line 372: self.img_id, self.vol_id)


Line 370: slot_before = self.get_next_free_slot(env)
Line 371: artifacts = env.sd_manifest.get_volume_artifacts(
Line 372: self.img_id, self.vol_id)
Line 373: artifacts._acquire_metadata_slot = \
Line 374: patch_and_fail(artifacts._acquire_metadata_slot)
> We can do:
Done
Line 375: self.assertRaises(ExpectedFailure, artifacts.create,
Line 376:   *BASE_RAW_PARAMS)
Line 377: self.assertEqual(slot_before, 
self.get_next_free_slot(env))
Line 378: self.validate_invisibility(env, artifacts, 
is_garbage=True)


Line 384: with self.fake_env() as env:
Line 385: slot_before = self.get_next_free_slot(env)
Line 386: artifacts = env.sd_manifest.get_volume_artifacts(
Line 387: self.img_id, self.vol_id)
Line 388: artifacts._create_metadata = self.failure
> same
Done
Line 389: self.assertRaises(ExpectedFailure, artifacts.create,
Line 390:   *BASE_RAW_PARAMS)
Line 391: self.validate_invisibility(env, artifacts, 
is_garbage=True)
Line 392: self.validate_domain_has_garbage(env.sd_manifest)


Line 396: # We leave behind a garbage LV and metadata area
Line 397: with self.fake_env() as env:
Line 398: artifacts = env.sd_manifest.get_volume_artifacts(
Line 399: self.img_id, self.vol_id)
Line 400: artifacts._create_lease = self.failure
> Same - you should mock the domain clusterlock object.
I just mocked BlockVolumeMetadata.newVolumeLease
Line 401: self.assertRaises(ExpectedFailure, artifacts.create,
Line 402:   *BASE_RAW_PARAMS)
Line 403: 

Change in vdsm[master]: storage: Add basic BlockVolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 18: Code-Review-1

(8 comments)

https://gerrit.ovirt.org/#/c/55987/18/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 331: # If we fail to create the LV then storage is clean and we 
can retry
Line 332: with self.fake_env() as env:
Line 333: artifacts = env.sd_manifest.get_volume_artifacts(
Line 334: self.img_id, self.vol_id)
Line 335: artifacts._create_lv_artifact = self.failure
We don't want to know about private methods in the tests. We should mock our 
fake lvm.createLV instead.
Line 336: self.assertRaises(ExpectedFailure, artifacts.create,
Line 337:   *BASE_RAW_PARAMS)
Line 338: self.validate_invisibility(env, artifacts, 
is_garbage=False)
Line 339: 


Line 341: artifacts = env.sd_manifest.get_volume_artifacts(
Line 342: self.img_id, self.vol_id)
Line 343: artifacts.create(*BASE_RAW_PARAMS)
Line 344: 
Line 345: def test_create_fail_acquiring_meta_slot(self):
We need to mock the domain method for acquiring metadata slot.
Line 346: # If we fail to acquire the meta_slot we have just a garbage 
LV
Line 347: with self.fake_env() as env:
Line 348: artifacts = env.sd_manifest.get_volume_artifacts(
Line 349: self.img_id, self.vol_id)


Line 352:   *BASE_RAW_PARAMS)
Line 353: self.validate_invisibility(env, artifacts, 
is_garbage=True)
Line 354: self.validate_domain_has_garbage(env.sd_manifest)
Line 355: 
Line 356: def test_create_fail_setting_meta_slot(self):
Name too similar to previous test - maybe test_create_fail_to_set_mdtag
Line 357: # If we fail to set the meta_slot in the LV tags that slot 
remains
Line 358: # available for allocation (even without garbage collection)
Line 359: 
Line 360: with self.fake_env() as env:


Line 354: self.validate_domain_has_garbage(env.sd_manifest)
Line 355: 
Line 356: def test_create_fail_setting_meta_slot(self):
Line 357: # If we fail to set the meta_slot in the LV tags that slot 
remains
Line 358: # available for allocation (even without garbage collection)
Nice!
Line 359: 
Line 360: with self.fake_env() as env:
Line 361: def patch_and_fail(f):
Line 362: @wraps(f)


Line 364: with MonkeyPatchScope([
Line 365: [env.lvm, 'changeLVTags', self.failure]
Line 366: ]):
Line 367: f(*args, **kwargs)
Line 368: return wrapper
Too complicated.
Line 369: 
Line 370: slot_before = self.get_next_free_slot(env)
Line 371: artifacts = env.sd_manifest.get_volume_artifacts(
Line 372: self.img_id, self.vol_id)


Line 370: slot_before = self.get_next_free_slot(env)
Line 371: artifacts = env.sd_manifest.get_volume_artifacts(
Line 372: self.img_id, self.vol_id)
Line 373: artifacts._acquire_metadata_slot = \
Line 374: patch_and_fail(artifacts._acquire_metadata_slot)
We can do:

with MonkeyPatchScope([[env.lvm, 'changeLVTags', self.failure]]):
try to create...
assert...

try to create again
assert
Line 375: self.assertRaises(ExpectedFailure, artifacts.create,
Line 376:   *BASE_RAW_PARAMS)
Line 377: self.assertEqual(slot_before, 
self.get_next_free_slot(env))
Line 378: self.validate_invisibility(env, artifacts, 
is_garbage=True)


Line 384: with self.fake_env() as env:
Line 385: slot_before = self.get_next_free_slot(env)
Line 386: artifacts = env.sd_manifest.get_volume_artifacts(
Line 387: self.img_id, self.vol_id)
Line 388: artifacts._create_metadata = self.failure
same
Line 389: self.assertRaises(ExpectedFailure, artifacts.create,
Line 390:   *BASE_RAW_PARAMS)
Line 391: self.validate_invisibility(env, artifacts, 
is_garbage=True)
Line 392: self.validate_domain_has_garbage(env.sd_manifest)


Line 396: # We leave behind a garbage LV and metadata area
Line 397: with self.fake_env() as env:
Line 398: artifacts = env.sd_manifest.get_volume_artifacts(
Line 399: self.img_id, self.vol_id)
Line 400: artifacts._create_lease = self.failure
Same - you should mock the domain clusterlock object.
Line 401: self.assertRaises(ExpectedFailure, artifacts.create,
Line 402:   *BASE_RAW_PARAMS)
Line 403: 

Change in vdsm[master]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 18:

* 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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 18
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
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]: storage: Add basic BlockVolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 17:

We need more reviews.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Freddy Rolland 
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]: storage: Add basic BlockVolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 17:

(1 comment)

https://gerrit.ovirt.org/#/c/55987/17/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 305: 
Line 306: GARBAGE
Line 307: 
Line 308: - States:
Line 309: - A logical volume with the TEMP_VOL_LVTAG tag exists
> True, but we don't describe the analogous sub-states in the file case eithe
OK
Line 310: - Operations:
Line 311: - is_garbage -> true
Line 312: - is_image -> true or false
Line 313: - clean -> change state to MISSING


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 17
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]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 17:

(7 comments)

https://gerrit.ovirt.org/#/c/55987/17/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 305: 
Line 306: GARBAGE
Line 307: 
Line 308: - States:
Line 309: - A logical volume with the TEMP_VOL_LVTAG tag exists
> We have many sub states here:
True, but we don't describe the analogous sub-states in the file case either.  
Here we should describe the markers only since those indicate that one or more 
pieces of garbage may exist and need to be cleaned.
Line 310: - Operations:
Line 311: - is_garbage -> true
Line 312: - is_image -> true or false
Line 313: - clean -> change state to MISSING


Line 393: tags = (TEMP_VOL_LVTAG,
Line 394: blockVolume.TAG_PREFIX_PARENT + parent_vol_id,
Line 395: blockVolume.TAG_PREFIX_IMAGE + self.img_id)
Line 396: lvm.createLV(self.sd_manifest.sdUUID, self.vol_id, lv_size,
Line 397:  activate=True, initialTags=tags)
> Do we have a test for lvm.createLV failure?
yep.
Line 398: 
Line 399: def _acquire_metadata_slot(self):
Line 400: with self.sd_manifest.acquireVolumeMetadataSlot(
Line 401: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as slot:


Line 395: blockVolume.TAG_PREFIX_IMAGE + self.img_id)
Line 396: lvm.createLV(self.sd_manifest.sdUUID, self.vol_id, lv_size,
Line 397:  activate=True, initialTags=tags)
Line 398: 
Line 399: def _acquire_metadata_slot(self):
> We need a test for failure to acquire the slot, and failure to change the l
yep.
Line 400: with self.sd_manifest.acquireVolumeMetadataSlot(
Line 401: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as slot:
Line 402: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id,
Line 403:  addTags=[blockVolume.TAG_PREFIX_MD + 
str(slot)])


Line 399: def _acquire_metadata_slot(self):
Line 400: with self.sd_manifest.acquireVolumeMetadataSlot(
Line 401: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as slot:
Line 402: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id,
Line 403:  addTags=[blockVolume.TAG_PREFIX_MD + 
str(slot)])
> I would like to make the code easier to read(or debug) like this:
Done
Line 404: return self.sd_manifest.sdUUID, slot
Line 405: 
Line 406: def _create_metadata(self, meta_id, size, vol_format, prealloc, 
disk_type,
Line 407:  desc, parent):


Line 400: with self.sd_manifest.acquireVolumeMetadataSlot(
Line 401: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as slot:
Line 402: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id,
Line 403:  addTags=[blockVolume.TAG_PREFIX_MD + 
str(slot)])
Line 404: return self.sd_manifest.sdUUID, slot
> Returning meta_id is useful for the callers, but it is not related to the p
Done
Line 405: 
Line 406: def _create_metadata(self, meta_id, size, vol_format, prealloc, 
disk_type,
Line 407:  desc, parent):
Line 408: # When the volume format is RAW the real volume capacity is 
the device


Line 408: # When the volume format is RAW the real volume capacity is 
the device
Line 409: # size.  The device size may have been rounded up if 'size' 
is not
Line 410: # divisible by the domain's extent size.
Line 411: if vol_format == volume.RAW_FORMAT:
Line 412: size = int(self.sd_manifest.getVSize(self.img_id, 
self.vol_id))
> We have a test for this, right?
yes several:
 - at lvm level: storagefakelibTests.test_lv_size_rounding
 - at volume_artifacts level: storage_volume_artifacts_test.test_size_rounded_up
Line 413: 
Line 414: # We use the BlockVolumeManifest API here because we create 
the
Line 415: # metadata in the standard way.  We cannot do this for file 
volumes
Line 416: # because the metadata needs to be written to a specially 
named file.


Line 430: volume.LEGAL_VOL)
Line 431: 
Line 432: def _create_lease(self, meta_id):
Line 433: self.vol_class.newVolumeLease(
Line 434: meta_id, self.sd_manifest.sdUUID, self.vol_id)
> We need a test for failure to create the lease.
yep


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 17
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer 

Change in vdsm[master]: storage: Add basic BlockVolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 17: Code-Review-1

(7 comments)

https://gerrit.ovirt.org/#/c/55987/17/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 305: 
Line 306: GARBAGE
Line 307: 
Line 308: - States:
Line 309: - A logical volume with the TEMP_VOL_LVTAG tag exists
We have many sub states here:

- setting lv md tag failed
- writing metadata failed
- creating the lease failed
Line 310: - Operations:
Line 311: - is_garbage -> true
Line 312: - is_image -> true or false
Line 313: - clean -> change state to MISSING


Line 393: tags = (TEMP_VOL_LVTAG,
Line 394: blockVolume.TAG_PREFIX_PARENT + parent_vol_id,
Line 395: blockVolume.TAG_PREFIX_IMAGE + self.img_id)
Line 396: lvm.createLV(self.sd_manifest.sdUUID, self.vol_id, lv_size,
Line 397:  activate=True, initialTags=tags)
Do we have a test for lvm.createLV failure?
Line 398: 
Line 399: def _acquire_metadata_slot(self):
Line 400: with self.sd_manifest.acquireVolumeMetadataSlot(
Line 401: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as slot:


Line 395: blockVolume.TAG_PREFIX_IMAGE + self.img_id)
Line 396: lvm.createLV(self.sd_manifest.sdUUID, self.vol_id, lv_size,
Line 397:  activate=True, initialTags=tags)
Line 398: 
Line 399: def _acquire_metadata_slot(self):
We need a test for failure to acquire the slot, and failure to change the lv.
Line 400: with self.sd_manifest.acquireVolumeMetadataSlot(
Line 401: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as slot:
Line 402: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id,
Line 403:  addTags=[blockVolume.TAG_PREFIX_MD + 
str(slot)])


Line 399: def _acquire_metadata_slot(self):
Line 400: with self.sd_manifest.acquireVolumeMetadataSlot(
Line 401: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as slot:
Line 402: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id,
Line 403:  addTags=[blockVolume.TAG_PREFIX_MD + 
str(slot)])
I would like to make the code easier to read(or debug) like this:

md_tag = blockVolume.TAG_PREFIX_MD + str(slot)
lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, addTags=[md_tag])
Line 404: return self.sd_manifest.sdUUID, slot
Line 405: 
Line 406: def _create_metadata(self, meta_id, size, vol_format, prealloc, 
disk_type,
Line 407:  desc, parent):


Line 400: with self.sd_manifest.acquireVolumeMetadataSlot(
Line 401: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as slot:
Line 402: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id,
Line 403:  addTags=[blockVolume.TAG_PREFIX_MD + 
str(slot)])
Line 404: return self.sd_manifest.sdUUID, slot
Returning meta_id is useful for the callers, but it is not related to the 
purpose of this function. The caller can build the meta_id using 
self.sd_manifest.sdUUID themself.
Line 405: 
Line 406: def _create_metadata(self, meta_id, size, vol_format, prealloc, 
disk_type,
Line 407:  desc, parent):
Line 408: # When the volume format is RAW the real volume capacity is 
the device


Line 408: # When the volume format is RAW the real volume capacity is 
the device
Line 409: # size.  The device size may have been rounded up if 'size' 
is not
Line 410: # divisible by the domain's extent size.
Line 411: if vol_format == volume.RAW_FORMAT:
Line 412: size = int(self.sd_manifest.getVSize(self.img_id, 
self.vol_id))
We have a test for this, right?
Line 413: 
Line 414: # We use the BlockVolumeManifest API here because we create 
the
Line 415: # metadata in the standard way.  We cannot do this for file 
volumes
Line 416: # because the metadata needs to be written to a specially 
named file.


Line 430: volume.LEGAL_VOL)
Line 431: 
Line 432: def _create_lease(self, meta_id):
Line 433: self.vol_class.newVolumeLease(
Line 434: meta_id, self.sd_manifest.sdUUID, self.vol_id)
We need a test for failure to create the lease.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 17
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 

Change in vdsm[master]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 17: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 17
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 17
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 16
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 15
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]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 14:

(9 comments)

https://gerrit.ovirt.org/#/c/55987/14/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 300: - No logical volume exists
Line 301: - Operations:
Line 302: - is_garbage -> false
Line 303: - is_image -> false
Line 304: - create artifacts -> change state GARBAGE
> The indentation is broken - we had the same problem in file volume.
Done
Line 305: 
Line 306: GARBAGE
Line 307: 
Line 308: - States:


Line 310: - Operations:
Line 311: - is_garbage -> true
Line 312: - is_image -> true or false
Line 313: - clean -> change state to MISSING
Line 314: - commit -> change state to VOLUME
> Same
Done
Line 315: 
Line 316: VOLUME
Line 317: 
Line 318: - States:


Line 320: - Operations:
Line 321: - is_garbage -> false
Line 322: - is_image -> true
Line 323: - create new volume -> change state to GARBAGE
Line 324: - destroy this volume -> change state to GARBAGE
> Same
Done
Line 325: """
Line 326: log = logging.getLogger('Storage.BlockVolumeArtifacts')
Line 327: 
Line 328: def __init__(self, sd_manifest, img_id, vol_id):


Line 338: return self.img_id in self.sd_manifest.getAllImages()
Line 339: 
Line 340: def is_garbage(self):
Line 341: lv = self._get_lv()
Line 342: return lv and TEMP_VOL_LVTAG in lv.tags
> The previous code was much better.
Done
Line 343: 
Line 344: @property
Line 345: def volume_path(self):
Line 346: return lvm.lvPath(self.sd_manifest.sdUUID, self.vol_id)


Line 360: 
Line 361: def commit(self):
Line 362: if self._get_lv() and not self.is_garbage():
Line 363: raise se.VolumeAlreadyExists("LV %r has already been 
committed" %
Line 364:  self.vol_id)
> This should fail if:
Done
Line 365: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, 
addTags=(),
Line 366:  delTags=(TEMP_VOL_LVTAG,))
Line 367: 
Line 368: def get_volume_preallocation(self, vol_format):


Line 361: def commit(self):
Line 362: if self._get_lv() and not self.is_garbage():
Line 363: raise se.VolumeAlreadyExists("LV %r has already been 
committed" %
Line 364:  self.vol_id)
Line 365: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, 
addTags=(),
> Isn't addTags=() redundant?
Done
Line 366:  delTags=(TEMP_VOL_LVTAG,))
Line 367: 
Line 368: def get_volume_preallocation(self, vol_format):
Line 369: if vol_format == volume.RAW_FORMAT:


Line 370: return volume.PREALLOCATED_VOL
Line 371: else:
Line 372: return volume.SPARSE_VOL
Line 373: 
Line 374: def _get_lv(self):
> This helper is harmful - returning None instead of raising lead to Attribut
Done
Line 375: try:
Line 376: return lvm.getLV(self.sd_manifest.sdUUID, self.vol_id)
Line 377: except se.LogicalVolumeDoesNotExistError:
Line 378: return None


Line 404: if vol_format == volume.RAW_FORMAT:
Line 405: dev_size = int(self.sd_manifest.getVSize(self.img_id, 
self.vol_id))
Line 406: if dev_size > size:
Line 407: return dev_size
Line 408: return size
> Since we don't check that dev_size < size, we can simplify this now to:
Done
Line 409: 
Line 410: def _create_metadata(self, size, vol_format, prealloc, disk_type, 
desc,
Line 411:  parent):
Line 412: size = self._get_lv_actual_size(vol_format, size)


Line 415: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as mdSlot:
Line 416: sd_id = self.sd_manifest.sdUUID
Line 417: lvm.changeLVTags(sd_id, self.vol_id,
Line 418:  addTags=[blockVolume.TAG_PREFIX_MD + 
str(mdSlot)])
Line 419: meta_id = (sd_id, mdSlot)
> This function does two things:
Done
Line 420: 
Line 421: # We use the BlockVolumeManifest API here because we create 
the
Line 422: # metadata in the standard way.  We cannot do this for file 
volumes
Line 423: # because the metadata needs to be written to a specially 
named file.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 14
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

Change in vdsm[master]: storage: Add basic BlockVolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 14: Code-Review-1

(9 comments)

https://gerrit.ovirt.org/#/c/55987/14/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 300: - No logical volume exists
Line 301: - Operations:
Line 302: - is_garbage -> false
Line 303: - is_image -> false
Line 304: - create artifacts -> change state GARBAGE
The indentation is broken - we had the same problem in file volume.

- States:
  - No logical volume exists
- Operations:
  - is_garbage ...
  - is_image ...
  - create artifacts ...
Line 305: 
Line 306: GARBAGE
Line 307: 
Line 308: - States:


Line 310: - Operations:
Line 311: - is_garbage -> true
Line 312: - is_image -> true or false
Line 313: - clean -> change state to MISSING
Line 314: - commit -> change state to VOLUME
Same
Line 315: 
Line 316: VOLUME
Line 317: 
Line 318: - States:


Line 320: - Operations:
Line 321: - is_garbage -> false
Line 322: - is_image -> true
Line 323: - create new volume -> change state to GARBAGE
Line 324: - destroy this volume -> change state to GARBAGE
Same
Line 325: """
Line 326: log = logging.getLogger('Storage.BlockVolumeArtifacts')
Line 327: 
Line 328: def __init__(self, sd_manifest, img_id, vol_id):


Line 338: return self.img_id in self.sd_manifest.getAllImages()
Line 339: 
Line 340: def is_garbage(self):
Line 341: lv = self._get_lv()
Line 342: return lv and TEMP_VOL_LVTAG in lv.tags
The previous code was much better.

try:
lv = lvm.getLV(self.sd_manifest.sdUUID, self.vol_id)
except se.LogicalVolumeDoesNotExistError:
return False
return TEMP_VOL_LVTAG in lv.tags
Line 343: 
Line 344: @property
Line 345: def volume_path(self):
Line 346: return lvm.lvPath(self.sd_manifest.sdUUID, self.vol_id)


Line 360: 
Line 361: def commit(self):
Line 362: if self._get_lv() and not self.is_garbage():
Line 363: raise se.VolumeAlreadyExists("LV %r has already been 
committed" %
Line 364:  self.vol_id)
This should fail if:

- lv does not exists - we just created it
- lv exist but it is does not contain the TEMP_VOL_LVTAG

Lets test both cases - it is trivial using our fake lvm.

So this should be:

lv = lvm.getLV(self.sd_manifest.sdUUID, self.vol_id)
if TEMP_VOL_LVTAG not in lv.tags:
raise se.VolumeAlreadyExists("LV %r has already been committed" % 
self.vol_id)
Line 365: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, 
addTags=(),
Line 366:  delTags=(TEMP_VOL_LVTAG,))
Line 367: 
Line 368: def get_volume_preallocation(self, vol_format):


Line 361: def commit(self):
Line 362: if self._get_lv() and not self.is_garbage():
Line 363: raise se.VolumeAlreadyExists("LV %r has already been 
committed" %
Line 364:  self.vol_id)
Line 365: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, 
addTags=(),
Isn't addTags=() redundant?
Line 366:  delTags=(TEMP_VOL_LVTAG,))
Line 367: 
Line 368: def get_volume_preallocation(self, vol_format):
Line 369: if vol_format == volume.RAW_FORMAT:


Line 370: return volume.PREALLOCATED_VOL
Line 371: else:
Line 372: return volume.SPARSE_VOL
Line 373: 
Line 374: def _get_lv(self):
This helper is harmful - returning None instead of raising lead to 
AttributeError when people forget to check for None. Please use lvm.getLV, we 
don't need such helpers.
Line 375: try:
Line 376: return lvm.getLV(self.sd_manifest.sdUUID, self.vol_id)
Line 377: except se.LogicalVolumeDoesNotExistError:
Line 378: return None


Line 404: if vol_format == volume.RAW_FORMAT:
Line 405: dev_size = int(self.sd_manifest.getVSize(self.img_id, 
self.vol_id))
Line 406: if dev_size > size:
Line 407: return dev_size
Line 408: return size
Since we don't check that dev_size < size, we can simplify this now to:

if vol_format == volume.RAW_FORMAT:
return int(self.sd_manifest.getVSize(self.img_id, self.vol_id)),

So I would inline it with the comment in _create_metadata().
Line 409: 
Line 410: def _create_metadata(self, size, vol_format, prealloc, disk_type, 
desc,
Line 411:  parent):
Line 412: size = self._get_lv_actual_size(vol_format, size)


Line 415: self.vol_id, blockVolume.VOLUME_MDNUMBLKS) as mdSlot:
Line 416: sd_id = self.sd_manifest.sdUUID
Line 417: lvm.changeLVTags(sd_id, self.vol_id,
Line 418:  addTags=[blockVolume.TAG_PREFIX_MD + 
str(mdSlot)])
Line 419: 

Change in vdsm[master]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 14: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 14
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 14
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]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 13:

(15 comments)

https://gerrit.ovirt.org/#/c/55987/13/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 313: self.assertEqual(volume.PREALLOCATED_VOL, vol.getType())
Line 314: 
Line 315: def test_size_rounded_up(self):
Line 316: # If the underlying device is larger the size will be updated
Line 317: initial_size = (9 * MB) + 1024
> change this to requested_size and only add 1.
Done
Line 318: expected_size = 10 * MB
Line 319: with fake_block_env() as env:
Line 320: artifacts = env.sd_manifest.get_volume_artifacts(
Line 321: self.img_id, self.vol_id)


Line 314: 
Line 315: def test_size_rounded_up(self):
Line 316: # If the underlying device is larger the size will be updated
Line 317: initial_size = (9 * MB) + 1024
Line 318: expected_size = 10 * MB
> get extent size from the vg and use that.  Change fakelvm to round using vg
Done
Line 319: with fake_block_env() as env:
Line 320: artifacts = env.sd_manifest.get_volume_artifacts(
Line 321: self.img_id, self.vol_id)
Line 322: artifacts.create(initial_size, volume.RAW_FORMAT,


Line 322: artifacts.create(initial_size, volume.RAW_FORMAT,
Line 323:  image.SYSTEM_DISK_TYPE, 'raw_volume')
Line 324: artifacts.commit()
Line 325: vol = env.sd_manifest.produceVolume(self.img_id, 
self.vol_id)
Line 326: self.assertEqual(expected_size / volume.BLOCK_SIZE, 
vol.getSize())
> also check lv size?
Done
Line 327: 
Line 328: # Invalid use of artifacts
Line 329: 
Line 330: def test_commit_without_create(self):


Line 338: with self.fake_env() as env:
Line 339: artifacts = env.sd_manifest.get_volume_artifacts(
Line 340: self.img_id, self.vol_id)
Line 341: artifacts.create(*BASE_RAW_PARAMS)
Line 342: artifacts.commit()
> check for the tag in commit so we can raise an exception for this.
Done
Line 343: artifacts.commit()  # removing nonexistent tags is allowed
Line 344: 
Line 345: def validate_artifacts(self, artifacts, env):
Line 346: try:


Line 354: md_slot = None
Line 355: for tag in lv.tags:
Line 356: if tag.startswith(blockVolume.TAG_PREFIX_MD):
Line 357: md_slot = int(tag[len(blockVolume.TAG_PREFIX_MD):])
Line 358: break
> Use blockVolume._getVolumeTag
Done
Line 359: 
Line 360: self.validate_metadata(env, md_slot, artifacts)
Line 361: 
Line 362: def validate_metadata(self, env, md_slot, artifacts):


Line 357: md_slot = int(tag[len(blockVolume.TAG_PREFIX_MD):])
Line 358: break
Line 359: 
Line 360: self.validate_metadata(env, md_slot, artifacts)
Line 361: 
> Add a TODO for validating the lease once sanlock is mocked.
Done
Line 362: def validate_metadata(self, env, md_slot, artifacts):
Line 363: md_lines = misc.readblock(
Line 364: env.lvm.lvPath(artifacts.sd_manifest.sdUUID, sd.METADATA),
Line 365: md_slot * volume.METADATA_SIZE, volume.METADATA_SIZE)


Line 359: 
Line 360: self.validate_metadata(env, md_slot, artifacts)
Line 361: 
Line 362: def validate_metadata(self, env, md_slot, artifacts):
Line 363: md_lines = misc.readblock(
> calc the lvpath in one line.
Done
Line 364: env.lvm.lvPath(artifacts.sd_manifest.sdUUID, sd.METADATA),
Line 365: md_slot * volume.METADATA_SIZE, volume.METADATA_SIZE)
Line 366: md = volume.VolumeMetadata.from_lines(md_lines)
Line 367: 


https://gerrit.ovirt.org/#/c/55987/13/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 330: super(BlockVolumeArtifacts, self).__init__(sd_manifest, 
img_id,
Line 331:vol_id)
Line 332: 
Line 333: def is_image(self):
Line 334: # TODO: Cache this value to avoid repeated expensive queries
> Change the comment to check if this call becomes too expensive.
Done
Line 335: return self.img_id in self.sd_manifest.getAllImages()
Line 336: 
Line 337: def is_garbage(self):
Line 338: try:


Line 350: prealloc = self.get_volume_preallocation(vol_format)
Line 351: self._validate_create_params(vol_format, parent, prealloc)
Line 352: 
Line 353: lv_size = self.vol_class.calculate_volume_alloc_size(
Line 354: prealloc, size / volume.BLOCK_SIZE, None)
> use a size_blk variable here.
Done
Line 355: parent_vol_id = parent.vol_id if parent else volume.BLANK_UUID
Line 356: self._create_lv_artifact(parent_vol_id, lv_size)
Line 

Change in vdsm[master]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 13:

(15 comments)

https://gerrit.ovirt.org/#/c/55987/13/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 313: self.assertEqual(volume.PREALLOCATED_VOL, vol.getType())
Line 314: 
Line 315: def test_size_rounded_up(self):
Line 316: # If the underlying device is larger the size will be updated
Line 317: initial_size = (9 * MB) + 1024
change this to requested_size and only add 1.
Line 318: expected_size = 10 * MB
Line 319: with fake_block_env() as env:
Line 320: artifacts = env.sd_manifest.get_volume_artifacts(
Line 321: self.img_id, self.vol_id)


Line 314: 
Line 315: def test_size_rounded_up(self):
Line 316: # If the underlying device is larger the size will be updated
Line 317: initial_size = (9 * MB) + 1024
Line 318: expected_size = 10 * MB
get extent size from the vg and use that.  Change fakelvm to round using vg 
extent size and write a test in storagefakelibtests for it.
Line 319: with fake_block_env() as env:
Line 320: artifacts = env.sd_manifest.get_volume_artifacts(
Line 321: self.img_id, self.vol_id)
Line 322: artifacts.create(initial_size, volume.RAW_FORMAT,


Line 322: artifacts.create(initial_size, volume.RAW_FORMAT,
Line 323:  image.SYSTEM_DISK_TYPE, 'raw_volume')
Line 324: artifacts.commit()
Line 325: vol = env.sd_manifest.produceVolume(self.img_id, 
self.vol_id)
Line 326: self.assertEqual(expected_size / volume.BLOCK_SIZE, 
vol.getSize())
also check lv size?
Line 327: 
Line 328: # Invalid use of artifacts
Line 329: 
Line 330: def test_commit_without_create(self):


Line 338: with self.fake_env() as env:
Line 339: artifacts = env.sd_manifest.get_volume_artifacts(
Line 340: self.img_id, self.vol_id)
Line 341: artifacts.create(*BASE_RAW_PARAMS)
Line 342: artifacts.commit()
check for the tag in commit so we can raise an exception for this.
Line 343: artifacts.commit()  # removing nonexistent tags is allowed
Line 344: 
Line 345: def validate_artifacts(self, artifacts, env):
Line 346: try:


Line 354: md_slot = None
Line 355: for tag in lv.tags:
Line 356: if tag.startswith(blockVolume.TAG_PREFIX_MD):
Line 357: md_slot = int(tag[len(blockVolume.TAG_PREFIX_MD):])
Line 358: break
Use blockVolume._getVolumeTag
Line 359: 
Line 360: self.validate_metadata(env, md_slot, artifacts)
Line 361: 
Line 362: def validate_metadata(self, env, md_slot, artifacts):


Line 357: md_slot = int(tag[len(blockVolume.TAG_PREFIX_MD):])
Line 358: break
Line 359: 
Line 360: self.validate_metadata(env, md_slot, artifacts)
Line 361: 
Add a TODO for validating the lease once sanlock is mocked.
Line 362: def validate_metadata(self, env, md_slot, artifacts):
Line 363: md_lines = misc.readblock(
Line 364: env.lvm.lvPath(artifacts.sd_manifest.sdUUID, sd.METADATA),
Line 365: md_slot * volume.METADATA_SIZE, volume.METADATA_SIZE)


Line 359: 
Line 360: self.validate_metadata(env, md_slot, artifacts)
Line 361: 
Line 362: def validate_metadata(self, env, md_slot, artifacts):
Line 363: md_lines = misc.readblock(
calc the lvpath in one line.
calculate the offset explicitly in its own line.
Line 364: env.lvm.lvPath(artifacts.sd_manifest.sdUUID, sd.METADATA),
Line 365: md_slot * volume.METADATA_SIZE, volume.METADATA_SIZE)
Line 366: md = volume.VolumeMetadata.from_lines(md_lines)
Line 367: 


https://gerrit.ovirt.org/#/c/55987/13/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 330: super(BlockVolumeArtifacts, self).__init__(sd_manifest, 
img_id,
Line 331:vol_id)
Line 332: 
Line 333: def is_image(self):
Line 334: # TODO: Cache this value to avoid repeated expensive queries
Change the comment to check if this call becomes too expensive.
Line 335: return self.img_id in self.sd_manifest.getAllImages()
Line 336: 
Line 337: def is_garbage(self):
Line 338: try:


Line 350: prealloc = self.get_volume_preallocation(vol_format)
Line 351: self._validate_create_params(vol_format, parent, prealloc)
Line 352: 
Line 353: lv_size = self.vol_class.calculate_volume_alloc_size(
Line 354: prealloc, size / volume.BLOCK_SIZE, None)
use a size_blk variable here.
Line 355: parent_vol_id = parent.vol_id if parent else volume.BLANK_UUID
Line 356: 

Change in vdsm[master]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 13
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 12
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]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 11:

(5 comments)

https://gerrit.ovirt.org/#/c/55987/11/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 44: import vdsm.supervdsm as svdsm
Line 45: 
Line 46: import misc
Line 47: import sd
Line 48: import sdm.volume_artifacts
> Why not:
Done
Line 49: import lvm
Line 50: import clusterlock
Line 51: import blockVolume
Line 52: import multipath


https://gerrit.ovirt.org/#/c/55987/11/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 381: raise se.VolumeAlreadyExists("Logical volume %s 
exists" %
Line 382:  self.vol_id)
Line 383: 
Line 384: lvm.createLV(self.sd_manifest.sdUUID, self.vol_id, lv_size,
Line 385:  activate=True, initialTag=TEMP_VOL_LVTAG)
> When creating raw volume, we don't want to activate the lv, since we don't 
This would be a risky break from current flow behavior.  Consider the case of 
replicating a volume chain on a different storage domain for the purposes of 
migrating a disk.  If we don't activate the base raw lv, then subsequent volume 
creations would fail due to qemu-img not finding the backing file.
Line 386: tags = [blockVolume.TAG_PREFIX_PARENT + parent_vol_id,
Line 387: blockVolume.TAG_PREFIX_IMAGE + self.img_id]
Line 388: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, 
addTags=tags)
Line 389: 


Line 384: lvm.createLV(self.sd_manifest.sdUUID, self.vol_id, lv_size,
Line 385:  activate=True, initialTag=TEMP_VOL_LVTAG)
Line 386: tags = [blockVolume.TAG_PREFIX_PARENT + parent_vol_id,
Line 387: blockVolume.TAG_PREFIX_IMAGE + self.img_id]
Line 388: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, 
addTags=tags)
> I think we need to add the support for multiple initial tags, it is pointle
:)  I'll resurrect the patch.
Line 389: 
Line 390: def _adjust_size_for_dev(self, vol_format, size):
Line 391: # When the volume format is RAW the real volume capacity is 
the device
Line 392: # size.  The device size may have been rounded up if 'size' 
is not


Line 386: tags = [blockVolume.TAG_PREFIX_PARENT + parent_vol_id,
Line 387: blockVolume.TAG_PREFIX_IMAGE + self.img_id]
Line 388: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, 
addTags=tags)
Line 389: 
Line 390: def _adjust_size_for_dev(self, vol_format, size):
> This does not adjust anything, so we should name it _get_lv_actual_size.
Done
Line 391: # When the volume format is RAW the real volume capacity is 
the device
Line 392: # size.  The device size may have been rounded up if 'size' 
is not
Line 393: # divisible by the domain's extent size.
Line 394: if vol_format == volume.RAW_FORMAT:


Line 411: lvm.changeLVTags(sd_id, self.vol_id,
Line 412:  addTags=[blockVolume.TAG_PREFIX_MD + 
str(mdSlot)])
Line 413: meta_id = (sd_id, mdSlot)
Line 414: 
Line 415: self.vol_class.newMetadata(
> Why not use the new shiny VolumeMetadata class here?
The reason I use the old code is because VolumeMetadata doesn't have support 
for writing the data and I wasn't sure about duplicating that logic here.  I'll 
try it and we can see how it looks.
Line 416: meta_id,
Line 417: sd_id,
Line 418: self.img_id,
Line 419: parent_vol_id,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 11
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]: storage: Add basic BlockVolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 11: Code-Review-1

(5 comments)

Partial review

https://gerrit.ovirt.org/#/c/55987/11/vdsm/storage/blockSD.py
File vdsm/storage/blockSD.py:

Line 44: import vdsm.supervdsm as svdsm
Line 45: 
Line 46: import misc
Line 47: import sd
Line 48: import sdm.volume_artifacts
Why not:

from sdm import volume_artifacts

In the mess of hsm.py, using these long names may make sense, but elsewhere we 
should import only modules.
Line 49: import lvm
Line 50: import clusterlock
Line 51: import blockVolume
Line 52: import multipath


https://gerrit.ovirt.org/#/c/55987/11/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 381: raise se.VolumeAlreadyExists("Logical volume %s 
exists" %
Line 382:  self.vol_id)
Line 383: 
Line 384: lvm.createLV(self.sd_manifest.sdUUID, self.vol_id, lv_size,
Line 385:  activate=True, initialTag=TEMP_VOL_LVTAG)
When creating raw volume, we don't want to activate the lv, since we don't need 
to create a qemu image on top of it.
Line 386: tags = [blockVolume.TAG_PREFIX_PARENT + parent_vol_id,
Line 387: blockVolume.TAG_PREFIX_IMAGE + self.img_id]
Line 388: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, 
addTags=tags)
Line 389: 


Line 384: lvm.createLV(self.sd_manifest.sdUUID, self.vol_id, lv_size,
Line 385:  activate=True, initialTag=TEMP_VOL_LVTAG)
Line 386: tags = [blockVolume.TAG_PREFIX_PARENT + parent_vol_id,
Line 387: blockVolume.TAG_PREFIX_IMAGE + self.img_id]
Line 388: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, 
addTags=tags)
I think we need to add the support for multiple initial tags, it is pointless 
to run 2 lvm operation when we can do it with one.
Line 389: 
Line 390: def _adjust_size_for_dev(self, vol_format, size):
Line 391: # When the volume format is RAW the real volume capacity is 
the device
Line 392: # size.  The device size may have been rounded up if 'size' 
is not


Line 386: tags = [blockVolume.TAG_PREFIX_PARENT + parent_vol_id,
Line 387: blockVolume.TAG_PREFIX_IMAGE + self.img_id]
Line 388: lvm.changeLVTags(self.sd_manifest.sdUUID, self.vol_id, 
addTags=tags)
Line 389: 
Line 390: def _adjust_size_for_dev(self, vol_format, size):
This does not adjust anything, so we should name it _get_lv_actual_size.
Line 391: # When the volume format is RAW the real volume capacity is 
the device
Line 392: # size.  The device size may have been rounded up if 'size' 
is not
Line 393: # divisible by the domain's extent size.
Line 394: if vol_format == volume.RAW_FORMAT:


Line 411: lvm.changeLVTags(sd_id, self.vol_id,
Line 412:  addTags=[blockVolume.TAG_PREFIX_MD + 
str(mdSlot)])
Line 413: meta_id = (sd_id, mdSlot)
Line 414: 
Line 415: self.vol_class.newMetadata(
Why not use the new shiny VolumeMetadata class here?

I would like to minimize dependencies on legacy code.
Line 416: meta_id,
Line 417: sd_id,
Line 418: self.img_id,
Line 419: parent_vol_id,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 11
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 11
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 10
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]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 4:

(3 comments)

https://gerrit.ovirt.org/#/c/55987/4/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 119: dict(img_id=self.img_id, vol_id=self.vol_id))
Line 120: params = BASE_COW_PARAMS + (parent,)
Line 121: 
Line 122: self.assertRaises(se.VolumeAlreadyExists,
Line 123:   artifacts.create, *params)
> Can you separate the new common tests into another patch (probably before t
Done
Line 124: 
Line 125: def test_new_image_create_and_commit(self):
Line 126: with self.fake_env() as env:
Line 127: artifacts = env.sd_manifest.get_volume_artifacts(


Line 149: self.img_id, self.vol_id)
Line 150: artifacts.create(*BASE_RAW_PARAMS)
Line 151: self.assertEqual({}, env.sd_manifest.getAllVolumes())
Line 152: artifacts.commit()
Line 153: self.assertIn(self.vol_id, 
env.sd_manifest.getAllVolumes())
> Can you explain this change?
It's a confusing diff artifact since the functions being moved are similar.  I 
am just moving tests around.
Line 154: 
Line 155: 
Line 156: class FileVolumeArtifactsTests(VolumeArtifactsTestsMixin, 
VdsmTestCase):
Line 157: 


Line 241: artifacts = env.sd_manifest.get_volume_artifacts(
Line 242: self.img_id, self.vol_id)
Line 243: artifacts.create(*BASE_RAW_PARAMS)
Line 244: artifacts.commit()
Line 245: self.assertRaises(OSError, artifacts.commit)
> These can also move to the patch modifying existing tests, before this patc
Done
Line 246: 
Line 247: def validate_new_image_path(self, artifacts, has_md=False,
Line 248: has_lease=False, has_volume=False):
Line 249: path = artifacts.artifacts_dir


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 9
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]: storage: Add basic BlockVolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 4:

(3 comments)

Partial review

https://gerrit.ovirt.org/#/c/55987/4/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 119: dict(img_id=self.img_id, vol_id=self.vol_id))
Line 120: params = BASE_COW_PARAMS + (parent,)
Line 121: 
Line 122: self.assertRaises(se.VolumeAlreadyExists,
Line 123:   artifacts.create, *params)
Can you separate the new common tests into another patch (probably before this 
patch)?
Line 124: 
Line 125: def test_new_image_create_and_commit(self):
Line 126: with self.fake_env() as env:
Line 127: artifacts = env.sd_manifest.get_volume_artifacts(


Line 149: self.img_id, self.vol_id)
Line 150: artifacts.create(*BASE_RAW_PARAMS)
Line 151: self.assertEqual({}, env.sd_manifest.getAllVolumes())
Line 152: artifacts.commit()
Line 153: self.assertIn(self.vol_id, 
env.sd_manifest.getAllVolumes())
Can you explain this change?

And separate it to the common tests fixes, so we can merge it quickly.
Line 154: 
Line 155: 
Line 156: class FileVolumeArtifactsTests(VolumeArtifactsTestsMixin, 
VdsmTestCase):
Line 157: 


Line 241: artifacts = env.sd_manifest.get_volume_artifacts(
Line 242: self.img_id, self.vol_id)
Line 243: artifacts.create(*BASE_RAW_PARAMS)
Line 244: artifacts.commit()
Line 245: self.assertRaises(OSError, artifacts.commit)
These can also move to the patch modifying existing tests, before this patch, 
so we can merge them now.
Line 246: 
Line 247: def validate_new_image_path(self, artifacts, has_md=False,
Line 248: has_lease=False, has_volume=False):
Line 249: path = artifacts.artifacts_dir


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 8
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
Gerrit-PatchSet: 7
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
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]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/55987/4/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 423: lvm.changeLVTags(sd_id, self.vol_id,
Line 424:  addTags=[blockVolume.TAG_PREFIX_MD + 
str(mdSlot)])
Line 425: meta_id = (sd_id, mdSlot)
Line 426: 
Line 427: self.vol_class.newMetadata(
Change to the VolumeMetadata implementation.
Line 428: meta_id,
Line 429: sd_id,
Line 430: self.img_id,
Line 431: parent_vol_id,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
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: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
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]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 2:

(5 comments)

https://gerrit.ovirt.org/#/c/55987/2/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 103: self.img_id, str(uuid.uuid4()))
Line 104: self.assertRaises(se.InvalidParameterException, 
second.create,
Line 105:   *BASE_RAW_PARAMS)
Line 106: 
Line 107: def test_create_same_volume_in_image(self):
> look at using brokentest
Done
Line 108: with self.fake_env() as env:
Line 109: artifacts = env.sd_manifest.get_volume_artifacts(
Line 110: self.img_id, self.vol_id)
Line 111: artifacts.create(*BASE_RAW_PARAMS)


Line 138: self.assertEqual(str(disk_type), vol.getDiskType())
Line 139: 
Line 140: # Artifacts visibility
Line 141: 
Line 142: def test_getallvolumes(self):
> this test duplicates blocksdtests and filesdtests logic.
Decided to keep this since it tests using artifacts code and also using our 
fake env whereas blocksdtests test by using faked lvm output.
Line 143: # Artifacts must not be recognized as volumes until commit is 
called.
Line 144: with fake_file_env() as env:
Line 145: self.assertEqual({}, env.sd_manifest.getAllVolumes())
Line 146: artifacts = env.sd_manifest.get_volume_artifacts(


Line 140: # Artifacts visibility
Line 141: 
Line 142: def test_getallvolumes(self):
Line 143: # Artifacts must not be recognized as volumes until commit is 
called.
Line 144: with fake_file_env() as env:
> use self.fake_env()
Done
Line 145: self.assertEqual({}, env.sd_manifest.getAllVolumes())
Line 146: artifacts = env.sd_manifest.get_volume_artifacts(
Line 147: self.img_id, self.vol_id)
Line 148: artifacts.create(*BASE_RAW_PARAMS)


Line 296: artifacts.commit()
Line 297: vol = env.sd_manifest.produceVolume(self.img_id, 
self.vol_id)
Line 298: self.assertEqual(volume.PREALLOCATED_VOL, vol.getType())
Line 299: 
Line 300: def validate_artifacts(self, artifacts, env):
> put this at the end.
Done
Line 301: try:
Line 302: lv = env.lvm.getLV(artifacts.sd_manifest.sdUUID, 
artifacts.vol_id)
Line 303: except se.LogicalVolumeDoesNotExistError:
Line 304: raise AssertionError("LV missing")


Line 305: 
Line 306: if TEMP_VOL_LVTAG not in lv.tags:
Line 307: raise AssertionError("Missing TEMP_VOL_LVTAG")
Line 308: 
Line 309: # TODO: Validate lease and metadata allocation
> Use VolumeArtifacts patches to validate the metadata block.
Added support for validating metadata.  Lease cannot be verified since lease 
initialization currently does nothing.
Line 310: 
Line 311: # Invalid use of artifacts
Line 312: 
Line 313: def test_commit_without_create(self):


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
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]: storage: Add basic BlockVolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


Patch Set 2:

(6 comments)

https://gerrit.ovirt.org/#/c/55987/2/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 103: self.img_id, str(uuid.uuid4()))
Line 104: self.assertRaises(se.InvalidParameterException, 
second.create,
Line 105:   *BASE_RAW_PARAMS)
Line 106: 
Line 107: def test_create_same_volume_in_image(self):
look at using brokentest
Line 108: with self.fake_env() as env:
Line 109: artifacts = env.sd_manifest.get_volume_artifacts(
Line 110: self.img_id, self.vol_id)
Line 111: artifacts.create(*BASE_RAW_PARAMS)


Line 138: self.assertEqual(str(disk_type), vol.getDiskType())
Line 139: 
Line 140: # Artifacts visibility
Line 141: 
Line 142: def test_getallvolumes(self):
this test duplicates blocksdtests and filesdtests logic.
Line 143: # Artifacts must not be recognized as volumes until commit is 
called.
Line 144: with fake_file_env() as env:
Line 145: self.assertEqual({}, env.sd_manifest.getAllVolumes())
Line 146: artifacts = env.sd_manifest.get_volume_artifacts(


Line 140: # Artifacts visibility
Line 141: 
Line 142: def test_getallvolumes(self):
Line 143: # Artifacts must not be recognized as volumes until commit is 
called.
Line 144: with fake_file_env() as env:
use self.fake_env()
Line 145: self.assertEqual({}, env.sd_manifest.getAllVolumes())
Line 146: artifacts = env.sd_manifest.get_volume_artifacts(
Line 147: self.img_id, self.vol_id)
Line 148: artifacts.create(*BASE_RAW_PARAMS)


Line 296: artifacts.commit()
Line 297: vol = env.sd_manifest.produceVolume(self.img_id, 
self.vol_id)
Line 298: self.assertEqual(volume.PREALLOCATED_VOL, vol.getType())
Line 299: 
Line 300: def validate_artifacts(self, artifacts, env):
put this at the end.
Line 301: try:
Line 302: lv = env.lvm.getLV(artifacts.sd_manifest.sdUUID, 
artifacts.vol_id)
Line 303: except se.LogicalVolumeDoesNotExistError:
Line 304: raise AssertionError("LV missing")


Line 305: 
Line 306: if TEMP_VOL_LVTAG not in lv.tags:
Line 307: raise AssertionError("Missing TEMP_VOL_LVTAG")
Line 308: 
Line 309: # TODO: Validate lease and metadata allocation
Use VolumeArtifacts patches to validate the metadata block.

Use poison to validate lease area initialization.
Line 310: 
Line 311: # Invalid use of artifacts
Line 312: 
Line 313: def test_commit_without_create(self):


Line 322: artifacts = env.sd_manifest.get_volume_artifacts(
Line 323: self.img_id, self.vol_id)
Line 324: artifacts.create(*BASE_RAW_PARAMS)
Line 325: artifacts.commit()
Line 326: artifacts.commit()  # removing nonexistent tags is allowed
check the tag in commit and fail.
Line 327: 
Line 328: 
Line 329: class BlockVolumeArtifactVisibilityTests(VdsmTestCase):
Line 330: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4120c557fc89d82cc6bc854a9fdc8935e53bc93d
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]: storage: Add basic BlockVolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: storage: Add basic BlockVolumeArtifacts
..


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'])

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

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