VDI.snapshot: check that SM has this capability

[gaborigloi]

No description provided.

[gaborigloi]

On a master version of XenServer, it seems that this capability is reported for most SR types, except for iscsi, hba, udev, iso:

[root@dt14 ~]# xe sm-list params=type,capabilities 
type ( RO)                         : iscsi
    capabilities ( RO) [DEPRECATED]: SR_PROBE; VDI_CREATE; VDI_DELETE; VDI_ATTACH; VDI_DETACH; VDI_INTRODUCE


type ( RO)                         : lvmofcoe
    capabilities ( RO) [DEPRECATED]: SR_PROBE; SR_UPDATE; SR_METADATA; SR_TRIM; VDI_CREATE; VDI_DELETE; VDI_ATTACH; VDI_DETACH; VDI_GENERATE_CONFIG; VDI_SNAPSHOT; VDI_CLONE; VDI_RESIZE; VDI_RESET_ON_BOOT; VDI_UPDATE; VDI_MIRROR; VDI_ACTIVATE; VDI_DEACTIVATE


type ( RO)                         : ext
    capabilities ( RO) [DEPRECATED]: SR_PROBE; SR_UPDATE; SR_SUPPORTS_LOCAL_CACHING; VDI_CREATE; VDI_DELETE; VDI_ATTACH; VDI_DETACH; VDI_UPDATE; VDI_CLONE; VDI_SNAPSHOT; VDI_RESIZE; VDI_MIRROR; VDI_GENERATE_CONFIG; VDI_RESET_ON_BOOT; VDI_ACTIVATE; VDI_DEACTIVATE


type ( RO)                         : lvmoiscsi
    capabilities ( RO) [DEPRECATED]: SR_PROBE; SR_UPDATE; SR_METADATA; SR_TRIM; VDI_CREATE; VDI_DELETE; VDI_ATTACH; VDI_DETACH; VDI_GENERATE_CONFIG; VDI_CLONE; VDI_SNAPSHOT; VDI_RESIZE; VDI_RESET_ON_BOOT; VDI_UPDATE; VDI_MIRROR; VDI_ACTIVATE; VDI_DEACTIVATE


type ( RO)                         : nfs
    capabilities ( RO) [DEPRECATED]: SR_PROBE; SR_UPDATE; VDI_CREATE; VDI_DELETE; VDI_ATTACH; VDI_DETACH; VDI_UPDATE; VDI_CLONE; VDI_SNAPSHOT; VDI_RESIZE; VDI_GENERATE_CONFIG; VDI_MIRROR; VDI_RESET_ON_BOOT; VDI_ACTIVATE; VDI_DEACTIVATE


type ( RO)                         : hba
    capabilities ( RO) [DEPRECATED]: SR_PROBE; VDI_CREATE; VDI_DELETE; VDI_ATTACH; VDI_DETACH; VDI_INTRODUCE


type ( RO)                         : udev
    capabilities ( RO) [DEPRECATED]: VDI_INTRODUCE; VDI_ATTACH; VDI_DETACH; VDI_UPDATE; SR_UPDATE


type ( RO)                         : lvmohba
    capabilities ( RO) [DEPRECATED]: SR_PROBE; SR_UPDATE; SR_METADATA; SR_TRIM; VDI_CREATE; VDI_DELETE; VDI_ATTACH; VDI_DETACH; VDI_GENERATE_CONFIG; VDI_SNAPSHOT; VDI_CLONE; VDI_MIRROR; VDI_RESIZE; VDI_RESET_ON_BOOT; VDI_UPDATE; VDI_ACTIVATE; VDI_DEACTIVATE


type ( RO)                         : lvm
    capabilities ( RO) [DEPRECATED]: SR_PROBE; SR_UPDATE; SR_TRIM; VDI_CREATE; VDI_DELETE; VDI_ATTACH; VDI_DETACH; VDI_MIRROR; VDI_CLONE; VDI_SNAPSHOT; VDI_RESIZE; VDI_RESET_ON_BOOT; VDI_UPDATE; VDI_ACTIVATE; VDI_DEACTIVATE


type ( RO)                         : smb
    capabilities ( RO) [DEPRECATED]: SR_PROBE; SR_UPDATE; VDI_CREATE; VDI_DELETE; VDI_ATTACH; VDI_DETACH; VDI_UPDATE; VDI_CLONE; VDI_SNAPSHOT; VDI_RESIZE; VDI_MIRROR; VDI_GENERATE_CONFIG; VDI_RESET_ON_BOOT; VDI_ACTIVATE; VDI_DEACTIVATE


type ( RO)                         : file
    capabilities ( RO) [DEPRECATED]: SR_PROBE; SR_UPDATE; VDI_CREATE; VDI_DELETE; VDI_ATTACH; VDI_DETACH; VDI_CLONE; VDI_SNAPSHOT; VDI_RESIZE; VDI_MIRROR; VDI_GENERATE_CONFIG; VDI_ACTIVATE; VDI_DEACTIVATE


type ( RO)                         : iso
    capabilities ( RO) [DEPRECATED]: VDI_CREATE; VDI_DELETE; VDI_ATTACH; VDI_DETACH


type ( RO)                         : dummy
    capabilities ( RO) [DEPRECATED]: SR_PROBE; VDI_CREATE; VDI_DELETE; VDI_ATTACH; VDI_DETACH; VDI_ACTIVATE; VDI_DEACTIVATE; VDI_CLONE; VDI_SNAPSHOT; VDI_RESIZE; VDI_INTRODUCE; VDI_MIRROR


[root@dt14 ~]# 

[mseri]

I find it more readable if we keep using the pattern matching instead of nesting if/else:

| `snapshot when not Smint.(has_capability Vdi_snapshot sm_features) ->
   Some (Api_errors.sr_operation_not_supported, [Ref.string_of sr])
| `snapshot when (record.Db_actions.vDI_sharable) ->
   Some (Api_errors.vdi_is_sharable, [ _ref ])
| `snapshot when reset_on_boot ->
   Some (Api_errors.vdi_on_boot_mode_incompatible_with_operation, [])
| `snapshot ->
   if List.exists (fun (_, op) -> op = `copy) current_ops
   then Some (Api_errors.operation_not_allowed, ["Snapshot operation not allowed during copy."])
   else None

[coveralls]

Coverage decreased (-0.01%) to 13.932% when pulling 6b18c34 on gaborigloi:add_snapshot_capability_check into 7a11e2d on xapi-project:master.

[gaborigloi]

Thanks, updated.

[coveralls]

Coverage decreased (-0.01%) to 13.932% when pulling 1315fa6 on gaborigloi:add_snapshot_capability_check into 7a11e2d on xapi-project:master.

[coveralls]

Coverage increased (+0.003%) to 13.948% when pulling 1315fa6 on gaborigloi:add_snapshot_capability_check into 7a11e2d on xapi-project:master.

[mseri]

I am happy with it but I think @jonludlam should have a look

[thomassa]

Looks fine to me... but why have we not seen a bug from this? Maybe we don't have a test-case that tries to snapshot a VDI backed by an SR type that doesn't support it... or do we have a check somewhere else?

Have you tried out making a snapshot of an iSCSI or ISO VDI to see what happens without this change?

[gaborigloi]

@thomassa it throws an error:

[root@dt14 ~]# xe vdi-list params=sr-name-label uuid=$V
sr-name-label ( RO)    : XenRT Linux ISOs
[root@dt14 ~]# xe vdi-snapshot uuid=$V
The SR backend does not support the operation (check the SR's allowed operations)
sr: b4674c4a-3382-7319-dd98-11dc8b5e3021 (XenRT Linux ISOs)
[root@dt14 ~]

[mseri]

What is the output of the new code for the same command?

[gaborigloi]

@mseri exactly the same thing:

[root@dt14 ~]# xe vdi-list params=sr-name-label uuid=$V
sr-name-label ( RO)    : XenRT Linux ISOs
[root@dt14 ~]# xe vdi-snapshot uuid=$V
The SR backend does not support the operation (check the SR's allowed operations)
sr: 921bf208-4fc3-e8e3-b453-052d9913d02b (XenRT Linux ISOs)
[root@dt14 ~]

[mseri]

I am going to merge. If you intend to add tests, we can get them in a separate PR