improve build volumes handling#75
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdded a new UseMemoryContainerStorage boolean to OSBuilds and BuildConfig, updated CRD/schema and sample docs, and changed Tekton task generation and controller flow to conditionally mark container-storage, run-dir, run-osbuild and var-tmp volumes as memory-backed when the flag is enabled. Changes
Sequence DiagramsequenceDiagram
participant User as User/Config
participant Controller as OperatorConfig Controller
participant TaskGen as Task Generator
participant Tekton as Tekton
User->>Controller: Submit OperatorConfig (useMemoryContainerStorage flag)
Controller->>Controller: Read OSBuilds -> build BuildConfig (UseMemoryContainerStorage)
Controller->>TaskGen: Call GeneratePrepareBuilderTask(namespace, buildConfig)
TaskGen->>TaskGen: Inspect BuildConfig.UseMemoryContainerStorage
alt UseMemoryContainerStorage true
TaskGen->>TaskGen: Set EmptyDir.medium=Memory for container-storage, run-dir, run-osbuild, var-tmp
TaskGen->>TaskGen: Apply memoryVolumeSize as SizeLimit if provided
else
TaskGen->>TaskGen: Leave volumes as disk-backed EmptyDir
end
TaskGen->>Tekton: Emit configured Tekton Task
Tekton->>Tekton: Run Task with configured volumes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: 5
🤖 Fix all issues with AI agents
In `@api/v1alpha1/operatorconfig_types.go`:
- Around line 31-35: The docstring for the optional boolean field
UseMemoryContainerStorage incorrectly states "Default: true" while the actual
omitted/default value is false; update that comment to "Default: false
(disk-backed)" and adjust any matching doc comments for other similar optional
memory-backed storage booleans in this file to reflect disk-backed default, and
ensure the CRD/sample manifests are kept in sync with the corrected default
text; locate the field named UseMemoryContainerStorage in
operatorconfig_types.go and the other analogous boolean fields nearby and change
their Default: text and sample/CRD docs accordingly.
In `@bundle/manifests/automotive.sdv.cloud.redhat.com_operatorconfigs.yaml`:
- Around line 542-547: The CRD field useMemoryContainerStorage's description
currently states the default is true; update the description for
useMemoryContainerStorage to reflect the new default behavior (default: false)
and that disk-backed volumes are used by default for
/var/lib/containers/storage, /run/osbuild, and /var/tmp; keep the existing
explanation about SELinux xattr and memory tradeoffs but flip the default value
text and adjust phrasing to indicate memory-backed volumes are optional when set
to true.
In `@config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yaml`:
- Around line 542-547: The CRD description for the boolean field
useMemoryContainerStorage is inaccurate (says "Default: true") — update the
description text for useMemoryContainerStorage to reflect that the field is
optional and defaults to false unless explicitly set (e.g., change "Default:
true" to "Default: false" and/or add "optional" wording) so the documentation
matches the actual default behavior.
In `@config/samples/automotive_v1_operatorconfig.yaml`:
- Around line 23-27: Update the sample comments to reflect the new disk-backed
default by changing the descriptive line to "Default: false (disk-backed, lower
memory usage)" and clarify trade-offs; also add an explicit example showing how
to opt in to memory-backed storage by including the commented key with a clear
note (useMemoryContainerStorage: true) so users know to uncomment/set it to opt
in. Reference the useMemoryContainerStorage sample entry and the surrounding
explanatory comment block so you update the default text and add the opt-in
example in the same comment section.
In `@internal/common/tasks/tasks.go`:
- Around line 406-412: The code incorrectly defaults useMemoryContainerStorage
to true when buildConfig is nil; update the initialization of
useMemoryContainerStorage (the variable used in the container storage branch) to
default to false and only set it to buildConfig.UseMemoryContainerStorage when
buildConfig != nil; apply this same change at both occurrences where
useMemoryContainerStorage is initialized (the block around the earlier branch
and the later block near the second occurrence, lines referenced by the variable
usage) so disk storage becomes the default unless explicitly enabled via
buildConfig.
bennyz
force-pushed
the
fix-memory-volumes
branch
from
f975fd0 to
41620ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/common/tasks/tasks.go`:
- Around line 406-428: When UseMemoryContainerStorage is true we currently set
EmptyDir.Medium but never apply the configured size limit; update the loop in
which useMemoryContainerStorage is read (and the analogous block at the other
location) to set EmptyDir.SizeLimit using buildConfig.MemoryVolumeSize whenever
you create or mutate an EmptyDir for the memory-backed volumes. Concretely:
inside the loop over task.Spec.Volumes, for volumes named "container-storage",
"run-dir" (and "var-tmp" if present in the other block) ensure vol.EmptyDir is
non-nil, set vol.EmptyDir.Medium = corev1.StorageMediumMemory if not already,
and set vol.EmptyDir.SizeLimit = &buildConfig.MemoryVolumeSize (or parse the
string into a resource.Quantity if MemoryVolumeSize is a string) so the limit is
applied; apply the same changes to the other block that handles memory volumes
(the block around lines 1230-1246).
we were using memory volumes by default for too many paths. Instead we should use disk, unless explictly asked by the config. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
fix-memory-volumes
branch
from
41620ea to
f6aef24
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@api/v1alpha1/operatorconfig_types.go`:
- Around line 167-177: The generated deepcopy code is missing the new
OSBuildsConfig fields; run the code generator (e.g., execute "make generate
manifests") to regenerate zz_generated.deepcopy.go so DeepCopyInto for
OSBuildsConfig includes copying UseMemoryContainerStorage and MemoryVolumeSize
(ensure the regenerated DeepCopyInto method for OSBuildsConfig explicitly
handles these two fields).
🧹 Nitpick comments (1)
internal/common/tasks/tasks.go (1)
420-423: Consider removing redundant nil check (optional).The
buildConfig != nilcheck is redundant sinceuseMemoryContainerStoragecan only betrueifbuildConfigwas non-nil (see lines 409-411 and 1231-1233). The code is correct but could be simplified.♻️ Optional simplification
if useMemoryContainerStorage { for i := range task.Spec.Volumes { vol := &task.Spec.Volumes[i] if vol.Name == "container-storage" || vol.Name == "run-dir" { vol.EmptyDir = &corev1.EmptyDirVolumeSource{ Medium: corev1.StorageMediumMemory, } - if buildConfig != nil && buildConfig.MemoryVolumeSize != "" { + if buildConfig.MemoryVolumeSize != "" { sizeLimit := resource.MustParse(buildConfig.MemoryVolumeSize) vol.EmptyDir.SizeLimit = &sizeLimit } } } }Apply the same pattern at line 1242.
Also applies to: 1242-1245
This was referenced Mar 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
we were using memory volumes by default for too many paths. Instead we should use disk, unless explictly asked by the config.
Summary by CodeRabbit
New Features
Documentation