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

feat: support zabbix installation on suse #1194

Merged
merged 7 commits into from
Jun 13, 2024

Conversation

jon4hz
Copy link
Contributor

@jon4hz jon4hz commented Mar 24, 2024

SUMMARY

Hi!
This PR adds full openSUSE and SLES 15 support for the zabbix installation roles with the following exceptions:

  • zabbix_server and zabbix_proxy version 6.2 is unsupported due to a library mismatch. Both components require libnetsnmp.so.30 but SLES 15 provides libnetsnmp.so.40. (v6.2 is now unsupported anyway)
  • zabbix_server and zabbix_proxy don't support the mysql version. For some reason I couldn't get the community.mysql collection to work on SLES 15. I'm not 100% sure why it isn't working. My best guess is because Suse ships an outdated version of the PyMySQL package. (workaround)
  • The zabbix_web role is currently not covered by molecule tests because of missing SLES support in the geerlingguy.php role.

Everything else should be working as expected and is covered by molecule tests.

Please feel free to add a first review and let me know what you think! :)

An update to the docs and a change fragment will follow soon.

ISSUE TYPE
  • Docs Pull Request
  • Feature Pull Request
COMPONENT NAME
  • zabbix_agent
  • zabbix_javagateway
  • zabbix_proxy
  • zabbix_server
  • zabbix_web

@pyrodie18
Copy link
Collaborator

@BGmot and @dj-wasabi what are our thoughts on adding another supported OS? I'll admit I don't think I've ever used SUSE and have no idea how widely used it is out there. Given its relatively short life cycle we're not adding a ton of additional testing. I would want to make sure we figure out the molecule testing for Web and ideally why mysql isn't working.

@dj-wasabi
Copy link
Contributor

I am a bit conflicted with this, but ideally the collection/roles should be to support all OS'es that Zabbix supports. I don't see it present on the https://www.zabbix.com/documentation/current/en/manual/installation/requirements page. But I also think to a certain degree we can support other OS'es.

But the problem with that is, providing the support and most specifically when you as a maintainer don't have the experience with it. And not only you/me as a maintainer, when someone else makes a PR that results in failing a CI run, can we ask that of the PR Author?

I am not sure if we can "decide" per OS if a molecule test fail should result into a Pipeline error or not. Like if we merge this PR, maybe we can allow the molecule tests to fail on a OS without interupting the main/supported OS'es.

@jon4hz
Copy link
Contributor Author

jon4hz commented Mar 28, 2024

I don't see it present on the https://www.zabbix.com/documentation/current/en/manual/installation/requirements page. But I also think to a certain degree we can support other OS'es.

According to https://www.zabbix.com/documentation/current/en/manual/installation/install_from_packages SLES is an officially supported OS.

@pyrodie18
Copy link
Collaborator

I am a bit conflicted with this, but ideally the collection/roles should be to support all OS'es that Zabbix supports. I don't see it present on the https://www.zabbix.com/documentation/current/en/manual/installation/requirements page. But I also think to a certain degree we can support other OS'es.

But the problem with that is, providing the support and most specifically when you as a maintainer don't have the experience with it. And not only you/me as a maintainer, when someone else makes a PR that results in failing a CI run, can we ask that of the PR Author?

I am not sure if we can "decide" per OS if a molecule test fail should result into a Pipeline error or not. Like if we merge this PR, maybe we can allow the molecule tests to fail on a OS without interupting the main/supported OS'es.

I'm curious if there is a larger community definition of what it means for a role to support an OS. I know a lot of the collections don't actually include roles (disappointing to me personally)

@dj-wasabi
Copy link
Contributor

I don't see it present on the https://www.zabbix.com/documentation/current/en/manual/installation/requirements page. But I also think to a certain degree we can support other OS'es.

According to https://www.zabbix.com/documentation/current/en/manual/installation/install_from_packages SLES is an officially supported OS.

Thank you! Then it has a +1 for me that we should include the code (and thus merging the PR) to the collection.

I am not sure yet how to support something like this, when - also based on the comment of @pyrodie18 - it also not sure how much it is used. Back then before the 'collections' and with my role, I added Suse and Windows as a nice "experiment" with working with them from an Ansible p.o.v. It was easy for me, as these roles were in my own 'github namespace' and thus could easily say that they were best effort. Not sure if we can do the same with the collection?

@dj-wasabi
Copy link
Contributor

I am a bit conflicted with this, but ideally the collection/roles should be to support all OS'es that Zabbix supports. I don't see it present on the https://www.zabbix.com/documentation/current/en/manual/installation/requirements page. But I also think to a certain degree we can support other OS'es.
But the problem with that is, providing the support and most specifically when you as a maintainer don't have the experience with it. And not only you/me as a maintainer, when someone else makes a PR that results in failing a CI run, can we ask that of the PR Author?
I am not sure if we can "decide" per OS if a molecule test fail should result into a Pipeline error or not. Like if we merge this PR, maybe we can allow the molecule tests to fail on a OS without interupting the main/supported OS'es.

I'm curious if there is a larger community definition of what it means for a role to support an OS. I know a lot of the collections don't actually include roles (disappointing to me personally)

Well, normally I would say you are never alone in a situation. So I think we - as the Zabbix collection community - should maybe find some that can either help us or forward us to some place/team where we can ask these kinds of questions?

@pyrodie18
Copy link
Collaborator

@dj-wasabi going to point you over to ansible-community/community-topics#197. There is nothing definitive in there about testing standards but some good discussion. May kick the tires since its been dead for a year.

@eb4x
Copy link
Collaborator

eb4x commented Jun 6, 2024

I'm all for this, I think it would make a great addition to the 3.0.0 release. You'll have wait and rebase once #1272 merges, but it should be much easier adding support after. I think you'll find the zabbix_server and zabbix_proxy roles will be much easier to port to ever since the refactors landed there (#1193 #1196) and you should be able to support mysql by reusing the workarounds in #1235 once that lands.

@jon4hz jon4hz force-pushed the suse-sles branch 4 times, most recently from e720249 to 1e33806 Compare June 7, 2024 22:57
@pyrodie18
Copy link
Collaborator

@jon4hz still have some failures happening. Also need you to please update the documentation

@jon4hz
Copy link
Contributor Author

jon4hz commented Jun 8, 2024

Yeah, there are still a few things to be changed due to the refactoring prs from @eb4x. I should be able to adjust this PR over the weekend 👍

@jon4hz jon4hz force-pushed the suse-sles branch 3 times, most recently from d5a4491 to 77214a0 Compare June 9, 2024 23:40
Comment on lines 22 to 23
__upgrade_debian_pymysql: "{{ (ansible_facts['distribution'] in ['CentOS', 'Debian', 'Ubuntu'] and ansible_facts['distribution_release'] in ['Core', 'buster', 'bullseye', 'bionic', 'focal']) }}"
__upgrade_suse_pymysql: "{{ ansible_facts['os_family'] in ['Suse'] }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I kinda see what you're doing here, and it's clever to give names to the checks. Unfortunately they are a bit misleading, and don't really warrant their own task.

The misleading would be calling the variable upgrade_debian_pymysql and checking if distro is 'CentOS'. The good news is I think we've dropped centos7 for this role, you're free to take out 'CentOS' and 'Core' from the comparison. And also buster and bionic.

Having a task to set_fact, when it's just needed for the Upgrade pymysql task is to much, so just put these named checks under a vars: section in the Upgrade pymysql task.

This task is a work-around for specific versions, and I wouldn't want this to be the default for all future releases of Suse, so you should really pin it down to a release, or major version, or something else.

Final note for context, the when condition that was used is more elaborate than it needed to be, mostly for the convenience of the readers/developers of this playbook. It almost reads "this is a bad idea, but upgrade pymysql when debian-bullseye or ubuntu-focal"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the naming isn't optimal, yeah.
What do you think about creating a variable in the os specific vars file? That would probably be more idiomatic than using the vars section of a task? Something like:

_zabbix_server_pymysql_upgrade:
  "15": true

That allows us to control the pymysql upgrade for each major versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that approach would hide the details of when this workaround is applied. Suddenly you have to look up information in a second file. (Also you've now created a lookup table, and you'll need to create an entry for all versions in the os_family.)

I think you're good by just setting

- name: Upgrade pymysql
  when: _pymysql_upgrade_suse or _pymysql_upgrade_debian
  pip:
    name: ...
  vars:
    _pymysql_upgrade_suse: "{{ ansible_facts['os_family'] == 'Suse' and ansible_facts['distribution_major_release'] == '15' }}"
    _pymysql_upgrade_debian: "{{ ...

Or something to that effect. If it was relevant for more versions for os_family, then the vars file would be the way to go.

@BGmot
Copy link
Collaborator

BGmot commented Jun 11, 2024

Hi @jon4hz , how soon you can update this PR? We are about to release new version of this collection and it would be nice to have this feature added.

@jon4hz
Copy link
Contributor Author

jon4hz commented Jun 11, 2024

Actually I was waiting for #1235 to land on main so I can rebase my PR.
If you want to merge this PR before that, I should be able to finish it today or tomorrow.

@pyrodie18
Copy link
Collaborator

I will be bringing in #1235 shortly. If you can get this done in the next day or two that would be great.

feat: zabbix-java-gateway installation on suse

feat: zabbix-proxy installation on suse

feat: zabbix-server installation on suse

fix: remove mysql stuff

fix: zabbix-server v6.2 is not working

feat: zabbix-web installation on suse

fix: zabbix-web php configuration

fix: rebasing issues

fix: install suse repo with zabbix_repo role

fix: zabbix_repo tests on suse

fix: zabbix 7

fix: zabbix_server role

ci: fix zabbix_repo test

fix: zabbix_proxy role

fix: apply suggestions from review
@jon4hz jon4hz force-pushed the suse-sles branch 4 times, most recently from 0dd4b1a to 6fa4f02 Compare June 11, 2024 21:16
@jon4hz
Copy link
Contributor Author

jon4hz commented Jun 12, 2024

This PR should be ready now.

@jon4hz jon4hz marked this pull request as ready for review June 12, 2024 17:38
@pyrodie18 pyrodie18 merged commit 0fbe9b6 into ansible-collections:main Jun 13, 2024
339 of 341 checks passed
emrocha pushed a commit to emrocha/community.zabbix that referenced this pull request Jul 1, 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.

5 participants