Skip to content

feat(opensearch): make setVMMaxMapCount configurable - #4026

Closed
europrinter (yankawai) wants to merge 1 commit into
cozystack:mainfrom
yankawai:fix/opensearch-vm-max-map-count
Closed

europrinter (yankawai) wants to merge 1 commit into
cozystack:mainfrom
yankawai:fix/opensearch-vm-max-map-count

Conversation

@yankawai

@yankawai europrinter (yankawai) commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

What this PR does

packages/apps/opensearch/templates/opensearch.yaml hardcodes spec.general.setVMMaxMapCount: true, so the opster operator always injects a privileged init-sysctl initContainer. Tenant namespaces carry no pod-security.kubernetes.io/enforce label and therefore inherit the cluster-wide default, which is baseline — and baseline forbids privileged containers. The StatefulSet never creates a Pod, the *-external Services get their LoadBalancer IPs but keep empty EndpointSlices, and the OpenSearchCluster still reports PHASE: RUNNING because only the non-privileged securityconfig Job runs. That is #3022, open since June with no fix.

The chart offers no way out: there is no values knob, so an operator who has already raised vm.max_map_count on the nodes still cannot stop the operator from adding the privileged container. This PR adds setVMMaxMapCount, defaulting to true so nothing changes for existing installations, and threads it into the CR.

Why the default stays true

Flipping the default to false would be the wrong trade. vm.max_map_count is a node-level sysctl, not a namespaced one, so it cannot be set through a Pod securityContext.sysctls — a cluster whose nodes still sit at the Linux default of 65530 would get a Pod that starts and then fails OpenSearch's own bootstrap check instead of failing admission. Worse, anyone who worked around #3022 by labelling the namespace privileged would silently lose the sysctl. Keeping true means this PR is purely additive.

Verification

Measured on a cluster reproducing #3022: nodes report vm.max_map_count = 65530, the machine config carries no vm.* sysctl, and the tenant namespace has no pod-security label, so the cluster-wide enforce: baseline applies and the StatefulSet logs the FailedCreate from the issue verbatim.

New suite tests/vm-max-map-count_test.yaml asserts both branches. Positive control: hardcoding the template back to true turns one of the two cases red, so the asserts really follow the value rather than the literal. Full chart suite 60/60 across 5 suites. values.schema.json, README.md, the generated Go type and the opensearch-rd cozyrds are all updated in step; the cozyrds openAPISchema is byte-identical to jq -c of the schema.

What this does not fix

Half the problem lives outside the chart. Cozystack ships no node sysctls — packages/core/talos builds images only — so an operator still has to set vm.max_map_count=262144 on the nodes before turning this knob off. This PR gives them the switch; whether the platform should ship the sysctl itself is the other half of #3022 and belongs in a separate change.

Refs #3022

Summary by CodeRabbit

  • New Features

    • Added the setVMMaxMapCount option for OpenSearch deployments, enabled by default.
    • OpenSearch can automatically configure the required vm.max_map_count system setting using a privileged initialization container.
    • The option can be disabled when the node already provides the required setting or PodSecurity restrictions prevent privileged containers.
  • Documentation

    • Documented the new configuration option, default behavior, and when to disable it.
  • Tests

    • Added coverage for the default-enabled behavior and explicit disabling.

@coderabbitai

coderabbitai Bot commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 1ead4af7-0bf2-4bbf-93ec-f5be4438211e

📥 Commits

Reviewing files that changed from the base of the PR and between 67b4e23 and ddcc871.

📒 Files selected for processing (7)
  • api/apps/v1alpha1/opensearch/types.go
  • packages/apps/opensearch/README.md
  • packages/apps/opensearch/templates/opensearch.yaml
  • packages/apps/opensearch/tests/vm-max-map-count_test.yaml
  • packages/apps/opensearch/values.schema.json
  • packages/apps/opensearch/values.yaml
  • packages/system/opensearch-rd/cozyrds/opensearch.yaml

Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The OpenSearch API and Helm chart now expose setVMMaxMapCount. The option defaults to true, can disable the privileged init container, and is validated by chart tests. Documentation and the ApplicationDefinition schema describe the setting and PodSecurity constraint.

Changes

OpenSearch vm.max_map_count configuration

Layer / File(s) Summary
Configuration contract
api/apps/v1alpha1/opensearch/types.go, packages/apps/opensearch/values.yaml, packages/apps/opensearch/values.schema.json, packages/system/opensearch-rd/cozyrds/opensearch.yaml, packages/apps/opensearch/README.md
Adds setVMMaxMapCount as a boolean option with default true. Documents the privileged init container and when to disable it.
Helm rendering and validation
packages/apps/opensearch/templates/opensearch.yaml, packages/apps/opensearch/tests/vm-max-map-count_test.yaml
Renders the configured value and tests both the default-enabled and explicitly disabled cases.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to ddcc8

The configurable setting preserves existing behavior by default and adds an opt-out for environments that manage the node sysctl themselves; no actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: making the OpenSearch setVMMaxMapCount setting configurable.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files. (6 skipped: 6 unsupported.)

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

@github-actions github-actions Bot added area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review kind/feature Categorizes issue or PR as related to a new feature size/M This PR changes 30-99 lines, ignoring generated files labels Sep 2, 2026
@scooby87

Copy link
Copy Markdown
Contributor

This PR conflicts with main right now — the branch is ~505 commits behind and GitHub reports a merge conflict, so it can't be reviewed against a clean diff or merged as-is. Could you rebase onto the latest main and resolve the conflicts? I'll run a full cozy-review pass as soon as it's mergeable again.

@yankawai

Copy link
Copy Markdown
Contributor Author

Closing this one, main has moved past it. #4152 raises vm.max_map_count on every node from a DaemonSet in the opensearch-operator package, and #2682 then set setVMMaxMapCount: false in the chart, so the privileged init container is gone and there is nothing left for a knob to switch. A rebase would put true back as the default, the opposite of what landed. #3022 looks closable on the back of those two.

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

Labels

area/uncategorized PR auto-labeler could not map title scope to a known area/*; please review kind/feature Categorizes issue or PR as related to a new feature size/M This PR changes 30-99 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants