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

zabbix_agent- Reworked Include logic similar to the Alias logic #1335

Merged

Conversation

Thulium-Drake
Copy link
Contributor

SUMMARY

Reworks the 'Include' logic to work in the same manner as 'Alias'.

Also updates the included paths for Zabbix Agent2 to include the plugins.d directory.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

zabbix_agent

ADDITIONAL INFORMATION

@Thulium-Drake Thulium-Drake changed the title Reworked Include logic similar to the Alias logic zabbix_agent- Reworked Include logic similar to the Alias logic Jul 9, 2024
@pyrodie18
Copy link
Collaborator

Please add a change fragment to this.

@pyrodie18
Copy link
Collaborator

@Thulium-Drake check out the testing failures

@Thulium-Drake
Copy link
Contributor Author

I've moved adding the *.conf to all paths for includes in the template. I couldn't wrap my head around something that didn't look horribly complex (and prolly also weird).

Also, it doesn't really make sense to use anything else then *.conf as the suffix for include paths.

@BGmot
Copy link
Collaborator

BGmot commented Jul 10, 2024

Can you please add tests to molecule to cover this "use case"?

@BGmot
Copy link
Collaborator

BGmot commented Jul 10, 2024

Oh actually tests are failing, please take a look.

@Thulium-Drake
Copy link
Contributor Author

Well I missed ensuring permissions for all included dirs on agent2 :-) and by fixing that, I also fixed the tests

@pyrodie18
Copy link
Collaborator

We still have a problem with the definition of zabbix_agent_docker_volumes in the defaults file which is expecting a single item

@BGmot
Copy link
Collaborator

BGmot commented Jul 13, 2024

@Thulium-Drake can you please add molecule tests for this case so the problem is obvious before your fix and it's gone with your code?

@Thulium-Drake
Copy link
Contributor Author

@Thulium-Drake can you please add molecule tests for this case so the problem is obvious before your fix and it's gone with your code?

I looked at the test that failed, it checks if the ownership of all mentioned include_dirs is root:zabbix. And because there's 2 scenarios, with a different amount of directories, I can't really see a way to make that 'sensing', as it will check all dirs if it's a list and only one if it's not.

We still have a problem with the definition of zabbix_agent_docker_volumes in the defaults file which is expecting a single item

I've changed the docker_include_dir to a new variable, which will be set in the Docker.yml task.

If it's a single dir, include that. If it's a list, mount the first directory on the list, as it doesn't really make sense to scatter all Zabbix config files around and keep them in subdirs in /etc/zabbix/zabbix_agent.d.

@Thulium-Drake
Copy link
Contributor Author

The errors don't seem related, they are all Galaxy errors

@BGmot
Copy link
Collaborator

BGmot commented Jul 19, 2024

I am sorry I don't get it. What benefits does this PR bring? What is the actual bug it is fixing? @pyrodie18 do you know?

@Thulium-Drake
Copy link
Contributor Author

The fact that role breaks agent2's include mechanism, I first made a different solution. Then @pyrodie18 suggested to re-implement that using similar logic as used for adding host Alias-es

@Thulium-Drake
Copy link
Contributor Author

#1316

@BGmot
Copy link
Collaborator

BGmot commented Jul 19, 2024

The fact that role breaks agent2's include mechanism, I first made a different solution. Then @pyrodie18 suggested to re-implement that using similar logic as used for adding host Alias-es

Ok, can you add to molecule tests that we see it is broken and after your code changes it is fixed?

@Thulium-Drake
Copy link
Contributor Author

Well the problem is, it doesn't result into any errors in the Zabbix agent, but it does not include recursively. So it's not broken because it's spitting errors, it's broken by no longer including configuration files for, among others, zabbix-agent2-plugin-*

I don't know to to make a test for that, because agent2 doesn't care if you not include the plugins.d folder

@BGmot
Copy link
Collaborator

BGmot commented Jul 19, 2024

So we need this fix to include lets say all the files in
/etc/zabbix/zabbix_agent2.d/plugins.d/
and
in /etc/zabbix/zabbix_agent2.d/
?

@Thulium-Drake
Copy link
Contributor Author

Yes, even though they don't mention it explicitly in the docs (https://www.zabbix.com/documentation/current/en/manual/appendix/config/special_notes_include) it will only match on files in the included dirs..

@BGmot
Copy link
Collaborator

BGmot commented Jul 19, 2024

The same story with zabbix-agent (the first version)?

@Thulium-Drake
Copy link
Contributor Author

OOTB, no, because agent (1) does not have plugins, but that depends on the user, if they have their own include setup, it might be something they use. Though I'd say unlikely (but that's from my own experience)

@BGmot
Copy link
Collaborator

BGmot commented Jul 19, 2024

Ok, thanks a lot for contributing and your explanation! I am fine with this PR. @pyrodie18 please approve too as you deal with roles more that I do -)

@BGmot BGmot requested a review from pyrodie18 July 19, 2024 12:35
@pyrodie18 pyrodie18 merged commit c229ac6 into ansible-collections:main Jul 19, 2024
102 checks passed
@pyrodie18
Copy link
Collaborator

Thanks for the addition

@Thulium-Drake Thulium-Drake deleted the rework_include_based_on_alias branch July 19, 2024 13:07
@pyrodie18 pyrodie18 mentioned this pull request Aug 14, 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.

3 participants