Idempotency broken (again) #142

xoxys · 2021-05-15T21:32:44Z

SUMMARY

We had fixed idempotency issue in this commit bcd7b9b now its broken again due to #119...

The service reports changed now on every playbook run even if stopped is set to true.

\cc @tonal

ISSUE TYPE

Bug Report

COMPONENT NAME

docker-compose

The text was updated successfully, but these errors were encountered:

xoxys · 2021-05-15T21:42:05Z

I don't get changes done in #119 why was the change in https://github.com/tonal/community.docker/blob/9c0c2b90b8c60abfe145b7d01afeac369b16892f/plugins/modules/docker_compose.py#L773 reverted if the pull is already done in https://github.com/tonal/community.docker/blob/9c0c2b90b8c60abfe145b7d01afeac369b16892f/plugins/modules/docker_compose.py#L729

felixfontein · 2021-05-16T07:23:19Z

I guess it would really help if someone would add tests for docker_compose (even better if they'd run in CI). Because right now, there are none...

xoxys · 2021-05-16T12:36:07Z

It's broken due to this or condition https://github.com/ansible-collections/community.docker/blob/main/plugins/modules/docker_compose.py#L800. Any idea why it was changed? This condition IMO will be true in nearly every normal situation https://github.com/ansible-collections/community.docker/blob/main/plugins/modules/docker_compose.py#L763-L764 and thanks to the or condition Ansible will report changed all the time.

aminvakil · 2021-05-16T14:04:22Z

@felixfontein I'd be glad to help on this matter and add integration tests for this module. I'll open a PR in a few days.

felixfontein · 2021-05-16T20:50:24Z

It's broken due to this or condition https://github.com/ansible-collections/community.docker/blob/main/plugins/modules/docker_compose.py#L800. Any idea why it was changed?

Yes, I suggested that in #119 (review)

The whole module code looks pretty fishy anyway, changed should basically be updated anywhere IMO since the resulting changed should be True if any step inbetween resulted in a changed state.

This condition IMO will be true in nearly every normal situation https://github.com/ansible-collections/community.docker/blob/main/plugins/modules/docker_compose.py#L763-L764 and thanks to the or condition Ansible will report changed all the time.

I don't understand what's the problem with these lines. If plan.action != 'noop', isn't some change happening? If not, why is changed set to True? That would rather be a bug, instead of claiming that correctly handling the changed state is a bug.

xoxys · 2021-05-16T21:33:54Z

If plan.action != 'noop', isn't some change happening?

Is it? Then perhaps my understanding of the code is too poor. I have a very simple compose project with a single service and a pretty simple Ansible task:

docker_compose:
        project_src: "{{ droneci_runner_docker_service_directory }}"
        pull: yes
        remove_orphans: yes
        stopped: yes
        state: present

The result is, plan.action is always start which results in a result['changed'] = True and an Ansible state of changed all the time even if I would not expect this on the given task definition. I don't want to claim anything, I just shared my observations after a short debugging 🤷

xoxys · 2021-05-16T22:25:44Z

To me, it looks like convergence_plan will always return the action start for stopped containers https://github.com/docker/compose/blob/master/compose/service.py#L415. But this is IMO wrong for intentionally stopped containers and results in the always changed loop.

felixfontein · 2021-05-17T05:10:21Z

I guess the code then needs to check whether the only action in the convergence plan is start: if starting is not requested, it has to be treated the same way as noop. But that's a bug which has always been there, and only been masked by other bugs in changed tracking (one of whom has been fixed recently).

felixfontein · 2021-06-01T19:52:01Z

I'm happy to review and merge a fix for this, but I won't write it. So if someone wants to step up, feel free :)

xoxys · 2021-06-01T19:54:12Z

I'll try to take a closer look during this week and create a PR.

aminvakil · 2021-06-02T05:59:43Z

@xoxys I totally misremembered writing integration tests for docker_compose module :(
I have not started anything yet, feel free to take this matter into your capable hands!

felixfontein · 2021-06-07T05:46:32Z

I've now added some basic CI for docker_compose, including preparations for a test that will fail due to this issue (https://github.com/ansible-collections/community.docker/blob/main/tests/integration/targets/docker_compose/tasks/tests/start-stop.yml#L222-L223). You can see in #152 that uncommenting these lines make CI fail (which verifies this issue in CI).

So if anyone wants to work on this, uncomment these two lines and fix the module to pass tests, that should be a good start ;-)

felixfontein · 2021-06-07T05:51:13Z

Please note that I'll release 1.7.0 tomorrow, so if you want a fix for this to get in, please hurry :)

aminvakil · 2021-06-07T20:47:28Z

I'm trying to understanding this, and running integration test with re-enabling failing test results in:

TASK [docker_compose : Present stopped (idempotent)] ***************************
task path: /root/ansible_collections/community/docker/tests/output/.tmp/integration/docker_compose-ggg0yq6x-ÅÑŚÌβŁÈ/tests/integration/targets/docker_compose/tasks/tests/start-stop.yml:148
<testhost> ESTABLISH LOCAL CONNECTION FOR USER: root
<testhost> EXEC /bin/sh -c 'echo ~root && sleep 0'
<testhost> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /root/.ansible/tmp `"&& mkdir "` echo /root/.ansible/tmp/ansible-tmp-1623097580.6201396-1304-17524494780593 `" && echo ansible-tmp-1623097580.6201396-1304-17524494780593="` echo /root/.ansible/tmp/ansible-tmp-1623097580.6201396-1304-17524494780593 `" ) && sleep 0'
Using module file /root/ansible_collections/community/docker/plugins/modules/docker_compose.py
<testhost> PUT /root/.ansible/tmp/ansible-local-10120ese1g_/tmpbfo_1tyc TO /root/.ansible/tmp/ansible-tmp-1623097580.6201396-1304-17524494780593/AnsiballZ_docker_compose.py
<testhost> EXEC /bin/sh -c 'chmod u+x /root/.ansible/tmp/ansible-tmp-1623097580.6201396-1304-17524494780593/ /root/.ansible/tmp/ansible-tmp-1623097580.6201396-1304-17524494780593/AnsiballZ_docker_compose.py && sleep 0'
<testhost> EXEC /bin/sh -c '/usr/bin/python3.6 /root/.ansible/tmp/ansible-tmp-1623097580.6201396-1304-17524494780593/AnsiballZ_docker_compose.py && sleep 0'
<testhost> EXEC /bin/sh -c 'rm -f -r /root/.ansible/tmp/ansible-tmp-1623097580.6201396-1304-17524494780593/ > /dev/null 2>&1 && sleep 0'
changed: [testhost] => {
    "changed": true,
    "invocation": {
        "module_args": {
            "api_version": "auto",
            "build": false,
            "ca_cert": null,
            "client_cert": null,
            "client_key": null,
            "debug": false,
            "definition": {
                "services": {
                    "ansible-test-9710bd45-hi": {
                        "command": "/bin/sh -c \"sleep 10m\"",
                        "image": "quay.io/ansible/docker-test-containers:alpine3.8",
                        "stop_grace_period": "1s"
                    }
                },
                "version": "2"
            },
            "dependencies": true,
            "docker_host": "unix://var/run/docker.sock",
            "files": null,
            "hostname_check": false,
            "nocache": false,
            "project_name": "ansible-test-9710bd45",
            "project_src": null,
            "pull": false,
            "recreate": "smart",
            "remove_images": null,
            "remove_orphans": false,
            "remove_volumes": false,
            "restarted": false,
            "scale": null,
            "services": null,
            "ssl_version": null,
            "state": "present",
            "stopped": true,
            "timeout": 10,
            "tls": false,
            "tls_hostname": null,
            "use_ssh_client": false,
            "validate_certs": false
        }
    },
    "services": {
        "ansible-test-9710bd45-hi": {
            "ansible-test-9710bd45_ansible-test-9710bd45-hi_1": {
                "cmd": [
                    "/bin/sh",
                    "-c",
                    "sleep 10m"
                ],
                "image": "quay.io/ansible/docker-test-containers:alpine3.8",
                "labels": {
                    "com.docker.compose.config-hash": "edf65c7c6f9d0ea1c62afc4a54fb503c1be1355e26b9c9e49ed1a0a7e887b4eb",
                    "com.docker.compose.container-number": "1",
                    "com.docker.compose.oneoff": "False",
                    "com.docker.compose.project": "ansible-test-9710bd45",
                    "com.docker.compose.project.config_files": "/tmp/ansible2449i7l8/docker-compose.yml",
                    "com.docker.compose.project.working_dir": "/tmp/ansible2449i7l8",
                    "com.docker.compose.service": "ansible-test-9710bd45-hi",
                    "com.docker.compose.version": "1.25.1"
                },
                "networks": {
                    "ansible-test-9710bd45_default": {
                        "IPAddress": "",
                        "IPPrefixLen": 0,
                        "aliases": [
                            "ansible-test-9710bd45-hi"
                        ],
                        "globalIPv6": "",
                        "globalIPv6PrefixLen": 0,
                        "links": null,
                        "macAddress": ""
                    }
                },
                "state": {
                    "running": false,
                    "status": "created"
                }
            }
        }
    }
}

TASK [docker_compose : Present stopped (idempotent check)] *********************
task path: /root/ansible_collections/community/docker/tests/output/.tmp/integration/docker_compose-ggg0yq6x-ÅÑŚÌβŁÈ/tests/integration/targets/docker_compose/tasks/tests/start-stop.yml:156
<testhost> ESTABLISH LOCAL CONNECTION FOR USER: root
<testhost> EXEC /bin/sh -c 'echo ~root && sleep 0'
<testhost> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /root/.ansible/tmp `"&& mkdir "` echo /root/.ansible/tmp/ansible-tmp-1623097581.7641206-1323-276727714580714 `" && echo ansible-tmp-1623097581.7641206-1323-276727714580714="` echo /root/.ansible/tmp/ansible-tmp-1623097581.7641206-1323-276727714580714 `" ) && sleep 0'
Using module file /root/ansible_collections/community/docker/plugins/modules/docker_compose.py
<testhost> PUT /root/.ansible/tmp/ansible-local-10120ese1g_/tmp74hxsykl TO /root/.ansible/tmp/ansible-tmp-1623097581.7641206-1323-276727714580714/AnsiballZ_docker_compose.py
<testhost> EXEC /bin/sh -c 'chmod u+x /root/.ansible/tmp/ansible-tmp-1623097581.7641206-1323-276727714580714/ /root/.ansible/tmp/ansible-tmp-1623097581.7641206-1323-276727714580714/AnsiballZ_docker_compose.py && sleep 0'
<testhost> EXEC /bin/sh -c '/usr/bin/python3.6 /root/.ansible/tmp/ansible-tmp-1623097581.7641206-1323-276727714580714/AnsiballZ_docker_compose.py && sleep 0'
<testhost> EXEC /bin/sh -c 'rm -f -r /root/.ansible/tmp/ansible-tmp-1623097581.7641206-1323-276727714580714/ > /dev/null 2>&1 && sleep 0'
changed: [testhost] => {
    "actions": [
        {
            "service": "ansible-test-9710bd45-hi",
            "start": [
                {
                    "id": "583342337a25427ca0528132265243a7e3a511e429a361b5cc9114078748d641",
                    "name": "ansible-test-9710bd45_ansible-test-9710bd45-hi_1",
                    "short_id": "583342337a25"
                }
            ]
        },
        {
            "service": "ansible-test-9710bd45-hi",
            "stop": []
        }
    ],
    "changed": true,
    "invocation": {
        "module_args": {
            "api_version": "auto",
            "build": false,
            "ca_cert": null,
            "client_cert": null,
            "client_key": null,
            "debug": false,
            "definition": {
                "services": {
                    "ansible-test-9710bd45-hi": {
                        "command": "/bin/sh -c \"sleep 10m\"",
                        "image": "quay.io/ansible/docker-test-containers:alpine3.8",
                        "stop_grace_period": "1s"
                    }
                },
                "version": "2"
            },
            "dependencies": true,
            "docker_host": "unix://var/run/docker.sock",
            "files": null,
            "hostname_check": false,
            "nocache": false,
            "project_name": "ansible-test-9710bd45",
            "project_src": null,
            "pull": false,
            "recreate": "smart",
            "remove_images": null,
            "remove_orphans": false,
            "remove_volumes": false,
            "restarted": false,
            "scale": null,
            "services": null,
            "ssl_version": null,
            "state": "present",
            "stopped": true,
            "timeout": 10,
            "tls": false,
            "tls_hostname": null,
            "use_ssh_client": false,
            "validate_certs": false
        }
    },
    "services": {
        "ansible-test-9710bd45-hi": {
            "ansible-test-9710bd45_ansible-test-9710bd45-hi_1": {
                "cmd": [
                    "/bin/sh",
                    "-c",
                    "sleep 10m"
                ],
                "image": "quay.io/ansible/docker-test-containers:alpine3.8",
                "labels": {
                    "com.docker.compose.config-hash": "edf65c7c6f9d0ea1c62afc4a54fb503c1be1355e26b9c9e49ed1a0a7e887b4eb",
                    "com.docker.compose.container-number": "1",
                    "com.docker.compose.oneoff": "False",
                    "com.docker.compose.project": "ansible-test-9710bd45",
                    "com.docker.compose.project.config_files": "/tmp/ansible2449i7l8/docker-compose.yml",
                    "com.docker.compose.project.working_dir": "/tmp/ansible2449i7l8",
                    "com.docker.compose.service": "ansible-test-9710bd45-hi",
                    "com.docker.compose.version": "1.25.1"
                },
                "networks": {
                    "ansible-test-9710bd45_default": {
                        "IPAddress": "",
                        "IPPrefixLen": 0,
                        "aliases": [
                            "ansible-test-9710bd45-hi"
                        ],
                        "globalIPv6": "",
                        "globalIPv6PrefixLen": 0,
                        "links": null,
                        "macAddress": ""
                    }
                },
                "state": {
                    "running": false,
                    "status": "created"
                }
            }
        }
    }
}

Why is it returning status: created everytime?

felixfontein · 2021-06-08T03:48:07Z

status: created is the state of the Docker container. created means it has been created, but it is not (yet) running. So that's to be expected. (That's what docker inspect would also tell you when running it on the container.)

felixfontein · 2021-06-21T11:21:16Z

I now created #159 to fix this.

xoxys · 2021-06-22T07:29:36Z

Thanks a lot! Sorry could not find free time to work on it on my own...

felixfontein mentioned this issue May 17, 2021

Release plan #4

Open

felixfontein added the docker-compose-v1 Docker Compose v1 label May 21, 2021

This was referenced Jun 7, 2021

Add more docker_compose tests #151

Merged

[DO NOT MERGE] Test failing tests for docker_compose #152

Closed

felixfontein mentioned this issue Jun 21, 2021

docker_compose: fix idempotency for stopped services #159

Merged

felixfontein closed this as completed in #159 Jun 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Idempotency broken (again) #142

Idempotency broken (again) #142

xoxys commented May 15, 2021

xoxys commented May 15, 2021 •

edited

Loading

felixfontein commented May 16, 2021

xoxys commented May 16, 2021

aminvakil commented May 16, 2021

felixfontein commented May 16, 2021

xoxys commented May 16, 2021

xoxys commented May 16, 2021

felixfontein commented May 17, 2021

felixfontein commented Jun 1, 2021

xoxys commented Jun 1, 2021

aminvakil commented Jun 2, 2021

felixfontein commented Jun 7, 2021

felixfontein commented Jun 7, 2021

aminvakil commented Jun 7, 2021

felixfontein commented Jun 8, 2021

felixfontein commented Jun 21, 2021

xoxys commented Jun 22, 2021

Idempotency broken (again) #142

Idempotency broken (again) #142

Comments

xoxys commented May 15, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

xoxys commented May 15, 2021 • edited Loading

felixfontein commented May 16, 2021

xoxys commented May 16, 2021

aminvakil commented May 16, 2021

felixfontein commented May 16, 2021

xoxys commented May 16, 2021

xoxys commented May 16, 2021

felixfontein commented May 17, 2021

felixfontein commented Jun 1, 2021

xoxys commented Jun 1, 2021

aminvakil commented Jun 2, 2021

felixfontein commented Jun 7, 2021

felixfontein commented Jun 7, 2021

aminvakil commented Jun 7, 2021

felixfontein commented Jun 8, 2021

felixfontein commented Jun 21, 2021

xoxys commented Jun 22, 2021

xoxys commented May 15, 2021 •

edited

Loading