Shubhendu Tripathi has posted comments on this change. Change subject: gluster: BLL commands for gluster volume snapshot ......................................................................
Patch Set 2: (7 comments) http://gerrit.ovirt.org/#/c/37325/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/DeleteAllGlusterVolumeSnapshotsCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/DeleteAllGlusterVolumeSnapshotsCommand.java: Line 19: import org.ovirt.engine.core.common.vdscommands.gluster.GlusterVolumeVDSParameters; Line 20: import org.ovirt.engine.core.compat.Guid; Line 21: import org.ovirt.engine.core.dao.gluster.GlusterVolumeSnapshotDao; Line 22: Line 23: public class DeleteAllGlusterVolumeSnapshotsCommand extends GlusterVolumeCommandBase<GlusterVolumeParameters> { > I think this could also extend from SnapshotCommandBase...fairly repetitive Actually deleteAll needs only volumeName as parameter so extending from command GlusterVolumeCommandBase with GlusterVolumeParameters. Also in this scenario we are having a local set of snapshots which need to be deleted and not a single snapshot. The class GlusterVolumeSnapshotCommandBase has a single snapshot as member variable. Line 24: List<GlusterVolumeSnapshotEntity> snapshots; Line 25: Line 26: public DeleteAllGlusterVolumeSnapshotsCommand(GlusterVolumeParameters params) { Line 27: super(params); http://gerrit.ovirt.org/#/c/37325/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/DeleteGlusterVolumeSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/DeleteGlusterVolumeSnapshotCommand.java: Line 35: @Override Line 36: protected boolean canDoAction() { Line 37: if (!super.canDoAction()) { Line 38: return false; Line 39: } > Any other validations - should snapshot be deactive before delete? not required as gluster allows deletion in active state Line 40: Line 41: return true; Line 42: } Line 43: http://gerrit.ovirt.org/#/c/37325/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterVolumeSnapshotCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterVolumeSnapshotCommandBase.java: Line 40: if (!super.canDoAction()) { Line 41: return false; Line 42: } Line 43: Line 44: snapshot = getGlusterVolumeSnapshotDao().getByName(getGlusterVolumeId(), getParameters().getSnapshotName()); > call getSnapshot() I feel the initialization of snapshot can be done in constructor and here we can use getSnapshot() Line 45: if (snapshot == null) { Line 46: failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_SNAPSHOT_DOES_NOT_EXIST, Line 47: getParameters().getSnapshotName()); Line 48: } Line 44: snapshot = getGlusterVolumeSnapshotDao().getByName(getGlusterVolumeId(), getParameters().getSnapshotName()); Line 45: if (snapshot == null) { Line 46: failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_SNAPSHOT_DOES_NOT_EXIST, Line 47: getParameters().getSnapshotName()); Line 48: } > Check if cluster supports snapshot here done Line 49: Line 50: return true; Line 51: } Line 52: http://gerrit.ovirt.org/#/c/37325/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/RestoreGlusterVolumeSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/RestoreGlusterVolumeSnapshotCommand.java: Line 48: if (getGlusterVolume().getAsyncTask().getStatus() == JobExecutionStatus.STARTED) { Line 49: failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_VOLUME_OPERATION_IN_PROGRESS, Line 50: getGlusterVolumeName(), Line 51: getVdsGroupName()); Line 52: } > How about geo-rep? Can you restore if geo-rep in progress? will check that. I feel all the geo-replicated volumes also should be restored in this case. Not so sure. Will check and update if required. Line 53: Line 54: return true; Line 55: } Line 56: http://gerrit.ovirt.org/#/c/37325/2/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 1172: ACTION_TYPE_FAILED_GEOREP_SESSION_ALREADY_PAUSED=Cannot ${action} ${type}. Geo-replication session is paused. Line 1173: ACTION_TYPE_FAILED_GLUSTER_VOLUME_SNAPSHOT_NAME_IS_EMPTY=Cannot ${action} ${type}. Gluster volume snapshot name is empty. Line 1174: ACTION_TYPE_FAILED_GLUSTER_VOLUME_SNAPSHOT_DOES_NOT_EXIST=Cannot ${action} ${type}. Snapshot ${snapname} does not exist. Line 1175: ACTION_TYPE_FAILED_GLUSTER_VOLUME_NO_SNAPSHOTS_EXIST=Cannot ${action} ${type}. No snapshots exist for volume ${volumeName}. Line 1176: ACTION_TYPE_FAILED_GLUSTER_VOLUME_SNAPSHOT_ALREADY_ACTIVATED=Cannot ${action} ${type}. Snapshot ${snapname} is already activated. > Gluster volume snapshot ...in all these? will be confused with VM snapshot done Line 1177: ACTION_TYPE_FAILED_GLUSTER_VOLUME_SNAPSHOT_ALREADY_DEACTIVATED=Cannot ${action} ${type}. Snapshot ${snapname} is already de-activated. Line 1178: Line 1179: ACTION_TYPE_FAILED_TAG_ID_REQUIRED=Cannot ${action} ${type}. Tag ID is required. Line 1180: http://gerrit.ovirt.org/#/c/37325/2/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties File backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties: Line 833: GLUSTER_VOLUME_SNAPSHOT_ACTIVATE_FAILED=Failed to activate the snapshot ${snapname} on volume ${glusterVolumeName}. Line 834: GLUSTER_VOLUME_SNAPSHOT_DEACTIVATED=De-activated the snapshot ${snapname} on volume ${glusterVolumeName}. Line 835: GLUSTER_VOLUME_SNAPSHOT_DEACTIVATE_FAILED=Failed to de-activate snapshot ${snapname} on volume ${glusterVolumeName}. Line 836: GLUSTER_VOLUME_SNAPSHOT_RESTORED=Restored the volume ${glusterVolumeName} to the state of snapshot ${snapname}. Line 837: GLUSTER_VOLUME_SNAPSHOT_RESTORE_FAILED=Failed to restore the volume ${glusterVolumeName} to the state of snapshot ${snapname}. > Same comment as previous file done Line 838: Line 839: VDS_UNTRUSTED=Host ${VdsName} was set to non-operational. Host is not trusted by the attestation service. Line 840: USER_ADDED_NETWORK_QOS=Network QoS ${QosName} was added. (User: ${UserName}) Line 841: USER_FAILED_TO_ADD_NETWORK_QOS=Failed to add Network QoS ${QosName}. (User: ${UserName}) -- To view, visit http://gerrit.ovirt.org/37325 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I74fa567e0910a8f97c3d127951481448983e3fd3 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
