Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it possible to list all hosts of an aggregate #1116

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+11 −0
Open

Make it possible to list all hosts of an aggregate #1116

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions osism/commands/compute.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,12 @@ def get_parser(self, prog_name):
type=str,
help="Filter by domain ID",
)
parser.add_argument(
"--aggregate",
default=None,
type=str,
help="Filter by aggregate",
)
parser.add_argument(
"host",
nargs="?",
Comment on lines 96 to 107
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method get_parser in the ComputeList class defines several command-line arguments for filtering compute resources. While the implementation is straightforward, it lacks explicit validation or sanitization of the input parameters such as project, domain, aggregate, and host. This could potentially lead to issues if malformed or unexpected inputs are provided.

Recommendation:

  • Implement input validation and sanitization for the command-line arguments to ensure they meet expected formats and criteria before being used in further operations. This will enhance the security and robustness of the command handling.

Expand All @@ -110,6 +116,7 @@ def take_action(self, parsed_args):
conn = get_cloud_connection()
domain = parsed_args.domain
project = parsed_args.project
aggregate = parsed_args.aggregate

result = []
if host:
Comment on lines 116 to 122
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method take_action uses all_projects=True when retrieving servers, which could lead to unauthorized access to data across different projects if not properly handled or intended. This could be a significant security risk depending on the deployment environment.

Recommendation:

  • Ensure that the use of all_projects=True is intentional and consider implementing additional checks or permissions to restrict access based on user roles or specific conditions.

Comment on lines 116 to 122
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The take_action method lacks validation for the host parameter before it's used to filter servers. This could lead to errors or unexpected behavior if an invalid or null host is provided.

Recommendation:

  • Implement validation for the host parameter to ensure it is not null and possibly verify its format or existence in the system before proceeding with the operation.

Comment on lines 116 to 122
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation in lines [116-122] does not handle potential exceptions that might arise from network issues or API errors when interacting with the cloud services. This could lead to unhandled exceptions and application crashes.

Recommendation:

  • Implement exception handling around the cloud service interactions to manage errors gracefully and provide feedback to the user.

Comment on lines 116 to 122
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The filtering logic in lines [116-122] does not account for the possibility that both project and domain filters might be provided simultaneously. This could lead to unexpected behavior where servers are filtered by project only if they match, ignoring the domain condition unless project is not specified. This is a logical flaw that could confuse users expecting both conditions to be applied.

Recommendation:

  • Modify the filtering logic to ensure that both project and domain conditions are evaluated independently. This could involve restructuring the if-else chain to check each condition separately and combining the results appropriately.

Comment on lines 116 to 122
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The take_action method in lines [116-122] lacks comprehensive error handling for the operations involving cloud service interactions. Given the critical nature of these operations, any failure in the cloud API calls, such as network issues or service unavailability, could lead to unhandled exceptions and potentially crash the application.

Recommendation:

  • Wrap the cloud service interactions within a try-except block to catch exceptions like openstack.exceptions.HttpException and handle them gracefully. Provide meaningful error messages to the user and ensure the application can continue running or exit cleanly.

Expand All @@ -132,6 +139,10 @@ def take_action(self, parsed_args):
)

else:
hypervisors = conn.compute.hypervisors()
for hypervisor in conn.compute.hypervisors(details=True):
print(hypervisor)

for service in conn.compute.services(**{"binary": "nova-compute"}):
result.append(
[
Comment on lines 139 to 148
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In lines [139-148], the method prints details of hypervisors and compute services. However, the loop in lines [143-145] that prints each hypervisor detail is inefficient as it makes a redundant API call to conn.compute.hypervisors(details=True) inside the loop. This results in unnecessary network requests, reducing the performance of the method.

Recommendation:

  • Fetch the list of hypervisors with details once before the loop starts and store it in a variable. Use this variable inside the loop to access hypervisor details. This change will reduce the number of API calls and improve the method's efficiency.

Expand Down