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

Move to docker_compose_v2 action #240

Closed
wants to merge 6 commits into from
Closed

Conversation

jlandahl
Copy link
Contributor

@jlandahl jlandahl commented May 7, 2024

As detailed in #239, the community.docker.docker_compose action from the community.docker Ansible collection has been deprecated in favor of community.docker.docker_compose_v2.

This was discovered while debugging a problem running the lemmy.yml playbook on an Ubuntu 24.04 system with Python 3.12, which broke during the docker_compose step due to an incompatibility with a newer version of urllib3 (docker/docker-py#3113). The problem was fixed, but apparently only in the docker_compose_v2 action. The result is that the deprecated docker_compose action doesn't work with newer versions of Python, but docker_compose_v2 does.

This PR is a simple change to lemmy.yml to use docker_compose_v2. One minor incompatibility is addressed by changing the pull parameter from true to always, since v2 changed it from a boolean to a string with the possible values of "always", "missing", "never", and "policy" (documented here). The value "missing" might be fine as well, but "always" seemed like a safe value to use.

The docker_compose (v1) action has been deprecated, and the v2 version
includes important bug fixes.
@jlandahl
Copy link
Contributor Author

jlandahl commented May 7, 2024

I see that PR #237 already has the change to docker_compose_v2, but it still has pull: true. That won't work anymore since the pull parameter is no longer a boolean, as described above.

@jlandahl jlandahl mentioned this pull request May 7, 2024
@codyro
Copy link
Collaborator

codyro commented May 7, 2024

Are there any backwards compatibility issues with the v2 method on older versions of Python/docker-compose (EX, those on LTS releases)?

This needs more consideration, I think.

(Find the RHEL guy 😅)

@jlandahl
Copy link
Contributor Author

jlandahl commented May 7, 2024

Are there any backwards compatibility issues with the v2 method on older versions of Python/docker-compose (EX, those on LTS releases)?

Good question, I was wondering this myself. If I get a chance I'll try it out on a VM or something. I would expect the risk to be low, however, since the community.docker project as a whole is moving forward on the v2 method and apparently not fixing Python compatibility issues in the deprecated v1 method.

@jlandahl
Copy link
Contributor Author

jlandahl commented May 7, 2024

Here's the deprecation commit, by the way: ansible-collections/community.docker@30faf0b

Looks like they're aiming at removing it in the 4.0.0 release, and since it's currently at 3.9.0 it sounds like v1 will go away completely soon.

@codyro
Copy link
Collaborator

codyro commented May 7, 2024

I want to ensure this doesn't break our existing compatibility matrix. If it does, we should re-evaluate how this is approached instead of mindlessly updating the usage. It could break the usage for most installations that are not running the latest release of the supported distributions.

We need to be less flippant.

@jlandahl
Copy link
Contributor Author

jlandahl commented May 7, 2024

Digging a little more, it looks like the problem was in docker-py and fixed a year ago, so now I'm confused as to why it would pop up now and only with docker_compose while docker_compose_v2 works fine.

@codyro
Copy link
Collaborator

codyro commented May 7, 2024

I can find some time tonight to test against the normal spread of distributions/versions to see if this is a breaking change or not.

My concern is we have no testing on these playbooks, so it's very easy to break them for other people while fixing them for your specific distribution/version.

Until we can get some Molecule magic going (I had some issues with docker-in-docker and testing these playbooks last year..), we need to be more diligent in these kinds of changes.

@jlandahl
Copy link
Contributor Author

jlandahl commented May 8, 2024

I'm with you on ensuring backward compatibility, and I was just noting that the community.docker project is going to remove docker_compose soonish and that will force the issue here. There was no flippancy intended. I didn't expect this change to be accepted blindly, and am offering to do more testing on older distributions when I get a chance. Consider this PR a mere suggestion, with notes from someone just coming to Lemmy who spent hours debugging the problems encountered with a new installation.

@codyro
Copy link
Collaborator

codyro commented May 8, 2024

I apologize! I wasn't calling you flippant. I said "we" to refer to every contributor, as we're all guilty of it. We've been trying to clean up everything this past year, so I'm cautious not to take two steps back accidentally. We are very merge happy.

We NEED and WANT all of the contributors possible!

TL;DR Thank you, and you're not wrong; I'm just an asshole 😁.

@codyro
Copy link
Collaborator

codyro commented May 8, 2024

It looks like the _v2 module works on everything except Debian 10:

root@cody-foss-test:~# cat /etc/*-release
PRETTY_NAME="Debian GNU/Linux 10 (buster)"
NAME="Debian GNU/Linux"
VERSION_ID="10"
VERSION="10 (buster)"
TASK [Start docker-compose] ***************************************************************************
[WARNING]: /srv/lemmy/foss.ly/docker-compose.yml: `version` is obsolete
[WARNING]: Cannot parse event from line: 'error getting credentials - err: exit status 1, out:
`GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.secrets was not
provided by any .service files`'. Please report this at https://github.com/ansible-
collections/community.docker/issues/new?assignees=&labels=&projects=&template=bug_report.md
fatal: [foss.ly]: FAILED! => {"actions": [{"id": "pictrs", "status": "Pulling", "what": "service"}, {"id": "lemmy-ui", "status": "Pulling", "what": "service"}, {"id": "proxy", "status": "Pulling", "what": "service"}, {"id": "lemmy", "status": "Pulling", "what": "service"}, {"id": "postgres", "status": "Pulling", "what": "service"}, {"id": "postfix", "status": "Pulling", "what": "service"}], "changed": false, "cmd": "/usr/bin/docker compose --ansi never --progress plain --project-directory /srv/lemmy/foss.ly up --detach --no-color --quiet-pull --pull always --remove-orphans --", "containers": [], "images": [], "msg": "Return code 1 is non-zero", "rc": 1, "stderr": "time=\"2024-05-08T01:41:57Z\" level=warning msg=\"/srv/lemmy/foss.ly/docker-compose.yml: `version` is obsolete\"\n pictrs Pulling \n lemmy-ui Pulling \n proxy Pulling \n lemmy Pulling \n postgres Pulling \n postfix Pulling \nerror getting credentials - err: exit status 1, out: `GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.secrets was not provided by any .service files`\n", "stderr_lines": ["time=\"2024-05-08T01:41:57Z\" level=warning msg=\"/srv/lemmy/foss.ly/docker-compose.yml: `version` is obsolete\"", " pictrs Pulling ", " lemmy-ui Pulling ", " proxy Pulling ", " lemmy Pulling ", " postgres Pulling ", " postfix Pulling ", "error getting credentials - err: exit status 1, out: `GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name org.freedesktop.secrets was not provided by any .service files`"], "stdout": "", "stdout_lines": []}

I wouldn't put a lot of money on there being a lot of users using these playbooks on Debian 10. We could probably drop support for this pretty painlessly.

Copy link
Collaborator

@codyro codyro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to merge this sooner rather than later, we should probably drop support for Debian 10 as it is a breaking change, and I don't think we realistically need to support it any longer.

If everyone is okay with that, would you mind updating the README.md to drop Debian 10 from the support matrix (https://github.com/LemmyNet/lemmy-ansible/blob/main/README.md?plain=1#L22)?

@ticoombs @dessalines @Nutomic Are you folks aware of any operators using Debian 10 (or even 11, for that matter)?

@Nutomic
Copy link
Member

Nutomic commented May 8, 2024

Are you folks aware of any operators using Debian 10 (or even 11, for that matter)?

I dont know anything about OS versions people are using. Anyway Debian 10 was released in 2019 and hasnt received security updates since 2022, so it sounds fine to discontinue support.

@jlandahl
Copy link
Contributor Author

jlandahl commented May 8, 2024

TL;DR Thank you, and you're not wrong; I'm just an asshole 😁.

Absolutely not, if anything I most likely misread you. And at this moment in time anyone of any conscience is, to put it extremely mildly, rather "on edge"... well, best not get distracted when we have work to do.

@codyro
Copy link
Collaborator

codyro commented May 9, 2024

@jlandahl would you mind updating this PR to remove Debian 10 from the README? We can then safely merge this, I believe.

@jlandahl
Copy link
Contributor Author

jlandahl commented May 9, 2024

Will do.

@jlandahl
Copy link
Contributor Author

Quick question. My new install was on Ubuntu 24.04, and with the docker_compose_v2 change everything worked. Would it be worth adding 24.04 to the supported distribution list, or should we hold off until a few more people have given positive feedback?

@codyro
Copy link
Collaborator

codyro commented May 10, 2024

Considering it’s the latest version of Ubuntu LTS, it’s not going anywhere and will likely be the primary distribution people install this on down the road. The containers are doing the heavy lifting, so I don’t think there are any concerns on my end.

I’d be in favor of adding official support for 24.04.

@sanatsathaye
Copy link
Contributor

Just throwing in my opinion, replacing docker_compose with docker_compose_v2 is not as straightforward as you think.

  1. Looking at the docs it was introduced only 4 months back in community.docker v3.6.0 which is not too long ago. This means you need to get people to either:
    a. Upgrade ansible to v9+ as ansible v8 ships with community.docker < 3.6.0. You will have to document this.
    b. Upgrade their locally installed version of community.docker. Ideal way is to force people to install the docker collections in requirements.yml and this is currently not part of the lemmy instructions. You will have to document this (and IMO, make running requirements.yml a mandatory step in the Lemmy installation process as well as version pin the collections in the requirements.yml)

  2. Looks like Woodpecker is broken because of this change as the module is too new.
    ERROR! couldn't resolve module/action 'community.docker.docker_compose_v2'
    The pipeline points to Alpine 3 and it looks like even the latest Alpine does not ship with ansible 9 yet.
    (46/46) Installing ansible (8.6.1-r0)
    Pretty sure this will have to be fixed.

@jlandahl
Copy link
Contributor Author

jlandahl commented May 10, 2024

Just throwing in my opinion, replacing docker_compose with docker_compose_v2 is not as straightforward as you think.

I'd be happy to work through these issues. My Ansible knowledge is rusty, but I used to use it extensively years ago so I should be able to catch up. I installed it recently via pipx, so I didn't hit any snags due to outdated distro-based packages.

I'll add a section in the readme on Ansible itself with a brief word on what it is, why it's useful, and notes on installation. I think this would be helpful anyway, since it would give important context to people wanting to setup Lemmy that are unfamiliar with Ansible and why it's being used as a layer above Docker Compose.

@jlandahl
Copy link
Contributor Author

On fixing Woodpecker, I see it's currently installing Ansible via apk. Would it make sense to start installing Ansible via pipx, since that seems to be what Ansible themselves recommend?

@Nothing4You
Copy link
Contributor

Anyway Debian 10 was released in 2019 and hasnt received security updates since 2022, so it sounds fine to discontinue support.

this isn't entirely accurate. debian is an LTS distro, and there is a separate team taking care of security updates beyond the regular lifecycle: https://wiki.debian.org/LTS

debian 10 still receives LTS security updates for a few more months, until June 30th, 2024.

this would only be cutting support short by less than 2 months though, so i don't think this should be an issue.

@codyro
Copy link
Collaborator

codyro commented May 10, 2024

On fixing Woodpecker, I see it's currently installing Ansible via apk. Would it make sense to start installing Ansible via pipx, since that seems to be what Ansible themselves recommend?

That should be sufficient.

debian 10 still receives LTS security updates for a few more months, until June 30th, 2024.

Similar to CentOS 7--the EOL is around the same time 😁. I think this is fine to discontinue support on.

@jlandahl
Copy link
Contributor Author

Woodpecker is happy now, but there are some warnings that look a little suspicious. Could someone more familiar with things take a look at this?

+ ansible-lint --warn-list experimental lemmy.yml lemmy-almalinux.yml uninstall.yml examples/vars.yml
WARNING  Unable to load module community.docker.docker_compose_v2 at lemmy.yml:314 for options validation
WARNING  Unable to resolve FQCN for module community.docker.docker_compose_v2
WARNING  Unable to load module community.docker.docker_compose at uninstall.yml:35 for options validation
WARNING  Unable to resolve FQCN for module community.docker.docker_compose
WARNING  Unable to load module ansible.posix.firewalld at lemmy-almalinux.yml:119 for options validation
WARNING  Unable to load module ansible.posix.seboolean at lemmy-almalinux.yml:133 for options validation
WARNING  Unable to resolve FQCN for module ansible.posix.firewalld
WARNING  Unable to resolve FQCN for module ansible.posix.seboolean

Passed: 0 failure(s), 0 warning(s) on 4 files. Last profile that met the validation criteria was 'production'.

.woodpecker.yml Outdated
Comment on lines 12 to 14
- apk add pipx
- pipx install --include-deps ansible
- export PATH=/root/.local/bin:$${PATH}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- apk add pipx
- pipx install --include-deps ansible
- export PATH=/root/.local/bin:$${PATH}
- export PATH=/root/.local/bin:$${PATH}
- apk add pipx
- pipx install --include-deps ansible

this just gets rid of the warning during pipx install about the bin path not being in $PATH, but it doesn't really affect the outcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I had considered this at some point, but then didn't get to it.

.woodpecker.yml Outdated
Comment on lines 24 to 26
- apk add pipx
- pipx install --include-deps ansible ansible-lint
- export PATH=/root/.local/bin:$${PATH}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- apk add pipx
- pipx install --include-deps ansible ansible-lint
- export PATH=/root/.local/bin:$${PATH}
- export PATH=/root/.local/bin:$${PATH}
- apk add pipx
- pipx install --include-deps ansible
- pipx inject --include-deps ansible ansible-lint

same as above, moving $PATH to the start gets rid of the warning during pipx install about the bin path not being in $PATH, but it doesn't really affect the outcome.

the main change is not installing both ansible and ansible-lint in the same pipx execution.
I haven't used pipx before, but using it this way creates a venv for every package. this results in the ansible-lint venv not having the packages that exist in the ansible venv. pipx inject seems to be just what we are looking for for this to install ansible-lint into the same venv as ansible.

the result is a clean woodpecker test: https://wpci.lem.rocks/repos/3/pipeline/19/5

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, once @codyro takes a look, we can merge.

@dessalines dessalines requested a review from codyro May 15, 2024 12:07
@Nothing4You
Copy link
Contributor

it's not good to merge, only the part that didn't affect the actual test was applied from my suggestions.
ansible-lint is still not running properly.

@dessalines
Copy link
Member

@Nothing4You could you do either a PR to this one, or possibly a separate PR to main?

@Nothing4You
Copy link
Contributor

both options are available now:

@dessalines
Copy link
Member

Thx @Nothing4You ! We can close this as its functionality is merged now in #244

@dessalines dessalines closed this May 22, 2024
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.

6 participants