Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 29:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has submitted this change and it was merged.

Change subject: Introduce VolumeArtifacts
..


Introduce VolumeArtifacts

In an SDM managed storage domain we will create and remove volumes using
a garbage collection approach rather than persistent tasks and rollback
operations.  Volumes consist of three separate artifacts: a data area, a
metadata area, and a lease area.  Once created on storage these objects
must be convertable into a Volume with a single atomic operation (ie.
rename a single file).  Conversely, a Volume can be destroyed by
reducing it to its artifacts with a single atomic operation.

VolumeArtifacts is an object to manage the creation and removal of the
three volume artifacts for both block and file based storage.  It also
has methods to manage the conversion of these artifacts to a Volume and
to deconstruct a Volume in order to remove the artifacts from storage.
The three artifacts on storage will not be detected as a volume by other
storage code until they are committed.

Proposed operations for VolumeArtifacts:
 - create: Create the artifacts on storage
 - commit: Convert the artifacts to a volume
 - dismantle: Convert a volume into artifacts
 - clean: Remove the artifacts from storage

Additional methods to identify and garbage collect artifacts will also
be required but the exact interface hasn't settled out yet.

This patch introduces the idea with minimal support.  We can create
artifacts on file storage and we only support raw format with no parent
volume.  Support for cow, parent volumes, block storage, and volume
removal will be added by subsequent patches.

Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
Signed-off-by: Adam Litke 
Reviewed-on: https://gerrit.ovirt.org/48097
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer 
---
M debian/vdsm.install
M lib/vdsm/storage/exception.py
M tests/Makefile.am
A tests/storage_volume_artifacts_test.py
M vdsm.spec.in
M vdsm/storage/blockSD.py
M vdsm/storage/fileSD.py
M vdsm/storage/sdm/Makefile.am
A vdsm/storage/sdm/volume_artifacts.py
9 files changed, 580 insertions(+), 1 deletion(-)

Approvals:
  Nir Soffer: Looks good to me, approved
  Adam Litke: Verified
  Jenkins CI: Passed CI tests



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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 28: Code-Review+2

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 27:

(2 comments)

https://gerrit.ovirt.org/#/c/48097/27/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 147: 
Line 148: # File volumes are always sparse regardless of format
Line 149: for fmt in volume.RAW_FORMAT, volume.COW_FORMAT:
Line 150: prealloc = artifacts._get_volume_preallocation(fmt)
Line 151: self.assertEqual(volume.SPARSE_VOL, prealloc)
> This tests the implementation instead of the behavior, so we will have to c
Done
Line 152: 
Line 153: def test_raw_volume_preallocation(self):
Line 154: with self.fake_env() as env:
Line 155: artifacts = env.sd_manifest.get_volume_artifacts(


https://gerrit.ovirt.org/#/c/48097/27/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 227: # If we created a new image directory, rename it to the 
correct name
Line 228: if not self.is_image():
Line 229: self._oop.os.rename(self.artifacts_dir, self._image_dir)
Line 230: 
Line 231: self._image_exists = True
> This is unused, any reason to keep it?
Nope.  Removed.
Line 232: 
Line 233: def _get_volume_preallocation(self, vol_format):
Line 234: # File volumes are always sparse regardless of format
Line 235: return volume.SPARSE_VOL


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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 28: Verified+1

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 28:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 27: Code-Review-1

(4 comments)

https://gerrit.ovirt.org/#/c/48097/26/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 211: # not allowed.
Line 212: with self.fake_env() as env:
Line 213: artifacts = env.sd_manifest.get_volume_artifacts(
Line 214: self.img_id, self.vol_id)
Line 215: artifacts._create_metadata_artifact = self.failure
> Needed for block volume tests coming up and I figured I might as well estab
OK
Line 216: self.assertRaises(ExpectedFailure, artifacts.create,
Line 217:   *BASE_RAW_PARAMS)
Line 218: artifacts = env.sd_manifest.get_volume_artifacts(
Line 219: self.img_id, self.vol_id)


https://gerrit.ovirt.org/#/c/48097/27/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 147: 
Line 148: # File volumes are always sparse regardless of format
Line 149: for fmt in volume.RAW_FORMAT, volume.COW_FORMAT:
Line 150: prealloc = artifacts._get_volume_preallocation(fmt)
Line 151: self.assertEqual(volume.SPARSE_VOL, prealloc)
This tests the implementation instead of the behavior, so we will have to 
change the test if we change the internal private helper. I suggest to remove 
this test and keep only the next test. When we add support for cow format, we 
can add another test  like the next test for sparseness.
Line 152: 
Line 153: def test_raw_volume_preallocation(self):
Line 154: with self.fake_env() as env:
Line 155: artifacts = env.sd_manifest.get_volume_artifacts(


https://gerrit.ovirt.org/#/c/48097/26/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 227: # If we created a new image directory, rename it to the 
correct name
Line 228: if not self.is_image():
Line 229: self._oop.os.rename(self.artifacts_dir, self._image_dir)
Line 230: 
Line 231: self._image_exists = True
> We can remove this now.
Adam?
Line 232: 
Line 233: def _get_volume_preallocation(self, vol_format):
Line 234: # File volumes are always sparse regardless of format
Line 235: return volume.SPARSE_VOL


https://gerrit.ovirt.org/#/c/48097/27/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 227: # If we created a new image directory, rename it to the 
correct name
Line 228: if not self.is_image():
Line 229: self._oop.os.rename(self.artifacts_dir, self._image_dir)
Line 230: 
Line 231: self._image_exists = True
This is unused, any reason to keep it?
Line 232: 
Line 233: def _get_volume_preallocation(self, vol_format):
Line 234: # File volumes are always sparse regardless of format
Line 235: return volume.SPARSE_VOL


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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 27:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 26:

(3 comments)

https://gerrit.ovirt.org/#/c/48097/26/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 112: vol = env.sd_manifest.produceVolume(self.img_id, 
self.vol_id)
Line 113: self.assertEqual(volume.type2name(volume.LEAF_VOL),
Line 114:  vol.getVolType())
Line 115: 
self.assertEqual(artifacts.get_volume_preallocation(vol_format),
Line 116:  vol.getType())
> I would test this property in a separate test in file and block volume, and
Done
Line 117: self.assertEqual(desc, vol.getDescription())
Line 118: self.assertEqual(volume.LEGAL_VOL, vol.getLegality())
Line 119: self.assertEqual(size / volume.BLOCK_SIZE, vol.getSize())
Line 120: self.assertEqual(vol_format, vol.getFormat())


Line 140: class FileVolumeArtifactsTests(VolumeArtifactsTestsMixin, 
VdsmTestCase):
Line 141: 
Line 142: def fake_env(self):
Line 143: return fake_file_env()
Line 144: 
> We should add a stupid test that get_volume_preallocate() returns SPARSE. O
Done
Line 145: def test_new_image_create_metadata_failure(self):
Line 146: # If we fail before the metadata is created we will have an 
empty
Line 147: # image directory with a garbage collection prefix left behind
Line 148: with self.fake_env() as env:


Line 211: self.assertEqual(has_md, 
os.path.exists(artifacts.meta_volatile_path))
Line 212: self.assertEqual(has_lease, 
os.path.exists(artifacts.lease_path))
Line 213: self.assertEqual(has_volume, 
os.path.exists(artifacts.volume_path))
Line 214: 
Line 215: def validate_artifacts(self, artifacts, env):
> env is unused
Needed for block volume tests coming up and I figured I might as well establish 
the interface now.
Line 216: self.assertTrue(os.path.exists(artifacts.meta_volatile_path))
Line 217: self.assertTrue(os.path.exists(artifacts.volume_path))
Line 218: self.assertTrue(os.path.exists(artifacts.lease_path))
Line 219: 


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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 26: Code-Review-1

(4 comments)

Few minor issues left.

https://gerrit.ovirt.org/#/c/48097/26/tests/storage_volume_artifacts_test.py
File tests/storage_volume_artifacts_test.py:

Line 112: vol = env.sd_manifest.produceVolume(self.img_id, 
self.vol_id)
Line 113: self.assertEqual(volume.type2name(volume.LEAF_VOL),
Line 114:  vol.getVolType())
Line 115: 
self.assertEqual(artifacts.get_volume_preallocation(vol_format),
Line 116:  vol.getType())
I would test this property in a separate test in file and block volume, and 
make artifacts.get_volume_preallocation private. Here we test that we get the 
value this method returns, but the value may be wrong. Previously we tested the 
actual result using the volume interface, which seems more robust.
Line 117: self.assertEqual(desc, vol.getDescription())
Line 118: self.assertEqual(volume.LEGAL_VOL, vol.getLegality())
Line 119: self.assertEqual(size / volume.BLOCK_SIZE, vol.getSize())
Line 120: self.assertEqual(vol_format, vol.getFormat())


Line 140: class FileVolumeArtifactsTests(VolumeArtifactsTestsMixin, 
VdsmTestCase):
Line 141: 
Line 142: def fake_env(self):
Line 143: return fake_file_env()
Line 144: 
We should add a stupid test that get_volume_preallocate() returns SPARSE. 
Otherwise the code may do the wrong thing and the test will pass.
Line 145: def test_new_image_create_metadata_failure(self):
Line 146: # If we fail before the metadata is created we will have an 
empty
Line 147: # image directory with a garbage collection prefix left behind
Line 148: with self.fake_env() as env:


Line 211: self.assertEqual(has_md, 
os.path.exists(artifacts.meta_volatile_path))
Line 212: self.assertEqual(has_lease, 
os.path.exists(artifacts.lease_path))
Line 213: self.assertEqual(has_volume, 
os.path.exists(artifacts.volume_path))
Line 214: 
Line 215: def validate_artifacts(self, artifacts, env):
env is unused
Line 216: self.assertTrue(os.path.exists(artifacts.meta_volatile_path))
Line 217: self.assertTrue(os.path.exists(artifacts.volume_path))
Line 218: self.assertTrue(os.path.exists(artifacts.lease_path))
Line 219: 


https://gerrit.ovirt.org/#/c/48097/26/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 227: # If we created a new image directory, rename it to the 
correct name
Line 228: if not self.is_image():
Line 229: self._oop.os.rename(self.artifacts_dir, self._image_dir)
Line 230: 
Line 231: self._image_exists = True
We can remove this now.
Line 232: 
Line 233: def get_volume_preallocation(self, vol_format):
Line 234: # File volumes are always sparse regardless of format
Line 235: return volume.SPARSE_VOL


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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 26:

Please rebase, jenkins build is fixed in master.

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 26:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 25:

(22 comments)

https://gerrit.ovirt.org/#/c/48097/25/debian/vdsm.install
File debian/vdsm.install:

Line 111: ./usr/share/vdsm/storage/sd.py
Line 112: ./usr/share/vdsm/storage/sdc.py
Line 113: ./usr/share/vdsm/storage/sdm/__init__.py
Line 114: ./usr/share/vdsm/storage/sdm/volume_artifacts.py
Line 115: ./usr/share/vdsm/storage/securable.py
> I this related? looks like bad merge.
Done
Line 116: ./usr/share/vdsm/storage/sp.py
Line 117: ./usr/share/vdsm/storage/spbackends.py
Line 118: ./usr/share/vdsm/storage/storageServer.py
Line 119: ./usr/share/vdsm/storage/storage_mailbox.py


https://gerrit.ovirt.org/#/c/48097/25/lib/vdsm/storage/exception.py
File lib/vdsm/storage/exception.py:

Line 1774: 
Line 1775: 
Line 1776: #
Line 1777: #  SDM Errors
Line 1778: #  Range: 909-?
> 910
Done
Line 1779: #
Line 1780: 
Line 1781: class DomainHasGarbage(StorageException):
Line 1782: code = 910


https://gerrit.ovirt.org/#/c/48097/25/tests/Makefile.am
File tests/Makefile.am:

Line 144:   vmTests.py \
Line 145:   vmTestsData.py \
Line 146:   vmUtilsTests.py \
Line 147:   vmXmlTests.py \
Line 148:   volume_artifacts_test.py \
> Lets start adding storage_ prefix to our tests.
Done
Line 149:   v2vTests.py \
Line 150:   $(NULL)
Line 151: 
Line 152: nodist_vdsmtests_PYTHON = \


https://gerrit.ovirt.org/#/c/48097/23/tests/volume_artifacts_test.py
File tests/volume_artifacts_test.py:

Line 68: self.assertFalse(artifacts.is_image())
Line 69: self.validate_artifacts(artifacts)
Line 70: 
Line 71: def test_state_garbage_create(self):
Line 72: with self.fake_env() as env:
> is_image()?
Done
Line 73: artifacts = get_artifacts(env, self.img_id, self.vol_id)
Line 74: artifacts.create(*BASE_RAW_PARAMS)
Line 75: self.assertRaises(se.DomainHasGarbage, artifacts.create,
Line 76:   *BASE_RAW_PARAMS)


https://gerrit.ovirt.org/#/c/48097/25/tests/volume_artifacts_test.py
File tests/volume_artifacts_test.py:

Line 28: 
Line 29: from storage import fileVolume, image, sd, volume
Line 30: 
Line 31: 
Line 32: class ValidationError(Exception):
> We don't need this, we should raise AssertionError for test failures. Raisi
Done
Line 33: pass
Line 34: 
Line 35: 
Line 36: class ExpectedFailure(Exception):


Line 36: class ExpectedFailure(Exception):
Line 37: pass
Line 38: 
Line 39: 
Line 40: BASE_RAW_PARAMS = (1073741824, volume.RAW_FORMAT, 
image.SYSTEM_DISK_TYPE, '')
> Lets add a non-empty description (.e.g "raw volume") to make this more clea
Done
Line 41: BASE_COW_PARAMS = (1073741824, volume.COW_FORMAT, 
image.SYSTEM_DISK_TYPE, '')
Line 42: 
Line 43: 
Line 44: def get_artifacts(env, img_id, vol_id):


Line 42: 
Line 43: 
Line 44: def get_artifacts(env, img_id, vol_id):
Line 45: artifacts_class = env.sd_manifest.get_volume_artifacts_class()
Line 46: return artifacts_class(env.sd_manifest, img_id, vol_id)
> This looks like useful method for StorageDomainManifest - why do we return 
Done
Line 47: 
Line 48: 
Line 49: class VolumeArtifactsTestsMixin(object):
Line 50: 


Line 57: artifacts = get_artifacts(env, self.img_id, self.vol_id)
Line 58: self.assertFalse(artifacts.is_garbage())
Line 59: self.assertFalse(artifacts.is_image())
Line 60: self.assertRaises(ValidationError,
Line 61:   self.validate_artifacts, artifacts)
> Can we simply assert inside validate_artifacts? Is there a reason to raise 
Done
Line 62: 
Line 63: def test_state_garbage_volatile_image_dir(self):
Line 64: with self.fake_env() as env:
Line 65: artifacts = get_artifacts(env, self.img_id, self.vol_id)


Line 67: self.assertTrue(artifacts.is_garbage())
Line 68: self.assertFalse(artifacts.is_image())
Line 69: self.validate_artifacts(artifacts)
Line 70: 
Line 71: def test_state_garbage_create(self):
> test_state_garbage_create_raises?
Done
Line 72: with self.fake_env() as env:
Line 73: artifacts = get_artifacts(env, self.img_id, self.vol_id)
Line 74: artifacts.create(*BASE_RAW_PARAMS)
Line 75: self.assertRaises(se.DomainHasGarbage, artifacts.create,


Line 200: self.assertTrue(
Line 201: 
os.path.basename(path).startswith(sd.REMOVED_IMAGE_PREFIX))
Line 202: self.assertTrue(os.path.exists(path))
Line 203: self.assertFalse(os.path.exists(artifacts._image_dir))
Line 204: self.assertEqual(nr_files, len(os.listdir(path)))
> I don't think we should count files but check which files exists instead. T
Done
Line 205: 
Line 206: def 

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 25: Code-Review-1

(6 comments)

Looks good, lets finish the few minor issues and merge it.

https://gerrit.ovirt.org/#/c/48097/25/vdsm/storage/fileSD.py
File vdsm/storage/fileSD.py:

Line 204: """
Line 205: return fileVolume.FileVolumeManifest
Line 206: 
Line 207: def get_volume_artifacts_class(self):
Line 208: return sdm.volume_artifacts.FileVolumeArtifacts
We return the class to be consistent with getvolumeClass, right?
Line 209: 
Line 210: def getDeletedImagePath(self, imgUUID):
Line 211: currImgDir = self.getImagePath(imgUUID)
Line 212: dirName, baseName = os.path.split(currImgDir)


https://gerrit.ovirt.org/#/c/48097/25/vdsm/storage/sdm/Makefile.am
File vdsm/storage/sdm/Makefile.am:

Line 1
Line 2
we can update this to 2015-2016 or maybe just 2016.


https://gerrit.ovirt.org/#/c/48097/25/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 47: 
Line 48: TODO: block based volume
Line 49: """
Line 50: 
Line 51: from __future__ import absolute_import
This may not work correctly inside vdsm, since vdsm is not a package, be 
careful.
Line 52: 
Line 53: import errno
Line 54: import logging
Line 55: import os


Line 114: 
Line 115: MISSING
Line 116: 
Line 117: - States:
Line 118: - no image or volatile directories
Need to indent the sub lists:
- States:
  - no image or...
- Operations:
  - is_garbage -> false
Line 119: - Operations:
Line 120: - is_garbage -> false
Line 121: - is_image -> false
Line 122: - create artifacts -> change state GARBAGE


Line 156: return True
Line 157: 
Line 158: vol_path = os.path.join(self._image_dir, self.vol_id)
Line 159: volatile_metadata_path = 
(self.vol_class.metaVolumePath(vol_path) +
Line 160:   TEMP_VOL_FILEEXT)
Why not:

self._oop.fileUtils.pathExists(self.meta_volatile_path?)
Line 161: return self._oop.fileUtils.pathExists(volatile_metadata_path)
Line 162: 
Line 163: def is_image(self):
Line 164: if self._image_exists is None:


Line 161: return self._oop.fileUtils.pathExists(volatile_metadata_path)
Line 162: 
Line 163: def is_image(self):
Line 164: if self._image_exists is None:
Line 165: self._image_exists = os.path.exists(self._image_dir)
Should use self._oop - this can block entire vdsm when NFS get stuck.

Do we have a reason to cache the result? we don't cache in is_garbage.
Line 166: return self._image_exists
Line 167: 
Line 168: @property
Line 169: def _oop(self):


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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 25:

(4 comments)

I like the tests, not sure they are complete but they make it very clear that 
is this class and how it should behave.

https://gerrit.ovirt.org/#/c/48097/25/tests/volume_artifacts_test.py
File tests/volume_artifacts_test.py:

Line 28: 
Line 29: from storage import fileVolume, image, sd, volume
Line 30: 
Line 31: 
Line 32: class ValidationError(Exception):
We don't need this, we should raise AssertionError for test failures. Raising 
this will be considered as test error instead of test failure.
Line 33: pass
Line 34: 
Line 35: 
Line 36: class ExpectedFailure(Exception):


Line 212: artifacts.vol_id + fileVolume.LEASE_FILEEXT)
Line 213: self.assertIn(leasefile, dirlist)
Line 214: 
Line 215: def validate_artifacts(self, artifacts):
Line 216: lease_path = artifacts.volume_path + fileVolume.LEASE_FILEEXT
Lets add an accessor for the lease file path to the class, like 
meta_volatile_path
Line 217: for f in [artifacts.meta_volatile_path, artifacts.volume_path,
Line 218:   lease_path]:
Line 219: if not os.path.exists(f):
Line 220: self.log.debug("Artifact missing: %s", f)


Line 217: for f in [artifacts.meta_volatile_path, artifacts.volume_path,
Line 218:   lease_path]:
Line 219: if not os.path.exists(f):
Line 220: self.log.debug("Artifact missing: %s", f)
Line 221: raise ValidationError()
AssertionError would make the test fail nicely like any other test failure.
Line 222: 
Line 223: def failure(*args):
Line 224: raise ExpectedFailure()
Line 225: 


Line 230: self.img_id = str(uuid.uuid4())
Line 231: self.vol_id = str(uuid.uuid4())
Line 232: 
Line 233: def test_getallimages(self):
Line 234: # The current behavior of getAllInages is to report garbage 
image
getAllImages (n -> m)
Line 235: # directories (perhaps this should be changed).
Line 236: with fake_file_env() as env:
Line 237: garbage_img_id = sd.REMOVED_IMAGE_PREFIX + self.img_id
Line 238: self.assertEqual(set(), env.sd_manifest.getAllImages())


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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 25:

(9 comments)

Partial review

https://gerrit.ovirt.org/#/c/48097/25/debian/vdsm.install
File debian/vdsm.install:

Line 111: ./usr/share/vdsm/storage/sd.py
Line 112: ./usr/share/vdsm/storage/sdc.py
Line 113: ./usr/share/vdsm/storage/sdm/__init__.py
Line 114: ./usr/share/vdsm/storage/sdm/volume_artifacts.py
Line 115: ./usr/share/vdsm/storage/securable.py
I this related? looks like bad merge.
Line 116: ./usr/share/vdsm/storage/sp.py
Line 117: ./usr/share/vdsm/storage/spbackends.py
Line 118: ./usr/share/vdsm/storage/storageServer.py
Line 119: ./usr/share/vdsm/storage/storage_mailbox.py


https://gerrit.ovirt.org/#/c/48097/25/lib/vdsm/storage/exception.py
File lib/vdsm/storage/exception.py:

Line 1774: 
Line 1775: 
Line 1776: #
Line 1777: #  SDM Errors
Line 1778: #  Range: 909-?
910
Line 1779: #
Line 1780: 
Line 1781: class DomainHasGarbage(StorageException):
Line 1782: code = 910


https://gerrit.ovirt.org/#/c/48097/25/tests/Makefile.am
File tests/Makefile.am:

Line 144:   vmTests.py \
Line 145:   vmTestsData.py \
Line 146:   vmUtilsTests.py \
Line 147:   vmXmlTests.py \
Line 148:   volume_artifacts_test.py \
Lets start adding storage_ prefix to our tests.

When we move all storage code to lib/vdsm/ we can move all the tests to 
tests/storage. Until we can, having storage_ prefix make it easy to run all the 
storage tests using storage_*_test.py.
Line 149:   v2vTests.py \
Line 150:   $(NULL)
Line 151: 
Line 152: nodist_vdsmtests_PYTHON = \


https://gerrit.ovirt.org/#/c/48097/25/tests/volume_artifacts_test.py
File tests/volume_artifacts_test.py:

Line 36: class ExpectedFailure(Exception):
Line 37: pass
Line 38: 
Line 39: 
Line 40: BASE_RAW_PARAMS = (1073741824, volume.RAW_FORMAT, 
image.SYSTEM_DISK_TYPE, '')
Lets add a non-empty description (.e.g "raw volume") to make this more clear.
Line 41: BASE_COW_PARAMS = (1073741824, volume.COW_FORMAT, 
image.SYSTEM_DISK_TYPE, '')
Line 42: 
Line 43: 
Line 44: def get_artifacts(env, img_id, vol_id):


Line 42: 
Line 43: 
Line 44: def get_artifacts(env, img_id, vol_id):
Line 45: artifacts_class = env.sd_manifest.get_volume_artifacts_class()
Line 46: return artifacts_class(env.sd_manifest, img_id, vol_id)
This looks like useful method for StorageDomainManifest - why do we return the 
class instead of creating an instance?
Line 47: 
Line 48: 
Line 49: class VolumeArtifactsTestsMixin(object):
Line 50: 


Line 57: artifacts = get_artifacts(env, self.img_id, self.vol_id)
Line 58: self.assertFalse(artifacts.is_garbage())
Line 59: self.assertFalse(artifacts.is_image())
Line 60: self.assertRaises(ValidationError,
Line 61:   self.validate_artifacts, artifacts)
Can we simply assert inside validate_artifacts? Is there a reason to raise 
special error and assert here?
Line 62: 
Line 63: def test_state_garbage_volatile_image_dir(self):
Line 64: with self.fake_env() as env:
Line 65: artifacts = get_artifacts(env, self.img_id, self.vol_id)


Line 67: self.assertTrue(artifacts.is_garbage())
Line 68: self.assertFalse(artifacts.is_image())
Line 69: self.validate_artifacts(artifacts)
Line 70: 
Line 71: def test_state_garbage_create(self):
test_state_garbage_create_raises?
Line 72: with self.fake_env() as env:
Line 73: artifacts = get_artifacts(env, self.img_id, self.vol_id)
Line 74: artifacts.create(*BASE_RAW_PARAMS)
Line 75: self.assertRaises(se.DomainHasGarbage, artifacts.create,


Line 200: self.assertTrue(
Line 201: 
os.path.basename(path).startswith(sd.REMOVED_IMAGE_PREFIX))
Line 202: self.assertTrue(os.path.exists(path))
Line 203: self.assertFalse(os.path.exists(artifacts._image_dir))
Line 204: self.assertEqual(nr_files, len(os.listdir(path)))
I don't think we should count files but check which files exists instead. This 
will make the tests more clear and prevent errors when wrong code create one 
file (but the wrong one) and the test pass.
Line 205: 
Line 206: def validate_metadata_artifact(self, artifacts, dirlist):
Line 207: metafile = os.path.basename(artifacts.meta_volatile_path)
Line 208: self.assertIn(metafile, dirlist)


Line 204: self.assertEqual(nr_files, len(os.listdir(path)))
Line 205: 
Line 206: def validate_metadata_artifact(self, artifacts, dirlist):
Line 207: metafile = os.path.basename(artifacts.meta_volatile_path)
Line 208: self.assertIn(metafile, dirlist)
I don't like the idea of getting file list and adding helper to check if a file 
exists in the list. This may be better if you 

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 25:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 24:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 23:

(3 comments)

I have to invest more time in this, some comments for now.

https://gerrit.ovirt.org/#/c/48097/23/tests/volume_artifacts_test.py
File tests/volume_artifacts_test.py:

Line 68: def test_state_missing(self):
Line 69: with self.fake_env() as env:
Line 70: artifacts = env.volume_artifacts(self.img_uuid, 
self.vol_uuid)
Line 71: self.assertFalse(artifacts.is_garbage())
Line 72: self.assertFalse(artifacts.image_exists)
is_image()?

Same for rest of the file.
Line 73: self.assertRaises(ValidationError,
Line 74:   self.validate_artifacts, artifacts)
Line 75: 
Line 76: def test_state_garbage_volatile_image_dir(self):


https://gerrit.ovirt.org/#/c/48097/23/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 114: - States:
Line 115: - no image or volatile directories
Line 116: - Operations:
Line 117: - is_garbage -> false
Line 118: - is_missing -> true
is_image (same for rest of the file)
Line 119: - create artifacts -> change state GARBAGE
Line 120: 
Line 121: GARBAGE
Line 122: 


Line 171: else:
Line 172: return self.sd_manifest.getDeletedImagePath(self.img_id)
Line 173: 
Line 174: @property
Line 175: def image_exists(self):
I think we wanted to replace this with is_image().

Also should move near is_garbage() - it is nice when methods are ordered by 
types (e.g. testing methods).
Line 176: if self._image_exists is None:
Line 177: self._image_exists = os.path.exists(self._image_dir)
Line 178: return self._image_exists
Line 179: 


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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 23:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


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', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 22:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


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', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


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', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 17:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


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', 
'ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 18:

(1 comment)

https://gerrit.ovirt.org/#/c/48097/18/tests/volume_artifacts_test.py
File tests/volume_artifacts_test.py:

Line 63: def setUp(self):
Line 64: self.img_uuid = str(uuid.uuid4())
Line 65: self.vol_uuid = str(uuid.uuid4())
Line 66: 
Line 67: def test_nonexist(self):
Need to improve test names, and add a docstring or a comment for each tests 
explaining the flow.
Line 68: with self.fake_env() as env:
Line 69: artifacts = env.volume_artifacts(self.img_uuid, 
self.vol_uuid)
Line 70: self.assertRaises(ValidationError,
Line 71:   self.validate_artifacts, artifacts)


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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 19:

This version removes unneeded import that made pyflakes grumpy.

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 18:

(5 comments)

https://gerrit.ovirt.org/#/c/48097/18//COMMIT_MSG
Commit Message:

Line 10: a garbage collection approach rather than persistent tasks and rollback
Line 11: operations.  Volumes consist of three separate artifacts: a data area, 
a
Line 12: metadata area, and a lease area.  Once created on storage these objects
Line 13: must be convertable into a Volume with a single atomic operation (ie.
Line 14: rename a single file).  Conversely, a Volume can be destroyed by
Lets use the same term we use for the volume artifacts api - "Volume can be 
uncommited" or "Volume can be dismantled"
Line 15: reducing it to its artifacts with a single atomic operation.
Line 16: 
Line 17: VolumeArtifacts is an object to manage the creation and removal of the
Line 18: three volume artifacts for both block and file based storage.  It also


Line 16: 
Line 17: VolumeArtifacts is an object to manage the creation and removal of the
Line 18: three volume artifacts for both block and file based storage.  It also
Line 19: has methods to manage the conversion of these artifacts to a Volume and
Line 20: to deconstruct a Volume in order to remove the artifacts from storage.
Again lets use the same terms we use for the api.
Line 21: The three artifacts on storage will not be detected as a volume by 
other
Line 22: storage code until they are committed.
Line 23: 
Line 24: Proposed operations for VolumeArtifacts:


Line 23: 
Line 24: Proposed operations for VolumeArtifacts:
Line 25:  - create: Create the artifacts on storage
Line 26:  - commit: Convert the artifacts to a volume
Line 27:  - dismantle: Convert a volume into artifacts
uncommit?
Line 28:  - clean: Remove the artifacts from storage
Line 29: 
Line 30: Additional methods to identify and garbage collect artifacts will also
Line 31: be required but the exact interface hasn't settled out yet.


Line 24: Proposed operations for VolumeArtifacts:
Line 25:  - create: Create the artifacts on storage
Line 26:  - commit: Convert the artifacts to a volume
Line 27:  - dismantle: Convert a volume into artifacts
Line 28:  - clean: Remove the artifacts from storage
destroy?
Line 29: 
Line 30: Additional methods to identify and garbage collect artifacts will also
Line 31: be required but the exact interface hasn't settled out yet.
Line 32: 


Line 27:  - dismantle: Convert a volume into artifacts
Line 28:  - clean: Remove the artifacts from storage
Line 29: 
Line 30: Additional methods to identify and garbage collect artifacts will also
Line 31: be required but the exact interface hasn't settled out yet.
Lets remove this, we discussed this today identifying garbage seems to the 
responsibility of the garbage collector, not this class.
Line 32: 
Line 33: This patch introduces the idea with minimal support.  We can create
Line 34: artifacts on file storage and we only support raw format with no parent
Line 35: volume.  Support for cow, parent volumes, block storage, and volume


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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 16: Code-Review-1

(8 comments)

https://gerrit.ovirt.org/#/c/48097/16/tests/volume_artifacts_test.py
File tests/volume_artifacts_test.py:

Line 38: pass
Line 39: 
Line 40: 
Line 41: @contextmanager
Line 42: def fake_file_env(cls=None):
cls is unused
Line 43: with namedTemporaryDir() as tmpdir:
Line 44: yield make_filesd_manifest(tmpdir)
Line 45: 
Line 46: 


Line 44: yield make_filesd_manifest(tmpdir)
Line 45: 
Line 46: 
Line 47: def img_and_vol():
Line 48: return str(uuid.uuid4()), str(uuid.uuid4())
This does not look very useful, maybe add a helper to create uuid?

utils.random_uuid()

?
Line 49: 
Line 50: 
Line 51: def base_raw_params():
Line 52: return (1073741824,


Line 51: def base_raw_params():
Line 52: return (1073741824,
Line 53: volume.RAW_FORMAT,
Line 54: image.SYSTEM_DISK_TYPE,
Line 55: '')
This returns a tuple, why not use a constant?

BASE_RAW_PARAMS = (...)
Line 56: 
Line 57: 
Line 58: class VolumeArtifactsTestsMixin(object):
Line 59: def setUp(self):


Line 56: 
Line 57: 
Line 58: class VolumeArtifactsTestsMixin(object):
Line 59: def setUp(self):
Line 60: self.sduuid = str(uuid.uuid4())
Maybe create all uuids here?
self.img_uuid = ...
self.vol_uuid = ...
Line 61: 
Line 62: def failure(*args):
Line 63: raise ExpectedFailure()
Line 64: 


Line 59: def setUp(self):
Line 60: self.sduuid = str(uuid.uuid4())
Line 61: 
Line 62: def failure(*args):
Line 63: raise ExpectedFailure()
You can fail a test with self.fail(msg)
Line 64: 
Line 65: def test_nonexist(self):
Line 66: with self.fake_env() as manifest:
Line 67: artifacts = self.artifacts_class(manifest, *img_and_vol())


Line 87: size, vol_format, disk_type, desc = base_raw_params()
Line 88: artifacts = self.artifacts_class(manifest, img_id, vol_id)
Line 89: artifacts.create(size, vol_format, disk_type, desc)
Line 90: artifacts.commit()
Line 91: vol = manifest.produceVolume(img_id, vol_id)
This test depends now on the manifest class, so bad volume_artifacts code can 
pass the tests if manifest is bugy, good volume_artifacts class code can fail 
the test because of manifest bug.

On the other hand this test ensure that both classes are compatible, which is 
also nice property.
Line 92: self.assertEqual(volume.type2name(volume.LEAF_VOL),
Line 93:  vol.getVolType())
Line 94: self.assertEqual(volume.SPARSE_VOL, vol.getType())
Line 95: self.assertTrue(vol.isSparse())


Line 91: vol = manifest.produceVolume(img_id, vol_id)
Line 92: self.assertEqual(volume.type2name(volume.LEAF_VOL),
Line 93:  vol.getVolType())
Line 94: self.assertEqual(volume.SPARSE_VOL, vol.getType())
Line 95: self.assertTrue(vol.isSparse())
This will not work for block volume. I think we should not start with a mixin 
class to prepare for block volume tests, but create only the classes we need 
now.

When we add the block volume tests, we can extract some stuff to a mixin class, 
or reuse code in another way.
Line 96: self.assertEqual(desc, vol.getDescription())
Line 97: self.assertEqual(volume.LEGAL_VOL, vol.getLegality())
Line 98: self.assertEqual(size / volume.BLOCK_SIZE, vol.getSize())
Line 99: self.assertEqual(vol_format, vol.getFormat())


Line 142: for f in [artifacts.meta_volatile_path, artifacts.volume_path,
Line 143:   lease_path]:
Line 144: if not os.path.exists(f):
Line 145: self.log.debug("Artifact missing: %s", f)
Line 146: raise ValidationError()
Why do we need ValidationError? test helpers like this should use the builtin 
asserts, like validate_lease_artifact.
Line 147: 
Line 148: def test_create_fail_before_meta(self):
Line 149: # If we fail before the metadata is created we will have an 
empty
Line 150: # image directory with a garbage collection prefix left behind


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
Gerrit-PatchSet: 16
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 16:

Partial review, looks good.

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 16:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 15: Verified+1

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 15:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 14:

(12 comments)

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

Line 83: def metadata_volatile_path(self):
Line 84: return self.metadata_stable_path + TEMP_VOL_FILEEXT
Line 85: 
Line 86: @property
Line 87: def metadata_stable_path(self):
> For consistency with volume_path and lease_path, I think we should call thi
Done
Line 88: vol_path = os.path.join(self.artifacts_dir, self.vol_id)
Line 89: return self.vol_class.metaVolumePath(vol_path)
Line 90: 
Line 91: @property


Line 108: self.sd_manifest.validateCreateVolumeParams(
Line 109: vol_format, parent_vol_id, preallocate=prealloc)
Line 110: 
Line 111: # If these artifacts are forming a new image we need to 
create a new
Line 112: # temporary image directory.
> This comment is not needed now, the code explain itself.
Done
Line 113: if not self.image_exists:
Line 114: self._create_image_artifact(self.artifacts_dir)
Line 115: 
Line 116: self._create_metadata_artifact(size, vol_format, prealloc, 
disk_type,


Line 110: 
Line 111: # If these artifacts are forming a new image we need to 
create a new
Line 112: # temporary image directory.
Line 113: if not self.image_exists:
Line 114: self._create_image_artifact(self.artifacts_dir)
> We don't need this parameter now
Done
Line 115: 
Line 116: self._create_metadata_artifact(size, vol_format, prealloc, 
disk_type,
Line 117:desc, parent_vol_id)
Line 118: self._create_lease_file()


Line 119: self._create_volume_file(vol_format, size)
Line 120: 
Line 121: def commit(self):
Line 122: lease_path = self.vol_class.leaseVolumePath(self.volume_path)
Line 123: for path in self.metadata_volatile_path, self.volume_path, 
lease_path:
> The lease_path is relevant only if the domain has volume leases.
Done
Line 124: if not os.path.exists(path):
Line 125: raise se.VolumeDoesNotExist("Path %s not found" % 
path)
Line 126: 
Line 127: self._oop.os.rename(self.metadata_volatile_path,


Line 120: 
Line 121: def commit(self):
Line 122: lease_path = self.vol_class.leaseVolumePath(self.volume_path)
Line 123: for path in self.metadata_volatile_path, self.volume_path, 
lease_path:
Line 124: if not os.path.exists(path):
> You must use _oop.os.path.exists - stat may block.
Done
Line 125: raise se.VolumeDoesNotExist("Path %s not found" % 
path)
Line 126: 
Line 127: self._oop.os.rename(self.metadata_volatile_path,
Line 128: self.metadata_stable_path)


Line 121: def commit(self):
Line 122: lease_path = self.vol_class.leaseVolumePath(self.volume_path)
Line 123: for path in self.metadata_volatile_path, self.volume_path, 
lease_path:
Line 124: if not os.path.exists(path):
Line 125: raise se.VolumeDoesNotExist("Path %s not found" % 
path)
> Use %r for such logs
Done
Line 126: 
Line 127: self._oop.os.rename(self.metadata_volatile_path,
Line 128: self.metadata_stable_path)
Line 129: 


Line 124: if not os.path.exists(path):
Line 125: raise se.VolumeDoesNotExist("Path %s not found" % 
path)
Line 126: 
Line 127: self._oop.os.rename(self.metadata_volatile_path,
Line 128: self.metadata_stable_path)
> This will raise OSError, and we do not handle it nicely yet. We need to rai
Going with VolumeCreationError.
Line 129: 
Line 130: # If we created a new image directory, rename it to the 
correct name
Line 131: if not self.image_exists:
Line 132: self._oop.os.rename(self.artifacts_dir, self._image_dir)


Line 140: leaf_type = volume.type2name(volume.LEAF_VOL)
Line 141: meta = self.vol_class.makeMetadata(
Line 142: self.sd_manifest.sdUUID, self.img_id, parent_vol_id, size,
Line 143: volume.type2name(vol_format), volume.type2name(prealloc),
Line 144: leaf_type, disk_type, desc, volume.LEGAL_VOL)
> Lets format this one parameter per line, it is very hard to read such blob.
Done
Line 145: 
Line 146: data = self.vol_class.formatMetadata(meta)
Line 147: # XXX: Do we need to use ioprocess here?
Line 148: if os.path.exists(self.metadata_volatile_path):


Line 143: volume.type2name(vol_format), volume.type2name(prealloc),
Line 144: leaf_type, disk_type, desc, volume.LEGAL_VOL)
Line 145: 
Line 146: data = self.vol_class.formatMetadata(meta)
Line 147: # XXX: Do we need to use ioprocess here?
> Yes
ok, 

Change in vdsm[master]: Introduce VolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 12:

(6 comments)

https://gerrit.ovirt.org/#/c/48097/12/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 69: """
Line 70: Create metadata file artifact, lease file, and volume file on 
storage.
Line 71: 
Line 72: Caller must hold the domain lock (paxos lease) and the image 
resource
Line 73: corresponding to self.img_id in exclusive mode.
> This is correct for all methods, right? better put this in __init__
Moved it to __init__ in VolumeArtifacts since it will also apply to 
BlockVolumeArtifacts.
Line 74: """
Line 75: # XXX: Remove these when support is added:
Line 76: if vol_format != volume.RAW_FORMAT:
Line 77: raise CannotCreateVolumeArtifacts("Only raw volumes are 
supported")


Line 84: vol_format, parent_vol_id, preallocate=prealloc)
Line 85: 
Line 86: # If these artifacts are forming a new image the artifacts 
path will be
Line 87: # a temporary directory.  Otherwise it's the existing image 
directory.
Line 88: artifacts_path = self._get_artifacts_path()
> We are doing this too many times, we can do it once if image does not exist
I refactored this all a bit and we now have properties for some of these things.
Line 89: if not self._image_exists():
Line 90: self._create_image_artifact(artifacts_path)
Line 91: 
Line 92: self._create_metadata_artifact(size, vol_format, prealloc, 
disk_type,


Line 86: # If these artifacts are forming a new image the artifacts 
path will be
Line 87: # a temporary directory.  Otherwise it's the existing image 
directory.
Line 88: artifacts_path = self._get_artifacts_path()
Line 89: if not self._image_exists():
Line 90: self._create_image_artifact(artifacts_path)
> I like image_exists() - it should be public.
The interface has changed a bit after our discussion.  image_exists will be a 
lazy property.
Line 91: 
Line 92: self._create_metadata_artifact(size, vol_format, prealloc, 
disk_type,
Line 93:desc, parent_vol_id)
Line 94: vol_path = os.path.join(artifacts_path, self.vol_id)


Line 122: leaf_type, disk_type, desc, volume.LEGAL_VOL)
Line 123: 
Line 124: data = self.vol_class.formatMetadata(meta)
Line 125: with open(self._get_meta_artifact_path(), "w") as f:
Line 126: f.write(data)
> We should fsync() here, otherwise the file may be empty or contain partial 
Done
Line 127: 
Line 128: def _create_lease_file(self, vol_path):
Line 129: if self.domain_manifest.hasVolumeLeases():
Line 130: meta_id = (vol_path,)


Line 157: 
Line 158: def _get_meta_artifact_path(self):
Line 159: return self._get_meta_path() + TEMP_VOL_FILEEXT
Line 160: 
Line 161: def _get_meta_path(self):
> Lets remove the _get from the names, and make these properties
Done
Line 162: artifacts_path = self._get_artifacts_path()
Line 163: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 164: return self.vol_class._metaVolumePath(vol_path)
Line 165: 


Line 160: 
Line 161: def _get_meta_path(self):
Line 162: artifacts_path = self._get_artifacts_path()
Line 163: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 164: return self.vol_class._metaVolumePath(vol_path)
> _metaVolumePath() should be public
Will add a patch for it.
Line 165: 
Line 166: def _create_image_artifact(self, artifacts_path):
Line 167: self.log.debug("Creating image artifact directory: %r", 
artifacts_path)
Line 168: try:


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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 14:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 14:

(12 comments)

I like this

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

Line 83: def metadata_volatile_path(self):
Line 84: return self.metadata_stable_path + TEMP_VOL_FILEEXT
Line 85: 
Line 86: @property
Line 87: def metadata_stable_path(self):
For consistency with volume_path and lease_path, I think we should call this 
meta_path, and meta_volatile_path.
Line 88: vol_path = os.path.join(self.artifacts_dir, self.vol_id)
Line 89: return self.vol_class.metaVolumePath(vol_path)
Line 90: 
Line 91: @property


Line 108: self.sd_manifest.validateCreateVolumeParams(
Line 109: vol_format, parent_vol_id, preallocate=prealloc)
Line 110: 
Line 111: # If these artifacts are forming a new image we need to 
create a new
Line 112: # temporary image directory.
This comment is not needed now, the code explain itself.
Line 113: if not self.image_exists:
Line 114: self._create_image_artifact(self.artifacts_dir)
Line 115: 
Line 116: self._create_metadata_artifact(size, vol_format, prealloc, 
disk_type,


Line 110: 
Line 111: # If these artifacts are forming a new image we need to 
create a new
Line 112: # temporary image directory.
Line 113: if not self.image_exists:
Line 114: self._create_image_artifact(self.artifacts_dir)
We don't need this parameter now
Line 115: 
Line 116: self._create_metadata_artifact(size, vol_format, prealloc, 
disk_type,
Line 117:desc, parent_vol_id)
Line 118: self._create_lease_file()


Line 119: self._create_volume_file(vol_format, size)
Line 120: 
Line 121: def commit(self):
Line 122: lease_path = self.vol_class.leaseVolumePath(self.volume_path)
Line 123: for path in self.metadata_volatile_path, self.volume_path, 
lease_path:
The lease_path is relevant only if the domain has volume leases.
Line 124: if not os.path.exists(path):
Line 125: raise se.VolumeDoesNotExist("Path %s not found" % 
path)
Line 126: 
Line 127: self._oop.os.rename(self.metadata_volatile_path,


Line 120: 
Line 121: def commit(self):
Line 122: lease_path = self.vol_class.leaseVolumePath(self.volume_path)
Line 123: for path in self.metadata_volatile_path, self.volume_path, 
lease_path:
Line 124: if not os.path.exists(path):
You must use _oop.os.path.exists - stat may block.

We don't need to validated the .meta.volatile path, as it will fail in the 
rename in the next block. 

Validating volume_path and lease_path can be nice though, but created them 
while we hold a lock, so I don't see any reason that will cause these files to 
disappear, and we will fail to use this volume in this case anyway, so I think 
we should simplify and not validate them now.
Line 125: raise se.VolumeDoesNotExist("Path %s not found" % 
path)
Line 126: 
Line 127: self._oop.os.rename(self.metadata_volatile_path,
Line 128: self.metadata_stable_path)


Line 121: def commit(self):
Line 122: lease_path = self.vol_class.leaseVolumePath(self.volume_path)
Line 123: for path in self.metadata_volatile_path, self.volume_path, 
lease_path:
Line 124: if not os.path.exists(path):
Line 125: raise se.VolumeDoesNotExist("Path %s not found" % 
path)
Use %r for such logs
Line 126: 
Line 127: self._oop.os.rename(self.metadata_volatile_path,
Line 128: self.metadata_stable_path)
Line 129: 


Line 124: if not os.path.exists(path):
Line 125: raise se.VolumeDoesNotExist("Path %s not found" % 
path)
Line 126: 
Line 127: self._oop.os.rename(self.metadata_volatile_path,
Line 128: self.metadata_stable_path)
This will raise OSError, and we do not handle it nicely yet. We need to raise 
vdsm api error for this, not sure about the error, as this may mean lot of 
errors (EPERM, ENOENT, etc.).
Line 129: 
Line 130: # If we created a new image directory, rename it to the 
correct name
Line 131: if not self.image_exists:
Line 132: self._oop.os.rename(self.artifacts_dir, self._image_dir)


Line 140: leaf_type = volume.type2name(volume.LEAF_VOL)
Line 141: meta = self.vol_class.makeMetadata(
Line 142: self.sd_manifest.sdUUID, self.img_id, parent_vol_id, size,
Line 143: volume.type2name(vol_format), volume.type2name(prealloc),
Line 144: leaf_type, disk_type, desc, volume.LEGAL_VOL)
Lets format this one parameter per line, it is very hard to read such blob. 
Then we don't need the 

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 14: Code-Review-1

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 13:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 12:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 11:

(14 comments)

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

Line 57: 
Line 58: def __init__(self, domain_manifest, img_id, vol_id):
Line 59: super(FileVolumeArtifacts, self).__init__(domain_manifest, 
img_id,
Line 60:   vol_id)
Line 61: self._image_path = self.domain_manifest.getImagePath(img_id)
> The old name is too abstract, this is not a path but a directory. We should
Done
Line 62: 
Line 63: @property
Line 64: def _oop(self):
Line 65: return self.domain_manifest.oop


Line 75: # XXX: Remove these when support is added:
Line 76: if vol_format != volume.RAW_FORMAT:
Line 77: raise CannotCreateVolumeArtifacts("Only raw volumes are 
supported")
Line 78: if parent_vol_id != volume.BLANK_UUID:
Line 79: raise CannotCreateVolumeArtifacts("parent_vol_id not 
supported")
> validating create parameters should be here.
Done
Line 80: 
Line 81: # If these artifacts are forming a new image the artifacts 
path will be
Line 82: # a temporary directory.  Otherwise it's the existing image 
directory.
Line 83: artifacts_path = self._get_artifacts_path()


Line 81: # If these artifacts are forming a new image the artifacts 
path will be
Line 82: # a temporary directory.  Otherwise it's the existing image 
directory.
Line 83: artifacts_path = self._get_artifacts_path()
Line 84: if artifacts_path != self._image_path:
Line 85: self._create_artifacts_path(artifacts_path)
> Add blank line.
Done
Line 86: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 87: meta_id = (vol_path,)
Line 88: 
Line 89: self._create_metadata_artifact(meta_id, size, vol_format, 
disk_type,


Line 83: artifacts_path = self._get_artifacts_path()
Line 84: if artifacts_path != self._image_path:
Line 85: self._create_artifacts_path(artifacts_path)
Line 86: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 87: meta_id = (vol_path,)
> This does not make any sense in this level. Methods that need to send meta_
Done
Line 88: 
Line 89: self._create_metadata_artifact(meta_id, size, vol_format, 
disk_type,
Line 90:desc, parent_vol_id)
Line 91: self._create_lease_artifact(meta_id)


Line 85: self._create_artifacts_path(artifacts_path)
Line 86: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 87: meta_id = (vol_path,)
Line 88: 
Line 89: self._create_metadata_artifact(meta_id, size, vol_format, 
disk_type,
> send vol_path instead.
Done
Line 90:desc, parent_vol_id)
Line 91: self._create_lease_artifact(meta_id)
Line 92: self._create_container_artifact(vol_path, vol_format, size)
Line 93: 


Line 87: meta_id = (vol_path,)
Line 88: 
Line 89: self._create_metadata_artifact(meta_id, size, vol_format, 
disk_type,
Line 90:desc, parent_vol_id)
Line 91: self._create_lease_artifact(meta_id)
> send vol_path instead
Done
Line 92: self._create_container_artifact(vol_path, vol_format, size)
Line 93: 
Line 94: def commit(self):
Line 95: artifacts_path = self._get_artifacts_path()


Line 94: def commit(self):
Line 95: artifacts_path = self._get_artifacts_path()
Line 96: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 97: commit_path = self.vol_class._metaVolumePath(vol_path)
Line 98: create_path = commit_path + constants.ARTIFACT_FILEEXT
> We should move this to self.meta_artifact_path, and use it when we create t
Done
Line 99: try:
Line 100: self._oop.os.rename(create_path, commit_path)
Line 101: except OSError as e:
Line 102: if e.errno == errno.ENOENT:


Line 115: 
Line 116: # File volumes are always created sparse
Line 117: prealloc = volume.SPARSE_VOL
Line 118: self.domain_manifest.validateCreateVolumeParams(
Line 119: vol_format, parent_vol_id, preallocate=prealloc)
> This should be first thing we do in create, before touching storage.
Done
Line 120: leaf_type = volume.type2name(volume.LEAF_VOL)
Line 121: 
Line 122: meta = self.vol_class.makeMetadata(
Line 123: self.domain_manifest.sdUUID, self.img_id, parent_vol_id, 
size,


Line 122: meta = self.vol_class.makeMetadata(
Line 123: self.domain_manifest.sdUUID, self.img_id, parent_vol_id, 
size,
Line 124: volume.type2name(vol_format), volume.type2name(prealloc),
Line 125: 

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 12: Code-Review-1

(6 comments)

https://gerrit.ovirt.org/#/c/48097/12/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 69: """
Line 70: Create metadata file artifact, lease file, and volume file on 
storage.
Line 71: 
Line 72: Caller must hold the domain lock (paxos lease) and the image 
resource
Line 73: corresponding to self.img_id in exclusive mode.
This is correct for all methods, right? better put this in __init__
Line 74: """
Line 75: # XXX: Remove these when support is added:
Line 76: if vol_format != volume.RAW_FORMAT:
Line 77: raise CannotCreateVolumeArtifacts("Only raw volumes are 
supported")


Line 84: vol_format, parent_vol_id, preallocate=prealloc)
Line 85: 
Line 86: # If these artifacts are forming a new image the artifacts 
path will be
Line 87: # a temporary directory.  Otherwise it's the existing image 
directory.
Line 88: artifacts_path = self._get_artifacts_path()
We are doing this too many times, we can do it once if image does not exists, 
and remember this state.
Line 89: if not self._image_exists():
Line 90: self._create_image_artifact(artifacts_path)
Line 91: 
Line 92: self._create_metadata_artifact(size, vol_format, prealloc, 
disk_type,


Line 86: # If these artifacts are forming a new image the artifacts 
path will be
Line 87: # a temporary directory.  Otherwise it's the existing image 
directory.
Line 88: artifacts_path = self._get_artifacts_path()
Line 89: if not self._image_exists():
Line 90: self._create_image_artifact(artifacts_path)
I like image_exists() - it should be public.

I think this class should work like this:

- detect() - look in storage and understand if we are adding a volume to 
existing
  image or creating a new image, and save this state.
- create() - create the directories and files based on the detected state
- commit() - commit the created artifacts based on the detected state
- dismantle() - convert the image/volume to artifacts based on the detected 
state
- purge() - remove the artifacts

After you detect(), the caller may like to learn about the instance, so methods 
such image_exits, or maybe is_existing_image should be public. Such info may be 
needed for the garbage collector.

detect can be part of init.
Line 91: 
Line 92: self._create_metadata_artifact(size, vol_format, prealloc, 
disk_type,
Line 93:desc, parent_vol_id)
Line 94: vol_path = os.path.join(artifacts_path, self.vol_id)


Line 122: leaf_type, disk_type, desc, volume.LEGAL_VOL)
Line 123: 
Line 124: data = self.vol_class.formatMetadata(meta)
Line 125: with open(self._get_meta_artifact_path(), "w") as f:
Line 126: f.write(data)
We should fsync() here, otherwise the file may be empty or contain partial 
content when after we rename it.
Line 127: 
Line 128: def _create_lease_file(self, vol_path):
Line 129: if self.domain_manifest.hasVolumeLeases():
Line 130: meta_id = (vol_path,)


Line 157: 
Line 158: def _get_meta_artifact_path(self):
Line 159: return self._get_meta_path() + TEMP_VOL_FILEEXT
Line 160: 
Line 161: def _get_meta_path(self):
Lets remove the _get from the names, and make these properties
Line 162: artifacts_path = self._get_artifacts_path()
Line 163: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 164: return self.vol_class._metaVolumePath(vol_path)
Line 165: 


Line 160: 
Line 161: def _get_meta_path(self):
Line 162: artifacts_path = self._get_artifacts_path()
Line 163: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 164: return self.vol_class._metaVolumePath(vol_path)
_metaVolumePath() should be public
Line 165: 
Line 166: def _create_image_artifact(self, artifacts_path):
Line 167: self.log.debug("Creating image artifact directory: %r", 
artifacts_path)
Line 168: try:


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Adam Litke 
Gerrit-Reviewer: Ala Hino 
Gerrit-Reviewer: Amit Aviram 
Gerrit-Reviewer: Daniel Erez 
Gerrit-Reviewer: Freddy Rolland 
Gerrit-Reviewer: Greg Padgett 
Gerrit-Reviewer: Idan Shaby 
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Liron Aravot 

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 10:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
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]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 11:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
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]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 11: Code-Review-1

(14 comments)

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

Line 57: 
Line 58: def __init__(self, domain_manifest, img_id, vol_id):
Line 59: super(FileVolumeArtifacts, self).__init__(domain_manifest, 
img_id,
Line 60:   vol_id)
Line 61: self._image_path = self.domain_manifest.getImagePath(img_id)
The old name is too abstract, this is not a path but a directory. We should 
call this self._image_dir.
Line 62: 
Line 63: @property
Line 64: def _oop(self):
Line 65: return self.domain_manifest.oop


Line 75: # XXX: Remove these when support is added:
Line 76: if vol_format != volume.RAW_FORMAT:
Line 77: raise CannotCreateVolumeArtifacts("Only raw volumes are 
supported")
Line 78: if parent_vol_id != volume.BLANK_UUID:
Line 79: raise CannotCreateVolumeArtifacts("parent_vol_id not 
supported")
validating create parameters should be here.
Line 80: 
Line 81: # If these artifacts are forming a new image the artifacts 
path will be
Line 82: # a temporary directory.  Otherwise it's the existing image 
directory.
Line 83: artifacts_path = self._get_artifacts_path()


Line 81: # If these artifacts are forming a new image the artifacts 
path will be
Line 82: # a temporary directory.  Otherwise it's the existing image 
directory.
Line 83: artifacts_path = self._get_artifacts_path()
Line 84: if artifacts_path != self._image_path:
Line 85: self._create_artifacts_path(artifacts_path)
Add blank line.
Line 86: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 87: meta_id = (vol_path,)
Line 88: 
Line 89: self._create_metadata_artifact(meta_id, size, vol_format, 
disk_type,


Line 83: artifacts_path = self._get_artifacts_path()
Line 84: if artifacts_path != self._image_path:
Line 85: self._create_artifacts_path(artifacts_path)
Line 86: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 87: meta_id = (vol_path,)
This does not make any sense in this level. Methods that need to send meta_id 
can generate this useless tuple.
Line 88: 
Line 89: self._create_metadata_artifact(meta_id, size, vol_format, 
disk_type,
Line 90:desc, parent_vol_id)
Line 91: self._create_lease_artifact(meta_id)


Line 85: self._create_artifacts_path(artifacts_path)
Line 86: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 87: meta_id = (vol_path,)
Line 88: 
Line 89: self._create_metadata_artifact(meta_id, size, vol_format, 
disk_type,
send vol_path instead.
Line 90:desc, parent_vol_id)
Line 91: self._create_lease_artifact(meta_id)
Line 92: self._create_container_artifact(vol_path, vol_format, size)
Line 93: 


Line 87: meta_id = (vol_path,)
Line 88: 
Line 89: self._create_metadata_artifact(meta_id, size, vol_format, 
disk_type,
Line 90:desc, parent_vol_id)
Line 91: self._create_lease_artifact(meta_id)
send vol_path instead
Line 92: self._create_container_artifact(vol_path, vol_format, size)
Line 93: 
Line 94: def commit(self):
Line 95: artifacts_path = self._get_artifacts_path()


Line 94: def commit(self):
Line 95: artifacts_path = self._get_artifacts_path()
Line 96: vol_path = os.path.join(artifacts_path, self.vol_id)
Line 97: commit_path = self.vol_class._metaVolumePath(vol_path)
Line 98: create_path = commit_path + constants.ARTIFACT_FILEEXT
We should move this to self.meta_artifact_path, and use it when we create the 
file.

Currently we use old code to create the file, and new code to rename it. Either 
we always get the path from the old code, or always compute it here, but not 
mix.
Line 99: try:
Line 100: self._oop.os.rename(create_path, commit_path)
Line 101: except OSError as e:
Line 102: if e.errno == errno.ENOENT:


Line 115: 
Line 116: # File volumes are always created sparse
Line 117: prealloc = volume.SPARSE_VOL
Line 118: self.domain_manifest.validateCreateVolumeParams(
Line 119: vol_format, parent_vol_id, preallocate=prealloc)
This should be first thing we do in create, before touching storage.
Line 120: leaf_type = volume.type2name(volume.LEAF_VOL)
Line 121: 
Line 122: meta = self.vol_class.makeMetadata(
Line 123: self.domain_manifest.sdUUID, self.img_id, parent_vol_id, 
size,


Line 122: meta = 

Change in vdsm[master]: Introduce VolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 8:

(2 comments)

https://gerrit.ovirt.org/#/c/48097/8/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 152: self.log.debug("Creating path for new image: %s", 
artifacts_path)
Line 153: try:
Line 154: self._oop.os.mkdir(artifacts_path)
Line 155: except OSError as e:
Line 156: if e.errno == errno.EEXIST:
> Lets always reject the error we cannot handle first, and then handle what w
Done
Line 157: # We have existing artifacts in the way.  Time to run
Line 158: # garbage collection
Line 159: self.log.error("Cannot create new image %s, garbage 
found "
Line 160:"at %s.", self.img_id, artifacts_path)


Line 162: raise
Line 163: 
Line 164: 
Line 165: class BlockVolumeArtifacts(VolumeArtifacts):
Line 166: pass
> Lets wait with this to the next iteration.
Done


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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 9:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
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]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 8: Code-Review-1

(2 comments)

Partial review, looks good.

https://gerrit.ovirt.org/#/c/48097/8/vdsm/storage/sdm/volume_artifacts.py
File vdsm/storage/sdm/volume_artifacts.py:

Line 152: self.log.debug("Creating path for new image: %s", 
artifacts_path)
Line 153: try:
Line 154: self._oop.os.mkdir(artifacts_path)
Line 155: except OSError as e:
Line 156: if e.errno == errno.EEXIST:
Lets always reject the error we cannot handle first, and then handle what we 
can (fail fast).

   if e.errno != errno.EEXIST:
   raise

# Handle EEXIST...
Line 157: # We have existing artifacts in the way.  Time to run
Line 158: # garbage collection
Line 159: self.log.error("Cannot create new image %s, garbage 
found "
Line 160:"at %s.", self.img_id, artifacts_path)


Line 162: raise
Line 163: 
Line 164: 
Line 165: class BlockVolumeArtifacts(VolumeArtifacts):
Line 166: pass
Lets wait with this to the next iteration.


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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 7:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
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]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 8:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
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]: Introduce VolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 1:

(3 comments)

https://gerrit.ovirt.org/#/c/48097/1/vdsm/storage/fileVolume.py
File vdsm/storage/fileVolume.py:

Line 54: class FileVolumeArtifacts(volume.VolumeArtifacts):
Line 55: def __init__(self, domain_manifest, img_id, vol_id):
Line 56: super(FileVolumeArtifacts, self).__init__(
Line 57: domain_manifest, img_id, vol_id)
Line 58: self._oop = domain_manifest.oop
> Why do you want to copy the manifest oop here?
Done
Line 59: self._artifacts_path = None
Line 60: 
Line 61: def create(self, size, vol_format, disk_type, desc, 
parent_vol_id=None):
Line 62: self._create_artifacts_path()


https://gerrit.ovirt.org/#/c/48097/1/vdsm/storage/volume.py
File vdsm/storage/volume.py:

Line 172: class CannotCreateVolumeArtifacts(Exception):
Line 173: pass
Line 174: 
Line 175: 
Line 176: class VolumeArtifacts(object):
> This looks like an interface - do we really want to keep instance variables
Sure... I'll also move the log down to the implementors.
Line 177: def __init__(self, domain_manifest, img_id, vol_id):
Line 178: self.domain_manifest = domain_manifest
Line 179: self.img_id = img_id
Line 180: self.vol_id = vol_id


Line 177: def __init__(self, domain_manifest, img_id, vol_id):
Line 178: self.domain_manifest = domain_manifest
Line 179: self.img_id = img_id
Line 180: self.vol_id = vol_id
Line 181: self.log = logging.getLogger('Storage.VolumeArtifacts')
> log must be class attribute, otherwise you are creating new logger for each
Done
Line 182: 
Line 183: def create(self, size, vol_format, disk_type, desc, 
parent_vol_id=None):
Line 184: raise NotImplementedError()
Line 185: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
Gerrit-PatchSet: 1
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]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 6:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
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]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 4:

(1 comment)

https://gerrit.ovirt.org/#/c/48097/4/vdsm/storage/sdm/volumeartifacts.py
File vdsm/storage/sdm/volumeartifacts.py:

Line 86: self._meta_file_commit_path())
Line 87: except OSError as e:
Line 88: if e.errno == errno.ENOENT:
Line 89: raise VolumeArtifactsNotFound()
Line 90: raise
> UUID_GLOB_PATTERN = '*-*-*-*-*'
Can we fix it to find only valid images?
Line 91: 
Line 92: # If we created a new image directory, rename it to the 
correct name
Line 93: if self._get_artifacts_path() == self._new_image_path():
Line 94: self._oop.os.rename(self._get_artifacts_path(),


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
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]: Introduce VolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 4:

(14 comments)

https://gerrit.ovirt.org/#/c/48097/4/vdsm/storage/sdm/volumeartifacts.py
File vdsm/storage/sdm/volumeartifacts.py:

Line 63: def _oop(self):
Line 64: return self.domain_manifest.oop
Line 65: 
Line 66: def create(self, size, vol_format, disk_type, desc,
Line 67:parent_vol_id=volume.BLANK_UUID):
> Please document our assumptions - which locks must be held when this is cal
Done
Line 68: # XXX: Remove these when support is added:
Line 69: if vol_format != volume.RAW_FORMAT:
Line 70: raise CannotCreateVolumeArtifacts("Only raw volumes are 
supported")
Line 71: if parent_vol_id != volume.BLANK_UUID:


Line 77: 
Line 78: self._create_metadata_artifact(meta_id, size, vol_format, 
disk_type,
Line 79:desc, parent_vol_id)
Line 80: self._create_lease_artifact(meta_id)
Line 81: self._create_container_artifact(vol_path, vol_format, size)
> All the logic was move out of create, so we don't understand anything from 
Done
Line 82: 
Line 83: def commit(self):
Line 84: try:
Line 85: self._oop.os.rename(self._meta_file_create_path(),


Line 86: self._meta_file_commit_path())
Line 87: except OSError as e:
Line 88: if e.errno == errno.ENOENT:
Line 89: raise VolumeArtifactsNotFound()
Line 90: raise
> This step is not needed when creating new image, so better avoid them and t
Not so fast... FileVolumes are identified by searching the domain for *.meta 
files.  This happens in all directories (even remove_me_* image directories).  
I don't want to break that behavior and affect old code.  So for commit we have 
two methods depending on whether this is a new image or not:

New Image:
Start -> rename metadata -> rename image dir
(So we transition as: Artifacts -> garbage image -> New image)

Existing image:
Start -> rename metadata
(So we transition as: Image with garbage -> Image without garbage)
Line 91: 
Line 92: # If we created a new image directory, rename it to the 
correct name
Line 93: if self._get_artifacts_path() == self._new_image_path():
Line 94: self._oop.os.rename(self._get_artifacts_path(),


Line 89: raise VolumeArtifactsNotFound()
Line 90: raise
Line 91: 
Line 92: # If we created a new image directory, rename it to the 
correct name
Line 93: if self._get_artifacts_path() == self._new_image_path():
> We should not created these paths multiple times. Keep the state, if we are
Done
Line 94: self._oop.os.rename(self._get_artifacts_path(),
Line 95: self._existing_image_path())
Line 96: 
Line 97: def _create_metadata_artifact(self, meta_id, size, vol_format, 
disk_type,


Line 97: def _create_metadata_artifact(self, meta_id, size, vol_format, 
disk_type,
Line 98:   desc, parent_vol_id):
Line 99: # Create the metadata artifact.  The metadata file is created 
with a
Line 100: # special extension to prevent these artifacts from being 
recognized as
Line 101: # a volume until FileVolumeArtifacts.commit() is called.
> I would like to have the same naming conversion for temporary entities (e.g
This contradicts your comment below where you'd like me to reuse 
_deletedImagePath().  I think the semantics of garbage images is different 
enough from these artifacts that we shouldn't steamroll the old 
ImageGarbageCollector code.
Line 102: 
Line 103: # File volumes are always created sparse
Line 104: prealloc = volume.SPARSE_VOL
Line 105: self.domain_manifest.validateCreateVolumeParams(


Line 109: meta = fileVolume.FileVolumeMetadata.makeMetadata(
Line 110: self.domain_manifest.sdUUID, self.img_id, parent_vol_id, 
size,
Line 111: volume.type2name(vol_format), volume.type2name(prealloc),
Line 112: leaf_type, disk_type, desc, volume.LEGAL_VOL)
Line 113: fileVolume.FileVolumeMetadata.putArtifactMetadata(meta_id, 
meta)
> Please avoid class methods in new code, it makes testing harder. We don't h
I'm conforming to the existing interface here.  At this point we really should 
not create a VolumeMetadata instance to work with VolumeArtifacts.  It seems 
safer/better to use a classmethod to prevent us from doing anything else with a 
VolumeMetadata that we should not be doing.
Line 114: 
Line 115: def _create_lease_artifact(self, meta_id):
Line 116: fileVolume.FileVolumeMetadata.newVolumeLease(
Line 117: meta_id, self.domain_manifest.sdUUID, self.vol_id)


Line 115: def _create_lease_artifact(self, meta_id):
Line 116: 

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 4:

(5 comments)

https://gerrit.ovirt.org/#/c/48097/4/vdsm/storage/sdm/volumeartifacts.py
File vdsm/storage/sdm/volumeartifacts.py:

Line 86: self._meta_file_commit_path())
Line 87: except OSError as e:
Line 88: if e.errno == errno.ENOENT:
Line 89: raise VolumeArtifactsNotFound()
Line 90: raise
> Not so fast... FileVolumes are identified by searching the domain for *.met
I think we search for image-uuid-pattern/*.meta, but lets keep this simple for 
now.
Line 91: 
Line 92: # If we created a new image directory, rename it to the 
correct name
Line 93: if self._get_artifacts_path() == self._new_image_path():
Line 94: self._oop.os.rename(self._get_artifacts_path(),


Line 97: def _create_metadata_artifact(self, meta_id, size, vol_format, 
disk_type,
Line 98:   desc, parent_vol_id):
Line 99: # Create the metadata artifact.  The metadata file is created 
with a
Line 100: # special extension to prevent these artifacts from being 
recognized as
Line 101: # a volume until FileVolumeArtifacts.commit() is called.
> This contradicts your comment below where you'd like me to reuse _deletedIm
Not really, ImageManifest.deleteImagePath can use the new format used for 
temporary volumes. This is storage format 4.0, we can and should make it better 
and more consistent.
Line 102: 
Line 103: # File volumes are always created sparse
Line 104: prealloc = volume.SPARSE_VOL
Line 105: self.domain_manifest.validateCreateVolumeParams(


Line 109: meta = fileVolume.FileVolumeMetadata.makeMetadata(
Line 110: self.domain_manifest.sdUUID, self.img_id, parent_vol_id, 
size,
Line 111: volume.type2name(vol_format), volume.type2name(prealloc),
Line 112: leaf_type, disk_type, desc, volume.LEGAL_VOL)
Line 113: fileVolume.FileVolumeMetadata.putArtifactMetadata(meta_id, 
meta)
> I'm conforming to the existing interface here.  At this point we really sho
Why we cannot use an instance here?
Line 114: 
Line 115: def _create_lease_artifact(self, meta_id):
Line 116: fileVolume.FileVolumeMetadata.newVolumeLease(
Line 117: meta_id, self.domain_manifest.sdUUID, self.vol_id)


Line 115: def _create_lease_artifact(self, meta_id):
Line 116: fileVolume.FileVolumeMetadata.newVolumeLease(
Line 117: meta_id, self.domain_manifest.sdUUID, self.vol_id)
Line 118: 
Line 119: def _create_container_artifact(self, vol_path, vol_format, size):
> In my parlance yes it is.
This contradict with the .artifact suffix. If all these are artifacts, then the 
prefix should be something else:

Volume artifacts - temporary
- volume
- volume.lease
- volume.meta.tmp

Volume artifacts - final
- volume
- volume.lease
- volume.meta

tmp is probably the most clear suffix, anyone looking at this without knowing 
anything about the design can tell that this is a temporary file.

Anyway, the word artifact in this method name does not add any value compared 
with create_container or create_data_file.
Line 120: size_bytes = size * volume.BLOCK_SIZE
Line 121: trunc_size = size_bytes if vol_format == volume.RAW_FORMAT 
else 0
Line 122: try:
Line 123: self._oop.truncateFile(vol_path, trunc_size,


Line 156: raise
Line 157: else:
Line 158: self.log.debug("Skipping directory creation for image: 
%s", path)
Line 159: 
Line 160: def _new_image_path(self):
> This could only be used if:
We will rename the method if needed of course. I'm not very happy with 
_remove_me prefix, but keeping the path of a deleted/temporary image inn the 
image class looks nicer.
Line 161: # If forming a new image, create artifacts in a 
garbage-collectible
Line 162: # image directory.
Line 163: return os.path.join(self.domain_manifest.domaindir,
Line 164: sd.DOMAIN_IMAGES,


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
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]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 5:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
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]: Introduce VolumeArtifacts

[alitke]

Adam Litke has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 4:

(2 comments)

https://gerrit.ovirt.org/#/c/48097/4/vdsm/storage/sdm/volumeartifacts.py
File vdsm/storage/sdm/volumeartifacts.py:

Line 86: self._meta_file_commit_path())
Line 87: except OSError as e:
Line 88: if e.errno == errno.ENOENT:
Line 89: raise VolumeArtifactsNotFound()
Line 90: raise
> I think we search for image-uuid-pattern/*.meta, but lets keep this simple 
UUID_GLOB_PATTERN = '*-*-*-*-*'

Which picks up garbage images too :)
Line 91: 
Line 92: # If we created a new image directory, rename it to the 
correct name
Line 93: if self._get_artifacts_path() == self._new_image_path():
Line 94: self._oop.os.rename(self._get_artifacts_path(),


Line 109: meta = fileVolume.FileVolumeMetadata.makeMetadata(
Line 110: self.domain_manifest.sdUUID, self.img_id, parent_vol_id, 
size,
Line 111: volume.type2name(vol_format), volume.type2name(prealloc),
Line 112: leaf_type, disk_type, desc, volume.LEGAL_VOL)
Line 113: fileVolume.FileVolumeMetadata.putArtifactMetadata(meta_id, 
meta)
> Why we cannot use an instance here?
Because you should only create a VolumeMetadata instance where there is an 
actual volume.  Here we only have artifacts and some assumptions the 
VolumeMetadata class will want to make will not hold true here (ie. 
validateVolumePath).
Line 114: 
Line 115: def _create_lease_artifact(self, meta_id):
Line 116: fileVolume.FileVolumeMetadata.newVolumeLease(
Line 117: meta_id, self.domain_manifest.sdUUID, self.vol_id)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
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]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 4:

Lets discuss first how this should work, before implementing it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: 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]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 4: Code-Review-1

(14 comments)

https://gerrit.ovirt.org/#/c/48097/4/vdsm/storage/sdm/volumeartifacts.py
File vdsm/storage/sdm/volumeartifacts.py:

Line 63: def _oop(self):
Line 64: return self.domain_manifest.oop
Line 65: 
Line 66: def create(self, size, vol_format, disk_type, desc,
Line 67:parent_vol_id=volume.BLANK_UUID):
Please document our assumptions - which locks must be held when this is called?
Line 68: # XXX: Remove these when support is added:
Line 69: if vol_format != volume.RAW_FORMAT:
Line 70: raise CannotCreateVolumeArtifacts("Only raw volumes are 
supported")
Line 71: if parent_vol_id != volume.BLANK_UUID:


Line 77: 
Line 78: self._create_metadata_artifact(meta_id, size, vol_format, 
disk_type,
Line 79:desc, parent_vol_id)
Line 80: self._create_lease_artifact(meta_id)
Line 81: self._create_container_artifact(vol_path, vol_format, size)
All the logic was move out of create, so we don't understand anything from this 
code now.

We should see here the logic of creating a new image and volume, or a new 
volume in existing image, without the details.

something like:

image = ImageMenifest(domain_manifest, image_id)
if image.exists()
create new volume in image.path
else:
create temporary image
create new volume in temporary image path

It should be easy to understand if we are using the same logic to create the 
new volume in both case, or we are using simpler code in the later case, since 
in this case commit is a single rename operation.
Line 82: 
Line 83: def commit(self):
Line 84: try:
Line 85: self._oop.os.rename(self._meta_file_create_path(),


Line 86: self._meta_file_commit_path())
Line 87: except OSError as e:
Line 88: if e.errno == errno.ENOENT:
Line 89: raise VolumeArtifactsNotFound()
Line 90: raise
This step is not needed when creating new image, so better avoid them and the 
possible failures.
Line 91: 
Line 92: # If we created a new image directory, rename it to the 
correct name
Line 93: if self._get_artifacts_path() == self._new_image_path():
Line 94: self._oop.os.rename(self._get_artifacts_path(),


Line 89: raise VolumeArtifactsNotFound()
Line 90: raise
Line 91: 
Line 92: # If we created a new image directory, rename it to the 
correct name
Line 93: if self._get_artifacts_path() == self._new_image_path():
We should not created these paths multiple times. Keep the state, if we are 
creating new image or adding a volume to existing image and use it here.
Line 94: self._oop.os.rename(self._get_artifacts_path(),
Line 95: self._existing_image_path())
Line 96: 
Line 97: def _create_metadata_artifact(self, meta_id, size, vol_format, 
disk_type,


Line 97: def _create_metadata_artifact(self, meta_id, size, vol_format, 
disk_type,
Line 98:   desc, parent_vol_id):
Line 99: # Create the metadata artifact.  The metadata file is created 
with a
Line 100: # special extension to prevent these artifacts from being 
recognized as
Line 101: # a volume until FileVolumeArtifacts.commit() is called.
I would like to have the same naming conversion for temporary entities (e.g 
both named xxx.tmp). Not use _remove_me_xxx for images, and volume.artifact for 
meta files.
Line 102: 
Line 103: # File volumes are always created sparse
Line 104: prealloc = volume.SPARSE_VOL
Line 105: self.domain_manifest.validateCreateVolumeParams(


Line 109: meta = fileVolume.FileVolumeMetadata.makeMetadata(
Line 110: self.domain_manifest.sdUUID, self.img_id, parent_vol_id, 
size,
Line 111: volume.type2name(vol_format), volume.type2name(prealloc),
Line 112: leaf_type, disk_type, desc, volume.LEGAL_VOL)
Line 113: fileVolume.FileVolumeMetadata.putArtifactMetadata(meta_id, 
meta)
Please avoid class methods in new code, it makes testing harder. We don't have 
a choice with old code, but new code should not use these.
Line 114: 
Line 115: def _create_lease_artifact(self, meta_id):
Line 116: fileVolume.FileVolumeMetadata.newVolumeLease(
Line 117: meta_id, self.domain_manifest.sdUUID, self.vol_id)


Line 115: def _create_lease_artifact(self, meta_id):
Line 116: fileVolume.FileVolumeMetadata.newVolumeLease(
Line 117: meta_id, self.domain_manifest.sdUUID, self.vol_id)
Line 118: 
Line 119: def _create_container_artifact(self, vol_path, vol_format, size):
This is not an artifact.
Line 120:   

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: 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]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 4:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: 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]: Introduce VolumeArtifacts

[automation]

gerrit-hooks has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 2:

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

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

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

Change in vdsm[master]: Introduce VolumeArtifacts

[nsoffer]

Nir Soffer has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 1: Code-Review-1

(4 comments)

I like this, needs more time to review properly.

https://gerrit.ovirt.org/#/c/48097/1/vdsm/storage/fileVolume.py
File vdsm/storage/fileVolume.py:

Line 54: class FileVolumeArtifacts(volume.VolumeArtifacts):
Line 55: def __init__(self, domain_manifest, img_id, vol_id):
Line 56: super(FileVolumeArtifacts, self).__init__(
Line 57: domain_manifest, img_id, vol_id)
Line 58: self._oop = domain_manifest.oop
Why do you want to copy the manifest oop here?

Define a property to access it instead. Otherwise we will have new entry in 
each instance dict for this.
Line 59: self._artifacts_path = None
Line 60: 
Line 61: def create(self, size, vol_format, disk_type, desc, 
parent_vol_id=None):
Line 62: self._create_artifacts_path()


https://gerrit.ovirt.org/#/c/48097/1/vdsm/storage/volume.py
File vdsm/storage/volume.py:

Line 164: TYPE_PATH = "path"
Line 165: TYPE_NETWORK = "network"
Line 166: 
Line 167: 
Line 168: class VolumeArtifactsNotFound(Exception):
Can't we use existing exceptions in storage_exceptions?
Line 169: pass
Line 170: 
Line 171: 
Line 172: class CannotCreateVolumeArtifacts(Exception):


Line 172: class CannotCreateVolumeArtifacts(Exception):
Line 173: pass
Line 174: 
Line 175: 
Line 176: class VolumeArtifacts(object):
This looks like an interface - do we really want to keep instance variables 
here?
Line 177: def __init__(self, domain_manifest, img_id, vol_id):
Line 178: self.domain_manifest = domain_manifest
Line 179: self.img_id = img_id
Line 180: self.vol_id = vol_id


Line 177: def __init__(self, domain_manifest, img_id, vol_id):
Line 178: self.domain_manifest = domain_manifest
Line 179: self.img_id = img_id
Line 180: self.vol_id = vol_id
Line 181: self.log = logging.getLogger('Storage.VolumeArtifacts')
log must be class attribute, otherwise you are creating new logger for each 
instance - these loggers are *never* deleted from logging.
Line 182: 
Line 183: def create(self, size, vol_format, disk_type, desc, 
parent_vol_id=None):
Line 184: raise NotImplementedError()
Line 185: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: Nir Soffer 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Change in vdsm[master]: Introduce VolumeArtifacts

[automation]

automat...@ovirt.org has posted comments on this change.

Change subject: Introduce VolumeArtifacts
..


Patch Set 1:

* 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.5', 
'ovirt-3.4', 'ovirt-3.3'])

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I352423e39a899b9b83ccf3b8f6c17ec433e9c353
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke 
Gerrit-Reviewer: automat...@ovirt.org
Gerrit-HasComments: No
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches