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

one_host: Fix ID logic #8907

Merged
merged 4 commits into from
Oct 10, 2024
+38 −23
Merged

one_host: Fix ID logic #8907

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b51c2e
Fix one_host module
abakanovskii Sep 24, 2024
a229504
Add CHANGELOG fragment
abakanovskii Sep 24, 2024
250fe5e
PR Fixes
abakanovskii Sep 28, 2024
3d676e4
Update exceptions
abakanovskii Oct 4, 2024
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
2 changes: 2 additions & 0 deletions changelogs/fragments/8907-fix-one-host-id.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bugfixes:
- one_host - fix if statements for cases when ``ID=0`` (https://github.com/ansible-collections/community.general/issues/1199, https://github.com/ansible-collections/community.general/pull/8907).
35 changes: 24 additions & 11 deletions plugins/modules/one_host.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,16 +152,19 @@ def __init__(self):
def allocate_host(self):
"""
Creates a host entry in OpenNebula
self.one.host.allocate returns ID of a host
Returns: True on success, fails otherwise.

"""
if not self.one.host.allocate(self.get_parameter('name'),
self.get_parameter('vmm_mad_name'),
self.get_parameter('im_mad_name'),
self.get_parameter('cluster_id')):
self.fail(msg="could not allocate host")
else:
try:
self.one.host.allocate(self.get_parameter('name'),
self.get_parameter('vmm_mad_name'),
self.get_parameter('im_mad_name'),
self.get_parameter('cluster_id'))
self.result['changed'] = True
except Exception as e:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to be on the safe side, have you checked whether the other if statements you are modifying do not fall under the same situation?
Methods:

  • one.host.status()
  • one.host.delete()
  • one.host.update()
  • one.host.addhost()
    Don't they also trigger exceptions when parameters are invalid?

Copy link
Contributor Author

@abakanovskii abakanovskii Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

they are all almost the same in terms of error handling but it seems to me that other if statements look right

here are 2 examples for one.host.status(), 1 when running with no parameters, 2 when running with wrong parameter
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, in that case, wouldn't it be good to add the try-except around them below as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you please show which if statemets are wrong as well? From my perspective other if statements look right

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not go as far as saying they are wrong, but the same way that you transformed the if statement into a try-except, maybe those could benefit from that as well - line numbers on your branch (right hand side):

  • line 229
  • line 245
  • line 259
  • line 282
  • line 291

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, I completely missed them, thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Beware that by using this, we are assuming that those calls will raise exceptions when failing (which makes sense, but not all developers write code that way). I would suggest you confirm that if you haven't yet.

Other than that, LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it's too much IMO, no need to use it for every call :)
I simply do not use this module that much, so if one day I would need to use this module directly then might as well check if these are necessary

self.fail(msg="Could not allocate host, ERROR: " + str(e))

return True

def wait_for_host_state(self, host, target_states):
Expand Down Expand Up @@ -221,7 +224,9 @@ def run(self, one, module, result):
if current_state == HOST_ABSENT:
self.fail(msg='absent host cannot be put in disabled state')
elif current_state in [HOST_STATES.MONITORED, HOST_STATES.OFFLINE]:
if one.host.status(host.ID, HOST_STATUS.DISABLED):
# returns host ID integer
status_results = one.host.status(host.ID, HOST_STATUS.DISABLED)
if status_results or status_results == 0:
self.wait_for_host_state(host, [HOST_STATES.DISABLED])
result['changed'] = True
else:
Expand All @@ -235,7 +240,9 @@ def run(self, one, module, result):
if current_state == HOST_ABSENT:
self.fail(msg='absent host cannot be placed in offline state')
elif current_state in [HOST_STATES.MONITORED, HOST_STATES.DISABLED]:
if one.host.status(host.ID, HOST_STATUS.OFFLINE):
# returns host ID integer
status_results = one.host.status(host.ID, HOST_STATUS.OFFLINE)
if status_results or status_results == 0:
self.wait_for_host_state(host, [HOST_STATES.OFFLINE])
result['changed'] = True
else:
Expand All @@ -247,7 +254,9 @@ def run(self, one, module, result):

elif desired_state == 'absent':
if current_state != HOST_ABSENT:
if one.host.delete(host.ID):
# returns host ID integer
delete_results = one.host.delete(host.ID)
if delete_results or delete_results == 0:
result['changed'] = True
else:
self.fail(msg="could not delete host from cluster")
Expand All @@ -268,14 +277,18 @@ def run(self, one, module, result):
if self.requires_template_update(host.TEMPLATE, desired_template_changes):
# setup the root element so that pyone will generate XML instead of attribute vector
desired_template_changes = {"TEMPLATE": desired_template_changes}
if one.host.update(host.ID, desired_template_changes, 1): # merge the template
# merge the template, returns host ID integer
update_results = one.host.update(host.ID, desired_template_changes, 1)
if update_results or update_results == 0:
result['changed'] = True
else:
self.fail(msg="failed to update the host template")

# the cluster
if host.CLUSTER_ID != self.get_parameter('cluster_id'):
if one.cluster.addhost(self.get_parameter('cluster_id'), host.ID):
# returns cluster id in int
cluster_results = one.cluster.addhost(self.get_parameter('cluster_id'), host.ID)
if cluster_results and cluster_results == 0:
result['changed'] = True
else:
self.fail(msg="failed to update the host cluster")
Expand Down