NMS-19541: Minion: Investigate if its possible to move away from Confd by mershad-manesh · Pull Request #8379 · OpenNMS/opennms

mershad-manesh · 2026-03-20T17:29:21Z

Opening this PR to gather feedback;

All Contributors

Have you read our Contribution Guidelines?
Have you (electronically) signed the OpenNMS Contributor Agreement?

External References

Jira (Issue Tracker): https://opennms.atlassian.net/browse/NMS-19541

This reverts commit 91cf192.

…artupDiagnostics

Copilot

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-23T16:57:13Z

+      jms|"")
+        # Default/package-provided feature boots are expected to cover JMS.
+        ;;


When MINION_IPC is set to jms (or unset), the validator does not ensure that previously-generated kafka-*.boot/grpc.boot files are removed. If /opt/minion/etc is persisted across restarts, switching from kafka/grpc back to jms can leave stale boot files behind and still enable non-JMS IPC features. Consider applying the feature-boot templates (or at least cleaning the managed boot files) on every startup based on the current MINION_IPC, or add explicit checks that conflicting boot files are absent for the jms/default case.

Copilot · 2026-03-23T16:57:14Z

+    # Clean only files managed by this script; keep baseline boot files from the image/package.
+    local managed_boot_files=(
+      "kafka-ipc.boot"
+      "kafka-rpc.boot"
+      "kafka-sink.boot"


The cleanup list for managed boot files only removes the new template-generated filenames. If users upgrade with a persisted /opt/minion/etc, older boot files created by previous entrypoint logic (e.g. kafka.boot, disable-activemq.boot, etc.) may remain and conflict with the new kafka-*.boot/disable-jms.boot composition. Consider adding the legacy filenames to the cleanup list (or a broader cleanup strategy) so upgrades don't leave contradictory boot configs behind.

Copilot

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-25T13:11:45Z

+            withEnv("KAFKA_IPC_BOOTSTRAP_SERVERS", OpenNMSContainer.KAFKA_ALIAS + ":9092");
+            withEnv("KAFKA_RPC_BOOTSTRAP_SERVERS", OpenNMSContainer.KAFKA_ALIAS + ":9092");
+            withEnv("KAFKA_SINK_BOOTSTRAP_SERVERS", OpenNMSContainer.KAFKA_ALIAS + ":9092");
+            withEnv("KAFKA_TWIN_BOOTSTRAP_SERVERS", OpenNMSContainer.KAFKA_ALIAS + ":9092");


When using Kafka IPC, the Minion container no longer propagates the stack’s Kafka compression strategy (previously written as compression.type in the generated config). This can lead to inconsistent producer settings vs OpenNMS/Sentinel and potentially larger network usage. Consider setting KAFKA_IPC_COMPRESSION_TYPE (and any other required Kafka client properties) from model.getKafkaCompressionStrategy().getCodec() when IpcStrategy.KAFKA is selected.

Suggested change

withEnv("KAFKA_TWIN_BOOTSTRAP_SERVERS", OpenNMSContainer.KAFKA_ALIAS + ":9092");

withEnv("KAFKA_TWIN_BOOTSTRAP_SERVERS", OpenNMSContainer.KAFKA_ALIAS + ":9092");

if (model.getKafkaCompressionStrategy() != null

&& model.getKafkaCompressionStrategy().getCodec() != null) {

withEnv("KAFKA_IPC_COMPRESSION_TYPE", model.getKafkaCompressionStrategy().getCodec());

}

Copilot · 2026-03-25T13:11:45Z

+        withEnv("OPENNMS_BROKER_URL", "failover:tcp://" + OpenNMSContainer.ALIAS + ":61616");

        if (IpcStrategy.KAFKA.equals(model.getIpcStrategy())) {
-            String kafkaIpc = "{\n" +
-                    "\t\"ipc\": {\n" +
-                    "\t\t\"kafka\": {\n" +
-                    "\t\t\t\"bootstrap.servers\": \""+ OpenNMSContainer.KAFKA_ALIAS +":9092\",\n" +
-                    "\t\t\t\"compression.type\": \""+ model.getKafkaCompressionStrategy().getCodec() +"\"\n" +
-                    "\t\t}\n" +
-                    "\t}\n" +
-                    "}";
-            OverlayUtils.writeYaml(minionConfigYaml, jsonMapper.readValue(kafkaIpc, Map.class));
+            withEnv("MINION_IPC", "kafka");


The removal of minion-config.yaml generation leaves several unused imports/static imports in this class (eg jsonMapper, createTempDirectory, File, FileUtils, MountableFile). Please remove the now-unused imports to keep the file clean and avoid failing style/lint checks.

Copilot · 2026-03-25T13:11:46Z

+        *)
+            echo "[Features] No IPC strategy set via MINION_IPC, using defaults."
+            ;;


parseEnvironment still writes Kafka config when KAFKA_IPC_* variables are set, but applyFeatureBootTemplates only enables the Kafka feature boot files when MINION_IPC=kafka. This allows a partially-configured state (Kafka properties set but Kafka features not enabled) that’s hard to diagnose. Consider auto-selecting MINION_IPC=kafka when KAFKA_IPC_* vars are present, or emitting a clear warning/error when Kafka vars are set without MINION_IPC.

…d sink

Copilot

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-25T17:46:07Z

+acks = ${env:KAFKA_RPC_ACKS:-1}
+bootstrap.servers = ${env:KAFKA_RPC_BOOTSTRAP_SERVERS:-kafka:29092}


The updateConfig helper in entrypoint.sh matches/replaces only key=value (no whitespace). With the current key = value formatting here, env-var updates will append a second entry (e.g. bootstrap.servers=...) instead of replacing the default, leaving duplicate keys in the same .cfg file. Use consistent key=value formatting (or update updateConfig to handle optional whitespace) to avoid ambiguous config resolution.

Suggested change

acks = ${env:KAFKA_RPC_ACKS:-1}

bootstrap.servers = ${env:KAFKA_RPC_BOOTSTRAP_SERVERS:-kafka:29092}

acks=${env:KAFKA_RPC_ACKS:-1}

bootstrap.servers=${env:KAFKA_RPC_BOOTSTRAP_SERVERS:-kafka:29092}

Copilot · 2026-03-25T17:46:08Z

+acks = ${env:KAFKA_SINK_ACKS:-1}
+bootstrap.servers = ${env:KAFKA_SINK_BOOTSTRAP_SERVERS:-kafka:29092}


The updateConfig helper in entrypoint.sh matches/replaces only key=value (no whitespace). With the current key = value formatting here, env-var updates will append a second entry instead of replacing the default, leaving duplicate keys in the same .cfg file. Use consistent key=value formatting (or update updateConfig to handle optional whitespace) to avoid ambiguous config resolution.

Suggested change

acks = ${env:KAFKA_SINK_ACKS:-1}

bootstrap.servers = ${env:KAFKA_SINK_BOOTSTRAP_SERVERS:-kafka:29092}

acks=${env:KAFKA_SINK_ACKS:-1}

bootstrap.servers=${env:KAFKA_SINK_BOOTSTRAP_SERVERS:-kafka:29092}

Copilot · 2026-03-25T17:46:08Z

+acks = ${env:KAFKA_TWIN_ACKS:-1}
+bootstrap.servers = ${env:KAFKA_TWIN_BOOTSTRAP_SERVERS:-kafka:29092}


The updateConfig helper in entrypoint.sh matches/replaces only key=value (no whitespace). With the current key = value formatting here, env-var updates will append a second entry instead of replacing the default, leaving duplicate keys in the same .cfg file. Use consistent key=value formatting (or update updateConfig to handle optional whitespace) to avoid ambiguous config resolution.

Suggested change

acks = ${env:KAFKA_TWIN_ACKS:-1}

bootstrap.servers = ${env:KAFKA_TWIN_BOOTSTRAP_SERVERS:-kafka:29092}

acks=${env:KAFKA_TWIN_ACKS:-1}

bootstrap.servers=${env:KAFKA_TWIN_BOOTSTRAP_SERVERS:-kafka:29092}

Copilot · 2026-03-25T17:46:08Z

+        withEnv("OPENNMS_BROKER_URL", "failover:tcp://" + OpenNMSContainer.ALIAS + ":61616");

        if (IpcStrategy.KAFKA.equals(model.getIpcStrategy())) {
-            String kafkaIpc = "{\n" +
-                    "\t\"ipc\": {\n" +
-                    "\t\t\"kafka\": {\n" +
-                    "\t\t\t\"bootstrap.servers\": \""+ OpenNMSContainer.KAFKA_ALIAS +":9092\",\n" +
-                    "\t\t\t\"compression.type\": \""+ model.getKafkaCompressionStrategy().getCodec() +"\"\n" +
-                    "\t\t}\n" +
-                    "\t}\n" +
-                    "}";
-            OverlayUtils.writeYaml(minionConfigYaml, jsonMapper.readValue(kafkaIpc, Map.class));
+            withEnv("MINION_IPC", "kafka");
+            withEnv("KAFKA_IPC_BOOTSTRAP_SERVERS", OpenNMSContainer.KAFKA_ALIAS + ":9092");
+            withEnv("KAFKA_RPC_BOOTSTRAP_SERVERS", OpenNMSContainer.KAFKA_ALIAS + ":9092");
+            withEnv("KAFKA_SINK_BOOTSTRAP_SERVERS", OpenNMSContainer.KAFKA_ALIAS + ":9092");
+            withEnv("KAFKA_TWIN_BOOTSTRAP_SERVERS", OpenNMSContainer.KAFKA_ALIAS + ":9092");
+            if (model.getKafkaCompressionStrategy() != null) {
+                withEnv("KAFKA_IPC_COMPRESSION_TYPE", model.getKafkaCompressionStrategy().getCodec());
+                withEnv("KAFKA_SINK_COMPRESSION_TYPE", model.getKafkaCompressionStrategy().getCodec());
+                withEnv("KAFKA_TWIN_COMPRESSION_TYPE", model.getKafkaCompressionStrategy().getCodec());


After removing the on-the-fly minion-config.yaml generation, this class appears to retain several now-unused imports/static imports (e.g. createTempDirectory, jsonMapper, FileUtils, MountableFile, etc.). If the build uses Checkstyle/Spotless or error-prone rules, these unused imports can fail CI; please remove any that are no longer referenced.

Copilot · 2026-03-25T17:46:08Z

+syslog.listen.interface = ${env:SYSLOG_LISTEN_INTERFACE:-0.0.0.0}
+syslog.listen.port = ${env:SYSLOG_LISTEN_PORT:-1514}


Environment variable naming is inconsistent between syslog and trapd configs in this PR (SYSLOG_LISTEN_* here vs MINION_TRAPD_LISTEN_* in trapd). Aligning the prefixes (or documenting why they differ) would reduce confusion for container users configuring multiple listeners.

Suggested change

syslog.listen.interface = ${env:SYSLOG_LISTEN_INTERFACE:-0.0.0.0}

syslog.listen.port = ${env:SYSLOG_LISTEN_PORT:-1514}

syslog.listen.interface = ${env:MINION_SYSLOG_LISTEN_INTERFACE:-0.0.0.0}

syslog.listen.port = ${env:MINION_SYSLOG_LISTEN_PORT:-1514}

Copilot

Pull request overview

Copilot reviewed 85 out of 86 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-26T13:28:27Z

 # - All other settings are optional and have sensible defaults
 #
 # Default behavior:
-# - Configuration is managed via confd templates
+# - Configuration is managed via environment variables, which can be set in the Dockerfile, via docker run -e, or in a docker-compose file.
 # - Template uses key/values from /java/agent/prom-jmx-exporter
 PROM_JMX_EXPORTER_ENABLED="${PROM_JMX_EXPORTER_ENABLED:-false}" # required


The comment still says the Prometheus JMX exporter "Template uses key/values from /java/agent/prom-jmx-exporter", but this PR removes confd/template rendering for that file. Update the comment to reflect the current behavior (env-var driven config + static config file) to avoid misleading operators.

Copilot · 2026-03-26T13:28:27Z

    # If config exists in file, replace it. Otherwise, append to file.
-    if grep -E -q "^#?$key=" "$file"; then
-        sed -r -i "s@^#?$key=.*@$key=$value@g" "$file" #note that no config values may contain an '@' char
+    if grep -E -q "^#?\s*$key\s*=" "$file"; then
+        sed -r -i "s@^#?\s*$key\s*=.*@$key=$value@g" "$file" #note that no config values may contain an '@' char
    else
        echo "$key=$value" >> "$file"
    fi


updateConfig treats key as an extended-regex in both grep -E and sed, but keys like bootstrap.servers contain . which will match any character. Also, the replacement uses the raw value, so characters like &, \, or the chosen delimiter (@) can corrupt the result. Consider switching to fixed-string matching (or escaping regex metacharacters in key) and escaping value for safe sed replacement (or using a non-regex-based updater) to make env-driven config robust.

Copilot · 2026-03-26T13:28:28Z

        if (profile.isLegacy()) {
            for (final Map.Entry<String, String> entry : profile.getLegacyConfiguration().entrySet()) {
                addEnv(entry.getKey(), entry.getValue());
            }
-        } else {
-            addFileSystemBind(writeMinionConfig(profile).toString(),
-                    "/opt/minion/minion-config.yaml", BindMode.READ_ONLY, SelinuxContext.SINGLE);
        }



After removing the minion-config YAML generation/bind logic, several imports at the top of this file are now unused (e.g. createTempDirectory, jsonMapper, File, FileUtils, MountableFile). This will fail the build under typical Java compiler settings/checkstyle—please remove the unused imports.

This reverts commit aea6221.

cgorantla

I think we should move templates folder which has boot files to something outside etc to avoid confusion with featuresBoot.d. For ex: ${MINION_HOME}/boot-templates/
We can keep current MINION_IPC=kafka logic where we disable JMS and enable Kafka.
I think we can avoid shipping with .cfg files. Let user customize. We can create all these .cfg files in smoke-test overlay for smoke tests to pass.
When user creates their own docker compose or helm charts, they can create their own version of cfg files with custom environment variables. We may not need to enforce this.

mershad-manesh · 2026-03-27T12:43:32Z

I think we should move templates folder which has boot files to something outside etc to avoid confusion with featuresBoot.d. For ex: ${MINION_HOME}/boot-templates/
We can keep current MINION_IPC=kafka logic where we disable JMS and enable Kafka.

We can move it to another folder, this shouldn't be a problem.

I think we can avoid shipping with .cfg files. Let user customize. We can create all these .cfg files in smoke-test overlay for smoke tests to pass.

Although this will work for smoke tests, it wouldn't be straight forward for users who are using Docker or Helm Charts. How would they know what they need to provide for each configuration file...

When user creates their own docker compose or helm charts, they can create their own version of cfg files with custom environment variables. We may not need to enforce this.

It's fine if user overrides these cfg files, however we still need a basic set of cfg files to allow Minion to start.

mershad-manesh added 25 commits March 16, 2026 15:51

remove confd

40b7fee

use predefined cfg files

985ed10

fix minor mistakes

e6af892

fix the Container

b377bd8

copy the etc files

d708b79

test by setting IPC strategy

be21a60

fix the ssh port

431357d

add missing envsubst

25ef1c2

fix the logic automatically set the fallback strategy

a357605

update the logic to startup boot files

2c913ff

avoid relying on OSGI env

8cb20ab

print the active boot file,add auto recovery to book files

cc11cce

add debugging

8e73b84

debug as to why karaf feature is missing from minion

abc6718

allow the script to run even if Jaeger and dominion is disabled

895d644

remove hardcoded rmiServerHost

da57937

don't use localhost by default, set it to OpenNMS Container

91cf192

Revert "don't use localhost by default, set it to OpenNMS Container"

97b4369

This reverts commit 91cf192.

enable trapd.useAddressFromVarbind

7ab5d5e

override the etc cfg for trapd

bc4c3e3

test

1de04b3

remove deadcode and clean up

8cb2c11

move properties.d for now

007c58f

revert some changes

c806491

add JTI and NXOS listeners

cb8a1f8

mershad-manesh requested review from cgorantla, christianpape, Copilot, indigo423 and synqotik March 20, 2026 17:29

mershad-manesh added 4 commits March 20, 2026 15:14

clean up, using flags for printKarafResolutionDiagnostics and printSt…

d6b8c9d

…artupDiagnostics

a simple test

5e1419f

expose the karaf shell by default

57d3d3d

clean up

d4c8191

mershad-manesh requested a review from Copilot March 23, 2026 16:48

Copilot started reviewing on behalf of mershad-manesh March 23, 2026 16:48 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

mershad-manesh added 5 commits March 23, 2026 12:59

fix the order of precedence

41efa7a

align the environment variables with confd

c618541

add missing Bootstraps env variables.

1db45dc

revert back the parsers index

1bb0ae7

remove the maxClockSkew for SFlowUdpParser

bf5d30a

mershad-manesh requested a review from Copilot March 25, 2026 13:03

Copilot started reviewing on behalf of mershad-manesh March 25, 2026 13:04 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

mershad-manesh added 3 commits March 25, 2026 09:58

Add a check for compression, extend entrypoint to support rpc,twin an…

ce51bb2

…d sink

change the parser

c3f3865

oops getKafkaCompressionType should be getKafkaCompressionStrategy

33ebebd

mershad-manesh requested a review from Copilot March 25, 2026 17:40

Copilot started reviewing on behalf of mershad-manesh March 25, 2026 17:41 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

mershad-manesh added 2 commits March 25, 2026 14:56

address few feedbacks

8fbb543

lets remove confd folder ; what could go wrong

5ddbe53

mershad-manesh requested a review from Copilot March 26, 2026 13:20

Copilot started reviewing on behalf of mershad-manesh March 26, 2026 13:20 View session

Copilot AI reviewed Mar 26, 2026

View reviewed changes

mershad-manesh added 2 commits March 26, 2026 13:34

Sentinel, Initial Change

aea6221

Revert "Sentinel, Initial Change"

2743b4d

This reverts commit aea6221.

cgorantla reviewed Mar 27, 2026

View reviewed changes

		acks = ${env:KAFKA_RPC_ACKS:-1}
		bootstrap.servers = ${env:KAFKA_RPC_BOOTSTRAP_SERVERS:-kafka:29092}

		acks = ${env:KAFKA_SINK_ACKS:-1}
		bootstrap.servers = ${env:KAFKA_SINK_BOOTSTRAP_SERVERS:-kafka:29092}

		acks = ${env:KAFKA_TWIN_ACKS:-1}
		bootstrap.servers = ${env:KAFKA_TWIN_BOOTSTRAP_SERVERS:-kafka:29092}

		syslog.listen.interface = ${env:SYSLOG_LISTEN_INTERFACE:-0.0.0.0}
		syslog.listen.port = ${env:SYSLOG_LISTEN_PORT:-1514}

Conversation

mershad-manesh commented Mar 20, 2026

All Contributors

External References

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

Uh oh!

cgorantla left a comment

Choose a reason for hiding this comment

Uh oh!

mershad-manesh commented Mar 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants