Podman Image task doesn't really support saving to disk

dmc5179 commented 3 years ago

/kind bug

Description

The podman image tasks looks like it should support exporting a container image to a tar ball. Looking at the documentation one would use the push options, combined with the transport as "docker-archive". In practice this doesn't work because the regex code in the plugins/modules/podman_image.py can't really handle it. It's difficult to explain, you kind of have to try it and see the issues. I'll include the examples and what happens below.

Steps to reproduce the issue:

Write a task to save a contianer image to disk
Run ansible

Describe the results you received:

Various error messages based on how I contstructed the task

Describe the results you expected:

A tar ball of the container image

Additional information you deem important (e.g. issue happens only occasionally):

Output of ansible --version:

ansible 2.9.17
  config file = /opt/data/danclark/workspace/openshift-disconnected/playbooks/ansible.cfg
  configured module search path = ['/home/danclark/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.9/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.9.1 (default, Jan 20 2021, 00:00:00) [GCC 10.2.1 20201125 (Red Hat 10.2.1-9)]

Output of podman version:

Version:      2.2.1
API Version:  2.1.0
Go Version:   go1.15.5
Built:        Tue Dec  8 09:37:50 2020
OS/Arch:      linux/amd64

Output of podman info --debug:

Version:      2.2.1
API Version:  2.1.0
Go Version:   go1.15.5
Built:        Tue Dec  8 09:37:50 2020
OS/Arch:      linux/amd64
[danclark@localhost playbooks]$ podman info --debug
host:
  arch: amd64
  buildahVersion: 1.18.0
  cgroupManager: systemd
  cgroupVersion: v2
  conmon:
    package: conmon-2.0.26-1.fc33.x86_64
    path: /usr/bin/conmon
    version: 'conmon version 2.0.26, commit: 777074ecdb5e883b9bec233f3630c5e7fa37d521'
  cpus: 8
  distribution:
    distribution: fedora
    version: "33"
  eventLogger: journald
  hostname: localhost.localdomain
  idMappings:
    gidmap:
    - container_id: 0
      host_id: 1000
      size: 1
    - container_id: 1
      host_id: 100000
      size: 65536
    uidmap:
    - container_id: 0
      host_id: 1000
      size: 1
    - container_id: 1
      host_id: 100000
      size: 65536
  kernel: 5.10.16-200.fc33.x86_64
  linkmode: dynamic
  memFree: 23347650560
  memTotal: 33555304448
  ociRuntime:
    name: crun
    package: crun-0.17-1.fc33.x86_64
    path: /usr/bin/crun
    version: |-
      crun version 0.17
      commit: 0e9229ae34caaebcb86f1fde18de3acaf18c6d9a
      spec: 1.0.0
      +SYSTEMD +SELINUX +APPARMOR +CAP +SECCOMP +EBPF +CRIU +YAJL
  os: linux
  remoteSocket:
    path: /run/user/1000/podman/podman.sock
  rootless: true
  slirp4netns:
    executable: /usr/bin/slirp4netns
    package: slirp4netns-1.1.8-1.fc33.x86_64
    version: |-
      slirp4netns version 1.1.8
      commit: d361001f495417b880f20329121e3aa431a8f90f
      libslirp: 4.3.1
      SLIRP_CONFIG_VERSION_MAX: 3
      libseccomp: 2.5.0
  swapFree: 21147672576
  swapTotal: 21147672576
  uptime: 11h 13m 12.5s (Approximately 0.46 days)
registries:
  search:
  - registry.fedoraproject.org
  - registry.access.redhat.com
  - registry.centos.org
  - docker.io
store:
  configFile: /home/danclark/.config/containers/storage.conf
  containerStore:
    number: 0
    paused: 0
    running: 0
    stopped: 0
  graphDriverName: overlay
  graphOptions:
    overlay.mount_program:
      Executable: /usr/bin/fuse-overlayfs
      Package: fuse-overlayfs-1.4.0-1.fc33.x86_64
      Version: |-
        fusermount3 version: 3.9.3
        fuse-overlayfs: version 1.4
        FUSE library version 3.9.3
        using FUSE kernel interface version 7.31
  graphRoot: /home/danclark/.local/share/containers/storage
  graphStatus:
    Backing Filesystem: extfs
    Native Overlay Diff: "false"
    Supports d_type: "true"
    Using metacopy: "false"
  imageStore:
    number: 4
  runRoot: /run/user/1000/containers
  volumePath: /home/danclark/.local/share/containers/storage/volumes
version:
  APIVersion: 2.1.0
  Built: 1607438270
  BuiltTime: Tue Dec  8 09:37:50 2020
  GitCommit: ""
  GoVersion: go1.15.5
  OsArch: linux/amd64
  Version: 2.2.1

First example tasks:

- name: pull image
  containers.podman.podman_image:
    name: 'registry.redhat.io/openshift3/ose-ansible'
    tag: 'v3.11'
    push: true
    #pull: false
    push_args:
      #compress: true
      dest: '/opt/data/openshift/ocp-3.11.318/images/infrastructure'
      transport: 'docker-archive'

This tasks results in a podman command that looks like this:

podman push registry.redhat.io/openshift3/ose-ansible:v3.11

That's clearly not going to work. What if I try

- name: pull image
  containers.podman.podman_image:
    name: 'registry'
    tag: '2'
    push: true
    #pull: false
    push_args:
      #compress: true
      dest: '/opt/data/openshift/images/infrastructure'
      transport: 'docker-archive'

That results in a podman command that looks like this

podman push registry:2 docker-archive:/opt/data/openshift/images/infrastructure

And the ansible output:

TASK [mirror_ocp_images : pull image] ********************************************************************************************************************************************************************************************************
fatal: [localhost]: FAILED! => {
    "changed": false
}

MSG:

Failed to push image registry:2: Error: error copying image to the remote destination: Error initializing destination docker-archive:/opt/data/openshift/images/infrastructure: error opening file "/opt/data/openshift/images/infrastructure": open /opt/data/openshift/images/infrastructure: is a directory

How about this:

- name: pull image
  containers.podman.podman_image:
    name: 'registry'
    tag: '2'
    push: true
    #pull: false
    push_args:
      #compress: true
      dest: '/opt/data/openshift/images/infrastructure/registry.tar'
      transport: 'docker-archive'

While results in a podman command like this:

podman push registry:2 docker-archive:/opt/data/openshift/images/infrastructure.tar

The only way I can get the image save to work is:

- name: pull image
  containers.podman.podman_image:
    name: 'registry'
    tag: '2'
    push: true
    #pull: false
    push_args:
      #compress: true
      dest: 'registry.tar'
      transport: 'docker-archive'

That will create a registry.tar file in the local working directory. But if I use the full name of the image:

- name: pull image
  containers.podman.podman_image:
    name: 'docker.io/library/registry'
    tag: '2'
    push: true
    #pull: false
    push_args:
      #compress: true
      dest: 'registry.tar'
      transport: 'docker-archive'

Ansible tries to push it to docker.io which won't work

TASK [mirror_ocp_images : pull image] ********************************************************************************************************************************************************************************************************
fatal: [localhost]: FAILED! => {
    "changed": false
}

MSG:

Failed to push image docker.io/library/registry:2: Getting image source signatures
Copying blob sha256:4f5aa08c5eaa01cf2d4f940eca389e3ef597af52836061640a46ce29b52bd18b
Copying blob sha256:8ebb9d6ed165ab026689173ee83d23eedfe8dd8128fd2dc5c0765f235e1f302d
Copying blob sha256:fb6b1a93008f9c70cded57e5469abfd34c383c053fd14a58ff5546ee287f5fe5
Copying blob sha256:6d2d8cb41f01e47d5959e4606e054fad6496d3d7711c270c446f3c6a83ff514b
Copying blob sha256:0fcbbeeeb0d7fc5c06362d7a6717b999e605574c7210eff4f7418f6e9be9fbfe
Error: error copying image to the remote destination: Error writing blob: Error initiating layer upload to /v2/library/registry/blobs/uploads/ in registry-1.docker.io: errors:
denied: requested access to the resource is denied
unauthorized: authentication required

It sort of looks like the docker-archive functionality was added but not really flushed out. The if/then cases and regex patterns in the code cause all sorts of weird cases.

dmc5179 commented 3 years ago

To add more detail, if the image name contains a '/', which is what the regex code is looking for, then it will always try to push to the registry, like docker.io. Because that's what push should do. But not what you really want if your push transport is docker-archive.

ocafebabe commented 3 years ago

Is there any workaround to this bug? This is really a blocker for us...

sshnaidm commented 3 years ago

I'll do my best to look at it this or next week, although this module has to be rewritten in some point. It's kind of legacy code that hard to maintain and fix.

dmc5179 commented 3 years ago

I haven't tested how this might effect every other permutation of this module but you can get the save function to work as expected pretty easily. I've attached a patch file for the containers.podman collection version 1.7.0 which works for me. I just patched the python file on disk under my user's collection path

podman_image_1.7.0_patch.txt

sshnaidm commented 3 years ago

@dmc5179 can you submit a PR with this patch? If not, I'll prepare from it.

dmc5179 commented 3 years ago

@sshnaidm Sure, I'll submit a PR today.

dmc5179 commented 3 years ago

PR submitted https://github.com/containers/ansible-podman-collections/pull/294

ocafebabe commented 3 years ago

@dmc5179 I see that your patch uses the "docker-archive" transport mode (which is logic), but what about "dir", what's the use case?

dmc5179 commented 3 years ago

@ocafebabe I was focused on the docker-archive transport in my use case but looking at the code again, there doesn't appear to be anything that handles the dir or oci-dir transports. I can take a look at adding that to the module. It shouldn't be that complicated. I'll add that and the tests to this PR

ocafebabe commented 3 years ago

@dmc5179 thanks mate, can't wait to test out your patch!

dmc5179 commented 3 years ago

The more I look at this module the more dreadful it is to untangle.

The task supports a transport called "dir" there is no transport called dir with podman. It's either docker-dir or oci-dir. I could go with the standard that everything is moving to, oci-dir, but that might not be what everyone wants. It almost makes more sense to fix the task parameter to support "oci-dir" and implement "dir" as "docker-dir". We could also just drop "dir" and change it to "oci-dir" or "docker-dir". I'd be more concerned with changing the name if the task actually worked in the past but since no one can use dir in the current implementation, chaning the name should haven't any impacts on prior uses.
The "format" parameter makes no sense: Manifest type to use when pushing an image using the 'dir' transport (default is manifest type of source).

When using "podman save" with the dir format (that's what podman calls the format) there is no option to set, oci, v2s1, v2s2. Which makes this parameter in the tasks pointless.

@sshnaidm Thoughts?

sshnaidm commented 3 years ago

@dmc5179 thanks for the investigation. according to man 5 containers-transports from podman-push help page, I can see that supported args are:

dir:path
docker://docker-reference
docker-archive:path[:{docker-reference|@source-index}]
docker-daemon:docker-reference|algo:digest
oci:path[:tag]
oci-archive:path[:tag]
ostree:docker-reference[@/absolute/repo/path]

by default it's docker://. oci-dir and docker-dir are options for podman-save command which saves image to tar.

I think we'd better to stick to podman push command here, while podman image save requires a separate module. WDYT?

dmc5179 commented 3 years ago

For me the question is, why is there a podman save and a podman push command? The man pages sure make it seem like podman push includes everything that podman save can do.

We're probably getting to the initial challege someone had implementing this ansible module due to the overlap. Much as it might seem easier to split them into 2 modules, if we put the tar producing transports into the save module, then the ansible push module wouldn't really line up with the podman man pages.

The PR I submitted probably shouldn't have tried to switch between podman push/save and just always used podman push. I was originally just looking for a quick fix to saving an image to disk. I'm going to go back to the main logic of that module and see if I can sort it so it works for the 5 transports and see what the code looks like. If the code looks nasty then that might be a sign we should split the modules.

rhatdan commented 3 years ago

Podman CLI is based on Docker CLI, which has a docker save command. Podman push, supports the same mechanisms as docker push, but also has support for additional transports that docker push does not support. One of these transports happens to be the docker-archive (Same as docker save and Podman save.)

sshnaidm commented 3 years ago

@dmc5179 I try to keep modules params close to docker modules ones, so people can easily move their playbooks to use podman. As I see in docker_image module there is parameter archive_path which actually saves image to module. Maybe we can use it here instead of push? Let's leave push to be whatever it is now and archive_path will be responsible for saving it to tar files with all options that push supports. We also won't have backward compatibility problems because it's a new param and push to dir never worked as you mentioned earlier. I've already created podman_save to anyone who wants just to save it.

ocafebabe commented 3 years ago

@sshnaidm I've just upgraded to 1.7.1 to test the new podman_save module but it doesn't work:

ERROR! couldn't resolve module/action 'containers.podman.podman_save'. This often indicates a misspelling, missing collection, or incorrect module path.

dmc5179 commented 3 years ago

@ocafebabe I think @sshnaidm just created that module. I don't think any work has been merged into it and I doubt anything from it has been merged into the actual containers.podman collection yet.

dmc5179 commented 3 years ago

@rhatdan I think what I was try to get at was that there seems to be almost complete overlap in functionality between the CLI podman push and podman save. Meaning that podman push can do anything that podman save can do. At least that's what I get from reading the man pages. If my reading is correct then I don't think that we should create another Ansible module for podman save. Probably going to be confusing to have 2 modules and if we implement them to fully mirror what the podman CLI can do then the Ansible modules will end up with the same functionality overlap.

@sshnaidm I took a roundabout way of saying, my original implementation what a hot fix to a problem I had. If I actually implement the logic for push fully in the Ansible module then I think we can do everything in the podman_image ansible module without being confusing. I haven't had time to actually pull the logic apart. I'm going to try to work on that now.

ocafebabe commented 3 years ago

@dmc5179 Yes I agree, but could we please all focus on making this feature work as expected?

dmc5179 commented 3 years ago

@ocafebabe Yep, that's what I'm working on. Funny enough I think it all boils down to this one line:

if '/' not in self.name or transport == 'docker-archive':

dmc5179 commented 3 years ago

@sshnaidm I updated the PR to simplify the change down to inverting the regex pattern on when to include the options. I also added in a could of tests. Let me know if I need to change something or if the PR can be merged. Thank you.

sshnaidm commented 3 years ago

@ocafebabe I didn't release it yet, you can try it by installing from git:: ansible-galaxy collection install git+https://github.com/containers/ansible-podman-collections. I'd like to wait for all fixes and release it hopefully this weekend. @dmc5179 I've already created podman_save and podman_load, I think duplication is not so big issue. If it exists in podman commands, it can exists in modules as well :smile: Going to review your PR asap, thanks!

ocafebabe commented 3 years ago

@dmc5179 @sshnaidm thanks a lot guys and let me know if you need help to test this!

ocafebabe commented 3 years ago

@sshnaidm what's the status on this?

sshnaidm commented 5 months ago

Should be fixed in https://github.com/containers/ansible-podman-collections/commit/4985d484155b2a0d6f90f21da99e6f152aad3d02

containers / ansible-podman-collections

Podman Image task doesn't really support saving to disk #205