Skip to content

fix: source jvm.config from peon.sh for K8s peon containers#19364

Open
Yomanz wants to merge 5 commits intoapache:masterfrom
widgetbot-io:fix/peon-sh-jvm-config
Open

fix: source jvm.config from peon.sh for K8s peon containers#19364
Yomanz wants to merge 5 commits intoapache:masterfrom
widgetbot-io:fix/peon-sh-jvm-config

Conversation

@Yomanz
Copy link
Copy Markdown

@Yomanz Yomanz commented Apr 22, 2026

Fixes #18791.

Description

distribution/docker/peon.sh never sourced $SERVICE_CONF_DIR/jvm.config. As a result, JVM options declared in a jvm.config ConfigMap, which is documented at docs/development/extensions-core/k8s-jobs.md under the Custom Template Pod Adapter are silently ignored by peon containers.
Peons started with whatever $JAVA_OPTS the container happened to inherit and then go back to container-aware JDK defaults (MaxRAMPercentage=25%, MaxDirectMemorySize=Xmx). This causes OutOfMemoryError with a mysterious cause.

The bug affects all three K8s pod adapters equally at the shell level. peon.sh is the entry point for every K8s-launched peon but is most impactful for customTemplateAdapter users.
K8sTaskAdapter (used by overlordSingleContainer / overlordMultiContainer) additionally injects a JAVA_OPTS env var from druid.indexer.runner.javaOptsArray, so its users had a working configuration path even without jvm.config sourcing. PodTemplateTaskAdapter#getEnv does not inject JAVA_OPTS, leaving customTemplateAdapter users with only the documented-but-broken jvm.config path and the workaround of hardcoding JAVA_OPTS into the pod template's env: block.

Fix: source jvm.config from peon.sh

Mirror the behaviour that distribution/docker/druid.sh already has at line 188:

if [ -f "$SERVICE_CONF_DIR/jvm.config" ]; then
    JAVA_OPTS="$(cat $SERVICE_CONF_DIR/jvm.config | xargs) $JAVA_OPTS"
fi

The -f guard is the only deviation from druid.sh. It preserves existing behaviour for clusters that don't mount a jvm.config at all, avoiding a new cat: No such file or directory stderr line on every peon startup.
Options in JAVA_OPTS take precedence over those in jvm.config under OpenJDK (same caveat as druid.sh), so adapters that inject JAVA_OPTS via javaOptsArray continue to win duplicates.

Docs update

Added a bit of info to docs/development/extensions-core/k8s-jobs.md under the Custom Template Pod Adapter section, making the mechanism explicit: peon.sh sources jvm.config from the directory mounted as nodetype-config-volume, prepends it to JAVA_OPTS, and JAVA_OPTS wins for duplicates. The existing example ConfigMap with -Xmx1024M -XX:MaxDirectMemorySize=1000M now behaves as documented.

Test added

Added KubernetesPeonJvmConfigDockerTest under embedded-tests/.../k8s/. It uses a new operator manifest variant that injects -Ddruid.test.peon.jvmconfig.marker=true into the cluster-level jvm.options (written by the Druid operator into each node's jvm.config), submits a long-running NoopTask, port-forwards to the resulting peon pod via the fabric8 client, and asserts the marker appears in the peon JVM's /status/properties. The test is disabled though, like the existing KubernetesTaskRunnerDirectModeDockerTest / KubernetesTaskRunnerCachingModeDockerTest pending resolution of the charts.datainfra.io availability issue (#19047).

Small harness refactor to support this:

  • BaseKubernetesTaskRunnerDockerTest now exposes getManifestTemplate() as an overridable hook and promotes the local k3sCluster variable to a protected field so subclasses can reach into the cluster.
  • K3sClusterResource gains a public getKubernetesClient() getter returning the fabric8 client.

Release note

Fixed: JVM options declared in a jvm.config ConfigMap are now honoured by peons launched via the Kubernetes overlord extension (including when using the customTemplateAdapter). Previously, peon.sh silently ignored jvm.config, causing the peon JVM to fall back to container-aware JDK defaults.


Key changed/added files in this PR
  • distribution/docker/peon.sh
  • docs/development/extensions-core/k8s-jobs.md
  • embedded-tests/src/test/java/org/apache/druid/testing/embedded/k8s/KubernetesPeonJvmConfigDockerTest.java (new)
  • embedded-tests/src/test/java/org/apache/druid/testing/embedded/k8s/BaseKubernetesTaskRunnerDockerTest.java
  • embedded-tests/src/test/java/org/apache/druid/testing/embedded/k8s/K3sClusterResource.java
  • embedded-tests/src/test/resources/manifests/druid-service-with-operator-peonjvmconfig.yaml (new)

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

* {@code /status/properties} on the peon pod.
*/
@Disabled("requires charts.datainfra.io chart, see https://github.com/apache/druid/pull/19047")
public class KubernetesPeonJvmConfigDockerTest extends BaseKubernetesTaskRunnerDockerTest
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

P2 Regression coverage is permanently disabled

The new regression test that verifies peon.sh sources jvm.config is annotated with @disabled, so the fix has no active automated coverage. Since this bug is specifically in container startup behavior, leaving the only regression test disabled means the same breakage can return without CI noticing. Please either make this test runnable in the existing embedded test setup or add a narrower active test around the script/config generation path.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Just pushed something to cover this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve on the Middlemanager-less documentation

3 participants