ansible-middleware / amq

A collection to manage AMQ brokers
Apache License 2.0
16 stars 12 forks source link

Overriding "logger config template" does not work as expected #112

Closed christianmasopust closed 6 months ago

christianmasopust commented 10 months ago
SUMMARY

In our setup we plan to forward the Artemis logs to Elasticsearch. To enable that, we would need to use our own log4j2.properties or logging.properties template.

Our idea was to use the existing option (_amq_broker_logger_configtemplate) to override this template, but it doesn't work as expected, it is only there for selecting the right logger config file:

amq_broker_logger_config_template: "{{ 'log4j2.properties' if amq_broker_version is version_compare('7.11.0', '>=') else 'logging.properties' }}"

And it's then used in the configuration of the broker as:

  ansible.builtin.template:
    src: "{{ amq_broker_logger_config_template }}.j2"
    dest: "{{ amq_broker.instance_home }}/etc/{{ amq_broker_logger_config_template }}"

Which, of course, will not work when _amq_broker_logger_configtemplate is a path.

Maybe the intention for _amq_broker_logger_configtemplate was not to allow the user to override the template, so you may this not consider as a bug and more as a feature request...

ISSUE TYPE
EXPECTED RESULTS

Having the possibility to use our own template for the broker logging configuration similar as it is already implemented for the systemd service.

Maybe a possible solution would be to not completely override the template but have the option to specify a "template path" and keep the decision which template (logging or log4j2) up to the role.

guidograzioli commented 10 months ago

Hello, thanks for reporting.

There's indeed a missing | basename here:

dest: "{{ amq_broker.instance_home }}/etc/{{ amq_broker_logger_config_template }}"

You should then be able to set amq_broker_logger_config_template to an arbitrary path, and if using a different filename than log4j2.properties you need also to amend activemq_java_opts with -Dlog4j2.configurationFile=...

christianmasopust commented 10 months ago

Yes, that may be a working fix. But then we would have to take care about which Artemis version we are using (to decide whether we have to use the previous logging.properties or the newer log4j2.properties template).

When adding something like "template_location" at the source (which by default is an empty string), then to role still would decide which "type" of logging template it has to be used and the location would just tell it where to find it.

What I mean would be:

  ansible.builtin.template:
    src: "{{ template_location }}{{ amq_broker_logger_config_template }}.j2"
    dest: "{{ amq_broker.instance_home }}/etc/{{ amq_broker_logger_config_template }}"

Or do you think that may cause other issues?

guidograzioli commented 10 months ago

I see your point; I'll do some testing to see if it has any drawback (not one I can think of atm).

Otherwise, you could define:

amq_broker_logger_config_template: "{{ 'custompath/' + amq_broker_logger_config_template }}"

and basename would be enough; but let me amend molecule and do some test with you proposal first