fix(config): bump default persistent home init container memory limit…#1644
Conversation
btjd
requested review from
akurinnoy,
dkwon17,
ibuziuk and
rohanKanojia
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (7)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDefault container memory limits are increased from ChangesDefault Memory Resource Update
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/config/defaults.go (2)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate copyright year to 2026 in
pkg/config/defaults.goandapis/controller/v1alpha1/devworkspaceoperatorconfig_types.go.Both files show copyright year 2025 in their headers, but the current year is 2026. As per coding guidelines, all Go source files must start with the copyright header format:
// Copyright (c) 2019-2026 Red Hat, Inc.The same one-line fix applies to both files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/defaults.go` at line 2, Replace the outdated header line "// Copyright (c) 2019-2025 Red Hat, Inc." with "// Copyright (c) 2019-2026 Red Hat, Inc." in both files that show this header (identify by that exact header string) so the file headers in pkg/config/defaults.go and apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go are updated to 2026.Source: Coding guidelines
18-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReorganize imports to follow project conventions.
The imports are not organized according to the coding guidelines. As per coding guidelines, imports should be organized into three groups separated by blank lines: (1) standard library, (2) third-party + Kubernetes, (3) project-local. Currently, the project-local imports appear before the Kubernetes packages.
♻️ Proposed fix - Run 'make fmt' to automatically fix
import ( "fmt" - "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" - "github.com/devfile/devworkspace-operator/pkg/infrastructure" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/utils/pointer" + + "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "github.com/devfile/devworkspace-operator/pkg/infrastructure" )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/config/defaults.go` around lines 18 - 27, Reorder the import block in pkg/config/defaults.go into three groups separated by blank lines—first standard library (fmt), then third-party/Kubernetes packages (k8s.io/* and k8s.io/utils/pointer), and finally project-local imports (github.com/devfile/devworkspace-operator/... and github.com/devfile/devworkspace-operator/pkg/infrastructure); ensure imports for appsv1, corev1, and k8s resource/pointer are in the Kubernetes group and devworkspace/infrastructure are in the project-local group, then run the repository formatter (make fmt) to apply the style automatically.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@deploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yaml`:
- Line 544: The generated CRD artifact is out of sync with the updated template:
update the default memory description from "128Mi/64Mi" to "256Mi/128Mi" in the
deployed CRD by regenerating the CRD objects from the source template
(controller.devfile.io_devworkspaceoperatorconfigs.yaml) and committing the
updated
devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml;
run the project’s CRD generation/make target (or the generator script used by
the repo), verify the memory default strings now read "256Mi" (limit) and
"128Mi" (request) in the generated CRD, and commit the regenerated file so both
files advertise the same defaults.
---
Outside diff comments:
In `@pkg/config/defaults.go`:
- Line 2: Replace the outdated header line "// Copyright (c) 2019-2025 Red Hat,
Inc." with "// Copyright (c) 2019-2026 Red Hat, Inc." in both files that show
this header (identify by that exact header string) so the file headers in
pkg/config/defaults.go and
apis/controller/v1alpha1/devworkspaceoperatorconfig_types.go are updated to
2026.
- Around line 18-27: Reorder the import block in pkg/config/defaults.go into
three groups separated by blank lines—first standard library (fmt), then
third-party/Kubernetes packages (k8s.io/* and k8s.io/utils/pointer), and finally
project-local imports (github.com/devfile/devworkspace-operator/... and
github.com/devfile/devworkspace-operator/pkg/infrastructure); ensure imports for
appsv1, corev1, and k8s resource/pointer are in the Kubernetes group and
devworkspace/infrastructure are in the project-local group, then run the
repository formatter (make fmt) to apply the style automatically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3953d8f2-a3f0-4068-a305-4777b6d82c96
📒 Files selected for processing (4)
apis/controller/v1alpha1/devworkspaceoperatorconfig_types.goapis/controller/v1alpha1/zz_generated.deepcopy.godeploy/templates/crd/bases/controller.devfile.io_devworkspaceoperatorconfigs.yamlpkg/config/defaults.go
| // DefaultContainerResources defines the resource requirements (memory/cpu limit/request) used for | ||
| // container components that do not define limits or requests. In order to not set a field by default, | ||
| // the value "0" should be used. By default, the memory limit is 128Mi and the memory request is 64Mi. | ||
| // the value "0" should be used. By default, the memory limit is 256Mi and the memory request is 128Mi. |
There was a problem hiding this comment.
Since you've modified CRD types you'd need to run this command so that changes are reflected in deploy/ manifests:
make generate_allYou don't need to manually modify deepcopy.go or any files in deploy/. When I ran the above-mentioned command, i saw these files being modified:
modified: apis/controller/v1alpha1/zz_generated.deepcopy.go
modified: deploy/bundle/manifests/controller.devfile.io_devworkspaceoperatorconfigs.yaml
modified: deploy/deployment/kubernetes/combined.yaml
modified: deploy/deployment/kubernetes/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml
modified: deploy/deployment/openshift/combined.yaml
modified: deploy/deployment/openshift/objects/devworkspaceoperatorconfigs.controller.devfile.io.CustomResourceDefinition.yaml
|
/retest |
…s to 256Mi Signed-off-by: Badre Tejado-Imam <btejado@redhat.com>
|
/che-ai-assistant ok-pr-review Review is complete. Please check the review comments below. |
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: 💬 Comment - The change is technically sound and well-executed. One discussion point worth addressing before merge.
Key Findings
1. Scope consideration ()
The DefaultContainerResources field applies to all workspace container components that don't define their own limits/requests, not just the init-persistent-home container. The PR title and description frame this as an init-persistent-home fix, but it will increase memory defaults for all containers across all workspaces.
This may be intentional and acceptable, but should be explicitly acknowledged. Consider whether a separate InitContainerResources or PersistentHomeResources config field would be more appropriate for targeting just the init-persistent-home container.
2. Operational impact - release notes recommended
Doubling the default memory limit (128Mi→256Mi) and request (64Mi→128Mi) will increase resource consumption for every container in every workspace that relies on defaults.
At scale (100 workspaces × 3 containers per workspace), this adds ~19 GiB of reserved memory to cluster capacity requirements. Clusters with tight ResourceQuota or LimitRange configurations may see workspace creation failures after this change. Consider noting this in a changelog or upgrade guide so cluster administrators can plan accordingly.
Positive Feedback
- Clean, minimal change - only the source-of-truth defaults file and its generated outputs are touched
- Good PR description with clear problem statement, technical rationale, and validation scenarios
- User override flexibility is preserved - custom
DevWorkspaceOperatorConfigsettings continue to supersede these defaults - Author was responsive to reviewer feedback and pushed fixes promptly
- All 6 generated manifest files are consistent with the source change
Reviews completed: Pre-flight summary, Standard review, Deep review, and Impact analysis
Generated by: ok-pr-review
tolusha
left a comment
tolusha
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: 💬 Comment - The change is technically sound and well-executed. One discussion point worth addressing before merge.
Key Findings
1. Scope consideration (pkg/config/defaults.go:85-88)
The DefaultContainerResources field applies to all workspace container components that don't define their own limits/requests, not just the init-persistent-home container. The PR title and description frame this as an init-persistent-home fix, but it will increase memory defaults for all containers across all workspaces.
This may be intentional and acceptable, but should be explicitly acknowledged. Consider whether a separate InitContainerResources or PersistentHomeResources config field would be more appropriate for targeting just the init-persistent-home container.
2. Operational impact - release notes recommended
Doubling the default memory limit (128Mi→256Mi) and request (64Mi→128Mi) will increase resource consumption for every container in every workspace that relies on defaults.
At scale (100 workspaces × 3 containers per workspace), this adds ~19 GiB of reserved memory to cluster capacity requirements. Clusters with tight ResourceQuota or LimitRange configurations may see workspace creation failures after this change. Consider noting this in a changelog or upgrade guide so cluster administrators can plan accordingly.
Positive Feedback
- Clean, minimal change - only the source-of-truth defaults file and its generated outputs are touched
- Good PR description with clear problem statement, technical rationale, and validation scenarios
- User override flexibility is preserved - custom
DevWorkspaceOperatorConfigsettings continue to supersede these defaults - Author was responsive to reviewer feedback and pushed fixes promptly
- All 6 generated manifest files are consistent with the source change
Reviews completed: Pre-flight summary, Standard review, Deep review, and Impact analysis
Generated by: ok-pr-review
|
I tested this PR, and it seems to work as expected. I enabled persistUserHome on my cluster: kubectl patch devworkspaceoperatorconfig devworkspace-operator-config -n openshift-operators --type merge -p '{"config":{"workspace":{"persistUserHome":{"enabled":true}}}}'Without any explicit DWOC Memory configurationCreating a plain DevWorkspace has these memory limits: name: init-persistent-home
resources:
limits:
memory: 256Mi
requests:
memory: 128MiWith explicit DWOC Memory configurationWhen I configure DWOC with custom limits: apiVersion: controller.devfile.io/v1alpha1
config:
workspace:
defaultContainerResources:
limits:
memory: 512Mi
requests:
memory: 256MiI can see the created workspace also contain specified limits: name: init-persistent-home
ready: true
resources:
limits:
memory: 512Mi
requests:
memory: 256Mi |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: btjd, rohanKanojia The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
4 participants
Description
This PR addresses issue CRW-10734 by increasing the default memory allocations assigned to the
init-persistent-homecontainer.The previous default threshold of
128Miwas causing transient Out-Of-Memory (OOM) faults (Exit Code 137) during initialization when deploying workspaces utilizing heavy developer images (such as the Universal Developer Image), due to the volume of file-stowing operations. Right-sizing the fallback bounds prevents container termination while preserving user override flexibility via custom operator configs.Technical Changes
pkg/config/defaults.goto shift defaultinit-persistent-homeresource settings to256Mi(limit) and128Mi(request).make generate).Validation Performed
256Mimemory bounds injected, execution completed with Exit Code 0).DevWorkspaceOperatorConfigsupersede defaults without regression.AI Assistance Disclosure
Summary by CodeRabbit