Nir Soffer has posted comments on this change. Change subject: sdm: Support snapshot creation ......................................................................
Patch Set 10: (5 comments) https://gerrit.ovirt.org/#/c/57057/10/vdsm/storage/sdm/volume_artifacts.py File vdsm/storage/sdm/volume_artifacts.py: Line 247 Line 248 Line 249 Line 250 Line 251 > This is fine as long as you're okay which having qcow2 metadata reference a I want to kill the internal setting. The only thing that makes a volume internal should be another volume linked to it. This means we can atomically add or remove a volume by atomic rename on file storage or adding/removing a tag on block storage. Line 379 Line 380 Line 381 Line 382 Line 383 > No reason that initial size should not be supported for snapshots. Yes, this can be handy if you want to control allocation of the new volume, for example during live storage migration - currently we use explicit extend, but we could use initial_size instead. Line 384 Line 385 Line 386 Line 387 Line 388 > Not necessarily true. If the base volume was created on a different host a Sure we may need to create the directory even for second volume, but this is for consuming the image, not for creating the underlying storage. We can move this up to the verb if an image must be consumable after we create it. Line 144: parent.vol_id) Line 145: parent_size_bytes = parent_vol.getSize() * sc.BLOCK_SIZE Line 146: if parent_size_bytes != size: Line 147: self.log.debug("New volume must have the same size as parent") Line 148: raise se.InvalidParameterException("size", size) > What do you mean by separate verb? A separate VolumeArtifacts API or a sep I don't like the idea of single api with lot of arguments and complex logic about the which arguments are needed or forbidden. For example: - size is required when creating new volume - size is forbidden when creating new volume with a parent. The code tells us that our design is wrong. What if we have 3 operations: - create_volume - make a new image with single volume - create_snapshot - add a new snapshot to existing image - clone_volume - make new image with single volume based on another volume All of them create a new volume artifact that must be committed. Each operation has less arguments and clear rules about them. Line 149: Line 150: if not parent_vol.isLegal(): Line 151: raise se.createIllegalVolumeSnapshotError(parent_vol.volUUID) Line 152: Line 151: raise se.createIllegalVolumeSnapshotError(parent_vol.volUUID) Line 152: Line 153: self.log.debug("Setting volType to internal for volume %s", Line 154: parent.vol_id) Line 155: parent_vol.setInternal() > It doesn't matter so long as the leaf's metadata has been written with a re The internal type is a design error that I want to eliminate. This is what break the atomicity of commit. I don't see any issue with having qemu image referencing another image without setting this flag. Line 156: parent_vol.prepare(rw=False) Line 157: try: Line 158: qemuimg.create(self.volume_path, backing=parent.vol_id, Line 159: format=sc.fmt2str(sc.COW_FORMAT), -- To view, visit https://gerrit.ovirt.org/57057 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3649ef8d87522105b645f249f2bc35f7db7e3e29 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
