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 support for --env-host #3557

Merged
merged 3 commits into from Jul 12, 2019

Conversation

@rhatdan
Copy link
Member

commented Jul 11, 2019

This flag passes the host environment into the container. The basic idea is to
leak all environment variables from the host into the container.

Environment variables from the image, and passed in via --env and --env-file
will override the host environment.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

@openshift-ci-robot

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

@rhatdan rhatdan force-pushed the rhatdan:env branch from cc0a85f to c3e3519 Jul 11, 2019

@rhatdan

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

@adrianreber Discovered a use case for this flag when running in MPI

@AkihiroSuda

This comment has been minimized.

Copy link

commented Jul 11, 2019

Wondering this should support exposing only variables with specific prefix string (eg. MPI_), for security reason

@adrianreber

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

Wondering this should support exposing only variables with specific prefix string (eg. MPI_), for security reason

For what I am trying to do I only need variables with PMIX_ and OMPI_. If there is an option to specify the prefix it would also work. Having something which just passes everything into Podman would be the easiest. But I am happy with both ways (all variables or only some).

I will try this PR to see if it works for my mpirun use case.

@adrianreber

This comment has been minimized.

Copy link
Collaborator

commented Jul 11, 2019

Besides the broken test case this is exactly what I was looking for. Thanks.

I tried it and now I can run an Open MPI program (rootless) on a system without InfiniBand:

$ mpirun --hostfile ~/hosts --mca orte_tmpdir_base /tmp/podman-mpirun podman run --env-host --security-opt label=disable -v /tmp/podman-mpirun:/tmp/podman-mpirun --userns=keep-id --net=host mpi-test /home/mpi/ring
Rank 0 has cleared MPI_Init
Rank 1 has cleared MPI_Init
Rank 0 has completed ring
Rank 0 has completed MPI_Barrier
Rank 1 has completed ring
Rank 1 has completed MPI_Barrier

I am using ring.c from Open MPI: https://raw.githubusercontent.com/open-mpi/ompi/master/orte/test/mpi/ring.c

This also works for InfiniBand based system by including the InfiniBand devices into the container:

$ mpirun --mca btl ^openib --hostfile ~/hosts --mca orte_tmpdir_base /tmp/podman-mpirun podman run --env-host -v /tmp/podman-mpirun:/tmp/podman-mpirun --security-opt label=disable --userns=keep-id --device /dev/infiniband/uverbs0 --device /dev/infiniband/umad0 --device /dev/infiniband/rdma_cm --net=host mpi-test /home/mpi/ring
Rank 0 has cleared MPI_Init
Rank 1 has cleared MPI_Init
Rank 0 has completed ring
Rank 0 has completed MPI_Barrier
Rank 1 has completed ring
Rank 1 has completed MPI_Barrier

I am running my tests with OpenMPI 4.0.1 on RHEL 8.

@rhatdan

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

A syntax of --env PMIX_* and --env OMPI_*, would be something I would consider. This would give you a little more fine grain control, and would match up closely to what we already do, but I still think adding --env-host is worth doing.

Add support for -env-host
This flag passes the host environment into the container.  The basic idea is to
leak all environment variables from the host into the container.

Environment variables from the image, and passed in via --env and --env-file
will override the host environment.

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

@rhatdan rhatdan force-pushed the rhatdan:env branch from c3e3519 to df75fc6 Jul 11, 2019

@rhatdan

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019


Environment variables within containers can be set using multiple different options: This section describes the presidence.

Presidence Order:

This comment has been minimized.

Copy link
@rst0git

rst0git Jul 11, 2019

s/Presidence/Precedence/

This comment has been minimized.

Copy link
@rhatdan

rhatdan Jul 11, 2019

Author Member

Fixed

Presidence Order:
**--env-host** : Host environment of the process executing podman is added.

Container image : Any enviroment variables specified in the contianer image.

This comment has been minimized.

Copy link
@rst0git

rst0git Jul 11, 2019

s/contianer/container/


Container image : Any enviroment variables specified in the contianer image.

**--env-file** : Any environment variables specfied via env-files. If multiple files specified, then they override each other in order of entry.

This comment has been minimized.

Copy link
@rst0git

rst0git Jul 11, 2019

s/specfied/specified/

This comment has been minimized.

Copy link
@rhatdan

rhatdan Jul 11, 2019

Author Member

Fixed


**--env-file** : Any environment variables specfied via env-files. If multiple files specified, then they override each other in order of entry.

**--env** : Any environment variables specified will overide previous settings.

This comment has been minimized.

Copy link
@rst0git

rst0git Jul 11, 2019

s/overide/override/

This comment has been minimized.

Copy link
@rhatdan

rhatdan Jul 11, 2019

Author Member

fixed

Add glob parsing for --env flag
Sometimes you want to add a few environmen variables based on the last field being a "*".

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

@openshift-ci-robot openshift-ci-robot added size/L and removed size/M labels Jul 11, 2019

Fix spelling mistakes in man pages and other docs
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

Added a new patch to go through and fix all spelling mistakes in documentation.

@rhatdan

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2019

@giuseppe

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

LGTM

@vrothberg
Copy link
Member

left a comment

LGTM

@vrothberg

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

/lgtm

@openshift-merge-robot openshift-merge-robot merged commit 6f3e7f7 into containers:master Jul 12, 2019

21 of 22 checks passed

tide Not mergeable.
Details
build_each_commit Task Summary
Details
ci/prow/images Job succeeded.
Details
ci/prow/lint Job succeeded.
Details
ci/prow/validate Job succeeded.
Details
gating Task Summary
Details
meta Task Summary
Details
release Task Summary
Details
special_testing_cross SPECIALMODE:darwin Task Summary
Details
special_testing_cross SPECIALMODE:windows Task Summary
Details
special_testing_in_podman Task Summary
Details
special_testing_rootless TEST_REMOTE_CLIENT:false Task Summary
Details
special_testing_rootless TEST_REMOTE_CLIENT:true Task Summary
Details
success Task Summary
Details
testing TEST_REMOTE_CLIENT:false image:fedora-29-libpod-5081463649730560 Task Summary
Details
testing TEST_REMOTE_CLIENT:false image:fedora-30-libpod-5081463649730560 Task Summary
Details
testing TEST_REMOTE_CLIENT:false image:ubuntu-18-libpod-5081463649730560 Task Summary
Details
testing TEST_REMOTE_CLIENT:true image:fedora-29-libpod-5081463649730560 Task Summary
Details
testing TEST_REMOTE_CLIENT:true image:fedora-30-libpod-5081463649730560 Task Summary
Details
testing TEST_REMOTE_CLIENT:true image:ubuntu-18-libpod-5081463649730560 Task Summary
Details
varlink_api Task Summary
Details
vendor Task Summary
Details
@adrianreber

This comment has been minimized.

Copy link
Collaborator

commented Jul 12, 2019

Thanks for the quick code changes. I mentioned this in my mail thread on the Open MPI user mailing list: https://www.mail-archive.com/users@lists.open-mpi.org/msg33342.html

For completeness I also want to mention here that intra node communication based on shared memory still does not work with Podman and Open MPI. So currently I have to disable the shared memory transport layer in Open MPI by telling mpirun to not use shared memory communication: --mca btl ^vader. I am guessing that Open MPI then switches to TCP for localhost communication.

@rhatdan

This comment has been minimized.

Copy link
Member Author

commented Jul 13, 2019

@adrianreber There is no reason to volume mount in /dev/shm with --ipc=host. --ipc=host does this automatically.

# touch /dev/shm/dan
# podman run --ipc=host fedora ls -l /dev/shm/dan
-rw-r--r--. 1 root root 0 Jul 13 10:55 /dev/shm/dan
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.