CP-10203: XenAPI: api call for RDP on/off request

[huizh]

Signed-off-by: Hui Zhang hui.zhang@citrix.com

[thomassa]

Here we read and write Xenstore nodes directly from xapi.
Wherever possible we want to do this only through xenopsd, so this function should just call a corresponding new function in xenopsd.
(Sorry I missed this on Friday when I was reviewing your earlier version of the code. In fact I should have mentioned it before you started.)

[thomassa]

This set of operations is currently in alphabetical order, which is helpful for readability.

It would be nice to preserve this by inserting the new items into the appropriate place (after awaiting_memory_live) instead of putting them at the end.

[thomassa]

The code looks good now (except for the trivial cosmetic issue of the ordering of the items in xapi_vm_lifecycle.ml around line 67).

Please would you squash the two commits into one?
This is easy with git rebase --interactive HEAD^^ (note that the commits will be listed with the earliest at the top, i.e. in forward not reverse order).

[huizh]

Thanks Thomas for the comments.
The code has been updated for xapi_vm_lifecycle.ml.
Also the two commits have been squashed to one.

[robhoes]

Why _ts? What does it stand for?

[thomassa]

Rob, "ts" is for "terminal services", Windows terminology for RDP.
This corresponds to the xenstore nodes data/ts, control/feature-ts and control-ts used by the guest agent.

[robhoes]

Ah, I see, fair enough. However, this error name is part of the high-level XenAPI, and does not necessarily need to use the same terminology as the low-level xenstore interface. In fact, the API calls use "rdp" rather than "ts" already. From a XenAPI user's point of view, it may be clearer to use "rdp" in the error as well.

[djs55]

Looking at the other FEATURE_s in the context (shutdown, suspend, vcpu_hotplug) they all correspond to operations on the VM "virtual hardware". The XenAPI VM.* is mostly about manipulating and monitoring the "virtual hardware" and mostly not about performing operations within the guest operating system (although the implementation of VM.clean_shutdown happens to do this as part of its implementation it could have been implemented in terms of an ACPI button push). This means that the VM.* functions don't really care whether you're running Linux, FreeBSD or Windows.

Since this feature seems very Windows-dependent ("terminal services") and is tied to our particular guest agent (and is not part of the standard Xen guest interface which is more minimal), I think we should reflect that in the name. Perhaps we should call the API VM_guest_agent.foo?

[robhoes]

As said on the xenopsd pull request, I'd prefer to use the verb request in the API calls rather than beg.

[robhoes]

I realise I should have made most of my above comments on xapi-project/xapi-project.github.io#12 :/

[huizh]

Thanks all the comments and suggestion.
Here is a summary of what should the code be updated:

1. Change the APIs' name from "beg_rdp_on" and "beg_rdp_off" to "request_rdp_on" and "request_rdp_off". All the corresponding code including xen-api, xcp-idl and xenopsd should be updated.
2. Change "vm_lacks_feature_ts" to "vm_lacks_feature_rdp" in api_errors.ml and corresponding code.
3. Remove "check_power_state" in xapi_xenops.ml.
4. About Dave's comment that changing the APIs from "VM." to "VM_guest_agent.", does it mean the APIs should be moved out from VM scope to VM_guest_agent (Is there any existing API in VM_guest_agent? I didn't find.)

Could you please correct me if I miss or misunderstand anything?
Thanks a lot.

Update: Just uploaded the code here to address comments 1 ~ 3.

[huizh]

Thanks all for the comments and suggestions.
Based on the decision made with
xapi-project/xapi-project.github.io#12 (comment),
here I update the code to implement.

Could you please help review it?
Much appreciate.

[thomassa]

What is the point of this? Either

• ignore args completely, or
• raise an exception if args <> [] (but this only makes sense after checking that fn is valid)

[huizh]

Thanks.
I should have added a comment in code that args might be used in future and here it is just an example.
Would remove this evaluation statement if it's not necessary.

[thomassa]

Looking good, but there are a few small changes to make.

datamodel.ml is in an odd state: the request_rdp items are still defined, but no longer listed in the messages of VM so I am not sure whether or in what form they will end up in the autogenerated documentation. Can you remove them from the datamodel altogether?

With those function specifications gone from the public API, it probably also makes sense to remove the datamodel entry for Errors.vm_lacks_feature_rdp, then catch that exception in the call_plugin implementation in ocaml/xapi/xapi_vm.ml and handle it as if the function name had not been recognised by the plugin (i.e. the plugin doesn't have the function).

[huizh]

Thanks Thomas.
Yes, for the datamodel.ml, request_rdp items should be deleted. Also I would remove Errors.vm_lacks_feature_rdp item and handle it in call_plugin correspondingly.

[huizh]

Thanks @thomassa