Added minimal cluster support

[felix-engelmann]

Added support to query cluster members and specify a target for creation of containers in a cluster

[codecov-io]

Codecov Report

Merging #331 into master will decrease coverage by 0.04%.
The diff coverage is 96.15%.

@@            Coverage Diff            @@
##           master    #331      +/-   ##
=========================================
- Coverage   96.84%   96.8%   -0.05%     
=========================================
  Files          11      12       +1     
  Lines         919     970      +51     
  Branches      106     108       +2     
=========================================
+ Hits          890     939      +49     
- Misses         10      12       +2     
  Partials       19      19

Impacted Files	Coverage Δ
pylxd/client.py	98.69% <100%> (+0.05%)	⬆️
pylxd/managers.py	100% <100%> (ø)	⬆️
pylxd/models/container.py	91.1% <100%> (ø)	⬆️
pylxd/models/cluster.py	94.59% <94.59%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e57eb0...a22c401. Read the comment docs.

[felix-engelmann]

I have this line in the model ClusterMember

    @property
    def api(self):
        return self.client.api.cluster_members[self.name]

which is not covered by tests, I would have removed it, but it feels like it is important somewhere

[ajkavanagh]

Firstly, thanks for submitting the PR. It's great to get adding clustering support moving along. I have a minor change to the code, along with a more substantial request.

The main premise for the change appears to be to add just support for the 'cluster/members" endpoint, and ignore the "cluster" endpoint. I'm fine with that, in general, but I wonder whether there should be an intermediary Cluster manager?

[ajkavanagh]

This confused me when I first looked at it, and then I realised that the ClusterMemberManager is straddling the cluster segment of the endpoint.

This means there is no ClusterManager for cluster? I feel there should be a ClusterManager and then a ClusterMemberManager that hangs off that for members.

[felix-engelmann]

Thank you for your comments. I tried a lot to have a ClusterManager and then a hierarchical ClusterMemberManager , but as there are no multiple cluster elements and /cluster/members is a more or less independent endpoint from /cluster I argue against such a hierarchy. It would take quite some tweaking to get the ClusterMemberManager below the ClusterManager, because unlike with the nested StoragePoolManagers, there is no such thing as a /cluster/0/members endpoint.
What is your opinion on it?

[felix-engelmann]

So a ClusterMemberManager and a ClusterManager can coexist as top level managers. There is also little semantic relation.

[ajkavanagh]

It feels wrong. My rationale is to help people who are reading the REST API documents; i.e. it's the difference between

client.cluster.members.all()

# and

client.cluster_members.all()

At the moment we have a "rule" that is effectively 'replace a / with a . in python code and add all() , get() etc to the end.

TBH, I'm not a big fan of the current API format (I didn't design it, so take my criticism with a grain of salt, as it might be NIH!!), but I'm holding out to keep it consistent.

However, if it's a lot of work to make the change, then let me know here. I'm just thinking of an empty manager that can then have another manager hung off of it.

[felix-engelmann]

I agree that client.cluster.members.all() feels better. However I did not manage to write such a nested Manager, where there exists only one object.

Here is how I tried to build it with a ClusterManager:

from pylxd.models import _model as model
from pylxd import managers

class Cluster(model.Model):
    """A LXD certificate."""

    server_name = model.Attribute(readonly=True)
    enabled = model.Attribute(readonly=True)

    members = model.Manager()

    def __init__(self, *args, **kwargs):
        super(Cluster, self).__init__(*args, **kwargs)

        self.members = ClusterMemberManager(self)


    @classmethod
    def get(cls, client):
        """Get a certificate by fingerprint."""

        client.assert_has_api_extension('clustering')
        response = client.api.cluster.get()

        return cls(client, **response.json()['metadata'])

    @property
    def api(self):
        print("in api:", type( self.client.api.cluster), " other type",  type(self.client.api.cluster[0]))
        print("in api:", self.client.api.cluster._api_endpoint, " other type",  self.client.api.cluster[0]._api_endpoint)
        return self.client.api.cluster

and it's child the ClusterMemberManager:

class ClusterMemberManager(managers.BaseManager):
    manager_for = 'pylxd.models.ClusterMember'


class ClusterMember(model.Model):
    name = model.Attribute(readonly=True)
    url = model.Attribute(readonly=True)
    database = model.Attribute(readonly=True)
    state = model.Attribute(readonly=True)
    server_name = model.Attribute(readonly=True)
    status = model.Attribute(readonly=True)
    message = model.Attribute(readonly=True)

    cluster = model.Parent()

    @property
    def api(self):
        print("in member api:",type(self))
        print("in member api:",dir(self))
        print("in member api:",self.__dir__())
        print("in member api:",self.__attributes__)
        print("in member api:",self.cluster._api_endpoint)
        print("in member api[0]:",type(self.cluster[0]))
        print("in member api[0]:",self.cluster[0]._api_endpoint)
        return self.cluster.api.members[self.name]

    @classmethod
    def get(cls, cluster, name):
        """Get a certificate by fingerprint."""
        print("here")
        response = cluster.api.members[name].get()

        #return cls(cluster.client, **response.json()['metadata'])
        return cls(cluster.client, **response.json()['metadata'])

    @classmethod
    def all(cls, cluster):
        response = cluster.api.members.get()

        nodes = []
        for node in response.json()['metadata']:
            name = node.split('/')[-1]
            nodes.append(cls(cluster.client, name=name))
        return nodes

As you see there is quite a lot of debug outputs inside. I screwed up the api references somehow, because the Cluster.api should return sth like self.client.api.cluster[0] but that would query /1.0/cluster/0 which is wrong.
Do you have a quick idea on how to fix this?

[ajkavanagh]

Hmm, interesting; it's sort of been done before with the snapshots manager, but those were on a container (obviously a list of containers). But I'm not sure how to do it ... I'll have a think and get back to you. I'll grab your master and see what I can see.

[felix-engelmann]

this is not pushed, just an idea. You find my WIP at: https://github.com/felix-engelmann/pylxd/tree/nested-cluster-manager

[ajkavanagh]

I kind of know what we're trying to do; "basically, create an object to hang .members off that does-the-right-thing; I'll have a ponder at the code.

[ajkavanagh]

Hi @felix-engelmann -- so I've had a go; not sure if I like it! What are your thoughts? It's here: https://github.com/ajkavanagh/pylxd/tree/felix-ajk-alternative, commit: ajkavanagh@755185a

I ended up hacking up a custom ClusterManager to be able to have the singleton and also access members; it looks like the code architecture wasn't designed to do it!

Also, it needs a bit of work (e.g. tests, etc.) -- is it heading in a direction that you'd be happy with?

[felix-engelmann]

Thx, cool! I was forcing it too much into the existing structure, what failed. But your solution looks as it is going in the right direction. I'll pick it up from there and adapt the tests etc.

[felix-engelmann]

What's your opinion on this? There are still a few print debugs. Was there any particular reason to hide the _members behind a @property getter?

[ajkavanagh]

What's your opinion on this? There are still a few print debugs. Was there any particular reason to hide the _members behind a @property getter?

TBH I was playing around with how to get it to work, and it "became" a property as I was trying things out. No, I guess there is no need for it to be a @property getter.

Otherwise, yes, I think it is good. If you can get some tests around it we'll land it. Nice addition! 👍

[ajkavanagh]

Are you happy with this now? Tests are passing and if it does the minimum you were looking for, then it's good to merge now.

[felix-engelmann]

I added a test for the Cluster itself, so I'm happy with it now. Thanks for your cooperation. If you want to release, please bump the version, I did not.

[ajkavanagh]

Okay, thanks very much for the PR -- let's get it merged.