Skip to content

feat(chart): hold container logs back instead of dropping them when ClickHouse is down - #678

Merged
blue4209211 merged 1 commit into
mainfrom
feat/logs-backpressure
Oct 24, 2026
Merged

blue4209211 merged 1 commit into
mainfrom
feat/logs-backpressure

Conversation

@reckless-sherixx

@reckless-sherixx reckless-sherixx commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

With logs.collection=node-agent, container logs were lost whenever the node-agent, the collector or ClickHouse restarted. This is the chart half of at-least-once delivery. The node-agent half (nudgebee/node-agent#407) saves its position in each file and retries until the collector accepts.

  • node-agent (logs.collection=node-agent only): LOGS_POSITIONS_FILE on a per-release hostPath under /var/lib/nudgebee-node-agent, so a restarted node-agent resumes where delivery stopped. Other modes keep the same pod spec.
  • Collector:
    • The logs pipeline gets its own exporter, clickhouse/logs. Its connection settings are templated from clickhouse, so overrides carry over.
    • It retries with no time limit, from a 100,000-record queue that blocks the receiver when full.
    • The logs pipeline no longer has the batch processor. That processor accepted records and then dropped them after 300 s of failed retries.
    • During a ClickHouse outage, node-agents now keep unsent lines in the node's log files. Traces and metrics keep the shared exporter.

Merge after the node-agent release that reads LOGS_POSITIONS_FILE, with nodeAgent.image.tag bumped in this PR. An older node-agent ignores the variable, but it doesn't retry when the collector holds a request.

Limit: an outage is absorbed while each container writes less than about one log file (10 MiB by default). Files rotated away in the meantime are compressed by the kubelet, and the node-agent doesn't read those.
The logs queue is in memory, so records waiting in it are lost if the collector itself restarts during a ClickHouse outage. A persistent queue is a follow-up.

Type of change

  • Bug fix (non-breaking)
  • New feature (non-breaking)
  • Breaking change (chart upgrade requires user action)
  • Documentation only
  • CI / tooling

Chart version

  • I bumped version in charts/nudgebee-agent/Chart.yaml (required for user-visible changes): 0.1.32
  • No version bump needed (docs/CI only)

Test plan

  • helm lint charts/nudgebee-agent passes.
  • ct lint --config .github/configs/ct.yaml: run by CI.
  • helm template reviewed. The default mode differs from main only in the collector config. The rendered config passes otelcol validate on collector-contrib 0.157.
  • Local docker: ClickHouse 24.12 and collector-contrib 0.157 with this rendered config, plus the node-agent change. 100 lines/s, rotating every 40 s. Lines lost per scenario:
Scenario before after
node-agent SIGTERM and SIGKILL, collector restart, ClickHouse restart 3,772 of 26,760 0 of 27,017, 0 duplicates
ClickHouse down 330 s 5,436 of 39,446 0 of 39,327

Related issues

Depends on nudgebee/node-agent#407 (and its nudgebee/logparser#36).

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces persistent log position tracking for the nudgebee-agent daemonset to ensure at-least-once log delivery. It mounts a host path volume to store log positions and configures a dedicated clickhouse/logs exporter in the OpenTelemetry Collector with custom queueing and retry settings to prevent log loss during ClickHouse outages. Feedback on the changes highlights a critical configuration issue where the batching configuration is incorrectly nested inside sending_queue instead of being a sibling batcher block, which would cause startup failure. Additionally, the queue_size is configured too high because sizer: items counts batches rather than individual records, posing an OOM risk.

Comment thread charts/nudgebee-agent/values.yaml Resolved

@blue4209211 blue4209211 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The design is right: a dedicated clickhouse/logs exporter with unbounded retry and a blocking queue, no batch processor in the logs pipeline, and positions on a per-release hostPath only in logs.collection=node-agent mode. I checked that both the env var and the mount sit inside that branch, so other modes' pod spec is unchanged. The loss numbers in the test table are convincing. Your reply on the sending_queue.batch form matches the 0.157 exporterhelper README.

Before merge:

  1. Release order. This needs the node-agent release that reads LOGS_POSITIONS_FILE (nudgebee/node-agent#407, still open), plus the nodeAgent.image.tag bump in this PR. Until then it shouldn't merge, since an older node-agent doesn't retry when the collector holds a request.
  2. Chart version. #677 takes 0.1.31 and this PR 0.1.32. Merge in that order, or rebase whichever lands second.

Known limits, fine as documented, worth tracking:

  • The queue is in memory, so records queued while ClickHouse is down are lost if the collector itself restarts or is OOM-killed. A file_storage-backed persistent queue is the natural follow-up.
  • Outages longer than about one log file per container (containerLogMaxSize) still lose the rotated-away files.
  • With #677, a disk at 95% holds logs indefinitely while traces and metrics are dropped after 300 s. That's the right trade-off, but worth stating in the chart docs.
  • Nit: the per-release hostPath directory stays on nodes after uninstall.

@reckless-sherixx

Copy link
Copy Markdown
Contributor Author

@blue4209211 thanks. Agreed on all points.

  1. Release order. This waits for the node-agent#407 release. I'll add the nodeAgent.image.tag bump to this PR then. It is still 0.1.11 now.
  2. Chart version. feat(chart): alert before the ClickHouse disk fills and stop inserts at 95% #677 is 0.1.31 and this is 0.1.32. Both touch the same Chart.yaml lines, so whichever merges second gets rebased. Merge order alone doesn't avoid that.
  3. Disk full with feat(chart): alert before the ClickHouse disk fills and stop inserts at 95% #677. At under 5% free, logs are held on the nodes (clickhouse/logs retries with max_elapsed_time: 0 from a blocking queue), while traces and metrics are dropped after 300 s. This goes into the operator docs. Whichever PR lands second rewords feat(chart): alert before the ClickHouse disk fills and stop inserts at 95% #677's values comment that says refused inserts are dropped.
  4. Known gaps, confirmed. The queue is in memory, so it is lost if the collector itself restarts during an outage. A file_storage persistent queue is the follow-up. An outage longer than about one log file per container still loses lines.

Evidence:

  • helm template renders LOGS_POSITIONS_FILE and the /var/lib/nudgebee-node-agent hostPath mount only with --set logs.collection=node-agent. With default values, neither appears (0 matches).
  • otelcol validate on otelcol-contrib 0.157.0 passes on the rendered collector config.

@mayankpande88

Copy link
Copy Markdown
Contributor

Chart version: please drop the Chart.yaml change. main is already at 0.1.31, and 0.1.31 hasn't been released. The latest release is 0.1.30, and #677 bumped main to 0.1.31 after it. This change goes out in 0.1.31 with #677. CI doesn't need a bump on every PR (ct lint runs with --check-version-increment=false, and release-rc stamps its own RC version). Bumping per PR is why 0.1.28 and 0.1.29 never shipped, and merging this as-is would skip 0.1.31 as well.

Other points:

  • Still blocked on the node-agent#407 release and the nodeAgent.image.tag bump in this PR. With the current node-agent, block_on_overflow holds requests that are never retried.
  • clickhouse/logs copies its settings as '{{ ... }}' inside single quotes. A password with a ' in it breaks the rendered config. Use {{ ... | quote }}-style quoting, or YAML anchors.
  • The hostPath directory /var/lib/nudgebee-node-agent/<ns>-<release> stays on every node after uninstall. Worth a note in the values comment.

…lickHouse is down

With logs.collection=node-agent, container logs were lost whenever the
node-agent, the collector or ClickHouse restarted. This is the chart half
of at-least-once delivery; the node-agent half resumes from saved
positions and retries until the collector accepts.

- node-agent (logs.collection=node-agent only): LOGS_POSITIONS_FILE on a
  per-release hostPath under /var/lib/nudgebee-node-agent, so a restarted
  node-agent resumes where delivery stopped. Other modes keep the same
  pod spec.
- Collector: the logs pipeline gets its own exporter, clickhouse/logs,
  with the connection settings of `clickhouse` (templated, so overrides
  carry over). It retries without a time limit and batches from a
  100000-record queue that blocks the receiver when full, and the logs
  pipeline drops the batch processor, which accepted records and dropped
  them after 300 s of failed retries. A ClickHouse outage now makes
  node-agents keep unsent lines in the node's log files. Traces and
  metrics keep the shared exporter.

Merge with the node-agent release that reads LOGS_POSITIONS_FILE
(nodeAgent.image.tag); older node-agents ignore it.
@reckless-sherixx

Copy link
Copy Markdown
Contributor Author

Thanks. All four addressed in the amended commit (97926b8):

  • Chart version: dropped; Chart.yaml matches main (0.1.31).
  • Release: agreed. This waits for the node-agent#407 release and its nodeAgent.image.tag bump. Two more log-loss fixes are going into fix: add --no-paginate to nudgebee-agent ECR query #407 first, found by a kind run of the test plan.
  • Quoting: confirmed: a password pa'ss rendered password: 'pa'ss'. | quote doesn't help here, because the collector chart renders its config with tpl (toYaml …), so the reference is already inside single quotes and quote only adds literal double quotes. Anchors would stop overrides of exporters.clickhouse from reaching clickhouse/logs. Added nudgebee-agent.collectorQuoted, which doubles ' inside the quotes. With ' in the password and database, the rendered config now parses to the same values as clickhouse.
  • hostPath: noted in the logs: comment, with how to remove it after uninstall.

@blue4209211
blue4209211 added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit 4428d5b Oct 24, 2026
5 checks passed
@blue4209211
blue4209211 deleted the feat/logs-backpressure branch October 24, 2026 02:16
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.

3 participants