Skip to content

feat: allow waitForMongodb init container resources to be specified - #270

Open
wrn14897 wants to merge 2 commits into
mainfrom
warren/waitformongo-resource
Open

feat: allow waitForMongodb init container resources to be specified#270
wrn14897 wants to merge 2 commits into
mainfrom
warren/waitformongo-resource

Conversation

@wrn14897

Copy link
Copy Markdown
Collaborator

What

Ports the still-applicable half of #187 (by @spoofedpacket) to the current chart layout.

Adds hyperdx.deployment.waitForMongodb.resources (empty by default) so the wait-for-mongodb init container can carry requests/limits:

hyperdx:
  deployment:
    waitForMongodb:
      resources:
        requests:
          memory: "100Mi"
          cpu: "100m"
        limits:
          memory: "200Mi"
          cpu: "500m"

Why

Clusters that enforce namespace resource quotas reject pods whose containers (including init containers) lack requests/limits. Without this value, the entire HyperDX pod is rejected even when hyperdx.deployment.resources is set.

Why not the MongoDB half of #187

#187 was written against the old chart layout where MongoDB was a plain Deployment (mongodb-deployment.yaml). MongoDB is now a MongoDBCommunity CR whose mongodb.spec is rendered verbatim, so container resources are already settable via the operator's statefulset override:

mongodb:
  spec:
    statefulSet:
      spec:
        template:
          spec:
            containers:
              - name: mongod
                resources:
                  requests: { memory: "512Mi", cpu: "500m" }
                  limits: { memory: "2Gi", cpu: "2" }

This is now documented in values.yaml instead of adding a template change.

Changes

  • templates/hyperdx/deployment.yaml — optional resources block on the wait-for-mongodb init container
  • values.yamlhyperdx.deployment.waitForMongodb.resources: {} with commented example; doc hint on mongodb.spec for MongoDB container resources
  • tests/hyperdx-deployment_test.yaml — tests for default (no resources) and override rendering
  • Changeset (minor)

Testing

  • helm unittest charts/clickstack: 195/195 passed (28 suites)
  • helm template verified default output is unchanged and the override renders correctly

Closes #187

Ports the still-applicable half of #187 to the current chart layout.
Adds hyperdx.deployment.waitForMongodb.resources (empty by default) so
the wait-for-mongodb init container can carry requests/limits, which is
required in clusters enforcing namespace resource quotas.

The MongoDB half of #187 is obsolete: MongoDB is now a MongoDBCommunity
CR whose spec is rendered verbatim, so container resources are already
settable via the operator statefulSet override -- documented in
values.yaml instead.

Co-authored-by: Robert Gallagher <spoofedpacket@users.noreply.github.com>
@wrn14897
wrn14897 requested a review from a team as a code owner August 24, 2026 17:33
@changeset-bot

changeset-bot Bot commented Aug 24, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 01141b5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
helm-charts Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Deep Review

✅ No critical issues found. The template change itself is correct: {{- with }} over a {} default renders nothing, nindent 12 matches the sibling securityContext block at the same container indent (charts/clickstack/templates/hyperdx/deployment.yaml:70-73), and default output is unchanged. Every finding below is in the new documentation — which matters here because the comments are the deliverable for the MongoDB half of this change.

🟡 P2 -- recommended

  • charts/clickstack/values.yaml:347 -- the new statefulSet example sits at the mongodb: level directly above spec: and is rooted at statefulSet:, but charts/clickstack/templates/mongodb/community.yaml:9 renders only .Values.mongodb.spec, so a user who pastes it where it appears lands at mongodb.statefulSet and gets no error and no rendered override.
    • Fix: Re-root the example inside the spec: mapping, or prefix the snippet with spec: so the documented path resolves to mongodb.spec.statefulSet.
    • previous-comments, reliability
  • charts/clickstack/values.yaml:149 -- the # Example: block is indented two spaces deeper than the resources: {} key it documents, so stripping the # markers in place leaves a block mapping under an already-terminated flow value and helm install -f fails with a YAML parse error.
    • Fix: Move the example above the key at the key's own indentation and include the resources: line in the snippet, matching the global.imagePullSecrets convention at charts/clickstack/values.yaml:3-9.
    • correctness, maintainability, project-standards, reliability
  • charts/clickstack/values.yaml:147 -- the comment attributes namespace-quota admission to this knob alone, but hyperdx.deployment.resources also defaults to {} at charts/clickstack/values.yaml:105 and is separately gated at charts/clickstack/templates/hyperdx/deployment.yaml:103, so under a ResourceQuota requiring requests on every container the pod is still rejected when only this value is set.
    • Fix: Note in the comment that hyperdx.deployment.resources must be set alongside it for quota-enforcing clusters.
    • previous-comments, api-contract
🔵 P3 nitpicks (3)
  • charts/clickstack/values.yaml:353 -- the example sets resources only on the mongod container, but MongoDBCommunity statefulsets also run a mongodb-agent sidecar, so a quota that requires requests on every container still rejects the MongoDB pod.
    • Fix: Add a second list entry for mongodb-agent with its own resources block.
  • charts/clickstack/templates/hyperdx/deployment.yaml:74 -- the new block guards with {{- with }} while the sibling resources block at line 103 guards the same concept with {{- if }}; behaviour is identical, but the file now uses two idioms for one field name.
    • Fix: Switch to {{- if .Values.hyperdx.deployment.waitForMongodb.resources }} with an explicit toYaml path to match line 103.
  • charts/clickstack/tests/hyperdx-deployment_test.yaml:244 -- both new tests index initContainers[0] without asserting name: wait-for-mongodb, so if template ordering ever changes the assertions silently target the wrong container.
    • Fix: Add - equal: {path: spec.template.spec.initContainers[0].name, value: wait-for-mongodb} to the override test.

Reviewers (9): correctness, testing, maintainability, project-standards, reliability, api-contract, previous-comments, agent-native, learnings-researcher.

Testing gaps:

  • No case for a partial resources map (requests-only or limits-only) — the shape most quota-driven users actually write.
  • No case combining waitForMongodb.resources with a non-empty hyperdx.deployment.initContainers, the one interaction this diff introduces.
  • The documented mongodb.spec.statefulSet override has no rendered-CR assertion, so a refactor of the verbatim tpl (toYaml .Values.mongodb.spec) passthrough would break the recipe with no test failure.

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.

1 participant