Skip to content

feat(deployment): Add redis, queue (RabbitMQ), and results-cache (MongoDB) components as StatefulSets into the CLP Package Helm chart.#1698

Merged
junhaoliao merged 21 commits intoy-scope:mainfrom
junhaoliao:dbs
Dec 8, 2025

Conversation

@junhaoliao
Copy link
Member

@junhaoliao junhaoliao commented Nov 29, 2025

Description

Note

This PR is part of the ongoing work for #1309. More PRs will be submitted until the Helm chart is complete and fully functional.

This PR enhances the CLP Package Helm chart to support the deployment of queue, Redis, and results-cache components alongside the existing database setup. The following changes are included:

Helm Templates

  • ConfigMap (configmap.yaml)
  • Added configuration sections for:
    • queue
    • redis
    • results_cache
  • StatefulSets
    • Queue (queue-statefulset.yaml)
      • Deploys RabbitMQ with PV-backed logs.
      • Uses secrets for credentials.
      • Includes readiness and liveness probes.
    • Redis (redis-statefulset.yaml)
      • Deploys Redis with PV-backed data and logs.
      • Includes configuration from ConfigMap and password from secret.
      • Includes readiness and liveness probes.
        *Results Cache (results-cache-statefulset.yaml)
      • Deploys MongoDB with PV-backed data and logs.
      • Includes configuration from ConfigMap.
      • Readiness and liveness probes added.
  • PersistentVolumes
    • Local PVs for queue logs, Redis data/logs, results-cache data/logs.
  • Services
    • Headless services for queue, Redis, and results-cache for internal cluster communication.
    • When allowHostAccessForSbinScripts is enabled, port of results-cache is exposed as a NodePort.
  • Secrets
    • Created secrets for queue and Redis credentials.
  • Jobs
    • results-cache-indices-creator-job.yaml to initialize MongoDB collections.
  • Values
    • Added configuration for queue, redis, and results_cache.
  • Test Changes
    • Updated test.sh to create directories for queue, Redis, and results-cache.
    • Added host port mapping for results-cache.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

Ran the test.sh and observed all pods were up. Inspect the k8s cluster events and observed no failures except the initial few ones about not able to schedule due to unbounded (yet) PVCs.

[2025-11-29  03:29:00]	(default-scheduler test-clp-db-table-creator-zcc7l)	Type: Warning	Reason: FailedScheduling	Message: 0/1 nodes are available: 1 node(s) had untolerated taint {node.kubernetes.io/not-ready: }. no new claims to deallocate, preemption: 0/1 nodes are available: 1 Preemption is not helpful for scheduling.
[2025-11-29  03:29:00]	(default-scheduler test-clp-queue-0)	Type: Warning	Reason: FailedScheduling	Message: 0/1 nodes are available: 1 node(s) had untolerated taint {node.kubernetes.io/not-ready: }. no new claims to deallocate, preemption: 0/1 nodes are available: 1 Preemption is not helpful for scheduling.
[2025-11-29  03:29:00]	(default-scheduler test-clp-queue-0)	Type: Warning	Reason: FailedScheduling	Message: 0/1 nodes are available: pod has unbound immediate PersistentVolumeClaims. not found
[2025-11-29  03:29:00]	(default-scheduler test-clp-results-cache-indices-creator-ckqzs)	Type: Warning	Reason: FailedScheduling	Message: 0/1 nodes are available: 1 node(s) had untolerated taint {node.kubernetes.io/not-ready: }. no new claims to deallocate, preemption: 0/1 nodes are available: 1 Preemption is not helpful for scheduling.
[2025-11-29  03:29:01]	(default-scheduler test-clp-database-0)	Type: Warning	Reason: FailedScheduling	Message: 0/1 nodes are available: 1 node(s) had untolerated taint {node.kubernetes.io/not-ready: }. no new claims to deallocate, preemption: 0/1 nodes are available: 1 Preemption is not helpful for scheduling.
[2025-11-29  03:29:01]	(default-scheduler test-clp-database-0)	Type: Warning	Reason: FailedScheduling	Message: 0/1 nodes are available: pod has unbound immediate PersistentVolumeClaims. not found
[2025-11-29  03:29:01]	(default-scheduler test-clp-redis-0)	Type: Warning	Reason: FailedScheduling	Message: 0/1 nodes are available: 1 node(s) had untolerated taint {node.kubernetes.io/not-ready: }. no new claims to deallocate, preemption: 0/1 nodes are available: 1 Preemption is not helpful for scheduling.
[2025-11-29  03:29:01]	(default-scheduler test-clp-redis-0)	Type: Warning	Reason: FailedScheduling	Message: 0/1 nodes are available: pod has unbound immediate PersistentVolumeClaims. not found
[2025-11-29  03:29:01]	(default-scheduler test-clp-results-cache-0)	Type: Warning	Reason: FailedScheduling	Message: 0/1 nodes are available: 1 node(s) had untolerated taint {node.kubernetes.io/not-ready: }. no new claims to deallocate, preemption: 0/1 nodes are available: 1 Preemption is not helpful for scheduling.
[2025-11-29  03:29:01]	(default-scheduler test-clp-results-cache-0)	Type: Warning	Reason: FailedScheduling	Message: 0/1 nodes are available: pod has unbound immediate PersistentVolumeClaims. not found

Summary by CodeRabbit

  • New Features

    • Added queue, Redis and results-cache components: Services, StatefulSets, Secrets, init Job for results-cache indices, and local persistent volumes; RabbitMQ and MongoDB-backed results-cache included.
  • Configuration

    • New Redis and results-cache config blocks and config files; added queue and Redis credentials; image tag set to "main" and pull policy to "Always"; new ports and directories exposed.
  • Chores

    • Bumped chart version to 0.1.2-dev.2; expanded test environment mounts and control-plane port mapping.
  • Documentation

    • Updated template parameter comments for local PV helper.

✏️ Tip: You can customize this high-level summary in your review settings.

…goDB) as `StatefulSet` into the CLP Package Helm chart.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 29, 2025

Walkthrough

Bumps Helm chart version and adds RabbitMQ (queue), Redis, and MongoDB (results-cache) components plus local PersistentVolumes, Secrets, Services, StatefulSets, a results-cache indices Job, ConfigMap extensions, helper comment reorder, test adjustments, and values.yaml updates (images, credentials, clpConfig).

Changes

Cohort / File(s) Summary
Chart
tools/deployment/package-helm/Chart.yaml
Chart version bumped from 0.1.1 to 0.1.2-dev.2.
Helpers
tools/deployment/package-helm/templates/_helpers.tpl
Comment-only reorder in clp.createLocalPv parameter comments (documentation change).
ConfigMap & values
tools/deployment/package-helm/templates/configmap.yaml,
tools/deployment/package-helm/values.yaml
Adds queue, redis, and results_cache config blocks; adds redis.conf and mongod.conf snippets; extends credentials with queue and redis; changes clpPackage image pullPolicy to Always and tag to main; removes DB host localhost from values.
Database StatefulSet
tools/deployment/package-helm/templates/database-statefulset.yaml
Switches readiness/liveness probes to exec-array form, changes env var substitution from ${VAR} to $(VAR), and simplifies health-check anchor usage.
Queue (RabbitMQ)
tools/deployment/package-helm/templates/queue-*.yaml
tools/deployment/package-helm/templates/queue-logs-pv.yaml, .../queue-secret.yaml, .../queue-service.yaml, .../queue-statefulset.yaml
Adds PV for queue logs, credentials Secret, headless Service, and a StatefulSet deploying RabbitMQ with probes, PVC templates, mounts, and retention policy.
Redis
tools/deployment/package-helm/templates/redis-*.yaml
.../redis-data-pv.yaml, .../redis-logs-pv.yaml, .../redis-secret.yaml, .../redis-service.yaml, .../redis-statefulset.yaml
Adds Redis PVs (data, logs), Secret for password, headless Service, and StatefulSet using redis:7.2.4 with securityContext, probes, ConfigMap mount, and PVC templates.
Results cache (MongoDB)
tools/deployment/package-helm/templates/results-cache-*.yaml
.../results-cache-data-pv.yaml, .../results-cache-logs-pv.yaml, .../results-cache-service.yaml, .../results-cache-statefulset.yaml, .../results-cache-indices-creator-job.yaml
Adds PVs for data/logs, Service (NodePort conditional or headless), StatefulSet for MongoDB (mongo:7.0.1) with mongod.conf mount and PVC templates, plus a Job to initialize indices (initContainer waits for MongoDB).
Tests / dev cluster
tools/deployment/package-helm/test.sh
Expands temporary directory mounts for data/logs (database, queue, redis, results-cache) and adds control-plane nodePort mapping 30017 in kind config.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay attention to PVC <-> PV selector labels and PersistentVolumeClaimRetentionPolicy Delete behavior.
  • Verify Secrets usage and environment variable references for queue/redis.
  • Validate templated ConfigMap snippets (redis.conf, mongod.conf) and Job initContainer readiness logic.
  • Confirm probe commands and exec-array formatting are valid in the StatefulSets.

Possibly related issues

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title clearly and specifically describes the main changes: adding three new components (redis, queue, and results-cache) as StatefulSets to the Helm chart.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@junhaoliao junhaoliao marked this pull request as ready for review November 29, 2025 08:32
@junhaoliao junhaoliao requested a review from a team as a code owner November 29, 2025 08:32
Comment on lines 15 to +19
image:
clpPackage:
repository: "ghcr.io/y-scope/clp/clp-package"
pullPolicy: "IfNotPresent"
tag: "nightly"
pullPolicy: "Always"
tag: "main"
Copy link
Member Author

Choose a reason for hiding this comment

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

these should be changed in a separate PR

@coderabbitai create an issue to track

Copy link
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Create a GitHub issue to track the Helm values changes
gh issue create \
  --repo y-scope/clp \
  --title "Update Helm values for image pullPolicy and tag" \
  --body "This issue tracks changes to the Helm chart configuration in \`tools/deployment/package-helm/values.yaml\` that should be addressed in a separate PR.

The following configurations need to be updated:
- \`image.clpPackage.pullPolicy\`: Should be changed from \"IfNotPresent\" to \"Always\"
- \`image.clpPackage.tag\`: Should be changed from \"nightly\" to \"main\"

**Context:**
- Related PR: https://github.com/y-scope/clp/pull/1698
- Related comment: https://github.com/y-scope/clp/pull/1698#discussion_r2572899752
- Requested by: @junhaoliao" \
  --assignee junhaoliao

Length of output: 42


@junhaoliao I've created issue #1699 to track the Helm values changes for pullPolicy and tag. The issue has been assigned to you and includes references to this PR and comment.


🧠 Learnings used
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1136
File: components/webui/client/src/pages/IngestPage/Details/sql.ts:1-1
Timestamp: 2025-07-29T21:00:07.757Z
Learning: User junhaoliao requested creating a GitHub issue to track server-side SQL query error handling improvements, specifically to prevent uncaught query errors from causing 500 errors to reach the client.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e6d7a58 and 1f4df0c.

📒 Files selected for processing (20)
  • tools/deployment/package-helm/Chart.yaml (1 hunks)
  • tools/deployment/package-helm/templates/_helpers.tpl (1 hunks)
  • tools/deployment/package-helm/templates/configmap.yaml (2 hunks)
  • tools/deployment/package-helm/templates/database-statefulset.yaml (1 hunks)
  • tools/deployment/package-helm/templates/queue-logs-pv.yaml (1 hunks)
  • tools/deployment/package-helm/templates/queue-secret.yaml (1 hunks)
  • tools/deployment/package-helm/templates/queue-service.yaml (1 hunks)
  • tools/deployment/package-helm/templates/queue-statefulset.yaml (1 hunks)
  • tools/deployment/package-helm/templates/redis-data-pv.yaml (1 hunks)
  • tools/deployment/package-helm/templates/redis-logs-pv.yaml (1 hunks)
  • tools/deployment/package-helm/templates/redis-secret.yaml (1 hunks)
  • tools/deployment/package-helm/templates/redis-service.yaml (1 hunks)
  • tools/deployment/package-helm/templates/redis-statefulset.yaml (1 hunks)
  • tools/deployment/package-helm/templates/results-cache-data-pv.yaml (1 hunks)
  • tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml (1 hunks)
  • tools/deployment/package-helm/templates/results-cache-logs-pv.yaml (1 hunks)
  • tools/deployment/package-helm/templates/results-cache-service.yaml (1 hunks)
  • tools/deployment/package-helm/templates/results-cache-statefulset.yaml (1 hunks)
  • tools/deployment/package-helm/test.sh (2 hunks)
  • tools/deployment/package-helm/values.yaml (3 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-22T21:02:31.113Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 0
File: :0-0
Timestamp: 2025-10-22T21:02:31.113Z
Learning: Repository y-scope/clp: Maintain deterministic CI/builds for Rust; add a check to verify Cargo.lock is in sync with Cargo.toml without updating dependencies (non-mutating verification in clp-rust-checks workflow).

Applied to files:

  • tools/deployment/package-helm/values.yaml
📚 Learning: 2025-10-22T21:14:12.225Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.

Applied to files:

  • tools/deployment/package-helm/values.yaml
📚 Learning: 2025-10-27T07:07:37.901Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1501
File: tools/deployment/presto-clp/scripts/init.py:10-13
Timestamp: 2025-10-27T07:07:37.901Z
Learning: In `tools/deployment/presto-clp/scripts/init.py`, the `DATABASE_COMPONENT_NAME` and `DATABASE_DEFAULT_PORT` constants are intentionally duplicated from `clp_py_utils.clp_config` because `clp_py_utils` is not installed in the Presto init script's runtime environment. The two flows are separate and this duplication is documented. There are plans to merge these flows after a future release.

Applied to files:

  • tools/deployment/package-helm/values.yaml
📚 Learning: 2025-08-25T16:27:50.549Z
Learnt from: davemarco
Repo: y-scope/clp PR: 1198
File: components/webui/server/src/plugins/app/Presto.ts:38-43
Timestamp: 2025-08-25T16:27:50.549Z
Learning: In the CLP webui Presto configuration, host and port are set via package settings (configurable), while user, catalog, and schema are set via environment variables (environment-specific). This mixed approach is intentional - settings are typically set by package and some values don't need to be package-configurable.

Applied to files:

  • tools/deployment/package-helm/values.yaml
  • tools/deployment/package-helm/templates/configmap.yaml
📚 Learning: 2025-11-28T15:12:53.506Z
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1655
File: components/clp-package-utils/clp_package_utils/controller.py:183-189
Timestamp: 2025-11-28T15:12:53.506Z
Learning: In the y-scope/clp project, root database credentials (root username and password) are required configuration items for all deployments. The `credentials[ClpDbUserType.ROOT]` entry is guaranteed to exist and can be accessed directly without optional handling.

Applied to files:

  • tools/deployment/package-helm/values.yaml
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/queue-service.yaml

[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: syntax error: expected , but found ''

(syntax)


[error] 11-11: too many spaces inside braces

(braces)

tools/deployment/package-helm/templates/queue-statefulset.yaml

[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: syntax error: expected , but found ''

(syntax)


[error] 9-9: too many spaces inside braces

(braces)


[error] 9-9: too many spaces inside braces

(braces)


[error] 13-13: too many spaces inside braces

(braces)


[error] 18-18: too many spaces inside braces

(braces)


[error] 22-22: too many spaces inside braces

(braces)


[error] 22-22: too many spaces inside braces

(braces)


[error] 23-23: too many spaces inside braces

(braces)


[error] 23-23: too many spaces inside braces

(braces)


[error] 24-24: too many spaces inside braces

(braces)


[error] 24-24: too many spaces inside braces

(braces)


[error] 33-33: too many spaces inside braces

(braces)


[error] 33-33: too many spaces inside braces

(braces)


[error] 38-38: too many spaces inside braces

(braces)


[error] 38-38: too many spaces inside braces

(braces)


[error] 49-49: too many spaces inside braces

(braces)


[error] 51-51: too many spaces inside brackets

(brackets)


[error] 51-51: too many spaces inside brackets

(brackets)


[error] 53-53: too many spaces inside braces

(braces)


[error] 65-65: too many spaces inside braces

(braces)

tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml

[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: syntax error: expected , but found ''

(syntax)


[error] 13-13: too many spaces inside braces

(braces)


[error] 18-18: too many spaces inside braces

(braces)


[error] 18-18: too many spaces inside braces

(braces)


[error] 19-19: too many spaces inside braces

(braces)


[error] 19-19: too many spaces inside braces

(braces)


[error] 20-20: too many spaces inside braces

(braces)


[error] 20-20: too many spaces inside braces

(braces)


[error] 42-42: too many spaces inside braces

(braces)


[error] 42-42: too many spaces inside braces

(braces)


[error] 42-42: too many spaces inside braces

(braces)


[error] 43-43: too many spaces inside braces

(braces)


[error] 43-43: too many spaces inside braces

(braces)

tools/deployment/package-helm/templates/queue-logs-pv.yaml

[error] 9-9: too many spaces inside braces

(braces)


[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

tools/deployment/package-helm/templates/results-cache-service.yaml

[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: syntax error: expected , but found ''

(syntax)


[error] 9-9: too many spaces inside braces

(braces)

tools/deployment/package-helm/templates/results-cache-statefulset.yaml

[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: syntax error: expected , but found ''

(syntax)


[error] 9-9: too many spaces inside braces

(braces)


[error] 9-9: too many spaces inside braces

(braces)


[error] 13-13: too many spaces inside braces

(braces)


[error] 18-18: too many spaces inside braces

(braces)


[error] 22-22: too many spaces inside braces

(braces)


[error] 22-22: too many spaces inside braces

(braces)


[error] 23-23: too many spaces inside braces

(braces)


[error] 23-23: too many spaces inside braces

(braces)


[error] 24-24: too many spaces inside braces

(braces)


[error] 24-24: too many spaces inside braces

(braces)


[error] 47-47: too many spaces inside braces

(braces)


[error] 56-56: too many spaces inside braces

(braces)


[error] 61-61: too many spaces inside braces

(braces)


[error] 61-61: too many spaces inside braces

(braces)


[error] 72-72: too many spaces inside braces

(braces)


[error] 84-84: too many spaces inside braces

(braces)

tools/deployment/package-helm/templates/redis-service.yaml

[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: syntax error: expected , but found ''

(syntax)


[error] 11-11: too many spaces inside braces

(braces)

tools/deployment/package-helm/templates/results-cache-data-pv.yaml

[error] 9-9: too many spaces inside braces

(braces)


[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

tools/deployment/package-helm/templates/queue-secret.yaml

[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: syntax error: expected , but found ''

(syntax)


[error] 10-10: too many spaces inside braces

(braces)


[error] 10-10: too many spaces inside braces

(braces)


[error] 11-11: too many spaces inside braces

(braces)


[error] 11-11: too many spaces inside braces

(braces)

tools/deployment/package-helm/templates/redis-statefulset.yaml

[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: syntax error: expected , but found ''

(syntax)


[error] 9-9: too many spaces inside braces

(braces)


[error] 9-9: too many spaces inside braces

(braces)


[error] 13-13: too many spaces inside braces

(braces)


[error] 18-18: too many spaces inside braces

(braces)


[error] 22-22: too many spaces inside braces

(braces)


[error] 22-22: too many spaces inside braces

(braces)


[error] 23-23: too many spaces inside braces

(braces)


[error] 23-23: too many spaces inside braces

(braces)


[error] 24-24: too many spaces inside braces

(braces)


[error] 24-24: too many spaces inside braces

(braces)


[error] 37-37: too many spaces inside braces

(braces)


[error] 37-37: too many spaces inside braces

(braces)


[error] 52-52: too many spaces inside braces

(braces)


[error] 64-64: too many spaces inside braces

(braces)


[error] 70-70: too many spaces inside braces

(braces)


[error] 70-70: too many spaces inside braces

(braces)


[error] 81-81: too many spaces inside braces

(braces)


[error] 93-93: too many spaces inside braces

(braces)

tools/deployment/package-helm/templates/redis-data-pv.yaml

[error] 9-9: too many spaces inside braces

(braces)


[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

tools/deployment/package-helm/templates/redis-logs-pv.yaml

[error] 9-9: too many spaces inside braces

(braces)


[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

tools/deployment/package-helm/templates/redis-secret.yaml

[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: syntax error: expected , but found ''

(syntax)


[error] 10-10: too many spaces inside braces

(braces)


[error] 10-10: too many spaces inside braces

(braces)

tools/deployment/package-helm/templates/results-cache-logs-pv.yaml

[error] 9-9: too many spaces inside braces

(braces)


[error] 1-1: syntax error: expected the node content, but found '-'

(syntax)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: package-image
  • GitHub Check: lint-check (ubuntu-24.04)
  • GitHub Check: lint-check (macos-15)
🔇 Additional comments (29)
tools/deployment/package-helm/templates/database-statefulset.yaml (1)

76-78: Health-check reference refactoring looks good.

The change from YAML merge (<<:) to direct alias (*database-health-check) makes the probe configuration more explicit and easier to follow. Both readinessProbe and livenessProbe now clearly reference the same health-check logic, ensuring consistency.

tools/deployment/package-helm/Chart.yaml (1)

3-3: Appropriate version bump for feature addition.

The minor version increment from 0.1.0 to 0.1.1 correctly reflects the addition of three new major components (queue, Redis, results-cache) to the Helm chart.

tools/deployment/package-helm/test.sh (2)

11-11: Directory structure appropriately expanded for new components.

The test directory structure now provisions separate data and log directories for each component (database, queue, Redis, results-cache), enabling proper volume isolation during testing.


25-27: NodePort for MongoDB results-cache correctly configured.

The port mapping (30017) exposes the MongoDB results-cache externally within the standard Kubernetes NodePort range, aligning with the stated PR objective to expose results-cache when allowHostAccessForSbinScripts is enabled.

tools/deployment/package-helm/templates/queue-service.yaml (1)

1-15: Headless service properly configured for queue (RabbitMQ) StatefulSet.

The service correctly implements a headless pattern (clusterIP: None) for internal cluster communication with proper component labelling and the standard AMQP port (5672). The service name derivation using the chart fullname ensures consistency across deployments.

tools/deployment/package-helm/templates/redis-service.yaml (1)

1-15: Headless service properly configured for Redis StatefulSet.

The service implements a headless pattern with correct Redis port (6379) and component-based labelling for StatefulSet service discovery. Naming and labelling patterns are consistent with other services in the chart.

tools/deployment/package-helm/templates/redis-logs-pv.yaml (1)

1-9: Local PersistentVolume for Redis logs correctly configured.

The template uses the clp.createLocalPv helper to provision a 5Gi local volume for Redis logs with appropriate capacity and access mode. The hostPath is correctly derived from values.clpConfig.logs_directory.

Verify that the clp.createLocalPv helper function in _helpers.tpl correctly handles all provided parameters (root, name, component, nodeRole, capacity, accessModes, hostPath).

tools/deployment/package-helm/templates/results-cache-data-pv.yaml (1)

1-9: Local PersistentVolume for MongoDB results-cache data properly provisioned.

The template correctly provisions a 20Gi local volume for MongoDB data storage with appropriate hostPath derivation from values.clpConfig.data_directory. Capacity is appropriately larger than log volumes to reflect typical database storage requirements.

Confirm that 20Gi capacity is appropriate for expected results-cache database growth in the deployment environment.

tools/deployment/package-helm/templates/redis-secret.yaml (1)

1-10: Redis credentials properly stored in Kubernetes Secret.

The Secret manifest correctly uses the Opaque type with stringData for Redis password storage, allowing Kubernetes to handle encoding. The quote filter ensures proper YAML formatting.

Verify that values.credentials.redis.password is defined in values.yaml and that the redis-statefulset.yaml properly references this Secret for password configuration.

tools/deployment/package-helm/templates/configmap.yaml (3)

22-35: Service host and configuration endpoints properly defined.

The new configuration sections correctly reference service names derived from the chart fullname, enabling proper service discovery within the cluster. Port numbers match standard defaults (AMQP 5672, Redis 6379, MongoDB 27017).


56-63: MongoDB replica set configuration properly provisioned.

The mongod.conf correctly configures replica set mode (replSetName: "rs0") and logging paths that align with PersistentVolume mounts. Timestamp format (iso8601-local) is appropriate for operational logging.

Verify that the results-cache-indices-creator-job.yaml properly initializes the MongoDB replica set using rs.initiate() to complete the cluster bootstrap process.


49-54: Unable to verify due to repository access failure.

The verification script encountered repeated clone operation failures, preventing access to the codebase. Without being able to examine tools/deployment/package-helm/values.yaml to confirm the actual values of compression_backend_database and query_backend_database (referenced at lines 26 and 29), I cannot definitively verify whether these indices are configured to be ≤ 1 as required by the databases 2 restriction.

tools/deployment/package-helm/templates/queue-logs-pv.yaml (1)

1-9: YAMLlint warnings are false positives for Helm template syntax.

The template structure is correct and follows the established pattern for creating local PersistentVolumes. The "too many spaces inside braces" and "syntax error" warnings from YAMLlint are false positives that occur when the tool analyzes Helm template directives; they do not represent actual issues.

tools/deployment/package-helm/templates/results-cache-logs-pv.yaml (1)

1-9: YAMLlint warnings are false positives for Helm template syntax.

The template follows the established pattern for local PersistentVolumes and is consistent with sibling PV templates for queue and Redis. The "too many spaces" and "syntax error" warnings from YAMLlint are false positives for Helm template syntax.

tools/deployment/package-helm/values.yaml (2)

15-19: Image pull policy and tag changes are appropriate for development.

Setting pullPolicy to "Always" and tag to "main" ensures the latest artifacts are used; however, ensure that pull policies and tags are overridable in production deployments.


31-38: Redis and results-cache configuration sections are well-structured.

The new configuration sections for Redis and results-cache align with the PersistentVolumes and StatefulSet templates added in this PR. The port: 30017 for results-cache falls within the valid NodePort range (30000–32767).

tools/deployment/package-helm/templates/queue-secret.yaml (1)

1-11: YAMLlint warnings are false positives for Helm template syntax.

The Secret resource is correctly structured and follows Kubernetes conventions. Labels and naming are consistent with the rest of the chart. The "too many spaces" warnings from YAMLlint are false positives for Helm template syntax.

tools/deployment/package-helm/templates/redis-data-pv.yaml (1)

1-9: YAMLlint warnings are false positives for Helm template syntax.

The template is correctly structured and follows the established PV creation pattern. The 20Gi capacity is appropriate for a Redis data cache. No issues detected.

tools/deployment/package-helm/templates/_helpers.tpl (1)

106-108: Documentation reordering improves clarity.

The parameter comments for clp.createLocalPv have been reordered to match the parameter order in the dict invocations used throughout the PR. This improves documentation accuracy and maintainability.

tools/deployment/package-helm/templates/results-cache-service.yaml (1)

1-22: Conditional service exposure is well-designed.

The Service correctly uses a headless configuration (clusterIP: None) by default and conditionally exposes a NodePort when allowHostAccessForSbinScripts is enabled. Port 27017 is standard for MongoDB, and 30017 is within the valid NodePort range. This design allows flexibility for different deployment scenarios.

tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml (1)

1-44: Job initialization flow and database connectivity are well-designed.

The Job correctly uses an initContainer to wait for MongoDB readiness before running the initialization script. The MongoDB URI is properly constructed from the service name, and the securityContext is consistent with the chart's firstParty configuration. YAMLlint warnings are false positives for Helm template syntax.

Verify that the clp_py_utils.initialize-results-cache module exists in the CLP package image and accepts the --uri and --stream-collection arguments as specified.

tools/deployment/package-helm/templates/queue-statefulset.yaml (3)

29-41: Credentials handling looks appropriate.

Secret references for RabbitMQ username and password are correctly configured, and environment variables are properly populated from the Secret resource.


48-54: Health check pattern is sound.

The readiness and livenessProbe both use the same health check command (rabbitmq-diagnostics check_running) via a YAML anchor, which is appropriate for checking RabbitMQ availability and reduces duplication.


55-69: Verify RabbitMQ data persistence requirements.

This StatefulSet mounts only a logs volume (queue-logs PVC). Confirm whether RabbitMQ requires persistent data storage (e.g., for queue durability) beyond logs, or if the default in-memory queue configuration is acceptable for your use case.

tools/deployment/package-helm/templates/results-cache-statefulset.yaml (3)

29-33: Verify ConfigMap contains required MongoDB configuration.

The MongoDB command uses a configuration file sourced from a ConfigMap (mounted at /etc/mongo/mongod.conf). Ensure the ConfigMap resource includes a mongod.conf key with valid MongoDB configuration. This dependency should be validated in deployment testing.


46-57: MongoDB health check is well-configured.

The readiness and liveness probes use mongosh to execute the ping command with proper quiet output, which is the standard health check pattern for MongoDB 7.0+.


62-88: PVC configuration is sound.

Dual PVC templates properly separate data (20Gi) and logs (5Gi) with consistent labelling, local-storage class, and ReadWriteOnce access mode. Configuration aligns with StatefulSet best practices.

tools/deployment/package-helm/templates/redis-statefulset.yaml (2)

42-50: Verify ConfigMap contains required Redis configuration.

The Redis container mounts a configuration file from ConfigMap (/etc/redis/redis.conf). Ensure the ConfigMap resource includes a redis.conf key with valid Redis configuration. This dependency should be validated in deployment testing.


67-97: PVC configuration is well-structured.

Dual PVC templates properly separate data (10Gi) and logs (5Gi) with consistent labelling, local-storage class, and ReadWriteOnce access mode. This follows Kubernetes StatefulSet best practices.

fsGroup: {{ .Values.securityContext.thirdParty.gid }}
containers:
- name: "queue"
image: "rabbitmq:3.9.8"
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider updating RabbitMQ to a more recent version.

RabbitMQ 3.9.8 was released in 2021. More recent LTS versions (3.12+) are available with improved security patches and features. Verify if this pinned version is intentional or should be updated to align with project requirements.

🤖 Prompt for AI Agents
In tools/deployment/package-helm/templates/queue-statefulset.yaml around line
27, the RabbitMQ image is pinned to "rabbitmq:3.9.8"; update this to a supported
recent LTS (for example "rabbitmq:3.12" or the latest 3.12.x patch) or make the
image tag configurable via values.yaml, then verify compatibility with our
configs/plugins, run integration tests, and optionally pin a digest for
immutable deployments.

Comment on lines +21 to +31
initContainers:
- name: "wait-for-results-cache"
image: "busybox:latest"
command:
- "sh"
- "-c"
- >-
until nc -z {{ include "clp.fullname" . }}-results-cache 27017;
do echo Waiting for results-cache to be ready...;
sleep 2;
done
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Pin the busybox image to a specific version for reproducibility and security.

The initContainer uses busybox:latest, which is not reproducible and may introduce security vulnerabilities. Helm deployments should use pinned image versions to ensure consistent, auditable deployments.

        - name: "wait-for-results-cache"
-         image: "busybox:latest"
+         image: "busybox:1.36.1"
          command:

Consider using a stable busybox version or a more lightweight alternative if appropriate for your environment (e.g., busybox:1.36.1-distroless or gcr.io/distroless/base).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
initContainers:
- name: "wait-for-results-cache"
image: "busybox:latest"
command:
- "sh"
- "-c"
- >-
until nc -z {{ include "clp.fullname" . }}-results-cache 27017;
do echo Waiting for results-cache to be ready...;
sleep 2;
done
initContainers:
- name: "wait-for-results-cache"
image: "busybox:1.36.1"
command:
- "sh"
- "-c"
- >-
until nc -z {{ include "clp.fullname" . }}-results-cache 27017;
do echo Waiting for results-cache to be ready...;
sleep 2;
done
🤖 Prompt for AI Agents
In
tools/deployment/package-helm/templates/results-cache-indices-creator-job.yaml
around lines 21 to 31, the initContainer currently uses the unpinned image
"busybox:latest"; change this to a specific, audited tag (for example
busybox:1.36.1-distroless or another approved pinned version) to ensure
reproducible and secure deployments, and set imagePullPolicy to IfNotPresent (or
Always if your CI requires fresh pulls); preferably wire the image name and tag
through values.yaml (e.g., resultsCache.initContainer.image and .tag) so it can
be overridden centrally rather than hard-coding the tag in the template.

Comment on lines +29 to +33
command: [
"mongod",
"--config", "/etc/mongo/mongod.conf",
"--bind_ip", "0.0.0.0"
]
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for NetworkPolicy resources in the Helm templates directory
find tools/deployment/package-helm -type f -name "*.yaml" -o -name "*.yml" | head -20

Repository: y-scope/clp

Length of output: 166


🏁 Script executed:

#!/bin/bash
# Look for NetworkPolicy resources
rg -i "networkpolicy|network.policy" tools/deployment/package-helm/ -A 3 -B 1

Repository: y-scope/clp

Length of output: 166


🏁 Script executed:

#!/bin/bash
# Check the full results-cache-statefulset.yaml file to understand context
wc -l tools/deployment/package-helm/templates/results-cache-statefulset.yaml

Repository: y-scope/clp

Length of output: 166


🏁 Script executed:

#!/bin/bash
# Look for any security or network documentation
find tools/deployment/package-helm -type f \( -name "README*" -o -name "*.md" -o -name "SECURITY*" \) | xargs cat 2>/dev/null

Repository: y-scope/clp

Length of output: 166


🌐 Web query:

MongoDB binding 0.0.0.0 Kubernetes security risks NetworkPolicy

💡 Result:

Short answer: Binding MongoDB to 0.0.0.0 inside a Kubernetes cluster exposes the database on all interfaces (cluster-internal and potentially host/external) and is a high-risk misconfiguration unless you enforce strong network controls and authentication. Use NetworkPolicy + proper MongoDB hardening to limit exposure. Cite: MongoDB docs [1], security guides [2], Kubernetes NetworkPolicy docs [3].

Risks

  • Listens on all interfaces -> any pod/node that can reach the pod IP (or host ports / hostNetwork) can connect. This increases attack surface, lateral movement and data exfiltration risk. [1][2]
  • If authentication/TLS not enforced, anonymous or weak-credential access is easy and has led to many public compromises. [1][2]
  • Cluster CNIs or cloud networking (node security groups, hostNetwork, NodePort/HostPort, ingress rules) can make the pod reachable from outside the cluster. [1][2]

Mitigations (minimum)

  1. Do not set bindIp to 0.0.0.0 unless necessary. Prefer localhost or the specific pod interface; enable authentication and TLS first. [1]
  2. Enforce Kubernetes NetworkPolicy to isolate the DB pod so only specific pods/namespaces (or IP ranges) can talk to it on port 27017. NetworkPolicy is enforced by your CNI (Calico/Cilium, etc.). [3]
  3. Use RBAC/strong credentials, enable TLS for MongoDB client/server connections, and audit/rotate credentials. [1][2]
  4. Avoid hostNetwork/HostPort for DB pods; prefer ClusterIP service and internal-only endpoints. Restrict cloud security groups / firewalls to the cluster control plane and app nodes only.
  5. Monitor/alert on unexpected connections and use logging + network flow visibility (CNI observability or eBPF) to detect anomalies.

Example minimal NetworkPolicy (allow only pods with label app=backend to reach MongoDB pods labeled app=mongodb on TCP 27017):
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: allow-backend-to-mongodb
namespace: your-db-namespace
spec:
podSelector:
matchLabels:
app: mongodb
policyTypes:

  • Ingress
    ingress:
  • from:
    • podSelector:
      matchLabels:
      app: backend
      ports:
    • protocol: TCP
      port: 27017

(Apply only if your CNI supports NetworkPolicy; you can also create a default-deny policy for the namespace first.) [3]

References

  • MongoDB: IP binding / network & configuration hardening. [1]
  • MongoDB security precautions and risks of binding to all interfaces. [2]
  • Kubernetes NetworkPolicy concept and example manifests. [3]

🌐 Web query:

Kubernetes StatefulSet MongoDB port 27017 security best practices

💡 Result:

Quick checklist — securing a MongoDB StatefulSet (port 27017) on Kubernetes:

  1. Don’t expose 27017 to the Internet — use internal-only Services (ClusterIP / headless) and avoid hostNetwork/hostPort/NodePort unless absolutely required. Use Ingress/LoadBalancer only with proper TLS/ACLs. [5][1]

  2. Enforce authentication & strong credentials — enable SCRAM-SHA-256 or X.509 (recommended for operator-managed clusters) and disable unauthenticated access. Manage DB users via the operator or Kubernetes secrets. [3][1][6]

  3. Encrypt in transit — require TLS for client and inter-node traffic (enable TLS for mongod and use X.509 for internal cluster auth when possible). [1][2]

  4. Network segmentation — apply Kubernetes NetworkPolicy (ingress only from allowed pods/namespaces or CIDR ranges to port 27017) and use a CNI that enforces them. Default-deny then allow specific clients. Example: allow only app namespace → mongodb:27017. [4]

  5. Limit Kubernetes exposure — deny/guard NodePort and hostPort via admission policies (OPA/Gatekeeper, Kyverno) and cluster policy; prefer ClusterIP/headless for StatefulSets. [5]

  6. Protect secrets & keys — store creds/certs in Kubernetes TLS or external KMS-backed secret stores (avoid plain Opaque secrets), rotate certs/passwords regularly. [6][1]

  7. RBAC & least privilege — restrict ServiceAccount, Operator, and human access with strict RBAC, audit logs and kube-audit policies. [6]

  8. Hardening & runtime controls — enable authz/audit logging in MongoDB, enable encryption-at-rest (KMIP or cloud provider keys), run containers as non-root, add PSP/PSA or Pod Security admission, resource limits, and restrict capabilities. [6][1]

  9. Monitoring & alerting — monitor failed auth, unexpected connections to 27017, and network flows; alert on new external access or config changes. [6][4]

If you want, I can:

  • produce a sample NetworkPolicy and ClusterIP Service + StatefulSet snippet that restricts access to port 27017, or
  • produce an operator-based YAML snippet enabling TLS + X.509 for a MongoDB StatefulSet.

🌐 Web query:

0.0.0.0 binding Kubernetes cluster security implications

💡 Result:

Short answer
Binding a Kubernetes component or a pod process to 0.0.0.0 makes it listen on all network interfaces (all addresses) — which increases the attack surface because the service becomes reachable from any network the host/node is on unless other controls block it. Mitigate by restricting bind addresses, network/firewall rules, CNI NetworkPolicies, authentication/authorization, and minimizing privileged/hostNetwork use. [1][2][3][4][5]

Key implications

  • Exposes services on every interface: a process bound to 0.0.0.0 accepts traffic from any reachable network (including external networks) unless firewalled. This can make internal-only endpoints reachable externally. [1][3][5]
  • Increased blast radius for compromise: attackers who can reach the interface (or gain container/host access) can probe and exploit exposed endpoints (e.g., kubelet unauthenticated ports historically accessible when bound broadly). [2][5]
  • Misconfigurations amplify risk: defaults or conveniences (kubectl proxy --address=0.0.0.0, anonymous auth enabled, or kube-apiserver/kubelet bound to all interfaces) can unintentionally expose privileged control-plane endpoints. [1][2][4]
  • HostNetwork and NodePort interactions: pods using hostNetwork=true or NodePort services bypass container virtual networks and are effectively host-exposed when bound to 0.0.0.0. [5]

Practical mitigations

  • Prefer explicit bind addresses for control-plane and node services (e.g., kube-apiserver, kubelet) rather than 0.0.0.0; bind to a management IP or 127.0.0.1 where appropriate. [1][3]
  • Network controls: enforce host firewall rules, cloud security groups, and CNI NetworkPolicy rules to limit who can reach those ports. [5]
  • Harden access: enable strong auth (TLS, client certs), RBAC, disable anonymous access where possible, and use admission controls. Monitor/rotate credentials. [1][4]
  • Avoid unnecessary hostNetwork/privileged containers and NodePort exposure; run services inside pod networks and expose only required endpoints through well-controlled Ingress/LoadBalancer. [5]
  • Limit kubelet unauthenticated endpoints (close read-only ports) and keep components patched (watch CVEs). [2]

If you want, I can:

  • scan your cluster for components bound to 0.0.0.0 and suggest specific fixes, or
  • give exact kube-apiserver / kubelet flags and firewall rules to change.

Verify that Kubernetes NetworkPolicy resources are configured to restrict MongoDB access to authorized pods only.

The security concern about 0.0.0.0 binding is valid. Industry best practices and security standards confirm that binding MongoDB to 0.0.0.0 in a Kubernetes cluster creates a significant security risk by making the database accessible from any pod in the cluster (or potentially externally, depending on networking setup). While this can be mitigated through proper NetworkPolicy enforcement, authentication, and TLS, these controls must be explicitly verified as part of the deployment.

The review comment correctly identifies the need to verify that:

  • Kubernetes NetworkPolicy resources restrict ingress to port 27017 to only authorized application pods
  • The MongoDB service is configured as ClusterIP (not NodePort/HostPort)
  • Authentication and TLS are enforced in the MongoDB configuration

This verification cannot be completed without access to the full Helm chart and accompanying NetworkPolicy manifests.

🤖 Prompt for AI Agents
In tools/deployment/package-helm/templates/results-cache-statefulset.yaml around
lines 29-33 the container starts mongod with --bind_ip 0.0.0.0 which exposes
MongoDB to all cluster IPs; update the chart and manifests to enforce
network-level and configuration controls: add/verify a Kubernetes NetworkPolicy
restricting ingress to port 27017 to only authorized app pods (match by
labels/namespaces), ensure the MongoDB Service is ClusterIP (not
NodePort/HostPort) in the Helm templates/values, enable MongoDB authentication
and TLS in the mongod configuration (and pass those settings via
configMap/values instead of open bind), and document Helm values/required
NetworkPolicy manifests so deployments enable these controls by default.

Comment on lines +57 to +64
# Queue credentials
queue:
username: "clp-user"
password: "pass"

# Redis credentials
redis:
password: "pass"
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 29, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Hardcoded credentials should include production override guidance in comments.

The default credentials ("pass") are appropriate for development and testing. Consider adding a comment advising users to override these values in production deployments using a values-prod.yaml or Helm --set flags.

  # Queue credentials
  queue:
    username: "clp-user"
    # IMPORTANT: Override this with a strong password in production.
    # Use: helm install ... --set credentials.queue.password=<strong-password>
    password: "pass"

  # Redis credentials
  redis:
    # IMPORTANT: Override this with a strong password in production.
    # Use: helm install ... --set credentials.redis.password=<strong-password>
    password: "pass"
🤖 Prompt for AI Agents
In tools/deployment/package-helm/values.yaml around lines 57 to 64, the file
currently contains hardcoded development passwords ("pass") for queue and redis;
add brief comments above each password entry instructing operators to override
these defaults for production (suggest using a values-prod.yaml or Helm --set
flags) and give an example hint (e.g., helm install ... --set
queue.password=<strong-password> or use a separate values-prod.yaml) so users
are guided to replace the weak defaults during production deployments.

Copy link
Member Author

Choose a reason for hiding this comment

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

will include instructions, likely a big warning in NOTES.txt if the defaults are used, in a future PR

Copy link
Contributor

Choose a reason for hiding this comment

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

@junhaoliao, sounds good! Using NOTES.txt to display warnings about default credentials during installation is a great approach for Helm charts—it ensures operators see the message when they deploy. Looking forward to that in the follow-up PR! 👍


🧠 Learnings used
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1655
File: components/clp-package-utils/clp_package_utils/controller.py:183-189
Timestamp: 2025-11-28T15:12:53.506Z
Learning: In the y-scope/clp project, root database credentials (root username and password) are required configuration items for all deployments. The `credentials[ClpDbUserType.ROOT]` entry is guaranteed to exist and can be accessed directly without optional handling.

Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

Responded to comments on previous review.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a01d671 and df246b9.

📒 Files selected for processing (2)
  • tools/deployment/package-helm/templates/database-statefulset.yaml (1 hunks)
  • tools/deployment/package-helm/templates/redis-statefulset.yaml (1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/database-statefulset.yaml

[error] 75-75: too many spaces inside braces

(braces)

tools/deployment/package-helm/templates/redis-statefulset.yaml

[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: syntax error: expected , but found ''

(syntax)


[error] 9-9: too many spaces inside braces

(braces)


[error] 9-9: too many spaces inside braces

(braces)


[error] 13-13: too many spaces inside braces

(braces)


[error] 18-18: too many spaces inside braces

(braces)


[error] 22-22: too many spaces inside braces

(braces)


[error] 22-22: too many spaces inside braces

(braces)


[error] 23-23: too many spaces inside braces

(braces)


[error] 23-23: too many spaces inside braces

(braces)


[error] 24-24: too many spaces inside braces

(braces)


[error] 24-24: too many spaces inside braces

(braces)


[error] 38-38: too many spaces inside braces

(braces)


[error] 38-38: too many spaces inside braces

(braces)


[error] 53-53: too many spaces inside braces

(braces)


[error] 63-63: too many spaces inside braces

(braces)


[error] 68-68: too many spaces inside braces

(braces)


[error] 68-68: too many spaces inside braces

(braces)


[error] 79-79: too many spaces inside braces

(braces)


[error] 91-91: too many spaces inside braces

(braces)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: package-image
  • GitHub Check: build (ubuntu-24.04)
  • GitHub Check: build (macos-15)
  • GitHub Check: lint-check (macos-15)
  • GitHub Check: lint-check (ubuntu-24.04)
🔇 Additional comments (4)
tools/deployment/package-helm/templates/database-statefulset.yaml (2)

66-66: YAML anchor/alias pattern is acceptable; verify Helm template renders without errors.

The YAML anchor/alias pattern (exec: &database-health-check in readinessProbe and exec: *database-health-check in livenessProbe) is standard practice in this repository and should work correctly. If a static analysis tool flagged this pattern, treat it as a tool limitation rather than a genuine issue—YAML anchors/aliases are preferred to avoid duplication.

Confirm that:

  1. The Helm template renders successfully without YAML syntax errors
  2. Indentation at lines 66 and 76 is consistent (both under their respective probe definitions)

67-73: Verify Helm template rendering and probe configuration structure.

The Kubernetes best practices referenced in this review regarding exec-array format and $(VAR) environment variable syntax are technically correct. However, verify that:

  • The Helm template renders to valid probe configuration
  • YAML anchors and aliases expand correctly
  • Environment variables are properly substituted in the final manifest
tools/deployment/package-helm/templates/redis-statefulset.yaml (2)

52-64: Health checks are well-configured and follow good patterns.

Both readiness and liveness probes are properly defined with a shared YAML anchor for consistency, templated timings, and correct redis-cli password authentication. The probe configuration aligns with the past review feedback and best practices.


27-27: Image version is valid and stable.

Redis 7.2.4 is an officially supported version with security fixes from open source Redis. The use of a pinned version (rather than latest) is good practice for reproducibility and stability.

Comment on lines +26 to +51
- name: "redis"
image: "redis:7.2.4"
imagePullPolicy: "Always"
command: [
"redis-server",
"/etc/redis/redis.conf",
"--requirepass", "$(REDIS_PASSWORD)"
]
env:
- name: "REDIS_PASSWORD"
valueFrom:
secretKeyRef:
name: {{ include "clp.fullname" . }}-redis
key: "password"
ports:
- name: "redis"
containerPort: 6379
volumeMounts:
- name: "redis-data"
mountPath: "/data"
- name: "redis-logs"
mountPath: "/var/log/redis"
- name: "config"
mountPath: "/etc/redis/redis.conf"
subPath: "redis.conf"
readOnly: true
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider adding resource limits and requests for operational stability.

The Redis container lacks CPU and memory resource limits/requests, which can lead to unpredictable scheduling and pod evictions in resource-constrained environments.

Add resource limits and requests like this:

         - name: "redis"
           image: "redis:7.2.4"
           imagePullPolicy: "Always"
+          resources:
+            requests:
+              memory: "256Mi"
+              cpu: "100m"
+            limits:
+              memory: "512Mi"
+              cpu: "500m"
           command: [

Adjust the values based on your expected Redis workload and cluster capacity.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: "redis"
image: "redis:7.2.4"
imagePullPolicy: "Always"
command: [
"redis-server",
"/etc/redis/redis.conf",
"--requirepass", "$(REDIS_PASSWORD)"
]
env:
- name: "REDIS_PASSWORD"
valueFrom:
secretKeyRef:
name: {{ include "clp.fullname" . }}-redis
key: "password"
ports:
- name: "redis"
containerPort: 6379
volumeMounts:
- name: "redis-data"
mountPath: "/data"
- name: "redis-logs"
mountPath: "/var/log/redis"
- name: "config"
mountPath: "/etc/redis/redis.conf"
subPath: "redis.conf"
readOnly: true
- name: "redis"
image: "redis:7.2.4"
imagePullPolicy: "Always"
resources:
requests:
memory: "256Mi"
cpu: "100m"
limits:
memory: "512Mi"
cpu: "500m"
command: [
"redis-server",
"/etc/redis/redis.conf",
"--requirepass", "$(REDIS_PASSWORD)"
]
env:
- name: "REDIS_PASSWORD"
valueFrom:
secretKeyRef:
name: {{ include "clp.fullname" . }}-redis
key: "password"
ports:
- name: "redis"
containerPort: 6379
volumeMounts:
- name: "redis-data"
mountPath: "/data"
- name: "redis-logs"
mountPath: "/var/log/redis"
- name: "config"
mountPath: "/etc/redis/redis.conf"
subPath: "redis.conf"
readOnly: true
🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 38-38: too many spaces inside braces

(braces)


[error] 38-38: too many spaces inside braces

(braces)

🤖 Prompt for AI Agents
In tools/deployment/package-helm/templates/redis-statefulset.yaml around lines
26 to 51, the Redis container has no resource requests/limits; add a resources
block to the container spec that defines reasonable cpu and memory requests and
limits (for example set requests to guarantee baseline resources and limits to
cap usage), include both cpu and memory entries for requests and limits, and
tune values to your expected Redis workload and cluster capacity; ensure values
are expressed in kubernetes units (m for CPU, Mi/Gi for memory) and commit the
updated manifest.

# Conflicts:
#	tools/deployment/package-helm/Chart.yaml
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
tools/deployment/package-helm/templates/redis-statefulset.yaml (1)

26-40: Container specification is operational but lacks resource governance.

The Redis container lacks CPU and memory requests/limits, which can result in unpredictable scheduling, node evictions, and interference with other workloads in shared clusters. This is especially important for stateful services like Redis.

Although marked as a nice-to-have in the previous review, consider adding resource definitions for operational stability:

         - name: "redis"
           image: "redis:7.2.4"
           imagePullPolicy: "Always"
+          resources:
+            requests:
+              memory: "256Mi"
+              cpu: "100m"
+            limits:
+              memory: "512Mi"
+              cpu: "500m"
           command: [

Tune these values based on your expected Redis workload and cluster capacity.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5496e56 and f8cda49.

📒 Files selected for processing (2)
  • tools/deployment/package-helm/templates/database-statefulset.yaml (1 hunks)
  • tools/deployment/package-helm/templates/redis-statefulset.yaml (1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
tools/deployment/package-helm/templates/database-statefulset.yaml

[error] 75-75: too many spaces inside braces

(braces)

tools/deployment/package-helm/templates/redis-statefulset.yaml

[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: too many spaces inside braces

(braces)


[error] 4-4: syntax error: expected , but found ''

(syntax)


[error] 9-9: too many spaces inside braces

(braces)


[error] 9-9: too many spaces inside braces

(braces)


[error] 13-13: too many spaces inside braces

(braces)


[error] 18-18: too many spaces inside braces

(braces)


[error] 22-22: too many spaces inside braces

(braces)


[error] 22-22: too many spaces inside braces

(braces)


[error] 23-23: too many spaces inside braces

(braces)


[error] 23-23: too many spaces inside braces

(braces)


[error] 24-24: too many spaces inside braces

(braces)


[error] 24-24: too many spaces inside braces

(braces)


[error] 38-38: too many spaces inside braces

(braces)


[error] 38-38: too many spaces inside braces

(braces)


[error] 53-53: too many spaces inside braces

(braces)


[error] 63-63: too many spaces inside braces

(braces)


[error] 68-68: too many spaces inside braces

(braces)


[error] 68-68: too many spaces inside braces

(braces)


[error] 79-79: too many spaces inside braces

(braces)


[error] 91-91: too many spaces inside braces

(braces)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: package-image
  • GitHub Check: build (macos-15)
  • GitHub Check: build (ubuntu-24.04)
  • GitHub Check: lint-check (macos-15)
🔇 Additional comments (4)
tools/deployment/package-helm/templates/redis-statefulset.yaml (1)

52-64: Health checks are well-configured with consistent patterns.

The readiness and liveness probes correctly use redis-cli with authentication, proper anchor sharing, and consistent YAML patterns. The configuration is operationally sound.

tools/deployment/package-helm/templates/database-statefulset.yaml (3)

75-75: YAMLlint warns of excess spaces in braces on line 75. Verify the byte-by-byte content of line 75 ({{- include "clp.livenessProbeTimings" . | nindent 12 }}) using the provided command to detect any hidden whitespace, and compare formatting consistency with line 65.


67-73: Environment variable substitution syntax in exec command is correct for Kubernetes.

The $(MYSQL_USER) and $(MYSQL_PASSWORD) syntax is the standard Kubernetes pattern for environment variable substitution within container command arguments. Ensure that MYSQL_USER and MYSQL_PASSWORD are defined as environment variables in the container spec before the exec probe runs. The array format used for the command is correct YAML for Kubernetes exec probes. Verify the YAML anchor referenced on line 76 is properly defined with &database-health-check earlier in the template and that all template syntax on line 75 is valid.


76-76: YAML anchors and aliases are not preserved during Helm template rendering.

The &database-health-check anchor definition and *database-health-check alias reference are valid YAML syntax, but Helm's YAML processing (which parses and re-serializes the template) will expand the alias inline and drop the anchor/alias mechanism in the final output. The rendered manifest will have the referenced content duplicated rather than using the anchor reference.

If deduplication is the goal, consider using Helm template helpers ({{ define }} / {{ include }}) instead of YAML anchors, or store the health check configuration as a reusable value in values.yaml and reference it through Helm templating.

⛔ Skipped due to learnings
Learnt from: junhaoliao
Repo: y-scope/clp PR: 1466
File: .github/workflows/clp-rust-checks.yaml:14-15
Timestamp: 2025-10-22T21:14:12.225Z
Learning: Repository y-scope/clp: In GitHub Actions workflows (e.g., .github/workflows/clp-rust-checks.yaml), YAML anchors/aliases are acceptable and preferred to avoid duplication; if actionlint flags an alias node (e.g., on push.paths) as an error, treat it as a tool limitation and do not require inlining unless the team asks to silence the warning.

Comment on lines +71 to +95
volumeClaimTemplates:
- metadata:
name: "redis-data"
spec:
accessModes: ["ReadWriteOnce"]
storageClassName: "local-storage"
selector:
matchLabels:
{{- include "clp.selectorLabels" . | nindent 12 }}
app.kubernetes.io/component: "redis"
resources:
requests:
storage: "10Gi"
- metadata:
name: "redis-logs"
spec:
accessModes: ["ReadWriteOnce"]
storageClassName: "local-storage"
selector:
matchLabels:
{{- include "clp.selectorLabels" . | nindent 12 }}
app.kubernetes.io/component: "redis"
resources:
requests:
storage: "5Gi"
Copy link
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Storage configuration could be parameterized for deployment flexibility.

The hard-coded storage sizes (10Gi for data, 5Gi for logs) work for the current deployment but limit flexibility if different environments need different capacity allocations. Consider making these configurable via values.yaml for future deployments.

If flexibility is desired, apply this optional refactor:

         resources:
           requests:
-            storage: "10Gi"
+            storage: {{ .Values.redis.storage.data | quote }}

Then add to values.yaml:

redis:
  storage:
    data: "10Gi"
    logs: "5Gi"
🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 79-79: too many spaces inside braces

(braces)


[error] 91-91: too many spaces inside braces

(braces)

🤖 Prompt for AI Agents
In tools/deployment/package-helm/templates/redis-statefulset.yaml around lines
71 to 95, the PVC storage sizes are hard-coded (10Gi and 5Gi); make them
configurable via values.yaml by adding redis.storage.data and redis.storage.logs
keys with sensible defaults, then replace the literal sizes in the
volumeClaimTemplates with template references to those values (falling back to
the defaults if unset). Update values.yaml to include the redis.storage.data and
redis.storage.logs entries and ensure any README or chart notes mention the new
options.

@kirkrodrigues kirkrodrigues changed the title feat(deployment): Add redis, queue (RabbitMQ), and results cache (MongoDB) as StatefulSet into the CLP Package Helm chart. feat(deployment): Add redis, queue (RabbitMQ), and results cache (MongoDB) as StatefulSets into the CLP Package Helm chart. Dec 8, 2025
@kirkrodrigues kirkrodrigues changed the title feat(deployment): Add redis, queue (RabbitMQ), and results cache (MongoDB) as StatefulSets into the CLP Package Helm chart. feat(deployment): Add redis, queue (RabbitMQ), and results-cache (MongoDB) components as StatefulSets into the CLP Package Helm chart. Dec 8, 2025
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

Edited the PR title directly.

@junhaoliao junhaoliao merged commit 70571df into y-scope:main Dec 8, 2025
22 checks passed
@junhaoliao junhaoliao deleted the dbs branch December 8, 2025 15:42
davidlion pushed a commit to davidlion/clp that referenced this pull request Jan 17, 2026
…` (MongoDB) components as `StatefulSet`s into the CLP Package Helm chart. (y-scope#1698)
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.

2 participants