Exoscale Compute API support

[falzm]

Thank you for the initial review! 🙏

Only looked at the compute part so far (not the tests), looks great! Some more generic comments:

* There's a number of `get_something()` methods that return lists, would it make sense to call them `list_something()` instead and keep `get_thing()` only when a single item is returned?

Actually I started by implementing both get_* and list_*, then I realized some CS calls don't have a single get form and so it started to impair API coherence, so I went out an standardized on only get_* for both list and single item retrieval – it has the additional benefit of having less code to write/test. I admit that this is not perfect, but I've weight in favor of consistency rather than convenience: let me know if you prefer that I re-introduce split get/list calls.

* Is the goal of catching `CloudStackApiException` and re-raising as `APIException` to return friendlier messages? The risk is to lose information about where the exception initially happened. It'd be worth retaining the initial exception info by doing `raise APIException(…) from e`. The  CloudStackException then stays in the newly raise exception's `__cause__` attribute.

More generally the goal of my implementation was to hide CS as much as possible in order to provide a high-level user API that didn't suffer from CS's consistency issues. I've created this exception because as a developer I would find weird to see CloudStack related exception pop up...

* the retry/backoff mechanism you have in the base session looks nice, it'd be worth investigating whether it can be plugged in `cs` for all `list*` calls.

Since the mechanism is intended for the Requests library which is also used by cs, it should be doable :)

[brutasse]

Actually I started by implementing both get_* and list_*, then I realized some CS calls don't have a single get form and so it started to impair API coherence, so I went out an standardized on only get_* for both list and single item retrieval – it has the additional benefit of having less code to write/test. I admit that this is not perfect, but I've weight in favor of consistency rather than convenience: let me know if you prefer that I re-introduce split get/list calls.

Indeed, with the CloudStack API the notion of get doesn't really exist, but the API can return empty lists or "entity does not exist" errors depending on the nature of the argument. E.g. listVirtualMachines state=Starting v.s. listVirtualMachines id=…. In a future API, the distinction between lists and individual gets would be worth having. Since the goal of this is to ease the transition from current API to a new one, I would tend to say it's worth splitting get_* and list_* here, to make the CloudStack API an implementation detail.

More generally the goal of my implementation was to hide CS as much as possible in order to provide a high-level user API that didn't suffer from CS's consistency issues. I've created this exception because as a developer I would find weird to see CloudStack related exception pop up...

OK, makes sense. It's worth re-raising with from e, it provides a convenient way to access the underlying CloudStackApiException if needed.

[falzm]

Indeed, with the CloudStack API the notion of get doesn't really exist, but the API can return empty lists or "entity does not exist" errors depending on the nature of the argument. E.g. listVirtualMachines state=Starting v.s. listVirtualMachines id=…. In a future API, the distinction between lists and individual gets would be worth having. Since the goal of this is to ease the transition from current API to a new one, I would tend to say it's worth splitting get_* and list_* here, to make the CloudStack API an implementation detail.

I don't mind re-introducing split get_*/list_*methods, but I'd prefer them to be consistent regarding the whole library API: do we agree that get_* methods should only return a single object and list_* should return a list of items? Ideally I'd like the get_* methods to have a common signature, however some CS resource don't have unique ID matching (e.g. listSSHKeyPairs which can match a unique name attribute) and some others such as listVirtualMachines that can accept a unique ID but the name attribute can return multiple results...

OK, makes sense. It's worth re-raising with from e, it provides a convenient way to access the underlying CloudStackApiException if needed.

OK I'll look into it.

[brutasse]

do we agree that get_* methods should only return a single object and list_* should return a list of items?

Yes, that's the idea. The inconsistencies and inconveniences of the API should be abstracted by the library, and ideally with a better API these abstractions are easier or not needed anymore :)

E.g. for an SSH key as you mention the identifier is the name. But doing listSSHKeyPairs with a name that doesn't exist returns an empty list, which differs from listing by ids where the API returns errors :disappointed:

I don't know how e.g. egoscale manages that complexity but I think it's worth doing if we want to provide stable code APIs while evolving the HTTP API.

[falzm]

do we agree that get_* methods should only return a single object and list_* should return a list of items?

Yes, that's the idea. The inconsistencies and inconveniences of the API should be abstracted by the library, and ideally with a better API these abstractions are easier or not needed anymore :)

Deal 👍

E.g. for an SSH key as you mention the identifier is the name. But doing listSSHKeyPairs with a name that doesn't exist returns an empty list, which differs from listing by ids where the API returns errors 😞

Yes... I'll do my best to level the library methods input/output.

I don't know how e.g. egoscale manages that complexity but I think it's worth doing if we want to provide stable code APIs while evolving the HTTP API.

Egoscale doesn't provide Get* calls, only List* ¯\(ツ)/¯ https://godoc.org/github.com/exoscale/egoscale

[falzm]

get_*/list_* methods split in f019f01.

[falzm]

@brutasse I think I've addressed most of your suggestions, let me know if I've missed something.

[tgrondier]

This is a lot of code and I've been putting off reviewing it seriously for a while; i'll set up a few hours tomorrow to do it properly

[falzm]

This is a lot of code and I've been putting off reviewing it seriously for a while; i'll set up a few hours tomorrow to do it properly

I know this kind of PR are horrible, there's no good way to thoroughly review such a large amount of code... Thank you in advance for your time! 🙏

[falzm]

Except a few nitpicks here and there it looks ok to me, good work !

Thanks! 😊

I've had troubles playing around with the storage part but it most likely is a layer 8 problem.

I also don't think the docstring should say we return lists when we actually (almost ?) always return generators.

Yes this is something I've been wondering about, and IMHO we should change them to reflect the actual generator type but I don't know exactly how to represent the actual type in a docstring (if you have suggestions feel free)

[tgrondier]

Yes this is something I've been wondering about, and IMHO we should change them to reflect the actual generator type but I don't know exactly how to represent the actual type in a docstring (if you have suggestions feel free)

I think it should simply be something like:

    @property
    def snapshots(self):
        """
        Snapshots of the instance storage volume.

        Yields:
            InstanceVolumeSnapshot: The next snapshot of this instance
        """ 

[falzm]

Yes this is something I've been wondering about, and IMHO we should change them to reflect the actual generator type but I don't know exactly how to represent the actual type in a docstring (if you have suggestions feel free)

I think it should simply be something like:

    @property
    def snapshots(self):
        """
        Snapshots of the instance storage volume.

        Yields:
            InstanceVolumeSnapshot: The next snapshot of this Instance
        """ 

Right thanks, I didn't see that there is a Yield keyword: I'll update the docstrings :)

[falzm]

I'm starting to have second thoughts regarding {ElasticIP,PrivateNetwork}.(at|de)tach_instances() methods, which accept a list of objects: maybe accepting a single object would be safer and more consistent with the rest of the lib's methods, WDYT @brutasse @jgrondier?

[falzm]

Generator functions docstrings fixed in 740ec8e.

[falzm]

Working on the egoscale rewrite and since Go is a strongly typed language compared to Python, returning a map[string]*[]SecurityGroupRule value for SecurityGroup.Rules() was really ugly so I went to implement the method as 2 SecurityGroup.IngressRules() and SecurityGroup.EgressRules() methods, each returning a []*SecurityGroup value.

All this to say that looking back at the Python implementation I realize we should do the same and split the SecurityGroup.rules property in 2 (SecurityGroup.ingress_rules and SecurityGroup.egress_rules), IMHO is feel more appropriate. WDYT @brutasse?

[falzm]

Also having second thoughts about the SecurityGroup.add_rules() method: maybe renaming the method add_rule() and accept a single object instead of a list is safer.

[brutasse]

@falzm yeah, splitting ingress and egress makes sense.

:+1: on add_rule(), we can always add add_rules() later on if it's really needed.

[falzm]

@falzm yeah, splitting ingress and egress makes sense.

👍 on add_rule(), we can always add add_rules() later on if it's really needed.

Thank you for your feedback @brutasse, I've made the changes (also on {ElasticIP,PrivateNetwork}.(at|de)tach_instance() methods.

[falzm]

Looks great! It's a solid basis for an initial release & we can start trying it out. Some comments/ideas for later:

* given the 1-1 instance-volume relationship, perhaps it'd be worth abstracting out the notion of volumes and provide volume attrs and actions (resize) directly on the instance class

I went with this implementation that may look like over-engineering at first look to be future-proof for the day we'll roll out additional Compute storage volumes, but if you feel it's not worth it then I'll simplify it for the InstanceVolume class methods to be applied directly on the Instance class.

* the properties doing API calls may lead to hard-to-investigate slownesses in client code when abused, we'll have to see if methods are more appropriate.

Good point. Maybe I should document the fact that those properties perform API calls and thus latency may be induced? I personally find this property-based design quite clean, it would be a shame to move all the calls to methods like in Go... 😳

* async actions (e.g. snapshot / register template / deploy instance) are blocking-only, it might be worth exposing an optional parameter to have the call return an async job instead of waiting for the response.

I understand: in order to keep the lib API consistent should we make all "write" methods available asynchronously? Should we return the CS job ID and expose a polling method?

* a sane retry policy would be nice to abstract here 😎

I've implemented one in the low-level API HTTP client, but in practice it isn't used by most of the API calls since we rely on cs to perform API calls: we could modify cs to accept an user-provided requests Session or HTTPAdapter class and pass the python-exoscale's one to the cs.CloudStack class constructor instance during init, WDYT?

Again, awesome work @falzm 🍾

Thank you! 😬

[brutasse]

* the properties doing API calls may lead to hard-to-investigate slownesses in client code when abused, we'll have to see if methods are more appropriate.

Good point. Maybe I should document the fact that those properties perform API calls and thus latency may be induced? I personally find this property-based design quite clean, it would be a shame to move all the calls to methods like in Go...

It's true that the notation is light and convenient. Documenting that they hit the network would be nice indeed.

* async actions (e.g. snapshot / register template / deploy instance) are blocking-only, it might be worth exposing an optional parameter to have the call return an async job instead of waiting for the response.

I understand: in order to keep the lib API consistent should we make all "write" methods available asynchronously? Should we return the CS job ID and expose a polling method?

Yeah I think it should be allowed to call async methods in a fire-and-forget way. Maybe for the beginning, returning the job ID would be enough? Tracking the job becomes lower-level, and cs exposes a _jobresult() method that allows tracking a job.

* a sane retry policy would be nice to abstract here 😎

I've implemented one in the low-level API HTTP client, but in practice it isn't used by most of the API calls since we rely on cs to perform API calls: we could modify cs to accept an user-provided requests Session or HTTPAdapter class and pass the python-exoscale's one to the cs.CloudStack class constructor instance during init, WDYT?

Yep, good idea -- passing a sessions sounds like a nice way to achieve that.

[falzm]

• given the 1-1 instance-volume relationship, perhaps it'd be worth abstracting out the notion of volumes and provide volume attrs and actions (resize) directly on the instance class

Changed in 4ab0f9591b09bcd31a078f6f1d8784f5611c7bb6.

[falzm]

It's true that the notation is light and convenient. Documenting that they hit the network would be nice indeed.

Added in d8d46c5936a5684bebc4fc10ffcce7e81245c2d3.

[falzm]

• a sane retry policy would be nice to abstract here 😎

I've implemented one in the low-level API HTTP client, but in practice it isn't used by most of the API calls since we rely on cs to perform API calls: we could modify cs to accept an user-provided requests Session or HTTPAdapter class and pass the python-exoscale's one to the cs.CloudStack class constructor instance during init, WDYT?

Yep, good idea -- passing a sessions sounds like a nice way to achieve that.

There is a PR in cs in order to implement this: https://github.com/exoscale/cs/pull/107 Once it's merged I'll publish a new cs release.

[falzm]

@brutasse HTTP session retry policy implemented in 0457ba0.

[falzm]

Looks like we're about ready to inaugurate the first release. Shall I merge the dev branch to master? 😬

[brutasse]

@falzm go for it! :shipit: