Do not create flavors during netbox/ironic sync #1312
Conversation
Nova is not used for baremetal provisioning. Therefore flavor creation for baremetal nodes is not required. Signed-off-by: Jan Horstmann <horstmann@osism.tech>
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| logger.info( | ||
| f"Cleaning up baremetal node not found in netbox: {node['Name']}" | ||
| ) | ||
| flavor_name = "osism-" + node["Name"] | ||
| flavor = openstack.compute_flavor_get(flavor_name) | ||
| if flavor: | ||
| logger.info(f"Deleting flavor {flavor_name}") | ||
| openstack.compute_flavor_delete(flavor) | ||
| for port in openstack.baremetal_port_list( | ||
| details=False, attributes=dict(node_uuid=node["UUID"]) | ||
| ): |
There was a problem hiding this comment.
Error Handling and Performance Concerns
The deletion process for baremetal nodes and their ports lacks exception handling, which could lead to unhandled errors if the deletion fails. This can disrupt the flow of the application and leave the system in an inconsistent state.
Recommendation:
Wrap the deletion operations in a try-except block to handle potential exceptions gracefully. Consider using batch operations if the API supports it to improve performance when deleting multiple ports.
| logger.info( | ||
| f"Validation of management interface failed for baremetal node for {device.name}\nReason: {node_validation['management'].reason}" | ||
| ) | ||
|
|
||
| flavor_name = "osism-" + device.name | ||
| flavor = openstack.compute_flavor_get(flavor_name) | ||
| if not flavor: | ||
| logger.info(f"Creating flavor for {flavor_name}") | ||
| flavor = openstack.compute_flavor_create( | ||
| flavor_name, flavor_attributes | ||
| ) | ||
| else: | ||
| flavor_updates = {} | ||
| deep_compare(flavor_attributes, flavor, flavor_updates) | ||
| flavor_updates_extra_specs = flavor_updates.pop("extra_specs", None) | ||
| if flavor_updates: | ||
| logger.info( | ||
| f"Updating flavor for {device.name} with {flavor_updates}" | ||
| ) | ||
| openstack.compute_flavor_delete(flavor) | ||
| flavor = openstack.compute_flavor_create( | ||
| flavor_name, flavor_attributes | ||
| ) | ||
| elif flavor_updates_extra_specs: | ||
| logger.info( | ||
| f"Updating flavor extra_specs for {device.name} with {flavor_updates_extra_specs}" | ||
| ) | ||
| openstack.compute_flavor_update_extra_specs( | ||
| flavor, flavor_updates_extra_specs | ||
| ) | ||
| flavor = openstack.compute_flavor_get(flavor_name) | ||
| for extra_specs_key in flavor["extra_specs"].keys(): | ||
| if ( | ||
| extra_specs_key | ||
| not in flavor_attributes["extra_specs"].keys() | ||
| ): | ||
| logger.info( | ||
| f"Deleting flavor extra_specs property {extra_specs_key} for {device.name}" | ||
| ) | ||
| flavor = ( | ||
| openstack.compute_flavor_delete_extra_specs_property( | ||
| flavor, extra_specs_key | ||
| ) | ||
| ) | ||
|
|
||
| except Exception as exc: | ||
| logger.info( | ||
| f"Could not fully synchronize device {device.name} with ironic: {exc}" |
There was a problem hiding this comment.
Error Handling and Logging Level
The exception handling in this block logs the exception at an info level, which might not appropriately reflect the severity of the issue. Additionally, the exception is not re-raised or otherwise managed, which could suppress important errors.
Recommendation:
Change the logging level to logger.error to indicate the severity of the issue. Consider re-raising the exception or implementing additional error handling strategies to ensure that critical issues are addressed appropriately.
| return result | ||
|
|
||
|
|
||
| @app.task(bind=True, name="osism.tasks.openstack.compute_flavor_get") | ||
| def compute_flavor_get(self, name_or_id): | ||
| conn = utils.get_openstack_connection() | ||
| result = conn.compute.find_flavor( | ||
| name_or_id, ignore_missing=True, get_extra_specs=True | ||
| ) | ||
| return result | ||
|
|
||
|
|
||
| @app.task(bind=True, name="osism.tasks.openstack.compute_flavor_create") | ||
| def compute_flavor_create(self, name, attributes=None): | ||
| if attributes is None: | ||
| attributes = {} | ||
| attributes.update({"name": name}) | ||
| extra_specs = attributes.pop("extra_specs", None) | ||
| conn = utils.get_openstack_connection() | ||
| flavor = conn.compute.create_flavor(**attributes) | ||
| if extra_specs: | ||
| flavor = conn.compute.create_flavor_extra_specs(flavor, extra_specs) | ||
| return flavor | ||
|
|
||
|
|
||
| @app.task(bind=True, name="osism.tasks.openstack.compute_flavor_delete") | ||
| def compute_flavor_delete(self, flavor): | ||
| conn = utils.get_openstack_connection() | ||
| conn.compute.delete_flavor(flavor, ignore_missing=True) | ||
|
|
||
|
|
||
| @app.task(bind=True, name="osism.tasks.openstack.compute_flavor_update_extra_specs") | ||
| def compute_flavor_update_extra_specs(self, flavor, extra_specs={}): | ||
| conn = utils.get_openstack_connection() | ||
| for key, value in extra_specs.items(): | ||
| conn.compute.update_flavor_extra_specs_property(flavor, key, value) | ||
|
|
||
|
|
||
| @app.task(bind=True, name="osism.tasks.openstack.compute_flavor_delete_extra_specs") | ||
| def compute_flavor_delete_extra_specs_property(self, flavor, prop): | ||
| conn = utils.get_openstack_connection() | ||
| conn.compute.delete_flavor_extra_specs_property(flavor, prop) | ||
|
|
||
|
|
||
| @app.task(bind=True, name="osism.tasks.openstack.image_manager") | ||
| def image_manager( | ||
| self, |
There was a problem hiding this comment.
The image_manager function lacks comprehensive error handling for the command execution when configs is None. This could lead to unhandled exceptions if the command fails, affecting the stability of the application.
Recommendation:
Wrap the run_command invocation in a try-except block to handle potential exceptions gracefully. This will improve the robustness of the task and ensure that errors are properly managed.
| return result | ||
|
|
||
|
|
||
| @app.task(bind=True, name="osism.tasks.openstack.compute_flavor_get") | ||
| def compute_flavor_get(self, name_or_id): | ||
| conn = utils.get_openstack_connection() | ||
| result = conn.compute.find_flavor( | ||
| name_or_id, ignore_missing=True, get_extra_specs=True | ||
| ) | ||
| return result | ||
|
|
||
|
|
||
| @app.task(bind=True, name="osism.tasks.openstack.compute_flavor_create") | ||
| def compute_flavor_create(self, name, attributes=None): | ||
| if attributes is None: | ||
| attributes = {} | ||
| attributes.update({"name": name}) | ||
| extra_specs = attributes.pop("extra_specs", None) | ||
| conn = utils.get_openstack_connection() | ||
| flavor = conn.compute.create_flavor(**attributes) | ||
| if extra_specs: | ||
| flavor = conn.compute.create_flavor_extra_specs(flavor, extra_specs) | ||
| return flavor | ||
|
|
||
|
|
||
| @app.task(bind=True, name="osism.tasks.openstack.compute_flavor_delete") | ||
| def compute_flavor_delete(self, flavor): | ||
| conn = utils.get_openstack_connection() | ||
| conn.compute.delete_flavor(flavor, ignore_missing=True) | ||
|
|
||
|
|
||
| @app.task(bind=True, name="osism.tasks.openstack.compute_flavor_update_extra_specs") | ||
| def compute_flavor_update_extra_specs(self, flavor, extra_specs={}): | ||
| conn = utils.get_openstack_connection() | ||
| for key, value in extra_specs.items(): | ||
| conn.compute.update_flavor_extra_specs_property(flavor, key, value) | ||
|
|
||
|
|
||
| @app.task(bind=True, name="osism.tasks.openstack.compute_flavor_delete_extra_specs") | ||
| def compute_flavor_delete_extra_specs_property(self, flavor, prop): | ||
| conn = utils.get_openstack_connection() | ||
| conn.compute.delete_flavor_extra_specs_property(flavor, prop) | ||
|
|
||
|
|
||
| @app.task(bind=True, name="osism.tasks.openstack.image_manager") | ||
| def image_manager( | ||
| self, |
There was a problem hiding this comment.
The function directly passes user-supplied arguments to an external command, which could lead to command injection if the inputs are not properly sanitized. This poses a significant security risk.
Recommendation:
Implement input validation or sanitization for the arguments before they are used in the command. This could involve checking for potentially dangerous patterns or using a list of allowed arguments to prevent unauthorized command execution.
2 participants
Nova is not used for baremetal provisioning. Therefore flavor creation for baremetal nodes is not required.