Bug 1439879 - [RFE] Provide generic way for looking up services in SDKv4
Summary: [RFE] Provide generic way for looking up services in SDKv4
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: ovirt-engine-sdk-python
Classification: oVirt
Component: Core
Version: 4.1.3
Hardware: Unspecified
OS: Unspecified
unspecified
medium
Target Milestone: ---
: ---
Assignee: Ondra Machacek
QA Contact: Pavel Stehlik
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-04-06 18:15 UTC by Fabrice Bacchella
Modified: 2017-04-07 10:48 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-04-07 10:48:12 UTC
oVirt Team: Infra
Embargoed:
rule-engine: planning_ack?
rule-engine: devel_ack?
rule-engine: testing_ack?


Attachments (Terms of Use)

Description Fabrice Bacchella 2017-04-06 18:15:49 UTC
The python sdk is a very thin wrapper around the HTTP request. It misses a lot of helpfull code, it's rough and some choice are strange.

Imagine that I want to write a generic search that will take a service name.

A simple signature will be :

def search(cnx, service_name, filter)

Service will be a string like "vm", "host".
filter will be a dict like {'name': 'MyVM)

That's a very simple and common case. But that needs a look of code writing:

    service = getattr(cnx.system_service(), service_name + "_service" + )()
    res = self.service.list(
                search=' and '.join('{}={}'.format(k, v) for k, v in filter.items())
            )
    (how to check that res is empty, is that None or [] ?)
    type = res[0].id
    return getattr(service, service_name + "_service" + )(type.id)

It's not my job to right such simple, generic and brittle code ? What will happen if one day a service function is not called <service_name>_service ? Are all corner case managed ? That should be managed by your unit test, that will help every one, not just me and lower the entry ticket for would be contributor.

Comment 1 Juan Hernández 2017-04-07 08:25:23 UTC
Note that there is already a generic way to locate a service, assuming that you know what is the "id" of that service:

  service = connection.service("vms")

I don't recommend that, as it makes code obscure.

Note also that there is no such thing as the "id" of a service. The closest thing to that is the "path", and that is what the "connection.service" methods uses as argument.

For example, it can't be safely assumed that there is only one service that returns virtual machines, as there are many. Virtual machines are returned by the top level "vms" service, but also by the export domain service, and by unattached storage domains (for unregistered virtual machines).

It can't either be safely assumed that the identifiers of objects provided by different services have unique ids. For example, it isn't safe to assume that there isn't a host that has the same id than a VM. That holds true currently, but it is an internal implementation detail of the engine, which may change in the future.

So if you have a method like this:

  obj = search('vms', '123')

What would that mean? Get the live virtual machine '123'? Get the virtual machine '123' from export storage domain '456'? Get the unregistered virtual machine '123' from the storage domain '789'?

How would the documentation for that method look like? Would it explain how search works for all kinds of objects?

How would that method work for services that don't support the "search" parameter?

To solve that ambiguity the "connection.service" method uses a path:

  connection.service("vms/123")
  connection.service("storagedomains/456/vms/123")
  connection.service("storagedomains/788/vms/123")

However, as I said, in general I don't recommend doing that, because that means that you need to build that string, and it makes code obscure. For example, what is this code doing?

  path = some_function_that_builds_the_path()
  service = connection.service(path)

The answer is that you can't tell if it is handling a VM, or a host, or something that jut doesn't exist. On the other hand, if you use the recommended approach:

  service = connection.system_service() \
      .vms_service() \
      .vm_service(vm_id)

Then it is crystal clear (and more verbose) what you intend.

Regarding "search", take into account that the syntax of the query language is not a set of key value pairs, it is much more complicated than that. If the simple joining of key value pairs works for some use cases, go for it, but that simple code doesn't belong in the SDK.

The "list" and "get" methods never return None. The "list" methods returns a list, maybe empty, or raises an exception. The "get" methods return the requested object, or raise an exception.

Comment 2 Fabrice Bacchella 2017-04-07 09:20:11 UTC
With you example:

  service = connection.system_service() \
      .vms_service() \
      .vm_service(vm_id)

I need some different code for every service

  service = connection.system_service() \
      .vms_service() \
      .vm_service(vm_id)

  service = connection.system_service() \
      .hosts_service() \
      .host_service(host_id)

That's non-sense

I don't say that should be the only way to find service but please at least provide :

  service = connection.system_service() \
      . service_by_name("vms") \
      .by_id(vm_id)

  service = connection.system_service() \
      .service_by_name("hosts") \
      . by_id(host_id)

For every service where connection.system_service().x_service() exists, connection.system_service() \
      .service_by_name("x") should work.

Call it top_service_by_name if you prefer.

There is a lot of service where a simple search("service", {'name': 'somename'}) can work. Why it's up to every single sdk-user to write such code ? That the whole point of a sdk, and throw an NonSupportedException if it's not possible, that's a very pythonic way to do. Search don't need to be exhaustive, it's just an helper function, with explicit contract to end users. Like it can only works for such or such object, given that or that conditions.

Nice to see that 

  service = connection.service("vms")

exists. Too bad there is not a real documentation to find about that. And no, it's not obscure, much less than

    service = getattr(cnx.system_service(), service_name + "_service" + )()

that don't provide any hint about what it does.

And there is no ambiguity with obj = search('vms', {'id:' '123'}). There is no context so it looks in all the vms. Any real doc could also lift any doubt for end user about that. And a decent api could generate meaninful search:

   obj = search('vms', {'id:' '123', 'storagedomains': 456})

Once again, as that would be written by ovirt's developers that would be implemented in the most efficient way. For that code, I'm reduced to guess work, as I can't find any really helpful documentation about the python SDK. Please don't send me to the rest API document , it's just confusing for a PYTHON developper. If i'm using python, it's not to wonder about json vs xml wire format or HTTP header, that's just a wast of time for me.

Comment 3 Juan Hernández 2017-04-07 10:00:27 UTC
You need different code for every service because every service has different functionality, different parameters, different return types. Most users need to do specific tasks, not generic ones, so that is completely OK and better for them. In your case, you want to build a generic tool on top of the SDK, so the burden to solve that complexity is on you. It makes all the sense, in my opinion.

As I said you can use this, if you want:

  connection.service("vms")

That works today, and it is documented, like the rest of the SDK:

  http://ovirt.github.io/ovirt-engine-sdk/master/#ovirtsdk4.Connection.service

I don't believe that service("vms") is better than vms_service(). The first one can't be documented to explain what it does specifically for virtual machines. The second can (look for 'def list'):

  http://ovirt.github.io/ovirt-engine-sdk/master/services.m.html#ovirtsdk4.services.VmsService

It may be better for users that want to write generic code, but not for users that want to write scripts that do specific things.

The objective of the SDK is to offer the same that the API offers, without the burden of the details of the HTTP and XML handling. Nothing less, and nothing more. Anything on top of that doesn't belong in the SDK, but in your scripts.

You can't be proficient with the SDK if you don't understand the API concepts:

  http://ovirt.github.io/ovirt-engine-api-model/master

The reference documentation of the SDK is available here:

  http://ovirt.github.io/ovirt-engine-sdk/master

Comment 4 Fabrice Bacchella 2017-04-07 10:19:30 UTC
Hoo ! really nice to see this URL finally published. I asked it on IRC, the mailing list, google, and no one talked about it. Nice to get if finally.

I need different code, but if I can reduce the number of almost identical written lines it will be very helpfull. I want to wrap types and services with python object. For all them, I need the strange and barely readable construct
    service = getattr(cnx.system_service(), service_name)()
, with service_name being a class attribute. That's why service("vms") is so much better than vms_service(). It does the same thing but in a more generic way, so no need to give special documentation, just for each <service_name>_service, provide the generic name to use in the service_by_name() method.

The very purpose of a sdk is too help users right generic and specific code, why should I be a second class citizen because I want to provide generic code ?

Comment 5 Juan Hernández 2017-04-07 10:45:24 UTC
The documentation has been there, since day one, it is in the code of the SDK itself.

The SDK is intended to help people that writes programs to do specific things. We are not going to sacrifice that to simplify things for other kinds of use cases. Someone has to bee the second class citizen.

If after studying it for a while you still feel like a second class citizen, I'd suggest you reconsider using the SDK, as it is going to be an obstacle more than an aid.

Comment 6 Fabrice Bacchella 2017-04-07 10:48:12 UTC
I give up, you refuse to provide a decent SDK.


Note You need to log in before you can comment on or make changes to this bug.