tests: Cleanup candlepin container

[spetrosi]

Enhancement: In always block, import a task to cleanup candlepin container in tests that import the task tests/tasks/setup_candlepin.yml, which starts candlepin container.

Reason: Candlepin container remains after test playbook finishes. If you remove container-selinux rpm, you got selinux denials because the container is running but selinux policies for containers are removed.

[spetrosi]

[citest]

[spetrosi]

[citest]

[spetrosi]

[citest]

[spetrosi]

[citest]

[spetrosi]

[citest]

[spetrosi]

[citest]

[spetrosi]

[citest]

[spetrosi]

[citest]

[spetrosi]

[citest]

[spetrosi]

[citest]

[spetrosi]

@ptoscano please take a look when you have time

[ptoscano]

I'd rename this file as "stop_candlepin.yml"

[spetrosi]

Since in fact we stop the container to remove it, and since this is specifically for the podman container, I find it clearer to call the file remove_candlepin_container.

[ptoscano]

Instead of these two tasks, simply use the existing Stop and remove Candlepin container task in setup_candlepin.yml, as it does exactly that.

[richm]

@ptoscano re: removing the candlepin image from the local registry

• tests should clean up all resources - you are correct that if this is an ephemeral machine, none of the cleanup steps are necessary - what we could do for performance is tag all of those tasks with tests::cleanup, then use --skip-tags tests::cleanup if the machine is ephermeral - but we definitely need the cleanup for the default case, which is non-ephemeral
• leaving the container image may cause problems in multi-role tests e.g. the podman tests expect a system without any local containers or images

So ideally, the rhc tests could leave the candlepin image locally until after all of the rhc tests are run, then do any post-rhc tests cleanup, like removing the candlepin image. The problem is that we don't have a way to run a "cleanup-after-all-role-tests-are-completed.yml" playbook.

[ptoscano]

what we could do for performance is tag all of those tasks with tests::cleanup, then use --skip-tags tests::cleanup if the machine is ephermeral - but we definitely need the cleanup for the default case, which is non-ephemeral

I'm fine with this. Could we also tweak tox-lsr (which spawns a new system) to also skip that tag by default?

The problem is that we don't have a way to run a "cleanup-after-all-role-tests-are-completed.yml" playbook.

I guess it also depends on how the tests are run, right? ie tox-lsr vs testing-farm vs something else.

[richm]

what we could do for performance is tag all of those tasks with tests::cleanup, then use --skip-tags tests::cleanup if the machine is ephermeral - but we definitely need the cleanup for the default case, which is non-ephemeral

I'm fine with this. Could we also tweak tox-lsr (which spawns a new system) to also skip that tag by default?

It's pretty easy to add --skip-tags tests::cleanup to your tox qemu run invocation e.g. tox -e qemu-ansible-core-2.17 -- --image-name centos-10 --log-level debug --skip-tags tests::cleanup -- tests/tests_repositories.yml

There are other ways to use tox e.g. with --make-batch where you do not want to skip cleanup.

The problem is that we don't have a way to run a "cleanup-after-all-role-tests-are-completed.yml" playbook.

I guess it also depends on how the tests are run, right? ie tox-lsr vs testing-farm vs something else.

Right - if we wanted to add something like a "run setup before all role tests" and "run cleanup after all role tests" then we would have to change tox-lsr, testing farm test runner, and the downstream test runners.

[spetrosi]

@richm maybe let's not clean up the image? It won't affect anything, and re-downloading it with each test upstream is a waste of network bandwidth.

[richm]

@richm maybe let's not clean up the image? It won't affect anything, and re-downloading it with each test upstream is a waste of network bandwidth.

Ok. That will affect the podman tests, in the scenarios where we run tests from multiple roles - the podman tests expect no images. I guess that means changing this PR to use __rhc_cleanup_image: false (or just omit __rhc_cleanup_image because the default is false)

[richm]

I'll fix the podman tests to work if there are images on the system.

[spetrosi]

[citest]

[richm]

I think just get rid of __rhc_cleanup_image which isn't used anymore, and we can merge

[spetrosi]

I'd wait for Pino approval too

[spetrosi]

[citest]

[ptoscano]

Thanks for the updates! I added few more notes that IMHO simplify things a bit, and make the changes more consistent with the current tasks.

Additionally a minor nit common to all the files:

        - name: Clean up Candlepin container
          include_tasks: tasks/remove_candlepin_container.yml

I suggest Clean up Candlepin instead (mimicks the setup tasks), or even Tear down Candlepin (see my previous comment on the naming of the new task file).

Thanks!

[ptoscano]

Since this file now only stops candlepin, better name it stop_candlepin.yml, or maybe even better teardown_candlepin.yml (symmetric with "setup")

[ptoscano]

Rather than two tasks, simply try to stop the container, and do not consider the missing container an error. In practice simply move here the single task Stop and remove Candlepin container as currently in setup_candlepin.yml.

[spetrosi]

Now this doesn't work because setup_candlepin.yml has the step to remove the container just in case the container was running from the previous playbook. But role always cleans up the container, and this step would fail. Although, it's good to have the step in setup_candlepin.yml for the case when you are testing locally and might have some container running from the previous test.
I think it's better to keep it the Ansible way and keep the task. By importing the task we say bring the system to the state where this container is removed, whether it exists now or not.

[ptoscano]

Not sure what you mean here: the existing task that tries to stop the candlepin container also takes care of not erroring out in case the container does not exist. Strictly speaking, doing first the existence check and then stopping it may be a minor TOCTOU issue: https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use.

[spetrosi]

[citest]

[ptoscano]

Thanks for the updates, a couple of final changes, and it should be ready.

Make sure to squash all the commits.

[ptoscano]

Not sure what you mean here: the existing task that tries to stop the candlepin container also takes care of not erroring out in case the container does not exist. Strictly speaking, doing first the existence check and then stopping it may be a minor TOCTOU issue: https://en.wikipedia.org/wiki/Time-of-check_to_time-of-use.

[spetrosi]

[citest]