From 3b1220d7de44f0d066b5327deea2f664cee01834 Mon Sep 17 00:00:00 2001 From: Gabor Igloi Date: Tue, 20 Feb 2018 17:17:51 +0000 Subject: [PATCH] Simplify & speed up data_destroy timing tests Signed-off-by: Gabor Igloi --- ocaml/xapi/test_vdi_cbt.ml | 231 +++++++++++++++---------------------- 1 file changed, 92 insertions(+), 139 deletions(-) diff --git a/ocaml/xapi/test_vdi_cbt.ml b/ocaml/xapi/test_vdi_cbt.ml index a9c2e8d4d30..e87de26f3c3 100644 --- a/ocaml/xapi/test_vdi_cbt.ml +++ b/ocaml/xapi/test_vdi_cbt.ml @@ -348,163 +348,116 @@ let test_data_destroy = (Db.VDI.get_type ~__context ~self:vdi) in - (** If [realistic_timing] is true, the tests will use the Message_forwarding module and - the original timeouts, otherwise they will call the implementation in - Xapi_vdi directly with a smaller timeout to make the tests faster. The - former leads to more comprehensive tests, as it goes through message - forwarding, which also includes a timeout / retry mechanism, and can thus - impact the timing of data_destroy: when the VBD unplug is already in - progress, Xapi_vdi.check_operation_error and the message forwarding layer - will first wait for that operation to finish. *) - let test_data_destroy_timing ~realistic_timing = - let speedup = if realistic_timing then 1.0 else 10.0 in - (* VDI.data_destroy currently waits for at most 4 seconds for the VBDs of - the VDI to be unplugged and destroyed *) - let timeout = 4.0 /. speedup in - (* We simulate a VBD.unplug call, which might take 1-2 seconds *) - let vbd_unplug_time = 1.5 /. speedup in - let () = assert (vbd_unplug_time < timeout) in - let other_delay = 0.2 /. speedup in - (* We have to leave some extra time for data_destroy to finish, otherwise - the tests will fail with a stricter timeout. *) - let timebox_timeout_delay = 1.0 in - - let uses_client = realistic_timing in - let call_data_destroy ~__context ~self = - if uses_client then begin - Api_server.Forwarder.VDI.data_destroy ~__context ~self - end else - Xapi_vdi._data_destroy ~__context ~self ~timeout - in - - let start_vbd_unplug ~__context self = - Db.VBD.set_current_operations ~__context ~self ~value:["x", `unplug] - in - let finish_vbd_unplug ~__context self = - Db.VBD.set_currently_attached ~__context ~self ~value:false; - Db.VBD.set_current_operations ~__context ~self ~value:[]; - in + let test_data_destroy_timing = + let bg f = Thread.create f () in + (* Creates a VBD that is currently_attached to our VDI *) let setup_test () = let __context, sR, vDI = setup_test_for_data_destroy () in let vM = Test_common.make_vm ~__context () in let vbd = Test_common.make_vbd ~__context ~vDI ~vM ~currently_attached:true () in - (__context, vDI, vbd) + let start_vbd_unplug () = + Db.VBD.set_current_operations ~__context ~self:vbd ~value:["x", `unplug] + in + let finish_vbd_unplug () = + Db.VBD.set_currently_attached ~__context ~self:vbd ~value:false; + Db.VBD.set_current_operations ~__context ~self:vbd ~value:[] + in + let destroy_vbd () = Db.VBD.destroy ~__context ~self:vbd in + let data_destroy ~timeout = + (* We need a 1-second tolerance here, otherwise the test will fail *) + let timebox_timeout = timeout +. 1.0 in + Helpers.timebox + ~timeout:timebox_timeout + ~otherwise:(fun () -> Alcotest.fail (Printf.sprintf "data_destroy did not return in %f seconds" timebox_timeout)) + (fun () -> Xapi_vdi._data_destroy ~__context ~self:vDI ~timeout) + in + (vDI, start_vbd_unplug, finish_vbd_unplug, destroy_vbd, data_destroy) in - let test_data_destroy_succeeds = - (* The parameters tell us whether the VBD unplug operation has started/finished by the time we call VDI.data_destroy. *) - let test_data_destroy_succeeds ~vbd_unplug_started ~vbd_unplug_finished () = - if vbd_unplug_finished && (not vbd_unplug_started) then failwith "VBD.unplug finished but not started by the time VDI.data_destroy is called: impossible"; - - let __context, vDI, vbd = setup_test () in - if vbd_unplug_started then start_vbd_unplug ~__context vbd; - if vbd_unplug_finished then finish_vbd_unplug ~__context vbd; - - let _: Thread.t = Thread.create (fun () -> - if not vbd_unplug_started then begin - Thread.delay other_delay; - start_vbd_unplug ~__context vbd - end; - if not vbd_unplug_finished then begin - Thread.delay vbd_unplug_time; - finish_vbd_unplug ~__context vbd - end; - Thread.delay other_delay; - Db.VBD.destroy ~__context ~self:vbd - ) () - in - let timeout = timeout +. timebox_timeout_delay in - Helpers.timebox - ~timeout - ~otherwise:(fun () -> failwith "data_destroy did not finish successfully in 5 seconds") - (fun () -> call_data_destroy ~__context ~self:vDI) - in - [ "test_data_destroy_succeeds_when_vbd_is_already_unplugged", `Slow, test_data_destroy_succeeds ~vbd_unplug_started:true ~vbd_unplug_finished:true - ; "test_data_destroy_succeeds_when_vbd_is_being_unplugged", `Slow, test_data_destroy_succeeds ~vbd_unplug_started:true ~vbd_unplug_finished:false - ; "test_data_destroy_succeeds_when_vbd_unplug_is_started_later", `Slow, test_data_destroy_succeeds ~vbd_unplug_started:false ~vbd_unplug_finished:false - ] + let test_data_destroy_succeeds_when_unplug_starts_later () = + let _vdi, start_vbd_unplug, finish_vbd_unplug, destroy_vbd, data_destroy = setup_test () in + let t = bg (fun () -> + Thread.delay 0.05; + start_vbd_unplug (); + Thread.delay 0.05; + finish_vbd_unplug (); + Thread.delay 0.05; + destroy_vbd () + ) in + data_destroy ~timeout:0.4; + Thread.join t in - let test_data_destroy_times_out = - (* The parameters tell us whether the VBD unplug operation has started/finished by the time VDI.data_destroy times out. *) - let test_data_destroy_times_out ~vbd_unplug_started ~vbd_unplug_finished () = - let __context, vDI, vbd = setup_test () in - - let t = Thread.create (fun () -> - (* The data_destroy operation will time out in 4 seconds *) - let unplug_start_delay = match vbd_unplug_started, vbd_unplug_finished with - | true, true -> timeout -. vbd_unplug_time -. other_delay (* unplug starts & finishes before the timeout *) - | true, false -> timeout -. (vbd_unplug_time /. 2.0) (* unplug starts before & finishes after the timeout *) - | false, false -> timeout +. other_delay (* unplug starts after the 4s timeout *) - | _ -> failwith "VBD.unplug finished but not started by the time VDI.data_destroy times out: impossible" - in - assert (unplug_start_delay >= 0.0); - Thread.delay unplug_start_delay; - start_vbd_unplug ~__context vbd; - (* Simulate a VBD.unplug call, which might take 1-2 seconds *) - Thread.delay vbd_unplug_time; - (* We have to finish the VBD unplug in the unit test, otherwise - message forwarding will wait for a long time for the in-progress - VBD.unplug operation to finish *) - finish_vbd_unplug ~__context vbd - ) () - in - (* If the timebox expires, we will fail because we will get an unexpected exception *) - let timeout = timeout +. timebox_timeout_delay in - Alcotest.check_raises "data_destroy times out" - Api_errors.(Server_error (vdi_in_use, [Ref.string_of vDI; "data_destroy"])) - (fun () -> - Helpers.timebox - ~timeout - ~otherwise:(fun () -> failwith "data_destroy did not time out") - (fun () -> call_data_destroy ~__context ~self:vDI) - ); - (* Wait for the background thread to finish to prevent it failing with - DBCache_NotFound exceptions due to missing references - this - probably happens when it tries to use the VBD reference but its __context already - got garbage collected *) - Thread.join t - in + let test_data_destroy_succeeds_when_vbd_is_being_unplugged () = + let _vdi, start_vbd_unplug, finish_vbd_unplug, destroy_vbd, data_destroy = setup_test () in + let t = bg (fun () -> + start_vbd_unplug (); + Thread.delay 0.05; + finish_vbd_unplug (); + Thread.delay 0.05; + destroy_vbd () + ) in + Thread.delay 0.02; + data_destroy ~timeout:0.4; + Thread.join t + in - let test_data_destroy_times_out_when_nothing_happens_to_vbd () = - let __context, vDI, vbd = setup_test () in - let timeout = timeout +. timebox_timeout_delay in - (* If the timebox expires, we will fail because we will get an unexpected exception *) - Alcotest.check_raises "data_destroy times out" - Api_errors.(Server_error (vdi_in_use, [Ref.string_of vDI; "data_destroy"])) - (fun () -> - Helpers.timebox - ~timeout - ~otherwise:(fun () -> failwith (Printf.sprintf "data_destroy did not time out in %f seconds" timeout)) - (fun () -> call_data_destroy ~__context ~self:vDI) - ) - in - [ "test_data_destroy_times_out_when_vbd_does_not_get_destroyed_in_time", `Slow, test_data_destroy_times_out ~vbd_unplug_started:true ~vbd_unplug_finished:true - ; "test_data_destroy_times_out_when_vbd_unplug_does_not_finish_in_time", `Slow, test_data_destroy_times_out ~vbd_unplug_started:true ~vbd_unplug_finished:false - ; "test_data_destroy_times_out_when_vbd_unplug_is_not_started_in_time", `Slow, test_data_destroy_times_out ~vbd_unplug_started:false ~vbd_unplug_finished:false - ; "test_data_destroy_times_out_when_nothing_happens_to_vbd", `Slow, test_data_destroy_times_out_when_nothing_happens_to_vbd - ] + let test_data_destroy_succeeds_when_vbd_is_being_destroyed () = + let _vdi, start_vbd_unplug, finish_vbd_unplug, destroy_vbd, data_destroy = setup_test () in + let t = bg (fun () -> + start_vbd_unplug (); + finish_vbd_unplug (); + Thread.delay 0.05; + destroy_vbd () + ) in + Thread.delay 0.02; + data_destroy ~timeout:0.4; + Thread.join t in - let suffix = if realistic_timing then "_with_realistic_timing" else "_with_simulated_timing" in - (test_data_destroy_succeeds - @ test_data_destroy_times_out) - |> List.map (function (n, s, t) -> (n ^ suffix, s, t)) - in + let test_data_destroy_times_out_when_vbd_does_not_get_unplugged_in_time () = + let vDI, start_vbd_unplug, finish_vbd_unplug, destroy_vbd, data_destroy = setup_test () in + let t = bg (fun () -> + Thread.delay 0.05; + start_vbd_unplug (); + (* finish_vbd_unplug does not happen in time *) + ) in + Alcotest.check_raises "data_destroy should raise VDI_IN_USE after its timeout" + Api_errors.(Server_error (vdi_in_use, [Ref.string_of vDI; "data_destroy"])) + (fun () -> data_destroy ~timeout:0.4); + Thread.join t + in + + let test_data_destroy_times_out_when_vbd_does_not_get_destroyed_in_time () = + let vDI, start_vbd_unplug, finish_vbd_unplug, destroy_vbd, data_destroy = setup_test () in + let t = bg (fun () -> + Thread.delay 0.05; + start_vbd_unplug (); + Thread.delay 0.05; + finish_vbd_unplug (); + (* destroy_vbd does not happen in time *) + ) in + Alcotest.check_raises "data_destroy should raise VDI_IN_USE after its timeout" + Api_errors.(Server_error (vdi_in_use, [Ref.string_of vDI; "data_destroy"])) + (fun () -> data_destroy ~timeout:0.4); + Thread.join t + in - (** This flag controls whether we should run the {!test_data_destroy_timing} - tests with realistic timing (with [realistic_timing = true). *) - let run_slow_tests = true in - let slow_tests = test_data_destroy_timing ~realistic_timing:true in + [ "test_data_destroy_succeeds_when_unplug_starts_later", `Slow, test_data_destroy_succeeds_when_unplug_starts_later + ; "test_data_destroy_succeeds_when_vbd_is_being_unplugged", `Slow, test_data_destroy_succeeds_when_vbd_is_being_unplugged + ; "test_data_destroy_succeeds_when_vbd_is_being_destroyed", `Slow, test_data_destroy_succeeds_when_vbd_is_being_destroyed + ; "test_data_destroy_times_out_when_vbd_does_not_get_unplugged_in_time", `Slow, test_data_destroy_times_out_when_vbd_does_not_get_unplugged_in_time + ; "test_data_destroy_times_out_when_vbd_does_not_get_destroyed_in_time", `Slow, test_data_destroy_times_out_when_vbd_does_not_get_destroyed_in_time + ] + + in [ "test_vdi_after_data_destroy", `Quick, test_vdi_after_data_destroy ; "test_vdi_managed_data_destroy", `Quick, test_vdi_managed_data_destroy ; "test_data_destroy_does_not_change_vdi_type_to_cbt_metadata_if_it_fails", `Quick, test_does_not_change_vdi_type_to_cbt_metadata_if_it_fails - ] @ test_data_destroy_timing ~realistic_timing:false - @ (if run_slow_tests then slow_tests else []) + ] @ test_data_destroy_timing -(* behaviour verification VDI.list_changed_blocks *) let test_vdi_list_changed_blocks () = let __context = Test_common.make_test_database () in let sR = make_mock_server_infrastructure ~__context in