CP-20676: Add gpumon interface #151

Frezzle · 2017-04-11T10:27:54Z

No description provided.

coveralls · 2017-04-11T10:40:31Z

Coverage remained the same at 47.16% when pulling 224cb70 on Frezzle:gpumon into c41d9e9 on xapi-project:master.

robhoes · 2017-04-11T10:58:11Z

gpumon/gpumon_interface.ml

+type vgpu_compatibility = unit (* temporarily unit, until real type is determined *)
+
+external get_pgpu_metadata: debug_info -> pgpu_address -> pgpu_metadata = ""
+external get_pgpu_vm_compatibility: debug_info -> pgpu_metadata -> dom_id -> vgpu_compatibility list = ""


I think that we need the address of the current pGPU as well (@mseri).

For the return type, we essentially just need a Boolean, which give the answer to the question "can this VM be live migrated to this pGPU", plus a reason in case the answer is "no". Possible reasons are "host driver", "guest driver", "gpu", "other", or a combination of those. How about this type:

type incompatibility_reason = Host_driver | Guest_driver | GPU | Other type compatibility = Compatible | Incompatible of incompatibility_reason list

I have extended the bindings in a way that may require some changes here. To get the pGPU handle, and then the metadata, you will need the pci.bus_id (a string) as pgpu_address.

Compatibility here can be any type useful to xapi, we can always reshape it inside gpumon when calling the idl to expose the correct informations. Seems good to simplify it as suggested by Rob

For specifying the PCI device, I had imagined we might be able to use the xapi database's (misleadingly-named) pci_id field of a pci db entry, provided by xapi with code like Db.PCI.get_pci_id ~__context ~self:pci so it would be of format "%04x:%02x:%02x.%01x"
See http://wiki.xen.org/wiki/Bus:Device.Function_%28BDF%29_Notation
and/or xen-api.git/ocaml/xapi/pciops.ml

I misunderstood the comment there. We do need the pgpu_address only to get the metadata. Fr the compatibility we need the current vgpu (we get it using the domid) and the destination pgpu metadata

Right, so:

external get_pgpu_vm_compatibility: debug_info -> pgpu_address -> pgpu_metadata -> dom_id -> compatibility

...where pgpu_address is for the pGPU that is currently in use, and pgpu_metadata is for the one we want to migrate the VM to. We can add labels or a comment to clarify that.

Ok, yes, I see what you mean. Indeed, we need the current pgpu and domid to get the current vgpu metadata, and we need the destination pgpu metadata.

Oh, I thought we could get the metadata about the VM's (current) vGPUs from the domid without needing to specify which pgpu (or pgpus) the VM is currently using.

@thomassa Rob told me we can but it's not as straight-forward as we like (IIRC it involves looping through the pGPUs to check which one we're looking for), so having it here makes it simpler.

robhoes · 2017-04-11T11:03:10Z

In any case, this needs to go onto a feature branch.

robhoes · 2017-04-11T12:37:07Z

gpumon/gpumon_interface.ml

+
+type pgpu_address = string
+type pgpu_metadata = string
+type dom_id = int


This is called domid in other places.

Note that domid is a string for the bindings and the underlying library.

We can translate it to a string inside gpumon where needed.

Right, I felt int was better for this interface, so that the translation to string will be done in gpumon rather than in xapi.

lindig · 2017-04-12T08:01:25Z

gpumon/gpumon_client.ml

+open Gpumon_interface
+open Xcp_client
+
+let json_url () = "file:" ^ json_path


This doesn't look like a file URL. I expected to see "file://"^json_path. https://en.wikipedia.org/wiki/File_URI_scheme#Unix

json_path is set in gpumon/gpumon_interface.ml:
let json_path = "/var/xapi/" ^ service_name ^ ".json"
so that json_url ends up as file:/var/xapi/gpu.json (as a side note, i'm changing the service_name to gpumon instead of gpu). I'm not sure if file:/... also works as the recommended file://..., but memory/memory_interface.ml does the same thing, and my assumption was that it worked.

Trying it out at http://www.freeformatter.com/url-parser-query-string-splitter.html
suggests that it is accepted. I still find it dubious but would have to read rules for URLs again whether this is syntactically valid.

Wikipedia says:

The // after the file: is part of the general syntax of URLs. (The double slash // should always appear in a file URL according to the specification, but in practice many Web browsers allow it to be omitted).

In that case, seeing as we seem to have had no problem with the memory interface (AFAIK) then maybe it's better to conform to what we're already doing?

I'd argue the other way round: do the right thing and check that it works. If not, do the wrong thing but document it.

The xenops interface appears to do the same (uses file:/...).

Edit: All the other interfaces also do the same.

I could have a separate commit to change them all to use // but I see no reason to if it works?

@jonludlam, @robhoes we need your opinion, here

First of all, I don't see json_url used anywhere, so it looks like dead code that should just be removed.

However, the same question applies to xml_url below. The place where this is used is in the function http_rpc in lib/xcp_client.ml, which passes it to Uri.of_string, which is provided by a library. I assume that this is the format that is accepted by this library, because indeed we use this format everywhere. For this PR, IMO, we should just comply with the other interfaces in xcp-idl, which is what the current patch does.

mseri · 2017-04-12T11:04:45Z

gpumon/gpumon_interface.ml

+ * GNU Lesser General Public License for more details.
+ *)
+
+let service_name = "gpu"


The client is called "gpumon" (if that is what ~dststr:"gpumon" means) but the service here is "gpu", is this correct?

You're right; I'm changing it to gpumon now. (we previously had everything as gpu and forgot to change this one)

mseri · 2017-04-12T11:05:19Z

gpumon/gpumon_interface.ml

+let service_name = "gpu"
+let queue_name = Xcp_service.common_prefix ^ service_name
+let json_path = "/var/xapi/" ^ service_name ^ ".json"
+let xml_path = "/var/xapi/" ^ service_name


As for the client, do we use these paths anywhere?

xml_path is used by the xml_url function in gpumon/gpumon_client.ml, which is used in the xml_http_rpc call below that in the same file. json_path, as @robhoes pointed out above, isn't used, so I'm removing it.

Frezzle · 2017-04-12T11:32:33Z

^ New commit with all discussed changes.

coveralls · 2017-04-12T11:44:20Z

Coverage remained the same at 47.16% when pulling acd8554 on Frezzle:gpumon into c41d9e9 on xapi-project:vgpu-migration.

robhoes · 2017-04-12T12:13:30Z

Looks good now. Please squash.

mseri · 2017-04-12T12:30:59Z

gpumon/gpumon_interface.ml

+type incompatibility_reason = Host_driver | Guest_driver | GPU | Other
+type compatibility = Compatible | Incompatible of incompatibility_reason list
+
+(* Get the metadata for a pGPU, given its address (PCI bus ID). *)


Shouldn't this be (** text *) so that it is read as a docstring?

Done in new commit!

mseri · 2017-04-12T12:31:52Z

gpumon/gpumon_interface.ml

+(* Get the metadata for a pGPU, given its address (PCI bus ID). *)
+external get_pgpu_metadata: debug_info -> pgpu_address -> pgpu_metadata = ""
+
+(* Check compatibility between a VM's vGPU(s) and another pGPU. *)


As above, I think this whole block of comments should be in

(** text ... ... ... *)

to be interpreted as a docstring

Changed it.

Commit 1 of 2 for new gpumon interface. Signed-off-by: Frederico Mazzone <frederico.mazzone@citrix.com>

Commit 2 of 2 for new gpumon interface. Signed-off-by: Frederico Mazzone <frederico.mazzone@citrix.com>

coveralls · 2017-04-12T13:08:28Z

Coverage remained the same at 47.16% when pulling a1942f2 on Frezzle:gpumon into c41d9e9 on xapi-project:vgpu-migration.

Frezzle · 2017-04-12T13:10:35Z

Squashed!

coveralls · 2017-04-12T13:22:12Z

Coverage remained the same at 47.16% when pulling 1e9d016 on Frezzle:gpumon into c41d9e9 on xapi-project:vgpu-migration.

robhoes reviewed Apr 11, 2017

View reviewed changes

Frezzle changed the base branch from master to vgpu April 11, 2017 11:15

Frezzle changed the base branch from vgpu to vgpu-migration April 11, 2017 11:18

robhoes reviewed Apr 11, 2017

View reviewed changes

lindig reviewed Apr 12, 2017

View reviewed changes

mseri reviewed Apr 12, 2017

View reviewed changes

robhoes approved these changes Apr 12, 2017

View reviewed changes

mseri reviewed Apr 12, 2017

View reviewed changes

Frezzle added 2 commits April 12, 2017 14:07

Add gpumon interface

0a52fd3

Commit 1 of 2 for new gpumon interface. Signed-off-by: Frederico Mazzone <frederico.mazzone@citrix.com>

Update OASIS for gpumon interface

1e9d016

Commit 2 of 2 for new gpumon interface. Signed-off-by: Frederico Mazzone <frederico.mazzone@citrix.com>

robhoes merged commit 5bcf130 into xapi-project:vgpu-migration Apr 12, 2017

CP-20676: Add gpumon interface #151

CP-20676: Add gpumon interface #151

Conversation

Frezzle commented Apr 11, 2017

coveralls commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mseri Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

thomassa Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robhoes Apr 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robhoes commented Apr 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frezzle Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mseri Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Frezzle Apr 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frezzle commented Apr 12, 2017

coveralls commented Apr 12, 2017

robhoes commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 12, 2017

Frezzle commented Apr 12, 2017

coveralls commented Apr 12, 2017

mseri Apr 11, 2017 •

edited

Loading

thomassa Apr 11, 2017 •

edited

Loading

robhoes Apr 11, 2017 •

edited

Loading

Frezzle Apr 12, 2017 •

edited

Loading

mseri Apr 12, 2017 •

edited

Loading

Frezzle Apr 12, 2017 •

edited

Loading