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

nsoffer at redhat.com nsoffer at redhat.com
Fri Apr 29 23:23:27 UTC 2016


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:             self.validate_invisibility(env, artifacts, is_garbage=True)
Line 404:             self.validate_domain_has_garbage(env.sd_manifest)


-- 
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 <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Ala Hino <ahino at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list