CA-273649: add xapi restriction when attach several host's usbs #1
Conversation
ocaml/idl/datamodel.ml
Outdated
| ~doc:"The operation is not allowed when the VM has VUSBs." () | ||
| ~doc:"The operation is not allowed when the VM has VUSBs." (); | ||
| error Api_errors.usb_vm_on_different_host [ "host" ] | ||
| ~doc:"USB and VM resident on different host." () |
There was a problem hiding this comment.
I would suggest to reword this message a little bit.
- USB is quite general, make it more specific such as "The USB device being pass-through";
- Similar issue as VM;
- The description stay quite on the reason side of the problem so to reader might get lost as what to do, it is better to describe the requirement more directly, such as:
The USB device being pass-through and the VM to host it have to be on the same host.
There was a problem hiding this comment.
good suggestion on the message
There was a problem hiding this comment.
BTW, how about pusb_not_in_possible_host? I think this may be better
ocaml/xapi/xapi_vusb.ml
Outdated
| if vusbs <> [] then | ||
| raise (Api_errors.Server_error(Api_errors.usb_group_conflict, [Ref.string_of uSB_group])); | ||
|
|
||
| (* USBs on different host can not be attached to one vm.*) |
There was a problem hiding this comment.
this comment does not align with the above error message and also what's being done below.
What's the actual intention?
| let possible_hosts = Xapi_vm.get_possible_hosts ~__context ~vm:vM in | ||
| let pusbs = Db.USB_group.get_PUSBs ~__context ~self:uSB_group in | ||
| let host = Db.PUSB.get_host ~__context ~self:(List.hd pusbs) in | ||
| if not (List.mem host possible_hosts) then |
There was a problem hiding this comment.
if you really want to fulfil the purpose of the comments, you have to:
- get the list of host from the pusbs;
- judge if they are the same one;
- determine if it (the same host from the pusbs) is in the possible host of the VM.
There was a problem hiding this comment.
I think no need to judge the list of pusbs , as all the pusbs must be on the same host. It is enough to judge one of them. Also now USB_group will only have one PUSB
There was a problem hiding this comment.
If that is the case, then fine. But it is better to add some comments saying so, otherwise the code will look odd; the USB_group looks like a group of some USBs, and though it is one PUSB for now, it might be used for extension in the future (think of you may want to have a group of USB device of similar kind of functionality on different hosts, so you can do VM migrate or so), so do not put mine in that.
There was a problem hiding this comment.
when usb_group has more than one host, the the logic will change a lot, not just change here will solve the issue. For now , USB_group will have no actual meaning.
|
LGTM. However I have concern about starting VM in a pool, and have left comments in the ticket. |
taoyongd
force-pushed
the
CA-273649
branch
3 times, most recently
from
58abf3c to
7ee2e63
Compare
This change is to add restriction when attaching usb to vm that the host of the usb is not in possible_hosts that vm can boot on. Signed-off-by: Taoyong Ding <taoyong.ding@citrix.com>
USB on different host can not attach to the same vm.
Signed-off-by: Taoyong Ding taoyong.ding@citrix.com