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.

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

Add Node List Function With Baremetal and Lease Info #6

Closed
wants to merge 2 commits into from

Conversation

ajamias
Copy link
Collaborator

@ajamias ajamias commented Jul 5, 2024

No description provided.

Copy link
Collaborator

@tzumainn tzumainn left a comment

Choose a reason for hiding this comment

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

Okay, after taking a closer look at the fields being returned, I'm of the opinion that we should just add the power state properties into the values that esi node list returns; everything else is already returned by that command. I suppose that was the purpose of the command in the first place; to more easily merge the two.

@@ -16,6 +16,48 @@
from esi.lib import networks


def node_list_full_info(connection):
"""Get a list of nodes with more attributes
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd clarify this as "list of nodes with information from Ironic and ESI-Leap"

@@ -61,7 +103,7 @@ def network_list(connection, filter_node=None, filter_network=None):
'network_info': [
{
'baremetal_port': openstack.baremetal.v1.port.Port,
'network_port': [openstack.network.v2.port.Port] or [],
'network_ports': [openstack.network.v2.port.Port] or [],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this effect python-esiclient?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This does not affect python-esiclient, just a documentation error

baremetal_nodes = list(f2.result())

esi_nodes.sort(key=lambda node: node.name)
baremetal_nodes.sort(key=lambda node: node.name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I admit that this sort of implicit matching makes me nervous, especially because I think Ironic node names don't actually have to be unique. In addition it depends on the two services returning the same list of nodes - which they should do, but if the behavior changes in one then the results will be messed up.

@ajamias
Copy link
Collaborator Author

ajamias commented Jul 5, 2024

How would those properties be added? Is that something that be changed in esisdk? Or from the service endpoint? I'm guessing part of it will be adding to esi/lease/v1/node.py

@tzumainn
Copy link
Collaborator

tzumainn commented Jul 5, 2024

It's done here: https://github.com/CCI-MOC/esi-leap/blob/master/esi_leap/api/controllers/v1/node.py#L65. Possibly the esisdk would need to be updated as well.

If you'd like, I'd be fine with temporarily having the join restricted to the UI code, with plans to remediate later.

@ajamias
Copy link
Collaborator Author

ajamias commented Jul 5, 2024

I'm ok with fixing this later

@ajamias ajamias marked this pull request as draft July 5, 2024 15:36
@ajamias ajamias closed this Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants