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

Make sure the right beat service gets restarted #34

Merged
merged 1 commit into from
Aug 30, 2019

Conversation

nyetwurk
Copy link
Contributor

When running more than one role at once, make sure all of the right notifies
get triggered, not just the one for the last beat role run.

@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?

@jmlrt
Copy link
Member

jmlrt commented Jul 30, 2019

jenkins test this please

@jmlrt jmlrt self-assigned this Jul 30, 2019
@jmlrt jmlrt self-requested a review July 30, 2019 15:15
@jmlrt jmlrt removed their assignment Jul 30, 2019
@jmlrt
Copy link
Member

jmlrt commented Jul 30, 2019

Hi @nyetwurk,
Thanks for this PR.

Every "debian-based" tests are failing => elastic+ansible-beats+pull-request/10.
It seems that you forgot to rename notify statement in beats-debian.yml.
Can you fix that?

@nyetwurk
Copy link
Contributor Author

Hi @nyetwurk,
Thanks for this PR.

Every "debian-based" tests are failing => elastic+ansible-beats+pull-request/10.
It seems that you forgot to rename notify statement in beats-debian.yml.
Can you fix that?

Hopefully fixed. Sorry about the oversight :)

@nyetwurk nyetwurk force-pushed the nyet-fix-restart-PR1 branch 2 times, most recently from 04e2c0a to 9c33a47 Compare July 30, 2019 15:58
@jmlrt
Copy link
Member

jmlrt commented Jul 30, 2019

jenkins test this please

@nyetwurk
Copy link
Contributor Author

nyetwurk commented Aug 3, 2019

Can someone please approve so I can keep my downstream clean :) thx

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.

Hi @nyetwurk,
Code review is good, Automatic test are almost good.

For any reason, I don't understand {{ beat }} isn't expanded on restart handler name on centos (see elastic+ansible-beats+pull-request/11/OS=centos-7,TEST_TYPE=multi at 17:52:40) also it's OK on debian based OS.

I'll relaunch tests as I don't what's the problem in the code.

In addition, I think we could use {{ beat }} in Start beats service task name also.

@jmlrt
Copy link
Member

jmlrt commented Aug 3, 2019

Jenkins test this please

@nyetwurk
Copy link
Contributor Author

nyetwurk commented Aug 3, 2019

@jmlrt agreed on the Starts beats service name as well. I will add it time permitting. Unfortunately I don't have CentosOS to test on, but do have Ubuntu/Debian/MacOS and all three of those seem fine.

@nyetwurk
Copy link
Contributor Author

nyetwurk commented Aug 3, 2019

@jmlrt I think there is something wrong with the tests. It doesn't seem as if the handler isn't working, it seems as though none of the tasks that notify are being set as changed: true

When running more than one role at once, make sure all of the right notifies
get triggered, not just the one for the last beat role run.
@jmlrt
Copy link
Member

jmlrt commented Aug 3, 2019

FYI you can run automated test locally on all supported OS with our KitchenCI setup assuming you have ruby and docker installed.

For this PR, you would need to run make converge PATTERN=multi-centos-7 to test a deployment with filebeat and metricbeat on centos (playbook test/integration/multi.yml).

More details on the testing setup here.

@jmlrt
Copy link
Member

jmlrt commented Aug 3, 2019

@jmlrt I think there is something wrong with the tests. It doesn't seem as if the handler isn't working, it seems as though none of the tasks that notify are being set as changed: true

The restart {{ beat }} handler is executed only when beat wasn't started by Start beats service as you can see in the not beats_started.changed condition.

This way beat doesn't need to be restarted by handler during first ansible role deployment because it is already started by Start {{ beat }} service with up to date configuration.

But if you change beats config and run the ansible role another time with beats already started, Start {{ beat }} service task will be in ok status (beats_started.changed == false) so the restart handler will restart beats to ensure new config is loaded.

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.

LGTM 👍

@jmlrt
Copy link
Member

jmlrt commented Aug 3, 2019

Jenkins test this please

@nyetwurk
Copy link
Contributor Author

nyetwurk commented Aug 3, 2019

Does that mean we need separate beats_not_changed for each kind of beat like we did for the handler (e.g. {{ beat }}_started.changed or {{beat}}_started.changed) since if you run two instances of the role from the same parent, the last one wins?

@jmlrt
Copy link
Member

jmlrt commented Aug 3, 2019

Yes we may need it, also I'm not sure.
I'm in holiday next week so I won't be able to test it.
Feel free to do some test if you have time for it or I'll take some time for that the week after.

@jmlrt
Copy link
Member

jmlrt commented Aug 30, 2019

@nyetwurk
I finally found the time to do a few tests.

I can confirm you that we don't need separated not beats_started.changed conditions.

When testing it with make converge multi-centos-7:

  • If I change only the configuration of filebeat (first instance in the role) and replay the role => only filebeat is restarted
  • If I change only the configuration of metricbeat (second instance in the role) and replay the role => only metricbeat is restarted
  • If I change only the configuration of both filebeat and metricbeat and replay the role => the two beats are restarted.

@jmlrt jmlrt merged commit 4155ca3 into elastic:master Aug 30, 2019
@nyetwurk nyetwurk deleted the nyet-fix-restart-PR1 branch August 30, 2019 15:01
@jmlrt jmlrt added the bug label Sep 17, 2019
@jmlrt jmlrt mentioned this pull request Nov 6, 2019
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.

None yet

3 participants