Add user home mount for ephemeral storage when init-persistent-home i…#1649
Add user home mount for ephemeral storage when init-persistent-home i…#1649dkwon17 wants to merge 1 commit into
Conversation
…s explicitly configured Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: dkwon17 <dakwon@redhat.com>
📝 WalkthroughWalkthrough
ChangesPersistent Home Eligibility Rework
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/library/home/persistentHome.go (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the copyright header year to match the current guideline.
Line 2 still uses
2019-2025; the guideline requires2019-{CURRENT_YEAR}.As per coding guidelines,
**/*.go: "All Go source files MUST start with the copyright header: '// Copyright (c) 2019-{CURRENT_YEAR} Red Hat, Inc.' followed by Apache License 2.0 license text".🤖 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/library/home/persistentHome.go` at line 2, Update the copyright header year in the copyright comment to reflect the current year. Change the copyright line from "Copyright (c) 2019-2025 Red Hat, Inc." to "Copyright (c) 2019-{CURRENT_YEAR} Red Hat, Inc." where {CURRENT_YEAR} is replaced with the actual current year, as required by the coding guidelines for Go source files.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.
Outside diff comments:
In `@pkg/library/home/persistentHome.go`:
- Line 2: Update the copyright header year in the copyright comment to reflect
the current year. Change the copyright line from "Copyright (c) 2019-2025 Red
Hat, Inc." to "Copyright (c) 2019-{CURRENT_YEAR} Red Hat, Inc." where
{CURRENT_YEAR} is replaced with the actual current year, as required by the
coding guidelines for Go source files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db651927-8b6f-4f94-90d1-d0ecaa655bc4
📒 Files selected for processing (4)
pkg/library/home/persistentHome.gopkg/library/home/testdata/persistent-home/creates-home-vm-with-ephemeral-storage.yamlpkg/library/home/testdata/persistent-home/noop-ephemeral-storage-without-init-container.yamlpkg/library/home/testdata/persistent-home/noop-if-init-prestartevent-already-defined.yaml
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
|
/che-ai-assistant ok-pr-review Review is complete. Please check the review comments below. |
tolusha
left a comment
There was a problem hiding this comment.
Code Review Summary
🎯 Overall Assessment: APPROVE ✅
This PR successfully implements support for persistent home directories in ephemeral storage workspaces when a custom init-persistent-home init container is explicitly configured. The changes are well-structured, properly tested, and solve a real consistency problem between ephemeral and non-ephemeral workspaces.
✅ Strengths
- Clean Architecture - Removed coupling with
storagepackage, making the home logic more self-contained and maintainable - Proper Test Coverage - Added 2 comprehensive test cases covering both new behavior and backward compatibility
- Clear Logic - The special handling for ephemeral storage is explicit and easy to understand
- Backward Compatible - Existing workspaces without custom init containers maintain current behavior
- Good Error Fix - Corrected error message from "command" to "event" for accuracy
🔍 Key Observations
1. Behavioral Change for Non-Ephemeral Storage (Medium Priority)
The removal of storage.WorkspaceNeedsStorage() check simplifies the logic but changes behavior:
Before:
return storage.WorkspaceNeedsStorage(&workspace.Spec.Template)After:
return true // for all non-ephemeral storageImpact: A workspace with non-ephemeral storage type but only ephemeral volumes will now get persistent home (previously wouldn't).
Question: Is this intentional? If so, consider adding a clarifying comment:
// For ephemeral storage, only add persistent home if init-persistent-home is configured
storageType := workspace.Spec.Template.Attributes.GetString(constants.DevWorkspaceStorageTypeAttribute, nil)
if storageType == constants.EphemeralStorageClassType {
return hasInitPersistentHomeInConfig(workspace)
}
// For non-ephemeral storage types, always create persistent home if enabled.
// This includes workspaces with all ephemeral volumes but non-ephemeral storage type.
return true2. Container Component Validation (Low Priority)
Good addition of the hasContainerComponents check - prevents edge cases where workspaces without containers would incorrectly trigger home creation.
💡 Suggestions for Improvement
-
Additional Test Case (Optional)
- Test workspace with non-ephemeral storage type but all volumes marked ephemeral
- Validates the behavioral change mentioned above
-
Documentation Update (Recommended)
- Update operator docs to explain the new ephemeral + custom init behavior
- Add example configuration showing this capability
📋 Pre-Merge Checklist
- Code quality - Clean, maintainable changes
- Unit tests - New tests added and passing
- Logic correctness - Functions as designed
- E2E tests - Waiting for
/test v8-devworkspace-operator-e2e, v8-che-happy-path - Manual testing - Per test plan in PR description
- Documentation - Consider updating user-facing docs
🎬 Recommendation
Approve and merge after E2E tests pass. The code changes are solid, well-tested, and implement the requested functionality correctly.
Technical Details Reviewed
- ✅ Logic refactoring in
NeedsPersistentHomeDirectory() - ✅ New helper function
hasInitPersistentHomeInConfig() - ✅ Test coverage for new behavior
- ✅ Backward compatibility maintained
- ✅ Error message correction
- ✅ Dependency reduction (removed storage import)
Review conducted: 2026-06-17
Files analyzed: 4 files (126 changed lines + context)
Test coverage: 2 new test cases added
Risk assessment: Low-Medium (E2E validation recommended)
|
I tested as per abovementioned comment and can confirm it's working as expected: When no init-persistent-home init container, ephemeral workspace starts without any init container When init-persistent-home init container is in DWOC, workspace has extra init |
|
|
||
| output: | ||
| error: "failed to add init container for home persistence setup: command with id init-persistent-home already exists in the devworkspace" | ||
| error: "failed to add init container for home persistence setup: event with id init-persistent-home already exists in the devworkspace" |
There was a problem hiding this comment.
Umm, why is this change required?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dkwon17, rohanKanojia, tolusha The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…s explicitly configured
What does this PR do?
If an init-persistent-home init container is explicitly defined in the DWOC's
config.workspaces.initContainersfield and if persistUserHome is enabled, a /user/home empty dir volume mount will be added to the workspace pod.This is to allow consistent behaviour between ephemeral and non-ephemeral workspaces when
persistUserHomeis enabled and a custominit-persistent-homeinit container is defined.What issues does this PR fix or reference?
https://redhat.atlassian.net/browse/CRW-11010
Is it tested? How?
Install DWO with the following catalog source:
For both cases, ensure that the global DWOC has
persistUserHomeenabled:Test case 1
init-persistent-homeinit container and no PVC createdTest case 2
init-persistent-homeexplicitly configured:init-persistent-homeran,persistent-homevolume isemptyDir(not PVC), no PVC created.For example:
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit
Release Notes
New Features
Tests