Graylog2 / graylog-ansible-role

Ansible role which installs and configures Graylog
Apache License 2.0
212 stars 127 forks source link

Do not update cache when installing server #51

Closed veger closed 7 years ago

veger commented 7 years ago

This prevents the Ansible changed state of "Install Graylog server" task when nothing actually changed.

The cache is (forced) updated when a new repo is installed. The OS updates (should) the cache periodically.

danvaida commented 7 years ago

@veger as I added the idempotence tests to this repo, I feel responsible to ask: where and under what conditions did you see this role (or even its dependencies) not being idempotent? If that's the case, then it means the idempotence test is also buggy and needs to be fixed. I just did now:

$ docker build -t graylog-ansible-role-wheezy -f tests/support/wheezy.Dockerfile tests/support
[...]
$ docker run -it -v $PWD:/role graylog-ansible-role-wheezy
[...]
Playbook is idempotent

Furthermore, if I understand correctly, you want to remove this line because the OS (or any other system) should care for checking the repositories for updates and that these are being checked when a new repo is being added in the first place, right?

Although I do understand people saying "don't assume anything", the Graylog role is not responsible for making sure the repos' cache is up-to-date. It does that, simply for ensuring its endeavor is successful and is according to what we (the committers) wanted. As a consequence, the whole cache is going to be updated. I don't regard that as a terrible thing to happen (apart from the potential performance penalty and broken idempotence on the long run) BUT it should NOT happen as we're not (and we really shouldn't) try to upgrade graylog-server; so if the package is already installed on the server, then that's it.

I understand the limitations of the idempotence tests: if one runs the same playbook 2 months after the initial run(s), the task will report as changed in this context. I believe this is your case, right?

Anyway, nice catch.

veger commented 7 years ago

For clarity, we are using Ubuntu 16.04 as OS running on Amazon EC2 instances. I never tried the docker images/tests.

I feel responsible to ask: where and under what conditions did you see this role (or even its dependencies) not being idempotent?

I'll try:

When running the role, the mentioned task always calls apt-get update due to theupdate_cache: True. This is (according to Ansible) a change in the system (which could be agreed on if you include repo content as part of the system, although one could run this over and over, so as far as I understand, it does not influence the impotence).

The Ansible output when running the role for a second time on a cluster of 3 clean Amazon EC2 instances:

... skipped part of output ...

TASK [Graylog2.graylog-ansible-role : Install Graylog repository] **************
ok: [54.89.70.110]
ok: [54.210.191.58]
ok: [52.91.4.185]

TASK [Graylog2.graylog-ansible-role : apt] *************************************
skipping: [52.91.4.185]
skipping: [54.89.70.110]
skipping: [54.210.191.58]

TASK [Graylog2.graylog-ansible-role : Install Graylog server] ******************
changed: [54.89.70.110]
changed: [54.210.191.58]
changed: [52.91.4.185]

TASK [Graylog2.graylog-ansible-role : Create directory] ************************
ok: [54.89.70.110]
ok: [54.210.191.58]
ok: [52.91.4.185]

... rest of output ...

As you can see, the Graylog2.graylog-ansible-role : Install Graylog serve is set to changed. With this PR, this task returns ok for all hosts.

If that's the case, then it means the idempotence test is also buggy and needs to be fixed

I guess...

Furthermore, if I understand correctly, you want to remove this line because the OS (or any other system) should care for checking the repositories for updates

Yes

and that these are being checked when a new repo is being added in the first place, right?

I can understand that a trigger, to check the repositories, is required when a new repository added. Also, in this situation the system is already changed and this additional change does not change this fact.

So 'no' in this case.

Although I do understand people saying "don't assume anything", the Graylog role is not responsible for making sure the repos' cache is up-to-date

I agree.

To make it stronger: I would like to choose which Graylog version is installed, e.g. when we need to reinstall one of the hosts in the cluster with exact the same configuration as the rest of the cluster. (We do this in a separate task now, since this role does not support this).

BUT it should NOT happen as we're not (and we really shouldn't) try to upgrade graylog-server; so if the package is already installed on the server, then that's it.

Ok, I did not know that it is a policy to 'not upgrade graylog-server', so this PR seems more important now...

Please let me know, if you are (still) unable to reproduce this. (Although we are running towards a deadline, so I do not have much time to try the docker tests...)

danvaida commented 7 years ago

I can confirm that the idempotence test passes while running in a container powered by the official Ubuntu Xenial docker image.

I'm going to push my changes and then open a PR myself that mentions support for Ubuntu Xenial.

+1 for merging this PR.

@mariussturm can you trigger the Travis job again?

mariussturm commented 7 years ago

Thanks!