Initial openstack support #113

TheRealFalcon · 2021-01-30T04:25:24Z

At this point I think the basic functionality is usable and reviewable.

Everything in the base_api.py test works, so the basics all work. I intentionally didn't add an example since all clouds should share the same base API right now. We should really only need one example unless we want to showcase specialized use cases of a specific cloud.

pycloudlib/openstack/cloud.py

TheRealFalcon · 2021-02-01T16:24:01Z

I recently noticed that on the CLI, I don't need to provide the network information like I do through the API. I'm curious how the CLI knows which network to create the instance on and if we could use that same knowledge to acquire the network to use when using the API.

TheRealFalcon · 2021-02-01T16:25:49Z

I'm also going on the assumption that we want a floating IP associated with instances that we launch. Obviously not every openstack deployment in the wild will require that, but I don't think that's something we need to worry about yet.

docs/clouds/openstack.md

OddBloke

Thanks for this work, James! By and large, this looks good. I have some thoughts inline.

docs/clouds/openstack.md

OddBloke · 2021-02-16T21:17:07Z

pycloudlib/openstack/cloud.py

+        instance = self.conn.compute.create_server(
+            name=self.tag,
+            image_id=image_id,
+            flavor_id=self.conn.compute.find_flavor(instance_type).id,


Flavors are not necessarily the same across OpenStack deployments: will this produce a meaningful error message if used against a cloud without "m1.small" defined?

Ah, I didn't realize that about flavors. I'll update it accordingly.

OddBloke · 2021-02-16T21:26:28Z

pycloudlib/openstack/instance.py

+        self.floating_ip = self.conn.create_floating_ip(
+            wait=True,
+        )
+        tries = 30
+        while tries:
+            try:
+                self.conn.compute.add_floating_ip_to_server(
+                    self.server,
+                    self.floating_ip.floating_ip_address
+                )
+                break
+            except BadRequestException as e:
+                if 'Instance network is not ready yet' in str(e):
+                    tries -= 1
+                    time.sleep(1)
+                    continue
+                raise e


I think this will assign a new floating IP address to existing instances which are found via (e.g.) get_instance, which I don't think is what we want to do.

(I guess this should move up into launch?)

pycloudlib/openstack/instance.py

OddBloke · 2021-02-16T21:29:32Z

pycloudlib/openstack/instance.py

+        self.conn.compute.delete_server(self.server.id)
+        self.conn.delete_floating_ip(self.floating_ip.id)


Should we ensure we delete the floating IP even if the server delete fails?

OddBloke · 2021-02-16T21:30:07Z

pycloudlib/openstack/instance.py

+        except ConflictException as e:
+            if 'while it is in vm_state active' in str(e):
+                return


An explanatory comment here would be good!

OddBloke · 2021-02-16T21:38:47Z

setup.py

@@ -27,6 +27,7 @@ def read_readme():
    "azure-mgmt-compute >= 13.0.0, < 17",
    "azure-cli-core >= 2.9.1",
    "knack >= 0.7.1",
+    "python-openstackclient >= 5.2.1",


This version declares a minimum version of Python 3.6. We might be OK, because (last I saw) OpenStack manages its Python dependencies globally, so that could be an artifically high lower bound in this specific case.

OddBloke · 2021-02-16T22:11:27Z

pycloudlib/openstack/tests/test_cloud.py

+@mock.patch('pycloudlib.key.KeyPair.public_key_content',
+            new_callable=mock.PropertyMock,
+            return_value='pretend public key')
+@mock.patch('openstack.connect', spec=Connection)


I'm not sure this spec is doing what you want here:

In [1]: import unittest.mock as mock In [3]: from openstack.connection import Connection In [4]: p = mock.patch("openstack.connect", spec=Connection) In [5]: m = p.start() In [6]: m() # this is what `openstack.connect()` will return Out[6]: <MagicMock name='connect()' id='140181990886512'> In [7]: m # and I think you actually want it to return this Out[7]: <openstack.connection.Connection at 0x7f7ea9c62fa0>

OddBloke · 2021-02-16T22:12:15Z

pycloudlib/openstack/tests/test_cloud.py

+class TestOpenstackKeypair:
+    """Tests covering _get_openstack_keypair."""
+
+    def test_keypair_doesnt_exist(self, m_openstack, m_keypair):


Suggested change

def test_keypair_doesnt_exist(self, m_openstack, m_keypair):

def test_keypair_doesnt_exist(self, m_openstack, _m_keypair):

OddBloke · 2021-02-16T22:17:08Z

pycloudlib/openstack/tests/test_cloud.py

+        cloud._get_openstack_keypair()
+        assert 1 == cloud.conn.create_keypair.call_count
+
+    def test_keypairs_match(self, m_openstack, m_keypair):


m_keypair threw me off here: I assumed that would be a mock of pycloudlib.key.KeyPair (making this test very confusing :p). Perhaps m_public_key_content? Still a little confusing, given it's a property mock (whereas you'd never call public_key_content of course).

TheRealFalcon · 2021-02-18T15:12:50Z

Addressed the code comments. Still need to address the test comments. @OddBloke , I addressed the floating IP issue a little differently than we initially discussed, so you might want to take a look.

TheRealFalcon · 2021-02-18T16:09:28Z

Updated the test contents, but now I have new floating ip tests to write 😉

TheRealFalcon · 2021-02-19T14:35:52Z

Added new tests. Should be ready for re-review.

OddBloke

Floating IP stuff LGTM, I've left a few inline comments, all of which are nits. I'm going to run some tests locally before approving but, assuming that's all good, this will be good to land.

OddBloke · 2021-02-19T15:10:52Z

pycloudlib/openstack/instance.py

+        self.delete_floating_ip = False
+        self.floating_ip = self._get_existing_floating_ip()
+        if self.floating_ip is None:
+            self._create_and_attach_floating_id()


A nit, but self.floating_ip = self._create_and_attach_floating_id() feels a little easier to follow; it would give _get_... and _create_... a (more) similar API and means that self.floating_ip is only set in one place.

OddBloke · 2021-02-19T15:15:02Z

pycloudlib/openstack/tests/test_instance.py

+NETWORK_IPS = [
+    {'floating_ip_address': '10.0.0.4'},
+    {'floating_ip_address': '10.0.0.3'},
+]


Another nit, but if the code under test incorrectly did the equivalent of chain(*[d.values() for d in NETWORK_IPS]), we wouldn't catch that with this input.

OddBloke · 2021-02-19T15:18:43Z

pycloudlib/openstack/tests/test_instance.py

+    def test_existing_floating_ip(self, m_create):
+        """Test that if a server has an existing floating IP, we use it."""
+        m_connection = mock.Mock()
+        m_server = m_connection.compute.get_server.return_value = mock.Mock()


A nit; I don't think we need this final assignment:

Suggested change

m_server = m_connection.compute.get_server.return_value = mock.Mock()

m_server = m_connection.compute.get_server.return_value

In [6]: from unittest import mock In [7]: m_connection = mock.Mock() In [8]: m_server = m_connection.compute.get_server.return_value In [9]: m_server Out[9]: <Mock name='mock.compute.get_server()' id='140123963984144'> In [10]: m_server = m_connection.compute.get_server.return_value = mock.Mock() In [11]: m_server Out[11]: <Mock name='mock.compute.get_server()' id='140123963995232'>

Co-authored-by: Daniel Watkins <oddbloke@ubuntu.com>

OddBloke

Thanks for all the work!

TheRealFalcon added the wip label Jan 30, 2021

TheRealFalcon force-pushed the openstack branch from 22c7a50 to 44115e2 Compare January 30, 2021 04:25

TheRealFalcon commented Jan 30, 2021

View reviewed changes

pycloudlib/openstack/cloud.py Show resolved Hide resolved

TheRealFalcon commented Jan 30, 2021

View reviewed changes

pycloudlib/openstack/cloud.py Show resolved Hide resolved

TheRealFalcon commented Jan 30, 2021

View reviewed changes

pycloudlib/openstack/cloud.py Outdated Show resolved Hide resolved

TheRealFalcon force-pushed the openstack branch from 44115e2 to cc7e1e4 Compare January 30, 2021 04:49

TheRealFalcon commented Feb 5, 2021

View reviewed changes

docs/clouds/openstack.md Show resolved Hide resolved

TheRealFalcon mentioned this pull request Feb 8, 2021

Add openstack as a testing platform canonical/cloud-init#804

Merged

3 tasks

TheRealFalcon removed the wip label Feb 8, 2021

TheRealFalcon changed the title ~~WIP: Initial openstack support~~ Initial openstack support Feb 8, 2021

TheRealFalcon force-pushed the openstack branch from 146824b to 9e9d9bd Compare February 9, 2021 15:14

OddBloke reviewed Feb 16, 2021

View reviewed changes

TheRealFalcon requested a review from OddBloke February 19, 2021 14:35

OddBloke reviewed Feb 19, 2021

View reviewed changes

TheRealFalcon and others added 11 commits February 19, 2021 13:24

Initial openstack support

30654e0

Remove obtaining image given release name

20fd850

Add openstack tests

c2288c0

Remove unused import

8d3c457

Update base_api test to support running only one cloud

076223f

Adding openstack documentation

83cc038

Fix some silly mistakes

171e23d

Remove todo comment...at least for now

d5bad35

Docs typo fix from Dan

55d3128

Co-authored-by: Daniel Watkins <oddbloke@ubuntu.com>

Actually return the console output

e78416e

Co-authored-by: Daniel Watkins <oddbloke@ubuntu.com>

Address code comments

7a49e18

TheRealFalcon added 4 commits February 19, 2021 13:24

test comments

2b3765d

Adding tests for floating ip behavior

c1d390e

linting

dc5b69b

review comments

02ad4b5

TheRealFalcon force-pushed the openstack branch from 0f1b4cf to 02ad4b5 Compare February 19, 2021 18:24

OddBloke approved these changes Mar 1, 2021

View reviewed changes

TheRealFalcon merged commit 22ca3df into master Mar 1, 2021

TheRealFalcon deleted the openstack branch March 1, 2021 20:06

paride mentioned this pull request Mar 2, 2021

import pycloudlib => ModuleNotFoundError: No module named 'pycloudlib.openstack' #126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial openstack support #113

Initial openstack support #113

TheRealFalcon commented Jan 30, 2021 •

edited

Loading

TheRealFalcon commented Feb 1, 2021

TheRealFalcon commented Feb 1, 2021

OddBloke left a comment

OddBloke Feb 16, 2021

TheRealFalcon Feb 17, 2021

OddBloke Feb 16, 2021

OddBloke Feb 16, 2021

OddBloke Feb 16, 2021

OddBloke Feb 16, 2021

OddBloke Feb 16, 2021

OddBloke Feb 16, 2021

OddBloke Feb 16, 2021

TheRealFalcon commented Feb 18, 2021

TheRealFalcon commented Feb 18, 2021

TheRealFalcon commented Feb 19, 2021

OddBloke left a comment

OddBloke Feb 19, 2021

OddBloke Feb 19, 2021

OddBloke Feb 19, 2021

OddBloke left a comment

		self.conn.compute.delete_server(self.server.id)
		self.conn.delete_floating_ip(self.floating_ip.id)

	def test_keypair_doesnt_exist(self, m_openstack, m_keypair):
	def test_keypair_doesnt_exist(self, m_openstack, _m_keypair):

	m_server = m_connection.compute.get_server.return_value = mock.Mock()
	m_server = m_connection.compute.get_server.return_value

Initial openstack support #113

Initial openstack support #113

Conversation

TheRealFalcon commented Jan 30, 2021 • edited Loading

TheRealFalcon commented Feb 1, 2021

TheRealFalcon commented Feb 1, 2021

OddBloke 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

TheRealFalcon commented Feb 18, 2021

TheRealFalcon commented Feb 18, 2021

TheRealFalcon commented Feb 19, 2021

OddBloke 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

OddBloke left a comment

Choose a reason for hiding this comment

TheRealFalcon commented Jan 30, 2021 •

edited

Loading