Skip to content
This repository has been archived by the owner on Apr 12, 2022. It is now read-only.

Rename the handlers #51

Merged
merged 1 commit into from Nov 26, 2019
Merged

Rename the handlers #51

merged 1 commit into from Nov 26, 2019

Conversation

ktibi
Copy link
Contributor

@ktibi ktibi commented Nov 5, 2019

Closes : #47

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@elasticcla
Copy link

Hi @ktibi, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@ktibi
Copy link
Contributor Author

ktibi commented Nov 5, 2019

@jmlrt Hi, can you approve CI ?

@jmlrt
Copy link
Member

jmlrt commented Nov 6, 2019

jenkins test this please

@jmlrt
Copy link
Member

jmlrt commented Nov 6, 2019

Hi @ktibi,
The {{ beat }} variable in handler's name was added in #34 because of an issue when running more than one role at once where only the one for the last beat role run was notified.
We'll need to do more test to ensure that we won't introduce regression here.

@jmlrt jmlrt self-assigned this Nov 6, 2019
@jmlrt jmlrt added the bug label Nov 6, 2019
@ktibi
Copy link
Contributor Author

ktibi commented Nov 12, 2019

@jmlrt Ok I see why need this feature. But right now, nothing works with ansible :/

@jmlrt
Copy link
Member

jmlrt commented Nov 12, 2019

@ktibi,

After some tests, I can confirm that your PR is breaking fix from #34 (only last {{ beat }} is restarted by handler when people deploy more than one Beat product like in multi test).

This is tricky!

First solution

Regarding ansible/ansible#48466 (comment), I think that {{ beat }} shoud never be set as host_var or group_var in this Ansible role and you should.

I guess the "Ansible way" to do that would be to have some install_filebeat=true, install_metricbeat=falsevariables in host_var or groups_var, then to use include_role with when conditionals in your playbook.

Example:

  hosts: all
  tasks:
    - include_role:
        name: "ansible-beats"
      vars:
        beat: "filebeat"
        beat_conf: {"filebeat": {"inputs":[{"paths":["/var/log/*.log"],"type":"log"}]} }
      when: install_filebeat==true
    - include_role:
        name: "ansible-beats"
      vars:
        beat: "metricbeat"
        beat_conf: { "metricbeat.modules": [{"module": "system", "metricsets": ["cpu", "filesystem", "network", "process"], "enabled": true, "period": "10s", "processes":[".*"], "cpu_ticks": false } ] }
      when: install_metricbeat==true

I tested it sucessfully in Kitchen tests with playbook variables but not with host_var ou groups_var, so that would need a real test to validate.

Second solution

Another option to make your PR works would be to set a fact based on {{ beat }} variable at the begining of the role (facts are not affected by ansible/ansible#48466 (comment)), then to flush the handlers at the end of the role (so that the good Beat is always restarted by handler before the fact is overrided in a new instance of the role for another Beat product).

EDIT:
Created #52 with changes for 2nd solution to test on CI.
@ktibi, if this is working (🤞 🤞 🤞) feel free to include my changes in your PR or implement them in another way.

jmlrt added a commit to jmlrt/ansible-beats that referenced this pull request Nov 12, 2019
@seanpringle
Copy link

I filed #47

+1 for @jmlrt 's second solution, if it's the only way to allow use of a beat host or group var. The first solution doesn't follow the principle of least astonishment.

@ktibi
Copy link
Contributor Author

ktibi commented Nov 26, 2019

@jmlrt Hi, I added your solution in my PR.

@jmlrt
Copy link
Member

jmlrt commented Nov 26, 2019

jenkins test this please

jmlrt
jmlrt previously approved these changes Nov 26, 2019
Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

for consistency maybe renaming {{ beat }} to {{ beat_product }} also in https://github.com/elastic/ansible-beats/pull/52/files#diff-bc46ab85c7ef0bbf4d92e78378717f8cR17-R18 would be nice.

Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

@jmlrt
Copy link
Member

jmlrt commented Nov 26, 2019

jenkins test this please

@jmlrt jmlrt merged commit 9368015 into elastic:master Nov 26, 2019
@jmlrt
Copy link
Member

jmlrt commented Nov 26, 2019

Thanks for the PR @ktibi 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'beat' host_var breaks restart handler
5 participants