Enable Passwordless ssh connection for Multinodes docker #61

louie-tsai · 2024-05-29T23:02:02Z

Description

passwordless ssh connection is needed for multinodes runs.
In order to have successful mpi init over multiple nodes, changes in the PR are needed.

Related Issue

NA

Changes Made

The code follows the project's coding standards.
No Intel Internal IP is present within the changes.
The documentation has been updated to reflect any changes in functionality.

Validation

I have tested any changes in container groups locally with test_runner.py with all existing tests passing, and I have added new tests where applicable.

github-actions · 2024-06-04T08:16:52Z

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

Package	Version	Score	Details

Scanned Manifest Files

tylertitsworth

Please run our pre-commit over your files before you commit, this will make the pre-commit check pass since it can't write to your fork.

Also, please see the linter errors introduced by the lint check.

If you're going to add some expected functionality, like ssh communication, you need to test that communication. Add a new test in pytorch/tests/tests.yaml

tylertitsworth · 2024-06-05T15:51:09Z

pytorch/Dockerfile

+        && echo "  StrictHostKeyChecking no" >> /etc/ssh/ssh_config
+
+EXPOSE ${SSHD_PORT}
+CMD ["/usr/sbin/sshd", "-D"]


Any command will override this docker run ipex-multinode:latest my-command, instead this should be run as either an entrypoint or better in ~/.bashrc.

This will certainly be overridden by another CMD command.
I'd go with ENTRYPOINT:

ENTRYPOINT service ssh start && bash

addressed accordingly

pytorch/Dockerfile

tylertitsworth · 2024-06-05T15:54:41Z

pytorch/Dockerfile

+RUN chown intel /etc/ssh/ssh_host_ecdsa_key && \
+   chown intel /etc/ssh/ssh_host_rsa_key && \
+   chown intel /etc/ssh/ssh_host_ed25519_key


Instead of using the already-generated keys, you should generate random keys on startup with this script: https://github.com/intel/transfer-learning/blob/main/docker/hf_k8s/generate_ssh_keys.sh

understand. will follow the instructions

addressed accordingly

tylertitsworth · 2024-06-05T15:55:14Z

pytorch/Dockerfile

+RUN echo "cp "$SSH_KEY_PATH"/id_rsa /home/intel/.ssh/" >> ~/.bashrc \
+    && echo "cp "$SSH_KEY_PATH"/id_rsa.pub /home/intel/.ssh/" >> ~/.bashrc \
+    && echo "chmod 600 /home/intel/.ssh/id_rsa" >> ~/.bashrc \
+    && echo "chown intel /home/intel/.ssh/id_rsa" >> ~/.bashrc \
+    && echo "chown intel /home/intel/.ssh/id_rsa.pub" >> ~/.bashrc \
+    && echo "if [ ! -f /home/intel/.ssh/authorized_keys ]; then" >> ~/.bashrc \
+    && echo "  cat /home/intel/.ssh/id_rsa.pub > /home/intel/.ssh/authorized_keys" >> ~/.bashrc \
+    && echo "fi" >> ~/.bashrc


You could copy the existing config from root instead of re-writing this code

could you explain more? thanks

cp ~/.bashrc /home/intel/.bashrc && chown ... && chmod ...
Replace all instances of /home/intel with ~.

understand, but don't want to also bring some local changes on ~/.bashrc into docker instance. might just stay with appending new codes into bashrc file in the docker instance.

tylertitsworth · 2024-06-05T15:55:30Z

pytorch/docker-compose.yaml

+      args:
+        SSH_KEY_PATH: ${SSH_KEY_PATH}


If you specify an arg, make sure it has a default value.

addressed accordingly

pytorch/Dockerfile

ashahba · 2024-06-06T21:28:11Z

pytorch/Dockerfile

+        && echo "  StrictHostKeyChecking no" >> /etc/ssh/ssh_config
+
+EXPOSE ${SSHD_PORT}
+CMD ["/usr/sbin/sshd", "-D"]


This will certainly be overridden by another CMD command.
I'd go with ENTRYPOINT:

ENTRYPOINT service ssh start && bash

tylertitsworth

Make all the checks green
Add a test to pytorch/tests/tests.yaml for SSH so we know it's working.

louie-tsai · 2024-06-11T23:47:53Z

@tylertitsworth
fixed some pre-commit issues. please help to merge it if it looks good to you.

tylertitsworth · 2024-06-12T02:59:40Z

@tylertitsworth
fixed some pre-commit issues. please help to merge it if it looks good to you.

You have an issue with the idp build of this container: https://github.com/intel/ai-containers/actions/runs/9474288343/job/26103601878#step:5:4409

And your lint is failing due to hadolint warnings.

tylertitsworth · 2024-06-12T05:36:52Z

@louie-tsai perhaps it would just be easier to get your feedback on #124 and merge that one.

Can you try testing with the image via GITHUB_RUN_NUMBER=557 docker compose pull multinode?
We can add a test to verify the ssh support and do some more to verify that the IDP openssl functions properly.

louie-tsai · 2024-06-12T23:57:55Z

@tylertitsworth
fixed some pre-commit issues. please help to merge it if it looks good to you.

You have an issue with the idp build of this container: https://github.com/intel/ai-containers/actions/runs/9474288343/job/26103601878#step:5:4409
don't understand why we need to run apt-get inside conda environment. also didn't face the issue on our SPR machine.
"#66 ERROR: process "conda run -n idp /bin/bash -c apt-get update -y && apt-get install -y --no-install-recommends --fix-missing python3-dev gcc libgl1-mesa-glx libglib2.0-0 openssh-server net-tools virtualenv && rm /etc/ssh/ssh_host_key /etc/ssh/ssh_host_key.pub && apt-get clean && rm -rf /var/lib/apt/lists/*" did not complete successfully: exit code: 100"
is it a testing environment issue?

And your lint is failing due to hadolint warnings.

louie-tsai · 2024-06-12T23:58:51Z

@louie-tsai perhaps it would just be easier to get your feedback on #124 and merge that one.

Can you try testing with the image via GITHUB_RUN_NUMBER=557 docker compose pull multinode? We can add a test to verify the ssh support and do some more to verify that the IDP openssl functions properly.

I don't see ssh client key handling there. have you tested it on bare metal without k8s?

tylertitsworth · 2024-06-13T00:11:09Z

I don't see ssh client key handling there. have you tested it on bare metal without k8s?

@louie-tsai users can mount the key at runtime, that is the safest and intended way to communicate between nodes via docker.

tylertitsworth · 2024-06-13T00:19:54Z

don't understand why we need to run apt-get inside conda environment. also didn't face the issue on our SPR machine.

This is the environment that is built when PACKAGE_OPTION=idp, it's one of the versions of this multinode image.

is it a testing environment issue?

No, the weekly testing builds pytorch fine. https://github.com/intel/ai-containers/actions/runs/9432405319/job/26043878040

louie-tsai · 2024-06-13T00:20:06Z

@tylertitsworth
fixed some pre-commit issues. please help to merge it if it looks good to you.

You have an issue with the idp build of this container: https://github.com/intel/ai-containers/actions/runs/9474288343/job/26103601878#step:5:4409

don't understand why we need to run apt-get inside conda environment. also didn't face the issue on our SPR machine.
"#66 ERROR: process "conda run -n idp /bin/bash -c apt-get update -y && apt-get install -y --no-install-recommends --fix-missing python3-dev gcc libgl1-mesa-glx libglib2.0-0 openssh-server net-tools virtualenv && rm /etc/ssh/ssh_host___key /etc/ssh/ssh_host___key.pub && apt-get clean && rm -rf /var/lib/apt/lists/*" did not complete successfully: exit code: 100"
is it a testing environment issue?

And your lint is failing due to hadolint warnings.

only saw below warning related to multinode session. Do we need to fix all warning?
There are warning for original Dockerfile. How do we pass hasolint for original Dockerfile?
"
Dockerfile:76 DL3006 warning: Always tag the version of an image explicitly
Dockerfile:78 DL3008 warning: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version>
Dockerfile:122 SC2027 warning: The surrounding quotes actually unquote this. Remove or escape them.
Dockerfile:134 SC2027 warning: The surrounding quotes actually unquote this. Remove or escape them.
Dockerfile:153 DL3006 warning: Always tag the version of an image explicitly
"

louie-tsai · 2024-06-13T00:22:42Z

I don't see ssh client key handling there. have you tested it on bare metal without k8s?

@louie-tsai users can mount the key at runtime, that is the safest and intended way to communicate between nodes via docker.

if they have 100 nodes, do they need to mount key 100 times?

tylertitsworth · 2024-06-13T00:24:38Z

only saw below warning related to multinode session. Do we need to fix all warning? There are warning for original Dockerfile. How do we pass hasolint for original Dockerfile? " Dockerfile:76 DL3006 warning: Always tag the version of an image explicitly Dockerfile:78 DL3008 warning: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version> Dockerfile:122 SC2027 warning: The surrounding quotes actually unquote this. Remove or escape them. Dockerfile:134 SC2027 warning: The surrounding quotes actually unquote this. Remove or escape them. Dockerfile:153 DL3006 warning: Always tag the version of an image explicitly"

We ignore some warnings in https://github.com/intel/ai-containers/blob/main/.github/linters/.hadolint.yaml, the lint error clearly says you only need to fix the SC2027 errors, which has to do with quoting.

tylertitsworth · 2024-06-13T00:25:35Z

if they have 100 nodes, do they need to mount key 100 times?

Yes, I would assume it would be automated from some blob storage. This is exactly how it works in k8s.

louie-tsai · 2024-06-13T01:16:04Z

@tylertitsworth
fixed some pre-commit issues. please help to merge it if it looks good to you.

You have an issue with the idp build of this container: https://github.com/intel/ai-containers/actions/runs/9474288343/job/26103601878#step:5:4409

don't understand why we need to run apt-get inside conda environment. also didn't face the issue on our SPR machine. "#66 ERROR: process "conda run -n idp /bin/bash -c apt-get update -y && apt-get install -y --no-install-recommends --fix-missing python3-dev gcc libgl1-mesa-glx libglib2.0-0 openssh-server net-tools virtualenv && rm /etc/ssh/ssh_host___key /etc/ssh/ssh_host___key.pub && apt-get clean && rm -rf /var/lib/apt/lists/*" did not complete successfully: exit code: 100" is it a testing environment issue?

And your lint is failing due to hadolint warnings.

only saw below warning related to multinode session. Do we need to fix all warning? There are warning for original Dockerfile. How do we pass hasolint for original Dockerfile? " Dockerfile:76 DL3006 warning: Always tag the version of an image explicitly Dockerfile:78 DL3008 warning: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version> Dockerfile:122 SC2027 warning: The surrounding quotes actually unquote this. Remove or escape them. Dockerfile:134 SC2027 warning: The surrounding quotes actually unquote this. Remove or escape them. Dockerfile:153 DL3006 warning: Always tag the version of an image explicitly "

fixed the idp build issue accordingly

tylertitsworth · 2024-06-13T03:17:25Z

@louie-tsai let's move this discussion to #124

tylertitsworth · 2024-07-24T16:07:14Z

Hi @louie-tsai, this PR has been open a while and might be stale. Do you want us to close the PR or keep it open for you?

louie-tsai requested review from jitendra42 and tylertitsworth as code owners May 29, 2024 23:02

tylertitsworth changed the base branch from main to tylertitsworth/fork-pr-target May 29, 2024 23:15

This comment was marked as outdated.

Sign in to view

tylertitsworth assigned louie-tsai May 29, 2024

tylertitsworth changed the base branch from tylertitsworth/fork-pr-target to main May 29, 2024 23:26

louie-tsai force-pushed the ltsai/multinodes branch from 7dcd1c5 to 92b7e3c Compare June 5, 2024 08:18

tylertitsworth suggested changes Jun 5, 2024

View reviewed changes

ashahba suggested changes Jun 6, 2024

View reviewed changes

ashahba previously approved these changes Jun 7, 2024

View reviewed changes

tylertitsworth suggested changes Jun 7, 2024

View reviewed changes

louie-tsai force-pushed the ltsai/multinodes branch from 0997dc6 to 598df1b Compare June 10, 2024 18:14

louie-tsai requested a review from sharvil10 as a code owner June 11, 2024 23:16

louie-tsai dismissed ashahba’s stale review via 0997dc6 June 11, 2024 23:17

louie-tsai force-pushed the ltsai/multinodes branch from 9bf98c2 to 0997dc6 Compare June 11, 2024 23:17

louie-tsai added 8 commits June 11, 2024 16:44

add passwordless ssh connection feature required by mpi among nodes

1fdf1d7

need to use no root accout for passwordless ssh connection

53cd774

remove sudo and ssh key gen

06f5923

copy ssh key at runtime instead of build time

c5c2331

Update README.md for passwordless ssh connection

ebd58e6

Update README.md for read access of ssh key

3b1207c

rm ssh server key

36c47d4

changes for pre-commit

5fe8477

louie-tsai force-pushed the ltsai/multinodes branch from 020af92 to 5fe8477 Compare June 11, 2024 23:47

Update Dockerfile for idp build failure

91efcf6

tylertitsworth requested a review from sramakintel as a code owner June 18, 2024 17:01

tylertitsworth added the duplicate This issue or pull request already exists label Jun 18, 2024

louie-tsai mentioned this pull request Jun 26, 2024

Improve simple demo for multi-nodes with README and minor changes intel/torch-ccl#63

Closed

tylertitsworth added Review and removed duplicate This issue or pull request already exists labels Jun 28, 2024

tylertitsworth force-pushed the main branch from 78aabf8 to e47a2d5 Compare July 3, 2024 05:44

tylertitsworth closed this Aug 9, 2024

Enable Passwordless ssh connection for Multinodes docker #61

Enable Passwordless ssh connection for Multinodes docker #61

Uh oh!

Conversation

louie-tsai commented May 29, 2024 • edited by tylertitsworth Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Changes Made

Validation

Uh oh!

This comment was marked as outdated.

Uh oh!

github-actions bot commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Dependency Review

OpenSSF Scorecard

Scanned Manifest Files

Uh oh!

tylertitsworth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylertitsworth left a comment

Choose a reason for hiding this comment

Uh oh!

louie-tsai commented Jun 11, 2024

Uh oh!

tylertitsworth commented Jun 12, 2024

Uh oh!

tylertitsworth commented Jun 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

louie-tsai commented Jun 12, 2024

Uh oh!

louie-tsai commented Jun 12, 2024

Uh oh!

tylertitsworth commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tylertitsworth commented Jun 13, 2024

Uh oh!

louie-tsai commented Jun 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

louie-tsai commented Jun 13, 2024

Uh oh!

tylertitsworth commented Jun 13, 2024

Uh oh!

tylertitsworth commented Jun 13, 2024

Uh oh!

louie-tsai commented Jun 13, 2024

louie-tsai commented May 29, 2024 •

edited by tylertitsworth

Loading

github-actions bot commented Jun 4, 2024 •

edited

Loading

tylertitsworth commented Jun 12, 2024 •

edited

Loading

tylertitsworth commented Jun 13, 2024 •

edited

Loading

louie-tsai commented Jun 13, 2024 •

edited

Loading

tylertitsworth commented Jun 13, 2024 •

edited

Loading