Pod Volumes Support

[cdoern]

added support for the --volume flag in pods using the new infra container design.
users can specify all volume options they can with regular containers.

Specifying the --volume flag causes the infra container to be populated with the specified mounts. All containers joining the pod then have a VolumesFrom container, causing them to inherit the mounts.

resolves #10379

Signed-off-by: cdoern [email protected]

[cdoern]

@mheon PTAL, not sure if I missed anything here.

[rhatdan]

LGTM

[TomSweeneyRedHat]

LGTM
but would like a @mheon head nod

[flouthoc]

Instead of command line i think more apt would be to add support volumeMount as kube spec, not sure but i think test should suffice mounting named volume or emptydir since that is how I think pod volume should work. This should be the required use-case of shared volume between pod containers.

[Please ignore this comment if that was not intention of the issue]

apiVersion: v1
kind: Pod
metadata:
  name: two-containers
spec:

  volumes:
  - name: shared-data
    emptyDir: {}

  containers:

  - name: first
    image: nginx
    volumeMounts:
    - name: shared-data
      mountPath: /usr/share/nginx/html

  - name: second
    image: debian
    volumeMounts:
    - name: shared-data
      mountPath: /pod-data
    command: ["/bin/sh"]
    args:
      - "-c"
      - >
        while true; do
          date >> /pod-data/index.html;

          echo Hello from the second container >> /pod-data/index.html;
          sleep 1;
        done

[cdoern]

@flouthoc this issue (I believe) is requesting a podman pod create option for volumes. The Kubernetes side of things already supports such a feature via the YAML.

[flouthoc]

@cdoern I tried a while ago afaik emptydir sharing between containers is not supported but i can confirm again.

[cdoern]

@mheon @TomSweeneyRedHat PTAL, I think I moved up the decoding to a good spot, podspec now matches container spec in terms of volume information.

[mheon]

LGTM

[rhatdan]

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, 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:

• OWNERS [rhatdan]

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

[mheon]

Wait - scratch that for a moment.

Are we not recording named volumes in the infra config?
/hold

[mheon]

This is wrong - named volumes are a separate field in the infra container's config, they are not in the spec

[cdoern]

see comment below

[mheon]

Nevermind, fire drill averted. They're stored correctly but we're retrieving from for podman pod inspect in the wrong way.

[TomSweeneyRedHat]

@mheon, want to remove the "hold" on this given the fire drill findings and the happy green test buttons?

[cdoern]

wait @mheon I am pretty sure .sortUserVolumes goes through the config to get the named volumes, see libpod/container_internal.go L 2151. I actually do not know why the spec is even passed, it is not used in the function.

container_inspect.go uses the same thing

[mheon]

It adds the additional specifications that all volumes listed must be user-specified, which is probably OK.

/hold cancel

[dispensable]

@ flouthoc this issue (I believe) is requesting a podman pod create option for volumes. The Kubernetes side of things already supports such a feature via the YAML.

Correcte me if I am wrong, so can we use spec.containers.volumeMounts[N].mountPropagation in the YAML file for podman play kube ?

eg:

- mountPath: xx
  name: xx
  mountPropagation: HostToContainer

Tested with podman 3.3.1/4.0.0-dev(with this pr merged), after podman play kube still got a rprivate bind

[mheon]

May not be supported, but should be easy to add. Please open a new issue.

[dispensable]

I was wrong, according to this func. just add a mountPath: /path/:rslave can get a rslave bind mount. Sorry for bothering you