Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

CA-130155: Port the fix of SCTX-1662 from trunk to clearwater-sp1-lcm #1674

Merged
merged 14 commits into from

3 participants

@huizh

The fix of SCTX-1662(CA-125187) has been merged in trunk branch with pull request 1656.
#1656

This pull request is to port the fix to clearwater-sp1-lcm branch to address SCTX-1744.

johnelse added some commits
@johnelse johnelse CA-125187: Add copy to VDI current_operations
Signed-off-by: John Else <john.else@citrix.com>
d50a616
@johnelse johnelse CA-125187: VBD allowed_operations fix
VBD allowed_operations checks should ignore running VDI.copy operations.
This wasn't needed before as VDIs weren't marked with the copy
operation.

Signed-off-by: John Else <john.else@citrix.com>
0313fd5
@johnelse johnelse CA-125187: Don't automatically dismiss VDI operations during copy
We still want to allow VDI.snapshot while a VDI is being copied, as
well as multiple parallel copies.

Signed-off-by: John Else <john.else@citrix.com>
a16264f
@johnelse johnelse CA-126097: Block clone of live VDIs
Signed-off-by: John Else <john.else@citrix.com>
4e18ce0
@johnelse johnelse Fix test case name
Signed-off-by: John Else <john.else@citrix.com>
869b218
@johnelse johnelse test VDIs should default to not sharable
Signed-off-by: John Else <john.else@citrix.com>
2362945
@johnelse johnelse Improve readability of VDI allowed_operations test failures
Signed-off-by: John Else <john.else@citrix.com>
dd1169a
@johnelse johnelse Add a case to a unit test
Signed-off-by: John Else <john.else@citrix.com>
df3810c
@johnelse johnelse Test that concurrent copies are allowed
Signed-off-by: John Else <john.else@citrix.com>
5d99bc3
@johnelse johnelse Add tests for clone/snapshot of VDIs during copying
Signed-off-by: John Else <john.else@citrix.com>
f8b8693
@johnelse johnelse Add a test for VBD.allowed_operations
Test that a VBD.plug is not blocked by the VDI.copy which triggered it.

Signed-off-by: John Else <john.else@citrix.com>
e180a22
@johnelse johnelse CA-125187: Add explanation of the new behaviour
Signed-off-by: John Else <john.else@citrix.com>
107cb48
@johnelse
Owner

n.b. we've had to revert part of this pull request on trunk (see #1676) as it broke checkpoint revert.

We're currently waiting on advice from the storage team as to what the final fix should be.

@huizh

Thanks John for the information. Please let me know once the decision is made.

@johnelse
Owner

Hi @huizh, he storage team have just pushed this fix to master: xapi-project/sm#101

This means the VDI clone patch can be reverted (I've notified the storage team that their fix will have to be backported to Clearwater at the same time) - please cherry pick the two patches from #1676 on top of this pull request.

Thanks :)

johnelse added some commits
@johnelse johnelse CA-130161: Revert "CA-126097: Block clone of live VDIs"
Disks attached to a checkpoint look like they're live, so blocking clone
of live VDIs broke checkpoint revert.

This reverts commit a8aece1.
dcc91e8
@johnelse johnelse CA-130161: Fix unit tests to test new behaviour
We've re-enabled clone of live VDIs, so the test needs to check this
instead of expecting failure.

Asserting whether a VDI can be cloned checks whether the backend
supports VDI_CLONE, so register the dummy SR with a dummy backend which
has a sensible set of features.

Signed-off-by: John Else <john.else@citrix.com>
7b753ed
@huizh

Hi, John, thanks for the information and requesting storage team to port their patches back to clearwater-sp1-lcm.
The two commits of #1676 are cherry picked here.
Could you please help double check?

Thanks.

@johnelse
Owner

Looks good, thanks! I'm happy for this to go in once Shiva says it's ready to be hotfixed.

@johnelse johnelse added the For hotfix label
@johnelse
Owner

SM fix was backported to clearwater for a previous hotfix, so this can go in.

@johnelse johnelse merged commit db2834e into xapi-project:clearwater-sp1-lcm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 31, 2014
  1. @johnelse @huizh

    CA-125187: Add copy to VDI current_operations

    johnelse authored huizh committed
    Signed-off-by: John Else <john.else@citrix.com>
  2. @johnelse @huizh

    CA-125187: VBD allowed_operations fix

    johnelse authored huizh committed
    VBD allowed_operations checks should ignore running VDI.copy operations.
    This wasn't needed before as VDIs weren't marked with the copy
    operation.
    
    Signed-off-by: John Else <john.else@citrix.com>
  3. @johnelse @huizh

    CA-125187: Don't automatically dismiss VDI operations during copy

    johnelse authored huizh committed
    We still want to allow VDI.snapshot while a VDI is being copied, as
    well as multiple parallel copies.
    
    Signed-off-by: John Else <john.else@citrix.com>
  4. @johnelse @huizh

    CA-126097: Block clone of live VDIs

    johnelse authored huizh committed
    Signed-off-by: John Else <john.else@citrix.com>
  5. @johnelse @huizh

    Fix test case name

    johnelse authored huizh committed
    Signed-off-by: John Else <john.else@citrix.com>
  6. @johnelse @huizh

    test VDIs should default to not sharable

    johnelse authored huizh committed
    Signed-off-by: John Else <john.else@citrix.com>
  7. @johnelse @huizh

    Improve readability of VDI allowed_operations test failures

    johnelse authored huizh committed
    Signed-off-by: John Else <john.else@citrix.com>
  8. @johnelse @huizh

    Add a case to a unit test

    johnelse authored huizh committed
    Signed-off-by: John Else <john.else@citrix.com>
  9. @johnelse @huizh

    Test that concurrent copies are allowed

    johnelse authored huizh committed
    Signed-off-by: John Else <john.else@citrix.com>
  10. @johnelse @huizh

    Add tests for clone/snapshot of VDIs during copying

    johnelse authored huizh committed
    Signed-off-by: John Else <john.else@citrix.com>
  11. @johnelse @huizh

    Add a test for VBD.allowed_operations

    johnelse authored huizh committed
    Test that a VBD.plug is not blocked by the VDI.copy which triggered it.
    
    Signed-off-by: John Else <john.else@citrix.com>
  12. @johnelse @huizh

    CA-125187: Add explanation of the new behaviour

    johnelse authored huizh committed
    Signed-off-by: John Else <john.else@citrix.com>
Commits on Apr 14, 2014
  1. @johnelse @huizh

    CA-130161: Revert "CA-126097: Block clone of live VDIs"

    johnelse authored huizh committed
    Disks attached to a checkpoint look like they're live, so blocking clone
    of live VDIs broke checkpoint revert.
    
    This reverts commit a8aece1.
  2. @johnelse @huizh

    CA-130161: Fix unit tests to test new behaviour

    johnelse authored huizh committed
    We've re-enabled clone of live VDIs, so the test needs to check this
    instead of expecting failure.
    
    Asserting whether a VDI can be cloned checks whether the backend
    supports VDI_CLONE, so register the dummy SR with a dummy backend which
    has a sensible set of features.
    
    Signed-off-by: John Else <john.else@citrix.com>
This page is out of date. Refresh to see the latest.
View
2  ocaml/test/test_common.ml
@@ -160,7 +160,7 @@ let make_vbd ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ()) ?(allowed_operat
let make_vdi ~__context ?(ref=Ref.make ()) ?(uuid=make_uuid ()) ?(name_label="")
?(name_description="") ?(allowed_operations=[]) ?(current_operations=[]) ?(sR=Ref.make ())
- ?(virtual_size=0L) ?(physical_utilisation=0L) ?(_type=`user) ?(sharable=true) ?(read_only=false)
+ ?(virtual_size=0L) ?(physical_utilisation=0L) ?(_type=`user) ?(sharable=false) ?(read_only=false)
?(other_config=[]) ?(storage_lock=false) ?(location="") ?(managed=false) ?(missing=false)
?(parent=Ref.null) ?(xenstore_data=[]) ?(sm_config=[]) ?(is_a_snapshot=false)
?(snapshot_of=Ref.null) ?(snapshot_time=Date.never) ?(tags=[]) ?(allow_caching=true)
View
113 ocaml/test/test_vdi_allowed_operations.ml
@@ -19,6 +19,36 @@ open Test_common
let setup_test ~__context vbd_fun =
let sr_ref = make_sr ~__context () in
+ let sr_uuid = Db.SR.get_uuid ~__context ~self:sr_ref in
+ (* Register the SR with a dummy processor which has a sensible
+ * set of features. *)
+ Storage_mux.register sr_uuid
+ (fun _ -> Rpc.({success = true; contents = Null}))
+ "0"
+ Storage_interface.({
+ driver = "";
+ name = "";
+ description = "";
+ vendor = "";
+ copyright = "";
+ version = "";
+ required_api_version = "";
+ features = [
+ "SR_PROBE";
+ "SR_UPDATE";
+ "VDI_CREATE";
+ "VDI_DELETE";
+ "VDI_ATTACH";
+ "VDI_DETACH";
+ "VDI_UPDATE";
+ "VDI_CLONE";
+ "VDI_SNAPSHOT";
+ "VDI_RESIZE";
+ "VDI_GENERATE_CONFIG";
+ "VDI_RESET_ON_BOOT/2";
+ ];
+ configuration = []
+ });
let (_: API.ref_PBD) = make_pbd ~__context ~sR:sr_ref () in
let vdi_ref = make_vdi ~__context ~sR:sr_ref () in
let vdi_record = Db.VDI.get_record_internal ~__context ~self:vdi_ref in
@@ -30,9 +60,17 @@ let my_cmp a b = match a,b with
| None, None -> a = b
| _ -> false
+let string_of_api_exn_opt = function
+ | None -> "None"
+ | Some (code, args) ->
+ Printf.sprintf "Some (%s, [%s])" code (String.concat "; " args)
+
let run_assert_equal_with_vdi ~__context ?(cmp = my_cmp) ?(ha_enabled=false) vbd_list op exc =
let vdi_ref, vdi_record = setup_test ~__context vbd_list in
- assert_equal ~cmp (Xapi_vdi.check_operation_error ~__context ha_enabled vdi_record vdi_ref op) exc
+ assert_equal
+ ~cmp
+ ~printer:string_of_api_exn_opt
+ exc (Xapi_vdi.check_operation_error ~__context ha_enabled vdi_record vdi_ref op)
(* This is to test Xapi_vdi.check_operation_error against CA-98944
code. This DO NOT fully test the aforementionned function *)
@@ -71,7 +109,7 @@ let test_ca98944 () =
`forget None
(* VDI.copy should be allowed if all attached VBDs are read-only. *)
-let test_ca101699 () =
+let test_ca101669 () =
let __context = Mock.make_context_with_new_db "Mock context" in
(* Attempting to copy a RW-attached VDI should fail with VDI_IN_USE. *)
@@ -89,11 +127,78 @@ let test_ca101699 () =
(* Attempting to copy an unattached VDI should pass. *)
run_assert_equal_with_vdi ~__context
(fun vdi_ref -> ())
- `copy None
+ `copy None;
+
+ (* Attempting to copy RW- and RO-attached VDIs should fail with VDI_IN_USE. *)
+ run_assert_equal_with_vdi ~__context
+ (fun vdi_ref ->
+ let (_: API.ref_VBD) = make_vbd ~__context ~vDI:vdi_ref ~currently_attached:true ~mode:`RW () in
+ make_vbd ~__context ~vDI:vdi_ref ~currently_attached:true ~mode:`RO ())
+ `copy (Some (Api_errors.vdi_in_use, []))
+
+let test_ca125187 () =
+ let __context = Mock.make_context_with_new_db "Mock context" in
+
+ (* A VDI being copied can be copied again concurrently. *)
+ run_assert_equal_with_vdi ~__context
+ (fun vdi_ref ->
+ let (_: API.ref_VBD) = make_vbd ~__context ~vDI:vdi_ref ~currently_attached:true ~mode:`RO () in
+ Db.VDI.set_current_operations ~__context
+ ~self:vdi_ref
+ ~value:["mytask", `copy])
+ `copy None;
+
+ (* A VBD can be plugged to a VDI which is being copied. This is required as
+ * the VBD is plugged after the VDI is marked with the copy operation. *)
+ let _, _ = setup_test ~__context
+ (fun vdi_ref ->
+ let vm_ref = make_vm ~__context () in
+ Db.VM.set_is_control_domain ~__context ~self:vm_ref ~value:true;
+ Db.VM.set_power_state ~__context ~self:vm_ref ~value:`Running;
+ let vbd_ref = Ref.make () in
+ let (_: API.ref_VBD) = make_vbd ~__context
+ ~ref:vbd_ref
+ ~vDI:vdi_ref
+ ~vM:vm_ref
+ ~currently_attached:false
+ ~mode:`RO () in
+ Db.VDI.set_current_operations ~__context
+ ~self:vdi_ref
+ ~value:["mytask", `copy];
+ Db.VDI.set_managed ~__context
+ ~self:vdi_ref
+ ~value:true;
+ Xapi_vbd_helpers.assert_operation_valid ~__context
+ ~self:vbd_ref
+ ~op:`plug)
+ in ()
+
+let test_ca126097 () =
+ let __context = Mock.make_context_with_new_db "Mock context" in
+
+ (* Attempting to clone a VDI being copied should fail with VDI_IN_USE. *)
+ run_assert_equal_with_vdi ~__context
+ (fun vdi_ref ->
+ let (_: API.ref_VBD) = make_vbd ~__context ~vDI:vdi_ref ~currently_attached:true ~mode:`RO () in
+ Db.VDI.set_current_operations ~__context
+ ~self:vdi_ref
+ ~value:["mytask", `copy])
+ `clone None;
+
+ (* Attempting to snapshot a VDI being copied should be allowed. *)
+ run_assert_equal_with_vdi ~__context
+ (fun vdi_ref ->
+ let (_: API.ref_VBD) = make_vbd ~__context ~vDI:vdi_ref ~currently_attached:true ~mode:`RO () in
+ Db.VDI.set_current_operations ~__context
+ ~self:vdi_ref
+ ~value:["mytask", `copy])
+ `snapshot None
let test =
"test_vdi_allowed_operations" >:::
[
"test_ca98944" >:: test_ca98944;
- "test_ca101699" >:: test_ca101699;
+ "test_ca101669" >:: test_ca101669;
+ "test_ca125187" >:: test_ca125187;
+ "test_ca126097" >:: test_ca126097;
]
View
11 ocaml/xapi/message_forwarding.ml
@@ -3304,17 +3304,18 @@ module Forward = functor(Local: Custom_actions.CUSTOM_ACTIONS) -> struct
let copy ~__context ~vdi ~sr ~base_vdi ~into_vdi =
info "VDI.copy: VDI = '%s'; SR = '%s'; base_vdi = '%s'; into_vdi = '%s'" (vdi_uuid ~__context vdi) (sr_uuid ~__context sr) (vdi_uuid ~__context base_vdi) (vdi_uuid ~__context into_vdi);
- Xapi_vdi.assert_operation_valid ~__context ~self:vdi ~op:`copy;
let local_fn = Local.VDI.copy ~vdi ~sr ~base_vdi ~into_vdi in
let src_sr = Db.VDI.get_SR ~__context ~self:vdi in
(* No need to lock the VDI because the VBD.plug will do that for us *)
(* Try forward the request to a host which can have access to both source
and destination SR. *)
let op session_id rpc = Client.VDI.copy rpc session_id vdi sr base_vdi into_vdi in
- try
- SR.forward_sr_multiple_op ~local_fn ~__context ~srs:[src_sr; sr] ~prefer_slaves:true op
- with Not_found ->
- SR.forward_sr_multiple_op ~local_fn ~__context ~srs:[src_sr] ~prefer_slaves:true op
+ with_sr_andor_vdi ~__context ~vdi:(vdi, `copy) ~doc:"VDI.copy"
+ (fun () ->
+ try
+ SR.forward_sr_multiple_op ~local_fn ~__context ~srs:[src_sr; sr] ~prefer_slaves:true op
+ with Not_found ->
+ SR.forward_sr_multiple_op ~local_fn ~__context ~srs:[src_sr] ~prefer_slaves:true op)
let pool_migrate ~__context ~vdi ~sr ~options =
let vbds = Db.VBD.get_records_where ~__context
View
7 ocaml/xapi/xapi_vbd_helpers.ml
@@ -144,7 +144,12 @@ let valid_operations ~expensive_sharing_checks ~__context record _ref' : table =
if not vdi_record.Db_actions.vDI_managed
then set_errors Api_errors.vdi_not_managed [ _ref ] all_ops;
- if vdi_record.Db_actions.vDI_current_operations <> [] then begin
+ let vdi_operations_besides_copy =
+ List.exists
+ (fun (_, operation) -> operation <> `copy)
+ vdi_record.Db_actions.vDI_current_operations
+ in
+ if vdi_operations_besides_copy then begin
debug "VBD operation %s not allowed because VDI.current-operations = [ %s ]"
(String.concat ";" (List.map vbd_operation_to_string current_ops))
(String.concat "; "
View
15 ocaml/xapi/xapi_vdi.ml
@@ -32,14 +32,19 @@ let check_operation_error ~__context ?(sr_records=[]) ?(pbd_records=[]) ?(vbd_re
let reset_on_boot = record.Db_actions.vDI_on_boot = `reset in
(* Policy:
- 1. any current_operation implies exclusivity; fail everything else
- 2. if doing a VM start then assume the sharing check is done elsewhere
+ 1. any current_operation besides copy implies exclusivity; fail everything
+ else
+ 2. if a copy is ongoing, don't fail with other_operation_in_progress, as
+ blocked operations could then get stuck behind a long-running copy.
+ Instead, rely on the blocked_by_attach check further down to decide
+ whether an operation should be allowed.
+ 3. if doing a VM start then assume the sharing check is done elsewhere
(so VMs may share disks but our operations cannot)
- 3. for other operations, fail if any VBD has currently-attached=true or any VBD
+ 4. for other operations, fail if any VBD has currently-attached=true or any VBD
has a current_operation itself
- 4. HA prevents you from deleting statefiles or metadata volumes
+ 5. HA prevents you from deleting statefiles or metadata volumes
*)
- if List.length current_ops > 0
+ if List.exists (fun (_, op) -> op <> `copy) current_ops
then Some(Api_errors.other_operation_in_progress,["VDI"; _ref])
else
(* check to see whether it's a local cd drive *)
Something went wrong with that request. Please try again.