xapi-project / xapi-storage-script

A xapi storage adapter that calls out to scripts, one script per operation
Other
6 stars 16 forks source link

CA-283693: Store & return correct snapshot_time for VDIs #62

Closed gaborigloi closed 6 years ago

gaborigloi commented 6 years ago

Signed-off-by: Gabor Igloi gabor.igloi@citrix.com

gaborigloi commented 6 years ago

Tested it and now it works:

[root@host ~]# cat repro.sh
SR=$(xe sr-list type=gfs2 --minimal)
VDI=$(xe vdi-create sr-uuid=$SR name-label=test virtual-size=4000000)
date
S=$(xe vdi-snapshot uuid=$VDI)
xe vdi-list uuid=$S params=snapshot-time
[root@host ~]# bash -exu repro.sh 
++ xe sr-list type=gfs2 --minimal
+ SR=2959708e-3686-6d3e-ba60-ab6a97b7313f
++ xe vdi-create sr-uuid=2959708e-3686-6d3e-ba60-ab6a97b7313f name-label=test virtual-size=4000000
+ VDI=b54c1703-0f04-4217-a6f1-cbd927c12bcb
+ date
Wed 28 Mar 12:08:57 UTC 2018
++ xe vdi-snapshot uuid=b54c1703-0f04-4217-a6f1-cbd927c12bcb
+ S=b3b88a35-360b-4870-8f83-a5d7ac9c69c4
+ xe vdi-list uuid=b3b88a35-360b-4870-8f83-a5d7ac9c69c4 params=snapshot-time
snapshot-time ( RO)    : 20180328T12:08:57Z

[root@host ~]# xe vdi-list uuid=b3b88a35-360b-4870-8f83-a5d7ac9c69c4 params=snapshot-of 
snapshot-of ( RO)    : b54c1703-0f04-4217-a6f1-cbd927c12bcb

[root@host ~]#
mseri commented 6 years ago

Does this need any change now that we merged REQ 477?

edwintorok commented 6 years ago

This PR uses the volume metadata (Volume.set) to store the snapshot time, there are no changes required there. The only question is whether it is ok to do an additional Volume.stat everytime a Volume.snapshot is done? Are the stats "cheap"? @marksymsctx

MarkSymsCtx commented 6 years ago

It's not too expensive but it does involve opening the file up (in the current GFS2 implementation), of course in the general case an SR could do anything in there so probably should be cautious about it.

edwintorok commented 6 years ago

Ok in that case it would probably be better that instead of:

>>= fun () ->
+    (* Stat the VDI to get the updated keys including the snapshot time *)
+    stat compat_out root_dir name dbg sr response.Xapi_storage.Volume.Types.key
+    >>= fun response ->

we just update the time in this response:

let response = { response with snapshot_time = now } in

And when we actually do a Volume.stat of course the snapshot_time will be read by vdi_of_volume

gaborigloi commented 6 years ago

Tested again, still works:

[root@fscbx2560m1 ~]# bash -eux repro.sh    
++ xe sr-list type=gfs2 --minimal           
+ SR=9d3e21c8-144f-de04-b598-8de78f3c266a   
++ xe vdi-create sr-uuid=9d3e21c8-144f-de04-b598-8de78f3c266a name-label=test virtual-size=4000000
+ VDI=c0a1c5b9-722a-49a3-bfae-05e3f9df660f  
+ date                
Mon  9 Apr 16:48:32 UTC 2018                
++ xe vdi-snapshot uuid=c0a1c5b9-722a-49a3-bfae-05e3f9df660f                            
+ S=29d1c52b-f210-4010-9e0a-e014fc1643cf    
+ xe vdi-list uuid=29d1c52b-f210-4010-9e0a-e014fc1643cf params=snapshot-time            
snapshot-time ( RO)    : 19700101T00:00:00Z 

======== after uploading new binary:
[root@fscbx2560m1 ~]# bash -eux repro.sh    
++ xe sr-list type=gfs2 --minimal           
+ SR=9d3e21c8-144f-de04-b598-8de78f3c266a   
++ xe vdi-create sr-uuid=9d3e21c8-144f-de04-b598-8de78f3c266a name-label=test virtual-size=4000000
+ VDI=886d8695-0757-47b1-89a8-36a6319d2689  
+ date                
Mon  9 Apr 16:49:23 UTC 2018                
++ xe vdi-snapshot uuid=886d8695-0757-47b1-89a8-36a6319d2689                            
+ S=936a66ae-402f-49d3-aaf1-7bd197505693    
+ xe vdi-list uuid=936a66ae-402f-49d3-aaf1-7bd197505693 params=snapshot-time            
snapshot-time ( RO)    : 20180409T16:49:23Z 

[root@fscbx2560m1 ~]# 
edwintorok commented 6 years ago

Travis failed, there is no ./configure

gaborigloi commented 6 years ago

This change is already in the relevant branch, because it's pinned there, we'll have to release it before the merge to master.