powervs: fix ports cleaning by cleaning them manually on delete

[nikromen]

ssia

[gemini-code-assist]

Summary of Changes

Hello @nikromen, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a critical issue in IBM Cloud PowerVS where network interfaces (ports) were not being automatically cleaned up after VM deletion, leading to the depletion of IP addresses within subnets. The changes introduce a dedicated utility for manual cleanup of orphaned interfaces and integrate proactive network interface deletion into the VM termination workflow, ensuring efficient resource management and preventing future IP pool exhaustion.

Highlights

• New Utility for Orphaned IP Cleanup: A new command-line utility, resalloc-ibm-cloud-powervs-cleanup-ips, has been introduced to identify and delete network interfaces (ports) that are no longer attached to active PowerVS VM instances, preventing IP pool exhaustion.
• Enhanced VM Deletion Process: The delete_vm function in powervs_vm.py was updated to explicitly retrieve and then delete associated network interfaces after a VM is terminated. This addresses a change in IBM Cloud's PowerVS behavior where these interfaces are no longer automatically cleaned up.
• PowerVS API Client Extension: The PowerVSClient was modified to support a new /v1 API endpoint, including a v1_api flag in its request method and new methods (list_network_interfaces, delete_network_interface) for managing network resources.

Changelog

• pyproject.toml
  • Added a new entry point for the resalloc-ibm-cloud-powervs-cleanup-ips utility.
  • Included a manpage entry for the new IP cleanup utility.
• resalloc_ibm_cloud/argparsers.py
  • Implemented powervs_cleanup_ips_parser to define command-line arguments for the new IP cleanup utility, including a required --network-id argument.
• resalloc_ibm_cloud/powervs/client.py
  • Modified the request method to accept a v1_api parameter, allowing requests to target the newer /v1 PowerVS API without the cloud-instance prefix.
  • Added list_network_interfaces method to retrieve network interfaces for a given network ID.
  • Added delete_network_interface method to remove a specific network interface.
• resalloc_ibm_cloud/powervs/credentials.py
  • Introduced a v1_url property to provide the base URL for the PowerVS /v1 API.
  • Updated the docstring for service_url to clarify its use with the /pcloud/v1 API path.
• resalloc_ibm_cloud/powervs/powervs_cleanup_ips.py
  • Added a new script to identify and delete orphaned network interfaces from specified PowerVS subnets.
  • Included logic to list active VM instances and compare them against network interface associations to determine orphaned interfaces.
• resalloc_ibm_cloud/powervs/powervs_vm.py
  • Updated the delete_vm method to first identify and store the network interface IDs associated with the VM being deleted.
  • After the VM deletion, it now explicitly calls the delete_network_interface method for each stored interface to ensure their removal.

Activity

• No activity has been recorded for this pull request yet.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a manual cleanup process for PowerVS network interfaces (ports) upon VM deletion, addressing an issue where they are not automatically removed by IBM Cloud. It adds a new utility script powervs-cleanup-ips to clean up orphaned IPs and modifies the VM deletion logic to remove associated network interfaces.

The changes to the VM deletion process appear correct. However, the new cleanup script contains a critical logic error that would cause it to delete network interfaces of active VMs instead of orphaned ones. There is also a minor issue with error logging in the same script. I've added comments with suggestions to fix these issues.

[nikromen]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces manual cleanup of network interfaces for PowerVS VMs upon deletion, addressing an issue where they were not being automatically removed. This is achieved by updating the delete_vm function and adding a new utility script, powervs-cleanup-ips, for broader cleanup. While the approach is sound, I've identified a potential race condition in the delete_vm function where network interfaces might be deleted before the VM deletion is complete, potentially leading to failures. My review includes a suggestion to add a waiting mechanism to ensure the VM is fully deleted before proceeding with interface cleanup, making the process more robust.

[nikromen]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a crucial fix for cleaning up orphaned network ports in PowerVS, addressing potential resource leaks. This is achieved by adding logic to manually delete network interfaces after a VM is deleted, and also by providing a new standalone cleanup script. The changes include adding new client methods for the v1 API, a polling mechanism to wait for instance deletion, and the core cleanup logic. My review focuses on improving the robustness of the error handling in the new functionality to make it more resilient against various network-related failures.

[praiskup]

is there a re-try mechanism? if yes, just catch all exceptions?

[nikromen]

this is just a helper script to use this to delete all the dangling ports we had, so try is even not needed at all, removed

[praiskup]

do you really want to break, or continue?

[nikromen]

on continue we may get stuck. 5xx errors are retried. Waiting for the VM to delete itself and then check and delete ports was gemini's idea, which makes sense... but in my experience with powervs this is not needed, so I'd rather break on error here (the API client in this tool I've written itself retries on error requests several times and after several retries it gives up, see powervs.client.PowerVSClient.request) and then we can proceed with the request of port deletion anyway. Whatever here goes wrong, we should still try to remove the dangling port (if there is any)