Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added minimal cluster support #331

Merged
merged 15 commits into from
Dec 12, 2018
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CONTRIBUTORS.rst
Original file line number Diff line number Diff line change
Expand Up @@ -34,5 +34,6 @@ These are the contributors to pylxd according to the Github repository.
chrismacnaughton Chris MacNaughton
ppkt Karol Werner
mrtc0 Kohei Morita
felix-engelmann Felix Engelmann
=============== ==================================

41 changes: 41 additions & 0 deletions integration/test_cluster_members.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# Copyright (c) 2016 Canonical Ltd
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.

from integration.testing import IntegrationTestCase


class ClusterMemberTestCase(IntegrationTestCase):

def setUp(self):
super(ClusterMemberTestCase, self).setUp()

if not self.client.has_api_extension('clustering'):
self.skipTest('Required LXD API extension not available!')


class TestClusterMembers(ClusterMemberTestCase):
"""Tests for `Client.cluster_members.`"""

def test_get(self):
"""A cluster member is fetched by its name."""

members = self.client.cluster_members.all()

random_member_name = "%s" % members[0].name
random_member_url = "%s" % members[0].url

member = self.client.cluster_members.get(random_member_name)

new_url = "%s" % member.url
self.assertEqual(random_member_url, new_url)
13 changes: 12 additions & 1 deletion pylxd/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,8 @@ def __getattr__(self, name):
# Special case for storage_pools which needs to become 'storage-pools'
if name == 'storage_pools':
name = 'storage-pools'
if name == 'cluster_members':
name = 'cluster/members'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@felix-engelmann felix-engelmann Dec 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return self.__class__('{}/{}'.format(self._api_endpoint, name),
cert=self.session.cert,
verify=self.session.verify)
Expand Down Expand Up @@ -151,7 +153,15 @@ def get(self, *args, **kwargs):
def post(self, *args, **kwargs):
"""Perform an HTTP POST."""
kwargs['timeout'] = kwargs.get('timeout', self._timeout)
response = self.session.post(self._api_endpoint, *args, **kwargs)
target = kwargs.get('target', None)
kwargs.pop("target", None)
felix-engelmann marked this conversation as resolved.
Show resolved Hide resolved

if target is not None:
endpoint = "{}?target={}".format(self._api_endpoint, target)
felix-engelmann marked this conversation as resolved.
Show resolved Hide resolved
else:
endpoint = self._api_endpoint

response = self.session.post(endpoint, *args, **kwargs)
# Prior to LXD 2.0.3, successful synchronous requests returned 200,
# rather than 201.
self._assert_response(response, allowed_status_codes=(200, 201, 202))
Expand Down Expand Up @@ -296,6 +306,7 @@ def __init__(
requests.exceptions.InvalidURL):
raise exceptions.ClientConnectionFailed()

self.cluster_members = managers.ClusterMemberManager(self)
self.certificates = managers.CertificateManager(self)
self.containers = managers.ContainerManager(self)
self.images = managers.ImageManager(self)
Expand Down
4 changes: 4 additions & 0 deletions pylxd/managers.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ def __init__(self, *args, **kwargs):
return super(BaseManager, self).__init__()


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


class CertificateManager(BaseManager):
manager_for = 'pylxd.models.Certificate'

Expand Down
1 change: 1 addition & 0 deletions pylxd/models/__init__.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from pylxd.models.cluster_member import ClusterMember # NOQA
from pylxd.models.certificate import Certificate # NOQA
from pylxd.models.container import Container, Snapshot # NOQA
from pylxd.models.image import Image # NOQA
Expand Down
48 changes: 48 additions & 0 deletions pylxd/models/cluster_member.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# Copyright (c) 2016 Canonical Ltd
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
from pylxd.models import _model as model


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

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)

@classmethod
def get(cls, client, name):
"""Get a certificate by fingerprint."""
response = client.api.cluster_members[name].get()

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

@classmethod
def all(cls, client):
"""Get all certificates."""
response = client.api.cluster_members.get()

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

@property
def api(self):
return self.client.api.cluster_members[self.name]
4 changes: 2 additions & 2 deletions pylxd/models/container.py
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,9 @@ def all(cls, client):
return containers

@classmethod
def create(cls, client, config, wait=False):
def create(cls, client, config, wait=False, target=None):
"""Create a new container config."""
response = client.api.containers.post(json=config)
response = client.api.containers.post(json=config, target=target)

if wait:
client.operations.wait_for_operation(response.json()['operation'])
Expand Down
53 changes: 53 additions & 0 deletions pylxd/tests/mock_lxd.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,31 @@ def snapshot_DELETE(request, context):
},


# Cluster Members
{
'text': json.dumps({
'type': 'sync',
'metadata': [
'http://pylxd.test/1.0/certificates/an-member',
'http://pylxd.test/1.0/certificates/nd-member',
]}),
'method': 'GET',
'url': r'^http://pylxd.test/1.0/cluster/members$',
},
{
'text': json.dumps({
'type': 'sync',
'metadata': {
"name": "an-member",
"url": "https://10.1.1.101:8443",
"database": "true",
"state": "Online",
}}),
'method': 'GET',
'url': r'^http://pylxd.test/1.0/cluster/members/an-member$', # NOQA
},


# Containers
{
'text': json.dumps({
Expand All @@ -212,6 +237,11 @@ def snapshot_DELETE(request, context):
'method': 'POST',
'url': r'^http://pylxd.test/1.0/containers$',
},
{
'text': containers_POST,
'method': 'POST',
'url': r'^http://pylxd.test/1.0/containers\?target=an-remote',
},
{
'json': {
'type': 'sync',
Expand Down Expand Up @@ -293,6 +323,29 @@ def snapshot_DELETE(request, context):
'method': 'GET',
'url': r'^http://pylxd.test/1.0/containers/an-container/state$', # NOQA
},
{
'json': {
'type': 'sync',
'metadata': {
'name': 'an-new-remote-container',

'architecture': "x86_64",
'config': {
'security.privileged': "true",
},
'created_at': "1983-06-16T00:00:00-00:00",
'last_used_at': "1983-06-16T00:00:00-00:00",
'description': "Some description",
'location': "an-remote",
'status': "Running",
'status_code': 103,
'unsupportedbypylxd': "This attribute is not supported by "\
"pylxd. We want to test whether the mere presence of it "\
"makes it crash."
}},
'method': 'GET',
'url': r'^http://pylxd.test/1.0/containers/an-new-remote-container$',
},
{
'status_code': 202,
'json': {
Expand Down
31 changes: 31 additions & 0 deletions pylxd/tests/models/test_cluster_member.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# Copyright (c) 2016 Canonical Ltd
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.

from pylxd.tests import testing


class TestClusterMember(testing.PyLXDTestCase):
"""Tests for pylxd.models.ClusterMember."""

def test_get(self):
"""A cluster member is retrieved."""
member = self.client.cluster_members.get('an-member')

self.assertEqual('https://10.1.1.101:8443', member.url)

def test_all(self):
"""All cluster members are returned."""
members = self.client.cluster_members.all()

self.assertIn('an-member', [m.name for m in members])
10 changes: 10 additions & 0 deletions pylxd/tests/models/test_container.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,16 @@ def test_create(self):

self.assertEqual(config['name'], an_new_container.name)

def test_create_remote(self):
"""A new container is created at target."""
config = {'name': 'an-new-remote-container'}

an_new_remote_container = models.Container.create(
self.client, config, wait=True, target="an-remote")

self.assertEqual(config['name'], an_new_remote_container.name)
self.assertEqual("an-remote", an_new_remote_container.location)

def test_exists(self):
"""A container exists."""
name = 'an-container'
Expand Down