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

LIU-378 Get node list from api/nodes GET result #238

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

calgray
Copy link
Collaborator

@calgray calgray commented Nov 14, 2023

When running the translator against a Master Manager instance setup by the run_engine.sh script a key error is appearing on the translator.

This is because the nodes REST API endpoint of the master manager is returning a dictionary:
http://0.0.0.0:8002/api/nodes -> {"nodes": ["172.17.0.4"]}

@awicenec
Copy link
Contributor

There is an error somewhere else and maybe that got lost during one of the merges before. The changes I have committed should actually never result in a list with just a single node and in addition all the nodes in the list should have a port as well. The reason behind that is that we need to be able to specify the port of the DIM and the NM, and with the previous list that was impossible, since it used the first n_island nodes in the list twice, once as a DIM and once as a NM (if the co-locate NM flag was set to True) and attached the default ports in order to access the managers. The updated list thus needs to have at least two entries in the minimal case, one for the DIM and one for the NM, like ['node1:8001', 'node1:8000'] and then this index error would never happen.

@myxie
Copy link
Collaborator

myxie commented Sep 25, 2024

I think we have updated this code in the meantime that resolves some of the issues this was originally intending to resolve. May we close this PR?

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.

3 participants