fix(oracle): only get ip address from primary vnic #436

a-dubs · 2024-10-03T01:35:59Z

Previously, the first vnic returned by the oci sdk is used to get the instance public IP from, but if a secondary vnic is created and attached after instance creation, this causes this logic to fail since the new secondary vnic is returned before the primary vnic by the oci sdk. This commit makes the ip property check for the is_primary property on all vnics before getting an IP address from the selected vnic.

Testing of Changes:

I switched to main, copied the tests/unit_tests/oci/test_instance.py file over and ran it and tests where the first vnic returned is NOT the primary vnic failed. This shows that these unit tests are actually testing something and that the source code changes from this PR actually fix what they claim to.

blackboxsw · 2024-10-15T00:39:13Z

pycloudlib/oci/instance.py

+                self.network_client.get_vnic(vnic_attachment.vnic_id).data
+                for vnic_attachment in vnic_attachment


Just for readability, lets name the loop-local variable name vnic instead of vnic_attachment. I have a hard time seeing the for loop use the same iterator variable name as the list it is walking.

Suggested change

self.network_client.get_vnic(vnic_attachment.vnic_id).data

for vnic_attachment in vnic_attachment

self.network_client.get_vnic(vnic.vnic_id).data

for vnic in vnic_attachment

@blackboxsw The naming is more so to make it clear that the thing we are iterating with is not actually a vnic but instead a vnic attachment from which we can call a function and get the actual vnic. but that being said i don't actually care that much and would be happy to rename to vnic if you think that is better for readability!

blackboxsw

Looks pretty good on the logic, some unit test nits and suggestions.

Here's a combined diff for some of the comments, but I leave tests_unittests/oci/test_instance.py changes to you.

csmith@midtown:~/src/pycloudlib$ git diff
diff --git a/tests/unit_tests/oci/test_cloud.py b/tests/unit_tests/oci/test_cloud.py
index b688680..1dd26b1 100644
--- a/tests/unit_tests/oci/test_cloud.py
+++ b/tests/unit_tests/oci/test_cloud.py
@@ -6,7 +6,6 @@ import oci
 import pytest
 import toml
 
-from pycloudlib.config import Config
 from pycloudlib.errors import (
     InstanceNotFoundError,
     PycloudlibException,
@@ -16,14 +15,13 @@ from pycloudlib.oci.instance import OciInstance
 
 
 @pytest.fixture
-def oci_cloud():
+def oci_cloud(tmp_path):
     """
     Fixture for OCI Cloud class.
-    
+
     This fixture mocks the oci.config.validate_config function to not raise an error. It also
     mocks the oci.core.ComputeClient and oci.core.VirtualNetworkClient classes to return mock
-    instances of the clients. The fixture also mocks the pycloudlib.config.parse_config function
-    to return a Config object with the default values from the pycloudlib.toml.template file.
+    instances of the clients.
     """
     oci_config = {
         "user": "ocid1.user.oc1..example",
@@ -32,6 +30,8 @@ def oci_cloud():
         "tenancy": "ocid1.tenancy.oc1..example",
         "region": "us-phoenix-1",
     }
+    pycloudlib_config = tmp_path / "pyproject.toml"
+    pycloudlib_config.write_text("[oci]\n")
 
     with mock.patch(
         "pycloudlib.oci.cloud.oci.config.validate_config",
@@ -40,11 +40,7 @@ def oci_cloud():
         "pycloudlib.oci.cloud.oci.core.ComputeClient"
     ) as mock_compute_client_class, mock.patch(
         "pycloudlib.oci.cloud.oci.core.VirtualNetworkClient"
-    ) as mock_network_client_class, mock.patch(
-        "pycloudlib.config.parse_config"
-    ) as mock_parse_config:
-        # mock loading the config file by just reading in the template, which we know will exist
-        mock_parse_config.return_value = toml.load("pycloudlib.toml.template", _dict=Config)
+    ) as mock_network_client_class:
         # Instantiate mocked clients
         mock_compute_client = mock.Mock()
         mock_network_client = mock.Mock()
@@ -55,6 +51,7 @@ def oci_cloud():
         oci_cloud = OCI(
             "test-instance",
             timestamp_suffix=True,
+            config_file=pycloudlib_config,
             availability_domain="PHX-AD-1",
             compartment_id="test-compartment-id",
             region="us-phoenix-1",
@@ -70,21 +67,34 @@ def oci_cloud():
         yield oci_cloud
 
 
+OCI_PYCLOUDLIB_CONFIG = """\
+[oci]
+availability_domain = "PYCL-AD-1"
+compartment_id = "pycloudlib-compartment-id"
+region = "pycl-pheonix-1"
+"""
+
+
 class TestOciInit:
     """Tests for OCI Cloud __init__."""
 
     def test_init_valid(self, oci_cloud):
-        """Test __init__ with valid parameters."""
-        # Ensure that oci.config.from_file is mocked with a valid configuration
+        """Test __init__ with valid parameters matches .oci/config over pycloudlib.toml."""
         assert oci_cloud.availability_domain == "PHX-AD-1"
         assert oci_cloud.compartment_id == "test-compartment-id"
         assert oci_cloud.region == "us-phoenix-1"
 
-    def test_init_invalid_config(self):
-        """Test __init__ with invalid OCI configuration."""
+    def test_init_invalid_config(self, tmp_path):
+        """Test __init__ with invalid OCI configuration raises ValueError."""
+        pycloudlib_config = tmp_path / "pyproject.toml"
+        pycloudlib_config.write_text(OCI_PYCLOUDLIB_CONFIG)
         with mock.patch("oci.config.from_file", side_effect=oci.exceptions.InvalidConfig):
             with pytest.raises(ValueError, match="Config dict is invalid"):
-                OCI(tag="test-instance", config_dict={"invalid": "config"})
+                OCI(
+                    tag="test-instance",
+                    config_file=pycloudlib_config,
+                    config_dict={"invalid": "config"},
+                )
 
 
 class TestOciImages:
@@ -175,17 +185,8 @@ class TestOciInstances:
         assert isinstance(instance, OciInstance)
         oci_cloud.compute_client.get_instance.assert_called_once_with("test-instance-id")
 
-    @mock.patch("oci.config.from_file")
-    def test_get_instance_not_found(self, mock_from_file, oci_cloud):
+    def test_get_instance_not_found(self, oci_cloud):
         """Test get_instance raises InstanceNotFoundError when instance does not exist."""
-        valid_config = {
-            "user": "ocid1.user.oc1..example",
-            "fingerprint": "mock-fingerprint",
-            "key_file": "/path/to/key",
-            "tenancy": "ocid1.tenancy.oc1..example",
-            "region": "us-phoenix-1",
-        }
-        mock_from_file.return_value = valid_config
 
         oci_cloud.compute_client.get_instance.side_effect = oci.exceptions.ServiceError(
             status=404,
@@ -201,8 +202,7 @@ class TestOciInstances:
     def test_launch_instance(self, mock_wait_till_ready, oci_cloud):
         """Test launch method with valid inputs."""
         # mock the key pair
-        oci_cloud.key_pair = mock.Mock()
-        oci_cloud.key_pair.public_key_content = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC"
+        oci_cloud.key_pair = mock.Mock(public_key_config="ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC")
         oci_cloud.compute_client.launch_instance.return_value = mock.Mock(
             data=mock.Mock(id="instance-id")
         )

tests/unit_tests/oci/test_cloud.py

blackboxsw · 2024-10-16T03:02:35Z

tests/unit_tests/oci/test_cloud.py

+        oci_cloud.key_pair = mock.Mock()
+        oci_cloud.key_pair.public_key_content = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC"


Can squash it to one line w/ our 100 char limit

Suggested change

oci_cloud.key_pair = mock.Mock()

oci_cloud.key_pair.public_key_content = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC"

oci_cloud.key_pair = mock.Mock(public_key_content="ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC")

:lets-gooo:

tests/unit_tests/oci/test_instance.py

a-dubs

finally posting my comments that i did not realize were stuck in "pending"

a-dubs · 2024-10-03T14:09:02Z

tests/unit_tests/oci/test_instance.py

+    assert oci_instance.ip == "2001:0db8:85a3::8a2e:0370:7334"
+
+
+@pytest.mark.parametrize("has_public_ip", [True, False])


use pytest.param

a-dubs · 2024-10-16T13:11:57Z

tests/unit_tests/oci/test_cloud.py

+        oci_cloud.key_pair = mock.Mock()
+        oci_cloud.key_pair.public_key_content = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC"


:lets-gooo:

tests/unit_tests/oci/test_instance.py

a-dubs · 2024-10-17T19:01:04Z

pycloudlib/oci/instance.py

+                self.network_client.get_vnic(vnic_attachment.vnic_id).data
+                for vnic_attachment in vnic_attachment


@blackboxsw The naming is more so to make it clear that the thing we are iterating with is not actually a vnic but instead a vnic attachment from which we can call a function and get the actual vnic. but that being said i don't actually care that much and would be happy to rename to vnic if you think that is better for readability!

a-dubs · 2024-10-17T20:10:54Z

@blackboxsw ready for re-review! i snuck in another commit to fix some pytest warnings that the CI job was giving me so i dont have to open up yet another PR lol

blackboxsw

LGTM! Fix version and you're good

blackboxsw · 2024-10-18T02:20:35Z

pyproject.toml

@@ -52,3 +52,12 @@ extend-select = [

 [tool.ruff.lint.pydocstyle]
 convention = "pep257"
+


pyproject.toml to rule them all +1

Until now, Oracle had zero unit tests. Now there is significant coverage over main functions of the cloud and instance class.

Previously, the first vnic returned by the oci sdk is used to get the instance public IP from, but if a secondary vnic is created and attached after instance creation, this causes this logic to fail since the new secondary vnic is returned before the primary vnic by the oci sdk. This commit makes the ip property check for the is_primary property on all vnics before getting an IP address from the selected vnic. Also, this commit adds unit tests validating new IP property changes

In PR canonical#406, two pytest marks were added to help with mocking ssh keys in unit tests, but the marks were not registered with pytest so it was raising errors. This commit fixes that! Also, migrate existing pytest configuration from tox.ini to pyproject.toml

a-dubs force-pushed the oracle-get-ip-only-from-primary-vnic branch 3 times, most recently from 5d794f5 to c64e786 Compare October 3, 2024 14:48

a-dubs marked this pull request as draft October 3, 2024 16:18

a-dubs force-pushed the oracle-get-ip-only-from-primary-vnic branch from 72e1ad2 to b6a6650 Compare October 11, 2024 13:04

a-dubs marked this pull request as ready for review October 11, 2024 13:05

a-dubs force-pushed the oracle-get-ip-only-from-primary-vnic branch 7 times, most recently from f9716ef to 564bfd8 Compare October 14, 2024 18:18

blackboxsw self-assigned this Oct 14, 2024

blackboxsw reviewed Oct 15, 2024

View reviewed changes

blackboxsw requested changes Oct 16, 2024

View reviewed changes

a-dubs force-pushed the oracle-get-ip-only-from-primary-vnic branch 2 times, most recently from d0d2156 to b729cd3 Compare October 16, 2024 15:41

a-dubs commented Oct 17, 2024

View reviewed changes

a-dubs force-pushed the oracle-get-ip-only-from-primary-vnic branch 4 times, most recently from e95edc3 to ae059af Compare October 17, 2024 19:30

blackboxsw approved these changes Oct 18, 2024

View reviewed changes

a-dubs force-pushed the oracle-get-ip-only-from-primary-vnic branch from ae059af to 32181a7 Compare October 18, 2024 13:45

a-dubs added 3 commits October 18, 2024 15:13

test(oracle): add unit tests for cloud and instance classes

2e2991d

Until now, Oracle had zero unit tests. Now there is significant coverage over main functions of the cloud and instance class.

a-dubs force-pushed the oracle-get-ip-only-from-primary-vnic branch from 32181a7 to 79903df Compare October 18, 2024 19:13

a-dubs merged commit faf2d9c into canonical:main Oct 18, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(oracle): only get ip address from primary vnic #436

fix(oracle): only get ip address from primary vnic #436

a-dubs commented Oct 3, 2024

blackboxsw Oct 15, 2024

a-dubs Oct 17, 2024 •

edited

Loading

blackboxsw left a comment

blackboxsw Oct 16, 2024

a-dubs Oct 16, 2024

a-dubs left a comment

a-dubs Oct 3, 2024

a-dubs Oct 16, 2024

a-dubs Oct 17, 2024 •

edited

Loading

a-dubs commented Oct 17, 2024

blackboxsw left a comment

blackboxsw Oct 18, 2024

		self.network_client.get_vnic(vnic_attachment.vnic_id).data
		for vnic_attachment in vnic_attachment

		oci_cloud.key_pair = mock.Mock()
		oci_cloud.key_pair.public_key_content = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC"

	oci_cloud.key_pair = mock.Mock()
	oci_cloud.key_pair.public_key_content = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC"
	oci_cloud.key_pair = mock.Mock(public_key_content="ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABgQC")

		assert oci_instance.ip == "2001:0db8:85a3::8a2e:0370:7334"


		@pytest.mark.parametrize("has_public_ip", [True, False])

		@@ -52,3 +52,12 @@ extend-select = [

		[tool.ruff.lint.pydocstyle]
		convention = "pep257"

fix(oracle): only get ip address from primary vnic #436

fix(oracle): only get ip address from primary vnic #436

Conversation

a-dubs commented Oct 3, 2024

Testing of Changes:

blackboxsw Oct 15, 2024

Choose a reason for hiding this comment

a-dubs Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

blackboxsw left a comment

Choose a reason for hiding this comment

blackboxsw Oct 16, 2024

Choose a reason for hiding this comment

a-dubs Oct 16, 2024

Choose a reason for hiding this comment

a-dubs left a comment

Choose a reason for hiding this comment

a-dubs Oct 3, 2024

Choose a reason for hiding this comment

a-dubs Oct 16, 2024

Choose a reason for hiding this comment

a-dubs Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

a-dubs commented Oct 17, 2024

blackboxsw left a comment

Choose a reason for hiding this comment

blackboxsw Oct 18, 2024

Choose a reason for hiding this comment

a-dubs Oct 17, 2024 •

edited

Loading

a-dubs Oct 17, 2024 •

edited

Loading