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

Refactor zabbix proxy #1196

Merged
merged 18 commits into from
May 18, 2024
Merged

Conversation

eb4x
Copy link
Collaborator

@eb4x eb4x commented Mar 25, 2024

SUMMARY

Continuing the refactor, this time for zabbix-proxy

This PR consists of a lot of iterative commits, they should all be (somewhat) self-explanatory, doing a "single self-contained" change and a rationale for it. If one of the changes are not to anyones liking, comment on it and we can either improve upon it or take it out.

This is a considerably big refactor of the zabbix_proxy role. I'm not expecting it to break for existing installations and whatever overrides have been provided. It's not intended to "fix" any issues, just clean up the tasks and make the experience somewhat more consistent.

Big changes include moving package installation out of the os_family tasks, which are otherwise responsible for setting up the repo. So the installation happens in main.yml, and the dependencies for db-variants are installed in the mysql/postgresql/sqlite3 task files.

Arguably the repo-setup should be it's own role, as it's repeated out for every single role.

ISSUE TYPE
  • Refactor Pull Request
COMPONENT NAME

zabbix_proxy

ADDITIONAL INFORMATION

I'd like for this refactor to retain all its commits and messages, and not become a squash merge. One thing I sorely missed during this refactor was the ability to find a log as to why things were done in a certain way.

@eb4x
Copy link
Collaborator Author

eb4x commented Mar 25, 2024

I think this is pretty complete. Not quite sure about the when: zabbix_proxy_database_creation | bool constructions. You kinda want them there if you're passing variables from the command line. I've brought some changes spotted here over to PR #1193 for zabbix-server.

I'll get to zabbix-web next.

@eb4x
Copy link
Collaborator Author

eb4x commented Mar 26, 2024

Hmm, the CentOS one we can probably fix by forcing python3?

I'll have to do some digging when it comes to the ubuntu fails.

@@ -41,8 +41,6 @@ jobs:
- v60
include:
- interpreter: python3
- interpreter: python
Copy link
Collaborator Author

@eb4x eb4x Mar 26, 2024

Choose a reason for hiding this comment

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

The geerlingguy centos doesn't come with python3, which is kind of a shame. Better to do a rehaul of that image.

@eb4x
Copy link
Collaborator Author

eb4x commented Apr 13, 2024

Apologies for the hold up, been busy trying to get my own actions-runner working so I can get these results myself, as currently I'm currently just testing against a few select platforms and using images generated from Dockerfiles, so we can atleast have python3 in centos7. (A platform we might just drop sometime after summer?)

I'm currently experimenting with running a kind kubernetes cluster, and the actions-runner-controller, and had some marginal success getting it to run atleast 😅

I'll get back to this once I have gotten repeatable tests.

@eb4x
Copy link
Collaborator Author

eb4x commented May 3, 2024

I've created a separate PR #1218 for some CI stuff, and this PR has been rebased on that. The geerlingguy centos7 image comes without python3, and we should all be using python3 going forward.

Now it turns out rocky and almalinux and centos have systemd container-images ready to go, which is perfect our type of testing. So we just use those and install python3-pip which is a good enough compromise to get just the bare minimum of usable python on a system.

The debian/ubuntu images need the systemd-sysv package in addition to have /sbin/init present.

These are brain-dead simple docker-images, with bare minimum requirements, and will probably be more up-to-date and reliable for us in the long run.

Otherwise this pretty much follows the same pattern as the zabbix-server refactor.

@eb4x eb4x force-pushed the refactor_zabbix_proxy branch 2 times, most recently from 77eab8a to 1a2d811 Compare May 5, 2024 20:23
@pyrodie18
Copy link
Collaborator

Not sure why you put the same CI changes in here. Won't say remove them but need to fix the conflict you have.

eb4x added 17 commits May 6, 2024 06:46
This table is incomplete, and a hinderance for deployments to valid
combinations. Allow the user to bypass this by supplying

  `-e enable_version_check=false`
Instead of having a partial url in vars/Debian.yml, and appending to
it with additional info via set_fact, if zabbix_repo_deb_url is not
defined. Just supply it as a default which can be overriden by user.

Notes on Raspbian
There are very few ways do differentiate Raspbian from Debian with
ansible_facts. The only candidate seems to be ansible_facts.lsb.id.

The lsb section does not get filled in unless some packages are
installed. But luckily those packages come installed on Raspbian
systems. And we just default it back to ansible_facts.distribution
if lsb.id is not present. So we're gonna simplify and drop some
tasks.

Tested with ansible-core 2.13.13 on;

  2024-03-12-raspios-bullseye-armhf-lite
  2024-03-15-raspios-bookworm-armhf-lite

If this for some unforseen reason wouldn't work on older or newer
versions of raspbian, there's always the option of just overriding
the zabbix_repo_deb_url.
Install the zabbix-proxy-{{ zabbix_proxy_database }} from a single
common task.

RedHat can pin the minor version, and has a toggle to disable repo.
Debian can't pin minor version. The other debian options
cache_valid_time, default_release and force seem irrelevant.

We use the common package module, which is just a wrapper around
apt/yum, and use this construction;

  user_supplied_var | default(_calculated_var | default(omit))

to send additional parameters to the respective modules.
We don't need additional tasks for installing zabbix-sql-scripts,
we can just tack them on to zabbix-proxy-{{ database }}, with the
when condition found only on the Debian side of things.
It's cleaner, less conditional checking, and a single task for all
supported systems.

Remove PyMySQL installation via pip task and switch from mysqldb to
pymysql for all debian based systems. The only reason we're
installing any python/mysql dependencies at all is to use the
ansible community.mysql collection, which has a preference for
pymysql, and mysqldb as a backup [1].

Upgrade system-packages of pymysql with pip for a couple of
distributions. Upgrading system-packages with pip is generally a
bad idea, older versions of pymysql has issues with newer version
of mysql (>=8).

pymysql>=0.9.0,<0.10.0 fixes the issue with passwords missing
during login, but still has a problem setting
log_bin_trust_function_creators.

pymysql>=0.10.0,<0.11.0, fixes the remaining issue. We don't want
to stray to far off from the system defaults

Also, drop zabbix_python_prefix. It's all python3.

[1] https://github.com/ansible-collections/community.mysql/blob/main/plugins/module_utils/mysql.py#L21-L36
It's cleaner, less conditional checking, and a single task for all
supported systems. The list of packages also remains the same for
all supported systems within the os_family, so we can reduce the
lookup.

We're also trimming out the last of zabbix_python_prefix, and
always go for python3

The Debian systems had split installing the dependencies in two
tasks, one for the python dependency and one that gets triggered if
zabbix_proxy_install_database_client is true.

We're gonna reuse this variable, and bring it to RedHat systems,
and for mysql aswell in this commit.

And to bring it all back to one task, we use this construction;

- package:
    name: "{{ _dependencies | select | list }}"
  vars:
    _dependencies:
      - "{{ install_client | ternary('client-package', '') }}"
      - some-python-dependency

This will create a list of two items, the python-dependency, and
possibly an empty string. We use `| select` to filter out the empty
strings, and `| list` while strictly not necessary, was
historically appended in case the preceeding result ended up being
a generator.
Less conditional checking, and should feel familiar as both mysql
and postgresql do this now.
It was difficult to see what this code was supposed to achieve. The
most important variable zabbix_proxy_dbhost_run_install was hidden
behind two conditions of delegated_dbhost. But here we try to bring
it back to view.

if zabbix_proxy_dbhost_run_install is true, we want the code to be
delegated_to the zabbix_proxy_dbhost, otherwise we want to run it
from the zabbix_proxy.

However, we have a funny situation where zabbix_proxy_dbhost is
by default localhost, which means we'd still be on the
zabbix_proxy. We store this calculation in delegated_dbhost.

  if (dbhost == localhost) then zabbix_proxy else dbhost

The mysql tasks have an additional variable
zabbix_proxy_real_dbhost, which we try applying first and if not,
we default to whatever zabbix_proxy_dbhost_run_install wants.
With a clear understanding of zabbix_proxy_dbhost_run_install, we
can now use it to determine whether or not we need to become the
postgres user, and rely upon the default(omit) construction for any
potentially provided or missing arguments to login_{user,pass,host}
Similar to pgsql, we check for existing dbversion in a block, if
the query fails, we create the database in a rescue section. There
are a few things to note however.

The check and schema creation is not delegated. It happens from
zabbix_proxy. This is to verify that our dbuser can access the
dbname from zabbix_proxy and has all the permissions needed (after
creation) to alter tables between zabbix version upgrades.

For the schema creation we actually need some extra privileges, so
we read what they currently are, set them to what we need, import
our schema, and revert the extra privileges back to their original
state. The reverting happens in the `always:` section, so if any
other task should fail in schema creation (`rescue:` section) we
always revert the privileges before failing.

We also have to deal with varying paths to zabbix-proxy schema on
RedHat based systems.

  e.g. /usr/share/doc/zabbix-proxy-pgsql-X.Y.Z

And you don't necessarily know which version you're installing when
you just want the latest. The code to retain support for older
versions is already in place, by leveraging the ls_output_schema
for the legacy path.
We take advantage of the community.postgres modules, and do a query
for 'mandatory' in 'dbversion'. If the query fails, we're going to
assume the database has not been populated yet, and rescue the task
by populating it. Thereby alleviating the need for .done files.

While zabbix-server has predictable paths to the schemas, the
zabbix-proxy schema paths are versioned on RedHat based systems.

  e.g. /usr/share/doc/zabbix-proxy-pgsql-X.Y.Z

And you don't necessarily know which version you're installing when
you just want the latest. The code to retain support for older
versions is already in place, by leveraging the ls_output_schema
for the legacy path.
Mostly reusing the code-style seen in the other database imports.
Simplifying the commands used for creation, while keeping support
for older style schemas (zabbix < 6). And the addition of setting a
secure mode (0600) to the database file.
It reflects more accurately what we're trying to achieve. Also
strip out zabbix_proxy_db_long, it serves no purpose anymore.
Python2 went out of style more than a decade ago, and was "sunset"
over four years ago, 2020-01-01.

Sure CentOS might have some additional python2 support thanks to
RedHat, but you know what else it has? Python3. And with
epel-release, we get access to python3-PyMySQL, which allows us to
keep things simple.

The geerlingguy centos7 comes without python3, so we can either
drop it, or roll our own. This is the roll-your-own approach.

Grab the latest packaged molecule 4 release and the latest
molecule-docker straight from git. There's a fix that went in after
2.1.0 (cb1c1c31a4a468fb243fc2000656504a3ca00bb3) that fixes a bug
where the molecule.yml would get overwritten during image building.
This simplifies the code by removing zabbix-5.0 quirks/workarounds.
@eb4x
Copy link
Collaborator Author

eb4x commented May 6, 2024

Not sure why you put the same CI changes in here. Won't say remove them but need to fix the conflict you have.

I needed the CI stuff in here to make the tests pass. Not quite sure how I managed to get the conflict there myself, normally the ci changes would be recognized as already in and get skipped.

You lost a good deal of "documentation" in the git logs when you merged in the ci branch, is there a reason for squashing the commits? (The logs might come in handy for future on-lookers if they ever wonder why things were done in a certain way.)

@pyrodie18
Copy link
Collaborator

As a rule we always squish commits. Most PRs have a fair number of meaningless commits to them as someone tries to figure out why something isn't working right (I'm guilty of this) or whatever so just to keep things as clean as possible we squish. You'll see that is the came with most of the community collections (or at least the one's I've spent time looking at.

roles/zabbix_proxy/tasks/Debian.yml Outdated Show resolved Hide resolved
@eb4x
Copy link
Collaborator Author

eb4x commented May 7, 2024

As a rule we always squish commits.

Can you make an exception for these PRs? As you see there's quite a bit of documentation in each commit. And if they contain errors or need changes, I go back and fix them up instead of adding gibberish commits at the end of the PR.

Copy link
Collaborator

@pyrodie18 pyrodie18 left a comment

Choose a reason for hiding this comment

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

Went through commit by commit and read your logic pulled out a few things. @BGmot and @D3DeFi what are your thoughts on not squishing commits?

Also forgot to leave another comment but missing change fragments.

ansible.builtin.assert:
that:
- "{{ zabbix_proxy_version|float in zabbix_valid_proxy_versions[ ansible_facts['distribution_major_version'] ] }}"
fail_msg: Zabbix version {{ zabbix_proxy_version }} is not supported on {{ ansible_facts['distribution'] }} {{ ansible_facts['distribution_major_version'] }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Two comments here. First need to update documentation to document enable_version_check . Secondly, when we released 2.0.0 (I believe it was), we standardized on this way of checking versions. While I'm not opposed to making this change, I'd rather it be a separate PR that changes all of the roles at once to stay consistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll see if I can figure out where to document the variable.

So I pull this out as a separate PR that updates all roles at once? I can do that 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be greatly appreciated....Thanks

roles/zabbix_proxy/tasks/Debian.yml Outdated Show resolved Hide resolved
roles/zabbix_proxy/tasks/Debian.yml Show resolved Hide resolved
- python3-PyMySQL
"7":
- MariaDB-client
- MySQL-python
- "{{ zabbix_proxy_install_database_client | ternary('mariadb', '') }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Centos 7 doesn't have python3. That's why (at least one reason) we kept testing with Geerlingguy.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, so this is potentially a breaking change. If people are using centos7 without python3. But it's super-easy to install python3 on these systems. And that's really the only requirement/prerequisite.

Can you perhaps point to documentation where I could announce this breaking change for the people stuck on this platform that has 54 days left to EOL on June 30, 2024? he said tongue-in-cheek.

But on a more serious note, I'm fine with these refactors sitting until you're ready for a 3.0.0 release that allows breaking stuff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The python3 thing is why I went through the trouble of overhauling molecule to provide docker-images of centos7 with python3.

So the tests pass, given python3 gets installed.

Copy link
Collaborator

@pyrodie18 pyrodie18 left a comment

Choose a reason for hiding this comment

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

Will sit on this PR until Zabbix releases 7.0 and we release 3.0

roles/zabbix_proxy/tasks/Debian.yml Outdated Show resolved Hide resolved
roles/zabbix_proxy/tasks/Debian.yml Show resolved Hide resolved
ansible.builtin.assert:
that:
- "{{ zabbix_proxy_version|float in zabbix_valid_proxy_versions[ ansible_facts['distribution_major_version'] ] }}"
fail_msg: Zabbix version {{ zabbix_proxy_version }} is not supported on {{ ansible_facts['distribution'] }} {{ ansible_facts['distribution_major_version'] }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be greatly appreciated....Thanks

@pyrodie18
Copy link
Collaborator

OK so just going back through and reading all of the comments, I think my only concern right now is the Centos 7 python3 stuff and my comment about the valid version variable. I would suggest you just remove the centos 7 changes for right now. Unfortunately there's no getting around it being a breaking change until we remove Centos 7 completely. I really expected Zabbix 7 to be out by now based on their roadmap but that obviously isn't happening. Ref the valid version, lets just keep it in here and we can do a separate PR to do the rest of the roles. Thoughts?

@D3DeFi
Copy link
Contributor

D3DeFi commented May 14, 2024

Went through commit by commit and read your logic pulled out a few things. @BGmot and @D3DeFi what are your thoughts on not squishing commits?

Also forgot to leave another comment but missing change fragments.

@pyrodie18 Hi, I think that historically, it was required to squash commits when modules were a part of upstream ansible. Now I am not sure that there is any reason or preference other than those of maintainers of community repos.

My 2 cents are that squashing should be default unless there is good reason to merge all commits from PR without squashing them.

@pyrodie18 pyrodie18 merged commit c263a6a into ansible-collections:main May 18, 2024
95 checks passed
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.

3 participants