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

Salt support for splunk-otel-collector #1050

Merged
merged 10 commits into from
Jan 19, 2022
Merged

Conversation

Mukeshr-Ram
Copy link
Contributor

@Mukeshr-Ram Mukeshr-Ram commented Dec 17, 2021

Salt support for splunk-otel-connector
Ticket: OTL-1270 OTL-1358
Changes:

@Mukeshr-Ram Mukeshr-Ram requested review from a team as code owners December 17, 2021 16:23

ENV container docker

RUN zypper -n install -l curl dbus-1 systemd-sysvinit tar wget python3-pip ca-certificates
Copy link
Contributor

Choose a reason for hiding this comment

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

Please include the 2 lines from https://github.com/signalfx/splunk-otel-collector/pull/1056/files that I added for the other tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added these 2 lines in opensuse-12 Dockerfile which is used in the salt test.

@@ -0,0 +1,119 @@
# Splunk OpenTelemetry Collector Salt Formula
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename this file to README.md.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, Updated

deployments/salt/Readme.md Outdated Show resolved Hide resolved
exit 1
{% else %}

{% set install_fluentd = salt['pillar.get']('splunk-otel-collector:install_fluentd', 'True') %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should install_fluentd be a boolean instead of string? If so, we should also use https://docs.saltproject.io/en/latest/topics/jinja/index.html#to-bool to convert the value to boolean.

Copy link
Contributor

@nitaliya nitaliya Dec 22, 2021

Choose a reason for hiding this comment

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

Yes, install_fluentd is a boolean variable.
As per your suggestion, I have updated the script to convert the value in boolean type with to_bool.
Thanks for the suggestion!

- require:
- pkg: Install FluentD Linux capability module dependencies

Start FluentD service:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a systemctl daemon-reload step before starting the td-agent service.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the suggestions.
Added systemctl daemon-reload step.
From this, my understanding is when we are making any change in td-agent service config so we need to restart/reload the service of td-agent, hear we are doing that by reloading daemon. Correct me if I am wrong.

deployments/salt/Readme.md Outdated Show resolved Hide resolved
Copy link
Contributor

@theletterf theletterf left a comment

Choose a reason for hiding this comment

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

Tagging @tmorrisonsfx for docs updates.

deployments/salt/README.md Outdated Show resolved Hide resolved
- name: Check out the codebase.
uses: actions/checkout@v2

- name: Lint
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move the lint step to a separate job since it doesn't need to be run twice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the suggestions.
Added salt-lint-test job

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant that the lint job should be separate from the test job, but they can both be in the same workflow. Also, please add the lint job as a dependency to the test job.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the guidance!
I have added dependency on salt-test and salt-lint-test. can you please verify it

{% if splunk_otel_collector_config_source != '' %}

Push custom config for Splunk Otel Collector, if provided:
file.managed:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure the collector service is restarted if the config file has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jcheng-splunk
We tried using the Watch module for restarting the collector service however it's only working with files changed using Salt, So added the file.copy module to copy the default config. This ensures that the service will restart if the config file and environment file get changed.

{% set splunk_ballast_size_mib = salt['pillar.get']('splunk-otel-collector:splunk_ballast_size_mib', '') %}

/etc/otel/collector/splunk-otel-collector.conf:
file.managed:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure the collector service is restarted if this environment file has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestions.
Updated with the suggested changes.

@@ -0,0 +1,115 @@
{% set splunk_fluentd_config = salt['pillar.get']('splunk-otel-collector:splunk_fluentd_config', '/etc/otel/collector/fluentd/fluent.conf') %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure that the td-agent service is restarted if the fluentd config file has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have added fluentd_service.sls file for restarting td-agent service if the fluentd config file has changed.

{% if splunk_fluentd_config_source != '' %}

Push custom FluentD config, if provided:
file.managed:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure that the td-agent service is restarted if the fluentd config has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have added fluentd_service.sls file for restarting td-agent service if the fluentd config file has changed.

@jeffreyc-splunk jeffreyc-splunk merged commit 2fffa8f into signalfx:main Jan 19, 2022
@nitaliya nitaliya deleted the salt branch January 20, 2022 07:35
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.

4 participants