NAT-244: apply merge review suggestions

gso/services/netbox_client.py

@@ -61,7 +61,7 @@ class NetboxClient:
         return device
 
     def get_interfaces_by_device(self, device_name: str, speed: str) -> list[Interfaces]:
-        """Get all interfaces of a device by name and speed that are free and not allocated."""
+        """Get all interfaces of a device by name and speed that are not reserved and not allocated."""
         device = self.get_device_by_name(device_name)
         return list(
             self.netbox.dcim.interfaces.filter(device_id=device.id, enabled=False, mark_connected=False, speed=speed)
@@ -171,8 +171,7 @@ class NetboxClient:
 
         This will delete a device and all interfaces that belong to it
         """
-        device = self.get_device_by_name(device_name)
-        device.delete()
+        self.get_device_by_name(device_name).delete()
         return
 
     def delete_interface(self, device_name: str, iface_name: str) -> None:
@@ -181,10 +180,9 @@ class NetboxClient:
         This will delete an interface even if another one has it as LAG interface
         (and will erase the LAG interface field of the latter)
         """
-        interface = self.get_interface_by_name_and_by_device_id(
+        self.get_interface_by_name_and_by_device_id(
             iface_name=iface_name, device_id=self.get_device_by_name(device_name).id
-        )
-        interface.delete()
+        ).delete()
         return
 
     def attach_interface_to_lag(
@@ -213,7 +211,7 @@ class NetboxClient:
         # Update physical interface
         return self.netbox.dcim.interfaces.update([iface])[0]
 
-    def unattach_interface_from_lag(self, device_name: str, lag_name: str, iface_name: str) -> Interfaces:
+    def detach_interface_from_lag(self, device_name: str, lag_name: str, iface_name: str) -> Interfaces:
         """Remove a given physical interface from a lag.
 
         Returns the updated physical interface object without the LAG
@@ -258,7 +256,7 @@ class NetboxClient:
         return interface
 
     def unreserve_interface(self, device_name: str, iface_name: str) -> Interfaces:
-        """Free (unreserve) an interface by disabling it."""
+        """Unreserve an interface by disabling it."""
 
         # First get interface from device
         interface = self.get_interface_by_name_and_by_device_name(iface_name=iface_name, device_name=device_name)
@@ -267,7 +265,7 @@ class NetboxClient:
         if not interface.enabled:
             raise WorkflowStateError(f"The interface: {iface_name} on device: {device_name} is not reserved.")
 
-        # Free interface by disabling it
+        # Unreserve interface by disabling it
         interface.enabled = False
         interface.save()
 
@@ -333,7 +331,7 @@ class NetboxClient:
         # Return available LAGs not assigned to the device
         return [lag for lag in all_feasible_lags if lag not in lag_interface_names]
 
-    def clear_interface(self, device_name: str, iface_name: str) -> Interfaces:
+    def free_interface(self, device_name: str, iface_name: str) -> Interfaces:
         """Deallocate and unreserve a physical interface, and delete its description."""
 
         interface = self.get_interface_by_name_and_by_device_name(iface_name=iface_name, device_name=device_name)

gso/workflows/iptrunk/terminate_iptrunk.py

@@ -61,31 +61,18 @@ def deprovision_ip_trunk_real(subscription: Iptrunk, process_id: UUIDstr, tt_num
 
 
 @step("Remove IP Trunk from NetBox")
-def remove_iptrunk_from_netbox(subscription: Iptrunk) -> State:
-    _router_sideA = subscription.iptrunk.iptrunk_sides[0].iptrunk_side_node
-    _router_sideB = subscription.iptrunk.iptrunk_sides[1].iptrunk_side_node
-    router_sideA = _router_sideA.router_fqdn
-    router_sideB = _router_sideB.router_fqdn
-    router_sideA_vendor = _router_sideA.router_vendor
-    router_sideB_vendor = _router_sideB.router_vendor
-    sideA_members = subscription.iptrunk.iptrunk_sides[0].iptrunk_side_ae_members
-    sideB_members = subscription.iptrunk.iptrunk_sides[1].iptrunk_side_ae_members
-    sideA_ae_iface = subscription.iptrunk.iptrunk_sides[0].iptrunk_side_ae_iface
-    sideB_ae_iface = subscription.iptrunk.iptrunk_sides[1].iptrunk_side_ae_iface
-
-    # Remove physical interfaces from LAGs
-    if router_sideA_vendor == RouterVendor.NOKIA:
-        for sideA_member in sideA_members:
-            NetboxClient().clear_interface(router_sideA, sideA_member)
-    if router_sideB_vendor == RouterVendor.NOKIA:
-        for sideB_member in sideB_members:
-            NetboxClient().clear_interface(router_sideB, sideB_member)
-
-    # Delete LAGs
-    if router_sideA_vendor == RouterVendor.NOKIA:
-        NetboxClient().delete_interface(router_sideA, sideA_ae_iface)
-    if router_sideB_vendor == RouterVendor.NOKIA:
-        NetboxClient().delete_interface(router_sideB, sideB_ae_iface)
+def free_interfaces_in_netbox(subscription: Iptrunk) -> State:
+    for side in [0, 1]:
+        router = subscription.iptrunk.iptrunk_sides[side].iptrunk_side_node
+        router_fqdn = router.router_fqdn
+
+        if router.router_vendor == RouterVendor.NOKIA:
+            nbclient = NetboxClient()
+            # Remove physical interfaces from LAGs
+            for member in subscription.iptrunk.iptrunk_sides[side].iptrunk_side_ae_members:
+                nbclient.free_interface(router_fqdn, member)
+            # Delete LAGs
+            nbclient.delete_interface(router_fqdn, subscription.iptrunk.iptrunk_sides[side].iptrunk_side_ae_iface)
 
     return {"subscription": subscription}
 
@@ -126,7 +113,7 @@ def terminate_iptrunk() -> StepList:
         >> store_process_subscription(Target.TERMINATE)
         >> unsync
         >> run_config_steps(config_steps)
-        >> remove_iptrunk_from_netbox
+        >> free_interfaces_in_netbox
         >> run_ipam_steps(ipam_steps)
         >> set_status(SubscriptionLifecycle.TERMINATED)
         >> resync

test/services/test_netbox.py

@@ -59,7 +59,7 @@ def interface():
 
 
 @patch("gso.services.netbox_client.pynetbox.api")
-def test_clear_interface(mock_api, device, interface):
+def test_free_interface(mock_api, device, interface):
     device_name = "mx1.lab.geant.net"
     interface_name = "et-0/0/1"
 
@@ -70,10 +70,10 @@ def test_clear_interface(mock_api, device, interface):
     # Create a NetboxClient instance
     netbox_client = NetboxClient()
 
-    # Test clear_interface method on success
+    # Test free_interface method on success
     interface.mark_connected = True
     interface.enabled = True
-    cleared_interface = netbox_client.clear_interface(device_name, interface_name)
+    cleared_interface = netbox_client.free_interface(device_name, interface_name)
     assert cleared_interface.enabled is False
     assert cleared_interface.mark_connected is False
     assert cleared_interface.description == ""
@@ -82,12 +82,12 @@ def test_clear_interface(mock_api, device, interface):
     interface.mark_connected = False
     interface.enabled = True
     with pytest.raises(WorkflowStateError) as e:
-        netbox_client.clear_interface(device_name, interface_name)
+        netbox_client.free_interface(device_name, interface_name)
         assert str(e.value) == f"The interface: {interface_name} on device: {device_name} is not allocated."
 
     # Test that WorkflowStateError is raised when the interface is not reserved
     interface.mark_connected = True
     interface.enabled = False
     with pytest.raises(WorkflowStateError) as e:
-        netbox_client.clear_interface(device_name, interface_name)
+        netbox_client.free_interface(device_name, interface_name)
         assert str(e.value) == f"The interface: {interface_name} on device: {device_name} is not reserved."

test/workflows/iptrunk/test_terminate_iptrunk.py

@@ -28,7 +28,7 @@ class MockedNetboxClient:
     def delete_interface(self):
         return None
 
-    def clear_interface(self):
+    def free_interface(self):
         return self.BaseMockObject(id=1, name="test")
 
 
@@ -41,14 +41,14 @@ def netbox_client_mock():
             "gso.services.netbox_client.NetboxClient.get_interface_by_name_and_by_device_id"
         ) as mock_get_interface_by_name_and_by_device_id,
         patch("gso.services.netbox_client.NetboxClient.delete_interface") as mock_delete_interface,
-        patch("gso.services.netbox_client.NetboxClient.clear_interface") as mock_clear_interface,
+        patch("gso.services.netbox_client.NetboxClient.free_interface") as mock_free_interface,
     ):
         mock_get_device_by_name.return_value = MockedNetboxClient().get_device_by_name()
         mock_get_interface_by_name_and_by_device_id.return_value = (
             MockedNetboxClient().get_interface_by_name_and_by_device_id()
         )
         mock_delete_interface.return_value = MockedNetboxClient().delete_interface()
-        mock_clear_interface.return_value = MockedNetboxClient().clear_interface()
+        mock_free_interface.return_value = MockedNetboxClient().free_interface()
 
         yield
 

utils/netboxcli.py

@@ -209,13 +209,13 @@ def attach_interface_to_lag(fqdn: str, lag: str, iface: str) -> None:
 
 
 @action.command()
-@click.option("--fqdn", help="Device name from where to get physical interface to unattach LAG")
-@click.option("--lag", help="LAG name to unattach from physical interface")
-@click.option("--iface", help="Interface name to unattach LAG from")
-def unattach_interface_from_lag(fqdn: str, lag: str, iface: str) -> None:
-    click.echo(f"Unattaching LAG from physical interface: device={fqdn}, LAG name={lag}, interface name={iface}")
-    unattached_iface = NetboxClient().unattach_interface_from_lag(fqdn, lag, iface)
-    click.echo(unattached_iface)
+@click.option("--fqdn", help="Device name from where to get physical interface to detach LAG")
+@click.option("--lag", help="LAG name to detach from physical interface")
+@click.option("--iface", help="Interface name to detach LAG from")
+def detach_interface_from_lag(fqdn: str, lag: str, iface: str) -> None:
+    click.echo(f"Detaching LAG from physical interface: device={fqdn}, LAG name={lag}, interface name={iface}")
+    detached_iface = NetboxClient().detach_interface_from_lag(fqdn, lag, iface)
+    click.echo(detached_iface)
 
 
 action.add_command(reserve_interface)
@@ -223,7 +223,7 @@ action.add_command(unreserve_interface)
 action.add_command(allocate_interface)
 action.add_command(deallocate_interface)
 action.add_command(attach_interface_to_lag)
-action.add_command(unattach_interface_from_lag)
+action.add_command(detach_interface_from_lag)
 
 
 if __name__ == "__main__":