[ovirt-devel] Gluster Volume Snapshots - Feature review

[Shubhendu Tripathi]

Hi Juan,

Incorporated the comments in the wiki.
Kindly do have a look.

Thanks and Regards,
Shubhendu

On 12/18/2014 06:38 PM, Shubhendu Tripathi wrote:
> Hi Juan,
>
> Thanks for the detailed comments.
> My comments inline.
>
> Thanks and Regards,
> Shubhendu
>
> On 12/11/2014 08:56 PM, Juan Hernández wrote:
>> On 12/11/2014 05:36 AM, Shubhendu Tripathi wrote:
>>> Hi Juan,
>>>
>>> Incorporated the review comments.
>>> Kindly have a look and let us know if everything looks well and good.
>>>
>>> Thanks and Regards,
>>> Shubhendu
>>>
>> Thanks for making the changes. I have some additional comments:
>>
>> 1. URL segments shouldn't have underscores. For example, you are
>> proposing URLs like this:
>>
>> /clusters/{cluster:id}/glustervolumes/{volume:id}/volume_snapshots
>>
>> The last component should be "volumesnapshots", without the underscore.
>> Note that on the other hand XML element should have underscores, like in
>> "<volume_snapshots>", that is correct.
>
> Sure. Will do the changes accordingly.
>
>>
>> 2. Try to avoid abbreviations. For example, instead of "whatever_params"
>> use "whatever_parameters", and instead of "scheduling_det" use
>> "scheduling_details", use "cron_expression" instead of "cronexpr", so 
>> on.
>
> Sure. Will do the changes accordingly.
>
>>
>> 3. If possible the action to delete a snapshot shouldn't receive any
>> parameters, not even an empty "<action/>" element.
>
> Sure. Will do the changes accordingly.
>
>>
>> 4. The "schedulesnapshot" action should be modeled using REST style, not
>> an action. My understanding is that you plan to have for each volume a
>> set of rules that define when the snapshots will be created. This should
>> be implemented as a sub-collection of the volume, so that these rules
>> can be queried, added, modified and deleted:
>>
>>    To query:
>>
>>    GET ...{volume:id}/snapshotrules
>>    <snapshot_rules>
>>      <snapshot_rule id="..." href="...">
>>        <crontab_expression>...</crontab_expression>
>>      </snapshot_rule>
>>      ...
>>    </snapshot_rules>
>>
>>    To add:
>>
>>    POST ...{volume:id{/snapshotrules
>>    <snapshot_rule>
>>      <crontab_expression>...</crontab_expression>
>>    </snapshot_rule>
>>
>>    To modify:
>>
>>    PUT ...{volume:id}/snapshotrules/{snapshotrule:id}
>>    <snapshot_rule>
>>      <crontab_expression>...</crontab_expression>
>>    </snapshot_rule>
>>
>>    To delete (note that there is no body):
>>
>>     DELETE ...{volume:id}/snapshotrules/{snapshotrule:id}
>>
>> If there will be only one of these rules per volume then you can model
>> them as attributes of the volume itself, without the sub-collection.
>
> At a time there would be only one rule for a volume, so as suggested 
> it could be a volume attribute. Will model accordingly.
>
>>
>> 5. The "snapshotconfigs" should be modeled as an attribute of the
>> volume, not as an action.
>
> As above.
>
>>
>>> On 12/09/2014 07:23 PM, Shubhendu Tripathi wrote:
>>>> Thanks Juan for the comments. I would update the wiki accordingly 
>>>> and send for confirmation.
>>>>
>>>> Regards
>>>> Shubhendu
>>>>
>>>> Sent from Samsung Mobile
>>>>
>>>> -------- Original message --------
>>>> From: Juan Hernández <jhernand at redhat.com>
>>>> Date: 09/12/2014  18:51  (GMT+05:30)
>>>> To: Shubhendu Tripathi 
>>>> <shtripat at redhat.com>,devel at ovirt.org,Michael Pasternak 
>>>> <mpastern at redhat.com>
>>>> Subject: Re: [ovirt-devel] Gluster Volume Snapshots - Feature review
>>>>    On 12/04/2014 07:11 PM, Shubhendu Tripathi wrote:
>>>>> Hi Juan/Michael,
>>>>>
>>>>> This is a gentle reminder for the review of the REST api design 
>>>>> for the
>>>>> below feature.
>>>>> We would be starting the REST development for the same soon 
>>>>> (probably by
>>>>> Dec 2014 end).
>>>>>
>>>>> If there are specific comments, please pass it on. If no comments we
>>>>> would go ahead with the current design and implement accordingly.
>>>>>
>>>>> Request your time for this.
>>>>>
>>>>> Thanks and Regards,
>>>>> Shubhendu
>>>>>
>>>>> On 11/10/2014 12:22 PM, Shubhendu Tripathi wrote:
>>>>>> Hi All,
>>>>>>
>>>>>> Please help us to review the design of Gluster Volume Snapshots 
>>>>>> in oVirt,
>>>>>>
>>>>>> Here are two design on wiki page
>>>>>>
>>>>>> General Feature Design
>>>>>> http://www.ovirt.org/Features/GlusterVolumeSnapshots
>>>>>>
>>>>>> Detailed Design
>>>>>> http://www.ovirt.org/Features/Design/GlusterVolumeSnapshots
>>>>>>
>>>>>> We target it in ovirt 3.6 release.
>>>>>>
>>>>>> Marked Juan/Michael specifically for REST review.
>>>>>>
>>>>>> Best Regards,
>>>>>> Shubhendu Tripathi
>>>> My comments about the RESTAPI:
>>>>
>>>> 1. You can't use the "snapshot" and "snapshots" XML elements, as those
>>>> are already in use for disk snapshots, and we don't have name space
>>>> support in the RESTAPI. You will have to use something different, for
>>>> example "gluster_volume_snapshots" and "gluster_volume_snapshot".
>>>>
>>>> 2. When adding a volume snapshot the name of the volume shouldn't be a
>>>> parameter, as that is implicit. Only the name and description of the
>>>> snapshot should be provided.
>>>>
>>>> 3. The operation to delete a snapshot should be performed on the
>>>> snapshot resource, not on the collection:
>>>>
>>>>     DELETE
>>>> /clusters/{cluster:id}/glustervolumes/{volume:id}/snapshots/{snapshot:id} 
>>>>
>>>>
>>>> Ideally this operation shouldn't receive any parameters (thus no 
>>>> body).
>>>> If it does require parameters then they should be contained inside an
>>>> "action" element.
>>>>
>>>> 4. The operation to update the snapshot configuration should be the 
>>>> PUT
>>>> operation of the volume, not a new "snapshotconfig" sub-resource, as
>>>> these kind of sub-resources aren't well supported by the SDKs and 
>>>> the CLI.
>>>>
>>> _______________________________________________
>>> Devel mailing list
>>> Devel at ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>
>>
>
> _______________________________________________
> Devel mailing list
> Devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/devel