move the reservation up to when updating envd#2180
move the reservation up to when updating envd#2180matthewlouisbrockman wants to merge 1 commit intomainfrom
Conversation
matthewlouisbrockman
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -219,16 +219,6 @@ func (ppb *PostProcessingBuilder) postProcessingFn(userLogger logger.Logger) lay | |||
| return | |||
There was a problem hiding this comment.
Regression: fresh builds no longer get guest-side disk reservation.
The old code called SetReservedBlocksInGuest unconditionally (modulo the feature flag) in postProcessingFn, covering both cached and fresh builds. The new code moves this call into layer_executor.go inside if cmd.UpdateEnvd, but in finalize/builder.go line 196 UpdateEnvd is set to sourceLayer.Cached. For a fresh build, sourceLayer.Cached == false so UpdateEnvd == false and SetReservedBlocksInGuest is never called. The guest filesystem ends up without the tune2fs -r reserved-blocks setting on non-cached builds.
There was a problem hiding this comment.
im not sure this is accurate but moved this back to draft to check
| // Apply guest-side reserved blocks on the resumed-template path so older | ||
| // cached templates get the reservation before subsequent build actions run. | ||
| if reservedDiskSpaceMB := int64(lb.featureFlags.IntFlag(ctx, featureflags.BuildReservedDiskSpaceMB)); reservedDiskSpaceMB > 0 { | ||
| err = sandboxtools.SetReservedBlocksInGuest( |
There was a problem hiding this comment.
Redundant calls on multi-step cached builds.
steps/builder.go also sets UpdateEnvd: sourceLayer.Cached, so for a build with N cached steps, SetReservedBlocksInGuest (a round-trip tune2fs command inside the guest) is called N times - once per cached step layer - and again when the finalize layer is cached. Since tune2fs -r is idempotent the result is correct, but every extra call is unnecessary VM overhead. Consider tracking whether the reservation has already been applied (e.g. via a flag on LayerExecutor) and skipping subsequent calls.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21b22ec8a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Apply guest-side reserved blocks on the resumed-template path so older | ||
| // cached templates get the reservation before subsequent build actions run. | ||
| if reservedDiskSpaceMB := int64(lb.featureFlags.IntFlag(ctx, featureflags.BuildReservedDiskSpaceMB)); reservedDiskSpaceMB > 0 { |
There was a problem hiding this comment.
UpdateEnvd
This turns the guest-side reservation into a one-time side effect of UpdateEnvd, but UpdateEnvd is only set from sourceLayer.Cached in the step/finalize builders. After the first uncached layer, later phases no longer reapply the setting, and postProcessingFn now goes straight from configuration/start commands to SyncChangesToDisk (packages/orchestrator/internal/template/build/phases/finalize/builder.go:217-233). In a build that runs any later root command which touches the ext4 superblock (for example tune2fs -r 0 /dev/vda in a RUN or start command), the exported template will keep the wrong reserved-space value; before this change, finalize restored the configured reservation immediately before snapshotting.
Useful? React with 👍 / 👎.
|
This PR is obsoleted by #2199. |
3 participants
Moves guest-side reserved disk setup earlier for cached template builds. No change to fresh builds.
relates to: #2082 (comment)
Note
Medium Risk
Moves reserved disk space setup to run immediately after
envdupdates on resumed/cached builds, affecting guest filesystem behavior before subsequent actions. Risk is moderate because it changes build-time disk reservation timing and relies on feature-flag configuration during layer execution.Overview
Moves guest-side reserved disk block configuration from the final post-processing step to immediately after
envdis updated when resuming from a cached template, ensuring older cached templates get the reservation applied before later build actions run.Written by Cursor Bugbot for commit 21b22ec. This will update automatically on new commits. Configure here.