Skip to content

Add resources to the subPath permissions init container - #304

Open
CaioMartins968 wants to merge 2 commits into
frappe:mainfrom
CaioMartins968:feat/subpath-init-resources
Open

CaioMartins968 wants to merge 2 commits into
frappe:mainfrom
CaioMartins968:feat/subpath-init-resources

Conversation

@CaioMartins968

Copy link
Copy Markdown
Contributor

The fix-subpath-permissions init container added in #297 declares no resources and they could not be configured. In a namespace with a ResourceQuota on CPU or memory (e.g. limits.memory), Kubernetes requires every container of a non-BestEffort pod, init containers included, to declare them, so pods using persistence subPath were rejected with FailedCreate.

Add persistence.subPathPermissions.resources (default {}), rendered on the init container only when set. Backward compatible: with the default, the init container renders exactly as before.

Please provide enough information so that others can review your pull request:

Explain the details for making this change. What existing problem does the pull request solve?

@greptile-apps

greptile-apps Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge.

Reviews (2) · Last reviewed commit: "Add resources to the subPath permissions..." · Reviewed by Greptile

The fix-subpath-permissions init container added in frappe#297 declares no
resources and they could not be configured. In a namespace with a
ResourceQuota on CPU or memory (e.g. limits.memory), Kubernetes requires
every container of a non-BestEffort pod, init containers included, to
declare them, so pods using persistence subPath were rejected with
FailedCreate.

Add persistence.subPathPermissions.resources (default {}), rendered on
the init container only when set. Backward compatible: with the default,
the init container renders exactly as before.
@CaioMartins968
CaioMartins968 force-pushed the feat/subpath-init-resources branch from a78d01d to 9b66999 Compare October 26, 2026 14:03
@CaioMartins968

Copy link
Copy Markdown
Contributor Author

@revant Hello, apologies for sending so many messages. I saw that you merged my last PR—thank you for the trust! I did, however, notice a small improvement that could be made: allowing the initContainer to accept resources. This brief PR enables the initContainer for subPaths to be configured with custom resources.

When persistence.logs.existingClaim is the same claim as the worker's
(the shared-PVC setup documented in frappe#297), every pod referenced that
PVC in two volumes, "sites-dir" and "logs". The kubelet mounts the claim
once and keeps waiting for the second volume, so the pods never leave
ContainerCreating (reproduced on AKS 1.34 with Azure Files NFS; a pod
mounting the claim from a single volume at both paths starts fine).

Add the erpnext.logsVolumeName helper: when worker and logs persistence
resolve to the same claim, logs are mounted from "sites-dir" with
logs.subPath and the "logs" volume is not declared. This covers all
deployments, jobs and the subPath permissions init container.

Backward compatible: with distinct claims or emptyDir logs, the
manifests render exactly as before.

@greptile-apps greptile-apps 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.

Reviews are paused because your organization has an unpaid invoice. Open billing settings to pay it and resume reviews.

This branch has not been deployed

No deployments
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