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

Recreate container if container definition has changed and restart is true, simplify logic #820

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

va1entin
Copy link

Hi team,

this PR is my proposal to fix #816. It changes make_started in podman_container_lib.py so that when a container definition has changed and restart/force_restart are True the container will be recreated and run to reflect the changes rather than restarted without reflecting any changes.

I also removed a call to container.restart later in the same function because the case it covers is sufficiently handled above imho.

Curious as to what you think! 😺 🙏

… true

Signed-off-by: Valentin Heidelberger <valh@mailbox.org>
…ner definition has not changed and restart is not true

Signed-off-by: Valentin Heidelberger <valh@mailbox.org>
@va1entin va1entin force-pushed the va1entin-force-restart-logic-refactor branch from e9bda01 to 832661f Compare August 19, 2024 13:19
@sshnaidm
Copy link
Member

Sorry, I'm not sure how it solved this case. Can you please provide a simple playbook to illustrate this?
Thanks

@va1entin
Copy link
Author

va1entin commented Sep 27, 2024

Hi @sshnaidm sure here is a test process:

Playbook ubuntu-test.yml

---
- name: Ubuntu test
  hosts: "cloud"
  tasks:
    - name: Run Ubuntu container
      containers.podman.podman_container:
        name: "ubuntu-test"
        image: docker.io/library/ubuntu:22.04
        detach: true
        hostname: "ubuntu"
        pull: always
        force_restart: true 
        restart_policy: always
        volumes:
          - "/etc/localtime:/etc/localtime:ro"
        command: "sleep infinity"
        state: started

Test change to container with force_restart: true

  1. Run ansible-playbook ubuntu-test.yml to create the container with Ubuntu 22.04
  2. Change image version from 22.04 to 24.04 and run ansible-playbook ubuntu-test.yml again
  3. Container created in step 1 will be restarted but image will still be 22.04

Test change to container with force_restart: false (or no force_restart in playbook)

  1. Run ansible-playbook ubuntu-test.yml to create the container with Ubuntu 22.04
  2. set force_restart to false, change image version from 22.04 to 24.04 and run ansible-playbook ubuntu-test.yml again
  3. Container created in step 1 will be recreated and image will be 24.04

To summarize:
New image version (or other changes to the container) will not be applied as long as the container already exists and force_restart is true

After applying the patch in this PR you should instead see:

  1. Run ansible-playbook ubuntu-test.yml to create the container with Ubuntu 22.04
  2. Change image version from 22.04 to 24.04 and run ansible-playbook ubuntu-test.yml again
  3. Container created in step 1 will be recreated and image will be 24.04

This PR allows users to keep force_restart: true in their container definition to have the container be restarted when there are no changes while also recreating the container if there are changes. Currently, only the first works, effectively forcing users to either not use force_restart, change it to false every time they're making any change to their container (and then change it back to true) or add a separate step just for restarting to the playbook.

This is also true if recreate: true by the way. So with recreate: true the user is actually not getting a recreate as long as force_restart: true even when they change container attributes like image version.

@Crapshit
Copy link

Crapshit commented Oct 9, 2024

Thank you for that pull request @va1entin
I'm looking forward for a final solution.
Thanks

@va1entin
Copy link
Author

Hi @sshnaidm do you have any feedback on the changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

force_restart: true ignores any changed parameters in container task
3 participants