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

fix proxy and SKHC flags #3

Closed
wants to merge 1 commit into from
Closed

fix proxy and SKHC flags #3

wants to merge 1 commit into from

Conversation

jpfleischer
Copy link
Member

@jpfleischer jpfleischer commented May 3, 2022

this pull request is meant to try and solve the issue at cloudmesh/cloudmesh-pi-burn#30

we tried to solve this issue on may 2nd but when i tried running gregor's patch, it gave problems such as:

ERROR: executing command 'host config abc,abc0[1-3] --local=yes --StrictHostKeyChecking=no'
UnboundLocalError: local variable 'proxy_jump' referenced before assignment

which we had seen before, but for some reason were still occurring.

thus i tried redoing the flags and parameters and tried every possible combination, such as StrictHostKeyChecking yes, no, such as if user supplies proxy, if user doesnt supply proxy, if local yes, if local no, and so on.

one issue i noticed was that Python differentiates, for instance, arguments.PROXY and arguments.proxy which inadvertently caused some issues.

i can verify that this pull request has been tested fully by me, such that i actually tried the config command for burning a cluster. i found that i had to do cms host config --local=no --proxy=red red0[1-3] for the temperature checker to return all the temperatures!

Please click here to view my results with --local=yes
CLOUDMESH PROXY CONFIG

Host red
HostName red.local
User pi
StrictHostKeyChecking no

Host red01
HostName red01.local
User pi
PreferredAuthentications publickey
StrictHostKeyChecking no
ProxyJump [email protected]

Host red02
HostName red02.local
User pi
PreferredAuthentications publickey
StrictHostKeyChecking no
ProxyJump [email protected]

Host red03
HostName red03.local
User pi
PreferredAuthentications publickey
StrictHostKeyChecking no
ProxyJump [email protected]

CLOUDMESH PROXY CONFIG

Timer: 0.0691s Load: 0.0100s host config --proxy=red red0[1-3]
(ENV3)
Sledgehammer@Sledgehammer MINGW64 ~

+--------+--------+-------+----------------------------+
| host | cpu | gpu | date |
|--------+--------+-------+----------------------------|
| red | 40.407 | 40.4 | 2022-05-03 05:36:38.556827 |
| red01 | 0 | 0 | 2022-05-03 05:36:44.876996 |
| red02 | 0 | 0 | 2022-05-03 05:36:44.759364 |
| red03 | 0 | 0 | 2022-05-03 05:36:45.049165 |
+--------+--------+-------+----------------------------+

Now click here to view my results with --local=no
CLOUDMESH PROXY CONFIG

Host red
HostName red
User pi
StrictHostKeyChecking no

Host red01
HostName red01
User pi
PreferredAuthentications publickey
StrictHostKeyChecking no
ProxyJump pi@red

Host red02
HostName red02
User pi
PreferredAuthentications publickey
StrictHostKeyChecking no
ProxyJump pi@red

Host red03
HostName red03
User pi
PreferredAuthentications publickey
StrictHostKeyChecking no
ProxyJump pi@red

CLOUDMESH PROXY CONFIG

Timer: 0.0740s Load: 0.0090s host config --local=no --proxy=red red0[1-3]

+--------+--------+-------+----------------------------+
| host | cpu | gpu | date |
|--------+--------+-------+----------------------------|
| red | 48.686 | 48.7 | 2022-05-03 05:40:18.535423 |
| red01 | 44.303 | 44.3 | 2022-05-03 05:40:19.264887 |
| red02 | 46.251 | 47.2 | 2022-05-03 05:40:19.265887 |
| red03 | 44.303 | 45.7 | 2022-05-03 05:40:19.659174 |
+--------+--------+-------+----------------------------+

i would like to reiterate that the above results were generated using my changes. the changes are in this pull request.

i did not commit because i wanted to confirm that it was ok to do so first

@dkkolli
Copy link
Collaborator

dkkolli commented May 4, 2022

This fix allows me to successfully ssh into my manager and workers on my windows machine! I think that the --local=yes option makes no difference in this update however? See my config files using and omitting the option:

$ cms host config red,red01
Adding to ~/.ssh/config

CLOUDMESH PROXY CONFIG

Host red
HostName red.local
User pi
StrictHostKeyChecking no

Host red01
HostName red01.local
User pi
PreferredAuthentications publickey
StrictHostKeyChecking no

CLOUDMESH PROXY CONFIG

$ cms host config --local=yes red,red01
Adding to ~/.ssh/config

CLOUDMESH PROXY CONFIG

Host red
HostName red.local
User pi
StrictHostKeyChecking no

Host red01
HostName red01.local
User pi
PreferredAuthentications publickey
StrictHostKeyChecking no

CLOUDMESH PROXY CONFIG

@laszewsk
Copy link
Member

laszewsk commented May 4, 2022

JP. please can you review this pull request. I have some open questions about it
a) there seem to be several non issues been addresses such as

  1. change of quotes from '' to ""
  2. move of names = parameter expansion
  3. unnecessary change in logic when using the mesh network while eleiminating the manager node from worker nodes as that seems not to be needed and returns the same result.

It is unclear to me if any of the changes are needed at all.
Also DK does not report if the code we implemented actually works or not and which error was found and addressed. Due to this reasons I can not merge as the impact of this merge on other activities is unkown and the description why its needed is not documented

@laszewsk
Copy link
Member

laszewsk commented May 4, 2022

DK can you please try our original code and document the things that do not work with it.

Copy link
Member

@laszewsk laszewsk left a comment

Choose a reason for hiding this comment

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

Pull request is rejected as it is not in sync with th elatest version.
Example: line 603 is not in the code

Please only fix things that are needed but before doing a pull request you need to check if you are indeed in sync with current repo

@laszewsk
Copy link
Member

laszewsk commented May 4, 2022

  1. moved ssh_config_output = f'\n##### CLOUDMESH PROXY CONFIG #####\n\n'
  2. made pi user the default

@laszewsk
Copy link
Member

laszewsk commented May 4, 2022

DK Please retest the original version and report back what does not work with an issue, then we address the issue.
I am not sure that your previous checkin fixes any issue yet. SO instead of looking at your pull request, please do a new test with the up to date code.

@dk
Copy link

dk commented May 4, 2022

@laszewsk error: E_WRONG_GITHUB_USER :D

@jpfleischer
Copy link
Member Author

this pull request is no longer needed with the changes made today to cloudmesh-inventory.

@jpfleischer jpfleischer closed this May 4, 2022
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.

4 participants