ansible-collections / community.zabbix

Zabbix Ansible modules
http://galaxy.ansible.com/community/zabbix
Other
315 stars 265 forks source link

Fix #1236 - removal of wrong include directory #1302

Closed Japje closed 1 week ago

Japje commented 1 week ago
SUMMARY

When both zabbix_agent2 and zabbix_agent_package_remove are true this removes the wrong include directory breaking the run.

Fixes #1236

ISSUE TYPE
COMPONENT NAME

tasks/remove.yml

ADDITIONAL INFORMATION

Since agent and agent2 both use the same variable names to determine which path to use for the include Directory the choice which one to use is based on the zabbix_agent2 variable.

loading var from agent_vars.yml and using the internal vars to stop the service, remove the package and include-dir prevent this issue.

Before:

TASK [community.zabbix.zabbix_agent : Remove | Remove the agent-include-dir]
changed: [zabbix.example.com]

<snip for readability>

TASK [community.zabbix.zabbix_agent : Install user-defined userparameters]
failed: [zabbix.example.com] (item={'name': 'custom_parameter'}) => {"ansible_loop_var": "item", "changed": false, "checksum": "a9e014f8469a37318d75f1dbcff55e0345402da4", "item": {"name": "custom_parameter"}, "msg": "Destination directory /etc/zabbix/zabbix_agent2.d does not exist"}

After:

TASK [community.zabbix.zabbix_agent : Remove | Remove the agent-include-dir]
ok: [zabbix.example.com]

<snip for readability>

TASK [community.zabbix.zabbix_agent : Install user-defined userparameters]
ok: [zabbix.example.com] => (item={'name': 'custom_parameter'})
Japje commented 1 week ago

Unless I've misread the code it's because remove.yml is specifically for removing zabbix-agent (non 2) according to:

https://github.com/ansible-collections/community.zabbix/blob/8805f3267854a5cb9f5502d00c32754ac996484a/roles/zabbix_agent/tasks/Linux.yml#L147

However because the current code loads the agent2 vars it removes the wrong include directory.

pyrodie18 commented 1 week ago

OK you're right. Unfortunately when you load the agent_vars.yml though then you overwrite other vars that get used later on.

Japje commented 1 week ago

The variables used throughout the play are based on the loaded agent vars that are set here: https://github.com/ansible-collections/community.zabbix/blob/8805f3267854a5cb9f5502d00c32754ac996484a/roles/zabbix_agent/tasks/main.yml#L27

That's where the internal vars get put in zabbix_agent_*

No where else are the internal variables from agent(2)_vars.yml used. This made me feel ok to use them in remove.yml since they are otherwise unused during the rest of the play.

Its a clean way of using the values of agent_vars.yml during a play that has agent2_vars loaded everywhere else.

If you prefer a different solution i would need a little bit of guidance what a preferred way would look like.

pyrodie18 commented 1 week ago

So the win* vars get used directly in the windows tasks. I understand that for a Windows install, there's no chance of the remove tasks running, but I just feel like we're playing with fire. I'm trying to figure out a way to load a var from the file for a single task but can't seem to figure out a way. My best bet right now would be to add a 2nd variable within the agent2 var file that has the path for agent. Its a little lazy and I don't love it but its an option.

Japje commented 1 week ago

I could also not find a way to limit the scope of the loaded vars to within the remove tasks. Adding a var to agent2 just to be used in a single task does not spark joy for me.

Even though I'm not a fan of it what would your opinion be on just hard coding the path in the task? Adding a second var in agent2 will remove the relation to the agent vars anyway. So that's hard coding but with an extra step.

pyrodie18 commented 1 week ago

I could also not find a way to limit the scope of the loaded vars to within the remove tasks. Adding a var to agent2 just to be used in a single task does not spark joy for me.

Even though I'm not a fan of it what would your opinion be on just hard coding the path in the task? Adding a second var in agent2 will remove the relation to the agent vars anyway. So that's hard coding but with an extra step.

Ya totally agree with not sparking joy. Here is a thought, why not do "{{ zabbix_agent_include | replace('agent2', 'agent') }}"?

Japje commented 1 week ago

@pyrodie18 thanks for the suggestion, i could not come up with anything nicer myself. New commit reflects this change. Tested on my local environment that had zabbix-agent installed:

TASK [community.zabbix.zabbix_agent : Remove | Make sure the "old" zabbix-agent service stopped]
changed: [zabbix.example.com]

TASK [community.zabbix.zabbix_agent : Remove | Package removal]
changed: [zabbix.example.com]

TASK [community.zabbix.zabbix_agent : Remove | Remove the agent-include-dir]
ok: [zabbix.example.com]

TASK [community.zabbix.zabbix_agent : Make sure the zabbix-agent service is running]
ok: [zabbix.example.com]

If there is any more feedback please let me know.

pyrodie18 commented 1 week ago

Looks good to me. Gonna run it through and assuming nothing breaks (don't expect it will) we'll get it pulled in. Thanks for the work.

derekpurdy commented 1 week ago

Looks like it's creating the zabbix_agent2.d directory during "community.zabbix.zabbix_agent : Create include dir zabbix-agent" and still deleting it in "Remove | Remove the agent-include-dir" when using agent2.

Japje commented 1 week ago

@derekpurdy could you provide me with the settings you use when running the role?

derekpurdy commented 1 week ago

@derekpurdy could you provide me with the settings you use when running the role?

@Japje I'm just calling the role via the playbook below. Within inventory, nothing more then just templates being added.

---

- name: Playbook for installing Zabbix Agent
  hosts:
    - jumphost
    - pi
    - zabbix
  become: true

  vars:
    zabbix_agent_inventory_mode: automatic
    zabbix_agent2: true
    zabbix_agent_package_remove: true
    zabbix_agent_package_state: latest
    zabbix_api_create_hostgroup: true
    zabbix_api_create_hosts: true
    zabbix_api_login_pass: '{{ vault_zabbix_api_pass }}'
    zabbix_api_login_user: Admin
    zabbix_api_server_host: zabbix.lan
    zabbix_install_pip_packages: true
    zabbix_agent_tlspsk_auto: true

  roles:
    - role: community.zabbix.zabbix_agent
Japje commented 1 week ago

I'm unable to reproduce the error with your config.

My requirements.yml:

---
collections:
  - name: git@github.com:ansible-collections/community.zabbix.git
    type: git
    version: main

list of installed collections:

$ ansible-galaxy collection list |grep zabbix
community.zabbix                         3.0.1

Play ran without issues on a clean Debian host every time. I used your exact playbook but did comment out all zabbix_api_* variables but that should not affect the run.

derekpurdy commented 1 week ago

@Japje I've deleted the collection locally and downloaded it again and I can't reproduce it either, even if I put the old collection back. Previously it was happening consistently, so I'm going to leave this as a configuration issue on my side. If some how it returns and I can identify a reason I will update you. But feel free to note my issue as unreproducible.

Thanks for the assistance.