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

podman support (podman_container) #46362

Closed
dprince opened this issue Oct 1, 2018 · 42 comments · Fixed by #57956
Closed

podman support (podman_container) #46362

dprince opened this issue Oct 1, 2018 · 42 comments · Fixed by #57956
Labels
affects_2.8 This issue/PR affects Ansible v2.8 has_pr This issue has an associated PR. needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. support:core This issue/PR relates to code supported by the Ansible Engineering Team.

Comments

@dprince
Copy link

dprince commented Oct 1, 2018

SUMMARY

We desire a module to interact with podman on the CLI via Ansible.

This would be similar to ./modules/cloud/docker/docker_container.py which has similar features to podman's CLI. Podman also supports some new "pod like" syntax which would also be nice to support.

ISSUE TYPE
  • A module called podman_container
@ansibot
Copy link
Contributor

ansibot commented Oct 1, 2018

@dprince Greetings! Thanks for taking the time to open this issue. In order for the community to handle your issue effectively, we need a bit more information.

Here are the items we could not find in your description:

  • ansible version
  • component name

Please set the description of this issue with this template:
https://raw.githubusercontent.com/ansible/ansible/devel/.github/ISSUE_TEMPLATE.md

click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Oct 1, 2018
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Oct 1, 2018
@samdoran
Copy link
Contributor

samdoran commented Oct 1, 2018

I am working on the following modules for inclusion in TripleO:

  • podman_image
  • podman_container

I also plan to write a podman connection plugin once the two modules are done.

@felixfontein
Copy link
Contributor

A podman connection plugin has been added in #47519.

The modules @samdoran mentioned are available here: https://github.com/openstack/tripleo-ansible/tree/master/tripleo_ansible/library
I think it would be nice to integrate these into Ansible and extend them to offer more choice.

@ikke-t
Copy link

ikke-t commented Feb 6, 2019

I created a role for myself to do that until there is such systemd enabled podman module. It would be great to have such feature as this issue is asking. My role here: https://github.com/ikke-t/podman-container-systemd

But that won't take parameters for podman like docker_container does for docker. But that has what such podman_container should do for systemd.

@sshnaidm
Copy link
Contributor

Started to work on this

@ikke-t
Copy link

ikke-t commented Jun 17, 2019

I also spend a moment looking at this at some point, looking for time to do it, which never happened. There would be a way to re-use the awx kubernetes templates, and build it all with podman play kube. At the time I got stopped due podman didn't do volumes from play kube, but they fixed it after I created ticket about it.

Good luck!

@sshnaidm
Copy link
Contributor

Created a PR #57956, still polishing it and testing. Will appreciate any help with reviews and/or testing.
The known issue currently is sometimes might be not working "command" overriding, still looking into it.
Calculation of differences between running and configured container (for better idempotency) is not implemented yet, will do in next patches.

@ikke-t
Copy link

ikke-t commented Jun 17, 2019

Never mind my previous comment, I didn't read the topic. I was thinking of another issue, it was about awx and podman. Regarding calculations, this issue was made for it, perhaps there is something for you: containers/podman#2600

@felixfontein
Copy link
Contributor

Is there any update on podman_container?

@ansibot ansibot added the has_pr This issue has an associated PR. label Jul 29, 2019
@alexanderadam
Copy link

@felixfontein isn't it merged and therefore fixed already?

@felixfontein
Copy link
Contributor

@alexanderadam it has been merged prematurely and has been reverted (#58422). @samdoran and @sshnaidm were talking on IRC about creating a new PR eventually, so that the module will be there for 2.9, but I haven't seen anything and was wondering what the current state is. Especially since feature freeze for 2.9 is in about one month.

@sshnaidm
Copy link
Contributor

When reverted my patch @samdoran said he has something WIP that he's working on his own, you can ask him. I didn't hear anything since then.
I think I can share my reverted patch as package in pypi or module in galaxy, while adding missed things there.

@alexanderadam
Copy link

alexanderadam commented Jul 30, 2019

@felixfontein ah okay, I didn't know that. Thank you for updating me! 👍

@samdoran
Copy link
Contributor

I have a very early WIP here. It's pretty much a skeleton that needs to be filled out. Lots of work to be done still.

@felixfontein
Copy link
Contributor

@samdoran do you plan to have this done before 2.9 feature freeze?

@samdoran
Copy link
Contributor

samdoran commented Aug 2, 2019

@felixfontein No, I am not actively working on that module.

@thomasmckay
Copy link

I would use this, if available.

@LukeShortCloud
Copy link
Contributor

The TripleO community is continuing work on a podman_container module. We would really appreciate some help. Once it meets the standards of Ansible (idempotency, tests, etc.) we aim to get it merged upstream directly into Ansible.

@felixfontein
Copy link
Contributor

Spontaneously, it looks like the version you have is similar to the one from #57956. How (and where) should development proceed? I can help reviewing, but for that I need to know where I can write something :)

Also, as we were discussing in various places (issues/PRs here and IRC), there are some design issues with the current module which should be discussed first before more implementation is done - especially with regard to idempotency. From how docker_container works internally, I've learned that it would be very nice if the code for handling all aspects for one option - parsing (if necessary), checking idempotency and making sure a container is created / changed that way - are close together. Otherwise, you end up with a rather big mess (like docker_container :) ).

@sshnaidm
Copy link
Contributor

@felixfontein you're right, I used #57956 as a base for this module. I think it definitely has 95% of all functionality, except of idempotency of running containers. Now we develop it in tripleo - Openstack Gerrit. It looks like more comfortable place for collaboration and reviews, unlike here. Also we start to use it in tripleo and it will be well tested. For anybody that want to review or contribute, just send patches or review: https://review.opendev.org/#/q/project:openstack/tripleo-ansible
What are design issues that you mean, except of idempotency?
Wrt idempotency of running containers, I don't think there are other options that comparing input params and inspection info from container. If you have other ideas, I'd be happy to know.
The question is how we bring format of params to format of inspection, or vice versa. I saw how it's done in docker module, and to be honest having function for comparing specific parameter seems reasonable for me. Some of them could be just used as is, some should be converted, some use explicit defaults of podman and we need to consider this, and some use implicit defaults of podman, and it should be taken into account also. (And what to do if podman defaults change I'm not sure)
Maybe we even will need changes in podman inspection functionality as well.
I'd be glad to discuss it either on IRC, or openstack review comments. I'm looking into idempotency exactly these days, so would be glad to get any ideas how to do it efficiently and nice way.

@felixfontein
Copy link
Contributor

@felixfontein you're right, I used #57956 as a base for this module. I think it definitely has 95% of all functionality, except of idempotency of running containers.

I would say that idempotency is at least 2/3 of the whole functionality.

What are design issues that you mean, except of idempotency?

Idempotency is for me the most basic part of a module, especially for a module of this size. To decide how the module works, you first need to find out how to handle idempotency, everything else is rather straightforward after that.

Wrt idempotency of running containers, I don't think there are other options that comparing input params and inspection info from container. If you have other ideas, I'd be happy to know.

Definitely. The main question is: how?

The question is how we bring format of params to format of inspection, or vice versa. I saw how it's done in docker module, and to be honest having function for comparing specific parameter seems reasonable for me. Some of them could be just used as is, some should be converted, some use explicit defaults of podman and we need to consider this, and some use implicit defaults of podman, and it should be taken into account also. (And what to do if podman defaults change I'm not sure)

Exactly.

I still think it is better to start with a subset of the module which covers some of the most important and interesting options, completely implement that, and then add all the other options. That will make implementation a lot easier.

Maybe we even will need changes in podman inspection functionality as well.

Assuming that the podman team really made podman behave like docker, everything important should already be there.

I'd be glad to discuss it either on IRC, or openstack review comments. I'm looking into idempotency exactly these days, so would be glad to get any ideas how to do it efficiently and nice way.

My personal idea on how to implement it in a better way is as follows.

I would have a class instance per option which handles:

  1. parsing and validating the relevant module input;
  2. converting it to a podman command line argument (for container creation);
  3. handles updating existing containers, and can say whether it can update the container or whether it needs to be recreated;
  4. can compare the state of an existing container to the user-specified option value, and tell whether it is different or not;
  5. know which version of podman is required (and know differences for various podman versions, sometimes stuff gets renamed or moved around).

There should be some utilities (classes and functions to instantiate them) so that for most options, this can be done in 1-2 lines.

Also, there's the question on whether you want the module to behave like the docker_container module with all its quirks (I tried to sum them up here: https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/docker/docker_container.py#L28-L36), or differently. (I would definitely stop using default values for container-affecting options, these really only cause problems. See the notes there.) Doing this differently also has its own dangers. This is something that should be discussed before discussing any implementation.

@samdoran
Copy link
Contributor

I agree with @felixfontein on the design goals of podman_container. We need separate functions for comparing each aspect of the state of a running container to the state specified in the playbook. The module would then build the appropriate command to run in a much more targeted manner rather than just appending args based on module parameters.

We will also have to account for differences in Podman versions. I have already been bit by that with podman_image_facts. Nothing too major, but something that needs to be accounted for in the module design.

We could start with the most common attributes, such as ports and mounts, and build from there.

Also, there's the question on whether you want the module to behave like the docker_container module with all its quirks...

My original intent was to allow playbook authors to switch from docker_* to podman_* simply by changing the module name. That worked for podman_image and podman_image_facts, but I'm not sure it will work for podman_container. Especially since we don't want to bring along any interface quirks since we are starting fresh.

I vote we make podman_container a fresh start and learn from the work done over the years in podman_container and not try to maintain interface/parameter parody between docker_container and podman_container.

@sshnaidm
Copy link
Contributor

The module that is located currently in tripleo-ansible package is different from that was merged and reverted here. I'd suggest to check its code to see what we discuss here at all: podman_container.py
@felixfontein I think all this is reasonable and partially already solved in current module. I was going the same way, but instead of having 80 classes I'd prefer to have 2 classes with 80 methods each one for each parameter.
Please see how idempotency can be implemented in WIP patch: https://review.opendev.org/688574
I think it's way easier to maintain module with changing defaults and parameters, when they're centralized and concentrated in one place. All parameters that are not set in the module args will get their default values according to podman version. Please comment about this patch in gerrit what do you think.
I don't think there is any need to start this module from scratch, the current solution with idempotency looks much better than docker module. Podman/docker compatibility can be achieved as best effort, a lot of options are pretty same which makes it possible for most of popular use-cases.

@felixfontein
Copy link
Contributor

@sshnaidm I did look at its current state, it still had a big TODO where the idempotency code should be.

Your WIP patch will work, but also doesn't scale very well. Think about how you would add --diff support, or something like comparisons: from docker_container. With the current design, you have to either add a lot of methods (for --diff) or rewrite huge chunks (for comparisons:).

And finally, what about container differences which can be changed without needing to restart the container? I don't know what that would be for podman, but for docker_container there are several ones (blkio_weight, cpu_period, cpu_quota, cpu_shares, cpuset_cpus, cpuset_mems, kernel_memory, memory, memory_reservation, memory_swap). These need to be handled in a sensible way as well. (Also note that for older docker API versions, these cannot be updated, so docker_container has to know about that as well and only update them for newer API versions, but for olders treat them like other module options which require container recreation.)

And this approach also opens up the possibility of forgetting something / something getting out of sync. The docker_container module had multiple options which never worked properly (because they were forgotten in idempotence checks), or weren't even used at all (neither for container creation/startup nor for idempotence).

@sshnaidm
Copy link
Contributor

@felixfontein

Your WIP patch will work, but also doesn't scale very well. Think about how you would add --diff support, or something like comparisons: from docker_container. With the current design, you have to either add a lot of methods (for --diff) or rewrite huge chunks (for comparisons:).

I added diff support in current revision: https://review.opendev.org/#/c/688574/ Fortunately it was easy, just create text with "before" and "after" and then "diff" callback will do all for you. Difference is collected in the same place where it's checked for difference, I think it's quite logical to do.
Also added support for "check mode", where nothing is done but just podman commands (that would run if no check_mode is set) are collected and printed.
But I can't predict how inspection would change and what info will be returned. I'm not sure it's even possible predict values like "HostsPath": "/tmp/1000/overlay-containers/e8a31dedd6c67c8bf218a839775c785ba6e100c8446ddfddc8ca4a2f6b058d4d/userdata/hosts", and I don't think we should do it at all.

And finally, what about container differences which can be changed without needing to restart the container? I don't know what that would be for podman, but for docker_container there are several ones (blkio_weight, cpu_period, cpu_quota, cpu_shares, cpuset_cpus, cpuset_mems, kernel_memory, memory, memory_reservation, memory_swap).

As was discussed in IRC podman doesn't support it and it's not in current roadmap.

And this approach also opens up the possibility of forgetting something / something getting out of sync. The docker_container module had multiple options which never worked properly (because they were forgotten in idempotence checks), or weren't even used at all (neither for container creation/startup nor for idempotence).

I hope when we arrange all arguments work in one place it would be easy to add/remove various arguments, I'll try to do some examples of newly added arguments like "env-host".

@felixfontein
Copy link
Contributor

Your WIP patch will work, but also doesn't scale very well. Think about how you would add --diff support, or something like comparisons: from docker_container. With the current design, you have to either add a lot of methods (for --diff) or rewrite huge chunks (for comparisons:).

I added diff support in current revision: https://review.opendev.org/#/c/688574/ Fortunately it was easy, just create text with "before" and "after" and then "diff" callback will do all for you. Difference is collected in the same place where it's checked for difference, I think it's quite logical to do.

Yes, that's what the docker_container module is doing as well.

Also added support for "check mode", where nothing is done but just podman commands (that would run if no check_mode is set) are collected and printed.

And I hope changed is set to True :)

But I can't predict how inspection would change and what info will be returned. I'm not sure it's even possible predict values like "HostsPath": "/tmp/1000/overlay-containers/e8a31dedd6c67c8bf218a839775c785ba6e100c8446ddfddc8ca4a2f6b058d4d/userdata/hosts", and I don't think we should do it at all.

I agree. (The docker_container module doesn't do that either. I think it simply returns the current container's inspect results, or none in case the container currently doesn't exist.)

And this approach also opens up the possibility of forgetting something / something getting out of sync. The docker_container module had multiple options which never worked properly (because they were forgotten in idempotence checks), or weren't even used at all (neither for container creation/startup nor for idempotence).

I hope when we arrange all arguments work in one place it would be easy to add/remove various arguments, I'll try to do some examples of newly added arguments like "env-host".

That would definitely be a good thing.

BTW, I noticed that you didn't address a lot of things I pointed out in the review in June (#57956 (review)), like mount and security_opt being strings instead of lists / dicts.

Also, is it not possible to connect a container to multiple networks (like in docker)? The module accepts only one network (as a str).

@sshnaidm
Copy link
Contributor

@felixfontein

And I hope changed is set to True :)

If it's gonna change, then yes :)
Wrt options, --security-opt is better to handle as a list, because that's how they're represented in inspection:

"SecurityOpt": [
                "no-new-privileges",
                "label=disable"
            ],

and as you can see, it's not always a dict. It could be just no-new-privileges.
About mount options, it could be a dict, although there's still non-keyword option like bind-nonrecursive. Maybe it can be used with boolean value, not sure about it, it's not obvious.

I've added a check about podman version like that:

    def addparam_env_host(self, c):
        self.check_version('--env-host', minv='1.5.0')
        return c + ['--env-host=%s' % self.params['env_host']]

that way module with throw AnsibleOptionsError if there is podman with version less than minv (in this case less than 1.5.0. (Maybe better other error or just fail a module?)
The same thing for maximum version with maxv.
Defaults are all in one place and there is way to correct them for different podman versions.

class PodmanDefaults:
    def __init__(self, module, podman_version):
        self.module = module
        self.version = podman_version
        self.defaults = {
    ............skipped
            "oom_score_adj": 0,
            "workdir": "/",
        }

    def default_dict(self):
        # make here any changes to self.defaults related to podman version
        if version.parse(self.version) > version.parse('1.6.5'):
            self.defaults.update({'some_param': 'new_default_value'})
        return self.defaults

But I hope we'll never use it, because changing defaults in different versions is really bad pattern.

Also, is it not possible to connect a container to multiple networks (like in docker)?

No, not at this moment. But actually we can do it as a list, just in case it will be supported someday.
Please check it out: https://review.opendev.org/#/c/688574/

@sshnaidm
Copy link
Contributor

Also, is it not possible to connect a container to multiple networks (like in docker)?

Well, a little fix, it's possible, just need to specify a comma separated string:
sudo podman run --network test1,test2,test alpine sh
So we definitely can do it as a list.

@felixfontein
Copy link
Contributor

Well, a little fix, it's possible, just need to specify a comma separated string:
sudo podman run --network test1,test2,test alpine sh
So we definitely can do it as a list.

Can you also do things like specify an IP address the container should use in a network, which hostname / aliases it should have in there, ...? (Docker allows you to do that.)

Wrt options, --security-opt is better to handle as a list, because that's how they're represented in inspection:

"SecurityOpt": [
                "no-new-privileges",
                "label=disable"
            ],

and as you can see, it's not always a dict. It could be just no-new-privileges.

In that case, list makes sense. For idempotence checking, you'd want to accept different orders of the list, though. (Or just check for subset.)

About mount options, it could be a dict, although there's still non-keyword option like bind-nonrecursive. Maybe it can be used with boolean value, not sure about it, it's not obvious.

bind-nonrecursive could also behave like a boolean. Just because the Podman CLI doesn't accept it as a boolean does not mean you have to do the same in the module options.

I've added a check about podman version like that:

    def addparam_env_host(self, c):
        self.check_version('--env-host', minv='1.5.0')
        return c + ['--env-host=%s' % self.params['env_host']]

that way module with throw AnsibleOptionsError if there is podman with version less than minv (in this case less than 1.5.0. (Maybe better other error or just fail a module?)

Modules should call module.fail_json(). Or raise an exception, but make sure that they also catch it and call module.fail_json(). Just raising an exception without catching it leads to a module crash.

@sshnaidm
Copy link
Contributor

@felixfontein @samdoran please look at new version of podman_container with idempotency: https://opendev.org/openstack/tripleo-ansible/src/branch/master/tripleo_ansible/ansible_plugins/modules/podman_container.py#L1341
It still doesn't have idempotency for a half of parameters and it takes a very long time because almost each one of parameters needs to be treated separately for idempotency.
If there are still issues about design, let's discuss them.

@jxs
Copy link

jxs commented Nov 15, 2019

Hi all, thanks for getting this moving, is this module planned to support creation of pods or should there be another one dedicated solely for it, like docker_network but for pod management?
thanks

@ssbarnea
Copy link
Member

Lets try to march currently existing docker modules in order to

  • ease cross migration
  • avoid creating an megalosaurus module that tried to do everything and that ends up being unmaintainable
  • lets aim to get this in ASAP, as there is a bit g need for it

@sshnaidm
Copy link
Contributor

Hi all, thanks for getting this moving, is this module planned to support creation of pods or should there be another one dedicated solely for it, like docker_network but for pod management?
thanks

In module that I prepared there is an option pod: for container. If there is such pod, container will be attached to it. If there is no such pod, it will be created on the fly and container will be attached to it. (Exactly as it works with podman container run --pod newpod ... ). For more advanced pod management it will be a separate podman_pod module.

@jxs
Copy link

jxs commented Nov 15, 2019

Lets try to march currently existing docker modules in order to

* ease cross migration

* avoid creating an megalosaurus module that tried to do everything and that ends up being unmaintainable

* lets aim to get this in ASAP, as there is a bit g need for it

yeah agree with it

In module that I prepared there is an option pod: for container. If there is such pod, container will be attached to it. If there is no such pod, it will be created on the fly and container will be attached to it. (Exactly as it works with podman container run --pod newpod ... ). For more advanced pod management it will be a separate podman_pod module.

ok awesome! ❤️

@sshnaidm
Copy link
Contributor

As I understand from the current roadmap there are the plans to extract all community modules, including containers ones, to collections and external repos. Not sure it even will be under ansible umbrella.
If this is the case, I doubt if it makes any sense to continue to submit podman modules here. I have a few on review and a bunch in a pipeline. Although their merge goes quite slow. Maybe if anyway it will be kicked off the ansible repo, better to submit them to different place?
libpod repo seems like a best logical place for them, having ansible modules aligned with latest versions of podman and always up to date. Also I think work and merge there will be much more fast and easy going.

@kawing-chiu
Copy link

Hi. Is there any progress recently? Or is this issue being tracked in some other place?

@sshnaidm
Copy link
Contributor

sshnaidm commented Jan 17, 2020

As all community modules will go from ansible in 2.10, podman modules are going to be in repository https://github.com/containers/ansible-podman-collections This is tracked in issue containers/podman#4654
I hope to do some progress next week when finishing moving openstack modules.
Till then you can try podman_container from TripleO :)

@aitorpazos
Copy link

This may be late to the party, but couldn't we make docker_container support podman as well. In a similar way I can ln -s podman docker and have all my scripts working, not being able to re-use the same playbooks is a big hurdle to move to podman..

@felixfontein
Copy link
Contributor

@aitorpazos that won't work this simply, since docker_container uses Docker SDK for Python to talk directly to the Docker daemon, instead of using the docker CLI executable. But there are plans for podman to support parts of the Docker daemon API; once that is on the way, it could well be that docker_container works with podman with slight modifications, or even out of the box! No idea what the exact state of this is right now though... (https://podman.io/new/2020/01/17/new.html)

@aitorpazos
Copy link

@felixfontein fair enough. That makes sense, thanks

@aitorpazos
Copy link

According to 1.7.1 podman RELEASE_NOTES currently in master (not released at the time of writing) podman service will expose that API from 1.7.1 as alpha.

@ansible ansible locked and limited conversation to collaborators May 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 has_pr This issue has an associated PR. needs_info This issue requires further information. Please answer any outstanding questions. needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.