Add support for the instances and virtual-machines endpoints. #387

ltrager · 2020-02-20T21:16:53Z

Signed-off-by: Lee Trager lee.trager@canonical.com

codecov-io · 2020-02-20T21:18:37Z

Codecov Report

Merging #387 into master will increase coverage by 0.06%.
The diff coverage is 90.96%.

@@            Coverage Diff             @@
##           master     #387      +/-   ##
==========================================
+ Coverage   96.48%   96.54%   +0.06%     
==========================================
  Files          12       14       +2     
  Lines        1053     1072      +19     
  Branches      123      127       +4     
==========================================
+ Hits         1016     1035      +19     
  Misses         16       16              
  Partials       21       21

Impacted Files	Coverage Δ
pylxd/models/instance.py	`90.28% <90.28%> (ø)`
pylxd/client.py	`98.87% <100.00%> (+0.01%)`	⬆️
pylxd/managers.py	`100.00% <100.00%> (ø)`
pylxd/models/_model.py	`100.00% <100.00%> (ø)`
pylxd/models/container.py	`100.00% <100.00%> (+9.74%)`	⬆️
pylxd/models/storage_pool.py	`100.00% <100.00%> (ø)`
pylxd/models/virtual_machine.py	`100.00% <100.00%> (ø)`

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 691916f...9ae2587. Read the comment docs.

ajkavanagh

Thanks for the large patch; I had some concerns (see inline), but I'm totally sure they are valid. Otherwise, looks great.

ajkavanagh · 2020-02-21T15:12:13Z

pylxd/models/container.py

@@ -1,4 +1,4 @@
-# Copyright (c) 2016 Canonical Ltd
+# Copyright (c) 2020 Canonical Ltd


I think we should keep the copyright at 2016 as that's when this file was started.

This happened because I did git mv container.py instance.py and then created a new container.py. In MAAS we always update the copyright year to the last time it was touched so I've changed this to 2016-2020. Let me know if it should just be 2016.

ajkavanagh · 2020-02-21T15:31:33Z

pylxd/models/instance.py

+
+    @property
+    def api(self):
+        return getattr(self.client.api, self._endpoint)[self.name]


Couldn't this be:

self.client.api[self._endpoint][self.name]

It took me a while to wrap my head around what the code was doing.

ajkavanagh · 2020-02-21T15:33:07Z

pylxd/models/instance.py

+            self._endpoint = getattr(
+                instance.client.api, instance._endpoint)[instance.name].files


Again, is the following possible:?

self.endpoint=instance.client.api[instance._endpoint][instance.name].files

ajkavanagh · 2020-02-21T15:37:50Z

pylxd/models/instance.py

+            response = self._endpoint.post(
+                params={'path': filepath},
+                data=data,
+                headers=headers or None)
+            if response.status_code == 200:


shouldn't this be:

response = self._client.api[self._endpoint][self.instance.name].files.post(...)

I thought self._endpoint was either containers or instance?

Instace._endpoint is either instance, containers, or virutal-machines. This code is in the subclass FilesManage. FilesManager._endpoint is a reference to the actual object that should be used.

I ended up reusing the name since I couldn't think of a better one :\

ajkavanagh · 2020-02-21T15:39:44Z

pylxd/models/instance.py

+
+        def delete(self, filepath):
+            self._instance.client.assert_has_api_extension('file_delete')
+            response = self._endpoint.delete(params={'path': filepath})


This again doesn't seem right? Don't we need to access the self._client.api here?

This is in FilesManager.

ajkavanagh · 2020-02-21T15:40:08Z

pylxd/models/instance.py

+            response = self._endpoint.get(
+                params={'path': filepath}, is_api=False)


ajkavanagh · 2020-02-21T15:40:28Z

pylxd/models/instance.py

+                        response = self._endpoint.post(
+                            params={'path': filepath},


ajkavanagh · 2020-02-21T15:42:20Z

pylxd/models/instance.py

+    def exists(cls, client, name):
+        """Determine whether a instance exists."""
+        try:
+            getattr(client, cls._endpoint).get(name)


Again, this might be nicer as:

client[cls._endpoint].get(name)

I get TypeError: 'Client' object is not subscriptable if I try that

ajkavanagh · 2020-02-21T15:42:33Z

pylxd/models/instance.py

+    @classmethod
+    def get(cls, client, name):
+        """Get a instance by name."""
+        response = getattr(client.api, cls._endpoint)[name].get()


client.api is subscritable but client isn't. I removed getattr here as suggested.

ajkavanagh · 2020-02-21T15:42:52Z

pylxd/models/instance.py

+        information is needed, `Instance.sync` is the method call
+        that should be used.
+        """
+        response = getattr(client.api, cls._endpoint).get()


And here, and in all subsequent locations.

ltrager · 2020-02-21T21:42:09Z

Thanks for the review! I've updated things as suggested. I do still need to use getattr when operating on the client object, and not client.api.

ajkavanagh · 2020-02-26T20:12:08Z

but I'm totally sure they are valid.

There was supposed to be a NOT in there. :( Sorry, I sounded like a prat; It meant to say "I'm not totally sure they are valid". (so easy to miss a word). Anyway, I'll try to do the review now, again.

Signed-off-by: Lee Trager <lee.trager@canonical.com>

ajkavanagh · 2020-04-12T13:54:39Z

It's still failing a couple of integration tests that the master branch isn't (the master branch is failing the broken pipe one: #387):

On xenial:

Run 16.04 (xenial) integration tests
+ tox -e integration
integration create: /opt/pylxd/.tox/integration
integration installdeps: -r/opt/pylxd/requirements.txt, -r/opt/pylxd/test-requirements.txt
integration develop-inst: /opt/pylxd
integration installed: attrs==19.3.0,certifi==2020.4.5.1,cffi==1.14.0,chardet==3.0.4,coverage==5.0.4,cryptography==2.9,ddt==1.3.1,entrypoints==0.3,flake8==3.7.9,funcsigs==1.0.2,idna==2.9,mccabe==0.6.1,mock==3.0.5,mock-services==0.3.1,nose==1.3.7,pbr==5.4.5,pkg-resources==0.0.0,pycodestyle==2.5.0,pycparser==2.20,pyflakes==2.1.1,-e git+git@github.com:lxc/pylxd.git@e6ead2c0b41c1df376d17e5dbd17751ee3494b35#egg=pylxd,python-dateutil==2.8.1,requests==2.23.0,requests-mock==1.1.0,requests-toolbelt==0.9.1,requests-unixsocket==0.2.0,six==1.14.0,urllib3==1.25.8,ws4py==0.5.1
integration runtests: PYTHONHASHSEED='3763442740'
integration runtests: commands[0] | nosetests integration
.S.E....SSSE...E..........SSSSSSS......SSSSSSSSSS
======================================================================
ERROR: A command is executed on the container.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/pylxd/integration/test_containers.py", line 152, in test_execute
    result = self.container.execute(['echo', 'test'])
  File "/opt/pylxd/pylxd/models/instance.py", line 441, in execute
    manager.close_all()
  File "/opt/pylxd/.tox/integration/lib/python3.5/site-packages/ws4py/manager.py", line 345, in close_all
    ws.close(code=code, reason=message)
  File "/opt/pylxd/.tox/integration/lib/python3.5/site-packages/ws4py/client/__init__.py", line 205, in close
    self._write(self.stream.close(code=code, reason=reason).single(mask=True))
  File "/opt/pylxd/.tox/integration/lib/python3.5/site-packages/ws4py/websocket.py", line 285, in _write
    self.sock.sendall(b)
BrokenPipeError: [Errno 32] Broken pipe

======================================================================
ERROR: A container is published.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/pylxd/integration/test_containers.py", line 219, in test_publish
    image = self.container.publish(wait=True)
  File "/opt/pylxd/pylxd/models/instance.py", line 590, in publish
    response = self.client.api.images.post(json=data)
  File "/opt/pylxd/pylxd/client.py", line 177, in post
    self._assert_response(response, allowed_status_codes=(200, 201, 202))
  File "/opt/pylxd/pylxd/client.py", line 117, in _assert_response
    raise exceptions.LXDAPIException(response)
pylxd.exceptions.LXDAPIException: Invalid images JSON

======================================================================
ERROR: A container snapshot is made, renamed, and deleted.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/pylxd/integration/test_containers.py", line 120, in test_snapshot
    snapshot = self.container.snapshots.create(name, wait=True)
  File "/opt/pylxd/pylxd/models/instance.py", line 750, in create
    'name': name, 'stateful': stateful})
  File "/opt/pylxd/pylxd/client.py", line 177, in post
    self._assert_response(response, allowed_status_codes=(200, 201, 202))
  File "/opt/pylxd/pylxd/client.py", line 116, in _assert_response
    raise exceptions.NotFound(response)
pylxd.exceptions.NotFound: not found

----------------------------------------------------------------------
Ran 49 tests in 39.722s

FAILED (SKIP=21, errors=3)
ERROR: InvocationError: '/opt/pylxd/.tox/integration/bin/nosetests integration'
_____________________________________________________________________ summary ______________________________________________________________________
ERROR:   integration: commands failed

On bionic it's the same:

integration runtests: commands[0] | nosetests integration
........SSSE...E.........................S....SSS
======================================================================
ERROR: A container is published.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/pylxd/integration/test_containers.py", line 219, in test_publish
    image = self.container.publish(wait=True)
  File "/opt/pylxd/pylxd/models/instance.py", line 590, in publish
    response = self.client.api.images.post(json=data)
  File "/opt/pylxd/pylxd/client.py", line 177, in post
    self._assert_response(response, allowed_status_codes=(200, 201, 202))
  File "/opt/pylxd/pylxd/client.py", line 117, in _assert_response
    raise exceptions.LXDAPIException(response)
pylxd.exceptions.LXDAPIException: Invalid images JSON

======================================================================
ERROR: A container snapshot is made, renamed, and deleted.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/pylxd/integration/test_containers.py", line 120, in test_snapshot
    snapshot = self.container.snapshots.create(name, wait=True)
  File "/opt/pylxd/pylxd/models/instance.py", line 750, in create
    'name': name, 'stateful': stateful})
  File "/opt/pylxd/pylxd/client.py", line 177, in post
    self._assert_response(response, allowed_status_codes=(200, 201, 202))
  File "/opt/pylxd/pylxd/client.py", line 116, in _assert_response
    raise exceptions.NotFound(response)
pylxd.exceptions.NotFound: not found

----------------------------------------------------------------------
Ran 49 tests in 57.820s

FAILED (SKIP=7, errors=2)
ERROR: InvocationError: '/opt/pylxd/.tox/integration/bin/nosetests integration'
_____________________________________________________________________ summary ______________________________________________________________________
ERROR:   integration: commands failed

Bionic passes on master.

The first fail is on master, but I think the other two need resolving before we can land this please?

Signed-off-by: Lee Trager <lee.trager@canonical.com>

ltrager · 2020-04-18T00:42:36Z

Thanks for looking into that. I updated #379 with the three tests that are failing for me with master on xenial. I've updated this pull request to fix the failing integration tests on Bionic.

ajkavanagh · 2020-04-23T13:04:44Z

Great work in sorting out those issues. I've tested both in LXD on bionic and it's just the original xenial failure that's also on master. So I reckon this is ready to land. Thanks for all the work on it.

ltrager requested a review from ajkavanagh February 21, 2020 02:31

ltrager mentioned this pull request Feb 21, 2020

GET /1.0/instances should return instance type end points canonical/lxd#6903

Closed

ajkavanagh suggested changes Feb 21, 2020

View reviewed changes

ltrager added 4 commits March 27, 2020 23:24

Add support for the instances and virtual-machines endpoints.

5c5d1ed

Signed-off-by: Lee Trager <lee.trager@canonical.com>

ajkavanagh fixes

88be3cd

Signed-off-by: Lee Trager <lee.trager@canonical.com>

Fix setting .__attributes__ on child classes.

4369217

Signed-off-by: Lee Trager <lee.trager@canonical.com>

Fix failing test

e6ead2c

Signed-off-by: Lee Trager <lee.trager@canonical.com>

ltrager force-pushed the master branch from e686c11 to e6ead2c Compare March 27, 2020 23:25

Fix failing integration tests

9ae2587

Signed-off-by: Lee Trager <lee.trager@canonical.com>

ajkavanagh approved these changes Apr 23, 2020

View reviewed changes

ajkavanagh merged commit bc5946e into canonical:master Apr 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the instances and virtual-machines endpoints. #387

Add support for the instances and virtual-machines endpoints. #387

ltrager commented Feb 20, 2020

codecov-io commented Feb 20, 2020 •

edited

Loading

ajkavanagh left a comment

ajkavanagh Feb 21, 2020

ltrager Feb 21, 2020

ajkavanagh Feb 21, 2020

ajkavanagh Feb 21, 2020

ajkavanagh Feb 21, 2020

ltrager Feb 21, 2020

ajkavanagh Feb 21, 2020

ltrager Feb 21, 2020

ajkavanagh Feb 21, 2020

ltrager Feb 21, 2020

ajkavanagh Feb 21, 2020

ltrager Feb 21, 2020

ajkavanagh Feb 21, 2020

ltrager Feb 21, 2020

ajkavanagh Feb 21, 2020

ltrager Feb 21, 2020

ajkavanagh Feb 21, 2020

ltrager commented Feb 21, 2020

ajkavanagh commented Feb 26, 2020

ajkavanagh commented Apr 12, 2020

ltrager commented Apr 18, 2020

ajkavanagh commented Apr 23, 2020

		@@ -1,4 +1,4 @@
		# Copyright (c) 2016 Canonical Ltd
		# Copyright (c) 2020 Canonical Ltd

		self._endpoint = getattr(
		instance.client.api, instance._endpoint)[instance.name].files

		response = self._endpoint.get(
		params={'path': filepath}, is_api=False)

Add support for the instances and virtual-machines endpoints. #387

Add support for the instances and virtual-machines endpoints. #387

Conversation

ltrager commented Feb 20, 2020

codecov-io commented Feb 20, 2020 • edited Loading

Codecov Report

ajkavanagh left a 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

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

ltrager commented Feb 21, 2020

ajkavanagh commented Feb 26, 2020

ajkavanagh commented Apr 12, 2020

ltrager commented Apr 18, 2020

ajkavanagh commented Apr 23, 2020

codecov-io commented Feb 20, 2020 •

edited

Loading