use global storage class for workspace PVCs and preserve memory volumes#188
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe pull request refactors volume configuration precedence in Kubernetes task processing and updates PVC creation logic. When Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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 (1)
internal/controller/imagebuild/controller.go (1)
2054-2061:⚠️ Potential issue | 🟠 MajorHandle transient
OperatorConfigread failures before creating the PVC.Right now, any non-NotFound error on Line 2056 falls through to default PVC settings, which can silently create a workspace PVC with the cluster default storage class and regress the storage-class fix.
Suggested fix
operatorConfig := &automotivev1alpha1.OperatorConfig{} err := r.Get(ctx, types.NamespacedName{Name: "config", Namespace: OperatorNamespace}, operatorConfig) + if err != nil && !errors.IsNotFound(err) { + return "", fmt.Errorf("failed to get OperatorConfig: %w", err) + } storageSize := resource.MustParse("8Gi") if err == nil && operatorConfig.Spec.OSBuilds != nil && operatorConfig.Spec.OSBuilds.PVCSize != "" { storageSize = resource.MustParse(operatorConfig.Spec.OSBuilds.PVCSize) log.Info("Using OSBuilds PVCSize", "size", operatorConfig.Spec.OSBuilds.PVCSize) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/imagebuild/controller.go` around lines 2054 - 2061, The code currently treats any r.Get(ctx, NamespacedName{...}, operatorConfig) error as a benign miss and falls back to defaults; change this so non-NotFound errors are handled as transient failures: after calling r.Get(...) check if err != nil and if apierrors.IsNotFound(err) then continue to use defaults, but if err != nil and not IsNotFound, log the error (including err) and return the error (or requeue) from the reconcile so we don't create a PVC with wrong defaults; update the block around OperatorConfig, r.Get, storageSize and OperatorConfig.Spec.OSBuilds.PVCSize to implement this error branching (use apierrors.IsNotFound from k8s.io/apimachinery/pkg/api/errors).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/common/tasks/scratch_volumes_test.go`:
- Around line 172-179: Add an explicit assertion that the container-storage
volume exists before checking its medium: iterate task.Spec.Volumes looking for
a volume with Name == volumeNameContainerStorage, set a found flag (e.g.,
foundContainerStorage), and after the loop call t.Fatalf if not found; then
verify that the found volume's EmptyDir is non-nil and EmptyDir.Medium ==
corev1.StorageMediumMemory (or call t.Fatalf with a clear message if those
checks fail) so the test fails when the volume is missing as well as when its
medium is incorrect.
---
Outside diff comments:
In `@internal/controller/imagebuild/controller.go`:
- Around line 2054-2061: The code currently treats any r.Get(ctx,
NamespacedName{...}, operatorConfig) error as a benign miss and falls back to
defaults; change this so non-NotFound errors are handled as transient failures:
after calling r.Get(...) check if err != nil and if apierrors.IsNotFound(err)
then continue to use defaults, but if err != nil and not IsNotFound, log the
error (including err) and return the error (or requeue) from the reconcile so we
don't create a PVC with wrong defaults; update the block around OperatorConfig,
r.Get, storageSize and OperatorConfig.Spec.OSBuilds.PVCSize to implement this
error branching (use apierrors.IsNotFound from
k8s.io/apimachinery/pkg/api/errors).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07d4cfd8-00dd-495f-bb36-44d0cb264e0c
📒 Files selected for processing (3)
internal/common/tasks/scratch_volumes_test.gointernal/common/tasks/tasks.gointernal/controller/imagebuild/controller.go
memory volumes Tekton workspace PVCs ignored operatorConfig.spec.osBuilds.storageClass, falling through to the cluster default (efs-sc/NFS). NFS does not support security xattrs, causing cap_set_file failures during RPM installs in osbuild. Also fix useMemoryVolumes being silently ignored when usePVCScratchVolumes is enabled — container-storage now gets tmpfs medium regardless of the PVC scratch setting. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-sonnet-4.6
2 participants
Tekton workspace PVCs ignored operatorConfig.spec.osBuilds.storageClass, falling through to the cluster default (efs-sc/NFS). NFS does not support security xattrs, causing cap_set_file failures during RPM installs in osbuild.
Also fix useMemoryVolumes being silently ignored when usePVCScratchVolumes is enabled — container-storage now gets tmpfs medium regardless of the PVC scratch setting.
Summary by CodeRabbit
Bug Fixes
Tests
container-storagevolume correctly handles combined configuration scenarios.