Skip to content

perf(sandbox): reduce memory dirtying from journald and envd logging #2423

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ValentaTomas wants to merge 1 commit into from
Closed

perf(sandbox): reduce memory dirtying from journald and envd logging #2423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/envd/internal/logs/logger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,21 @@ func NewLogger(ctx context.Context, isNotFC bool, mmdsChan <-chan *host.MMDSOpts

exporters := []io.Writer{}

var level zerolog.Level
if isNotFC {
exporters = append(exporters, os.Stdout)
} else {
exporters = append(exporters, exporter.NewHTTPLogsExporter(ctx, isNotFC, mmdsChan), os.Stdout)
// HTTP exporter only — stdout goes to journald which dirties guest memory pages.
exporters = append(exporters, exporter.NewHTTPLogsExporter(ctx, isNotFC, mmdsChan))
}
level = zerolog.DebugLevel

@claude claude Bot Apr 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var level zerolog.Level initializes to 0, which equals zerolog.DebugLevel. The unconditional level = zerolog.DebugLevel outside the if-else is dead code — it reassigns the same zero value. If the intent is per-branch level selection in the future, the assignment belongs inside each branch; as written it just adds noise.


l := zerolog.
New(io.MultiWriter(exporters...)).
With().
Timestamp().
Logger().
Level(zerolog.DebugLevel)
Level(level)

return &l
}
2 changes: 1 addition & 1 deletion packages/envd/pkg/version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
package pkg

const Version = "0.5.14"
const Version = "0.5.15"
82 changes: 82 additions & 0 deletions packages/orchestrator/cmd/resume-build/measure-memory-dirtying.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
#!/bin/bash
# Measure memory/rootfs diff sizes across pause-resume cycles.
#
# This script builds a base template, then runs a series of pause-resume
# cycles to measure how many pages get dirtied during each cycle —
# both idle and under normal envd operations (file writes, process starts).
#
# Usage:
# sudo ./measure-memory-dirtying.sh [storage-path]
#
# Requires: root, KVM, Docker, NBD, hugepages
set -euo pipefail

@claude claude Bot Apr 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The paths ./packages/orchestrator/cmd/create-build and ./packages/orchestrator/cmd/resume-build are relative to the repo root, but the usage comment says sudo ./measure-memory-dirtying.sh — implying the caller is in the script's own directory, where those paths do not exist. The script should either document that it must be run from the repo root, or resolve it dynamically.

STORAGE="${1:-.local-build}"
CREATE_BUILD="go run ./packages/orchestrator/cmd/create-build"
RESUME_BUILD="go run ./packages/orchestrator/cmd/resume-build"

Check warning on line 16 in packages/orchestrator/cmd/resume-build/measure-memory-dirtying.sh

View check run for this annotation

Claude / Claude Code Review

measure-memory-dirtying.sh: go run paths require repo root but usage implies script-local invocation 

The `go run` paths in `measure-memory-dirtying.sh` are relative to CWD and only work when invoked from the repo root, but the Usage comment (`sudo ./measure-memory-dirtying.sh`) implies running from the script's own directory where those paths don't exist. Either update the Usage comment to show repo-root invocation (e.g. `sudo ./packages/orchestrator/cmd/resume-build/measure-memory-dirtying.sh`) or add a `cd $(dirname "$0")/../../..` at the top to make the script location-independent.
Comment on lines +15 to +16

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve go-run paths from the script directory

The script’s documented usage (sudo ./measure-memory-dirtying.sh) implies running it from packages/orchestrator/cmd/resume-build, but these hard-coded commands use repo-root-relative paths, so invocation from the script directory resolves to non-existent paths and the benchmark fails before step 1 starts. This makes the new benchmark unusable unless callers happen to run it from the repository root.

Useful? React with 👍 / 👎.

Comment on lines +14 to +16

@claude claude Bot Apr 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The go run paths in measure-memory-dirtying.sh are relative to CWD and only work when invoked from the repo root, but the Usage comment (sudo ./measure-memory-dirtying.sh) implies running from the script's own directory where those paths don't exist. Either update the Usage comment to show repo-root invocation (e.g. sudo ./packages/orchestrator/cmd/resume-build/measure-memory-dirtying.sh) or add a cd $(dirname "$0")/../../.. at the top to make the script location-independent.

Extended reasoning...

The script at packages/orchestrator/cmd/resume-build/measure-memory-dirtying.sh defines:

CREATE_BUILD="go run ./packages/orchestrator/cmd/create-build"
RESUME_BUILD="go run ./packages/orchestrator/cmd/resume-build"

These paths are relative to the current working directory at the time the script is invoked — not relative to the script file itself. The Usage comment says:

sudo ./measure-memory-dirtying.sh [storage-path]

The ./ prefix in ./measure-memory-dirtying.sh conventionally implies the user is already in the script's directory (packages/orchestrator/cmd/resume-build/). If a developer follows that convention literally:

Step-by-step proof of failure:

  1. Developer runs: cd packages/orchestrator/cmd/resume-build && sudo ./measure-memory-dirtying.sh
  2. CWD is now packages/orchestrator/cmd/resume-build/
  3. CREATE_BUILD expands to: go run ./packages/orchestrator/cmd/create-build
  4. Go looks for packages/orchestrator/cmd/resume-build/packages/orchestrator/cmd/create-build — which does not exist
  5. go run fails with a "no Go files" or "cannot find package" error

The RESUME_BUILD variable is equally broken under this scenario: go run ./packages/orchestrator/cmd/resume-build would resolve to packages/orchestrator/cmd/resume-build/packages/orchestrator/cmd/resume-build, which also does not exist.

The script only works correctly when invoked from the repo root (e.g. sudo ./packages/orchestrator/cmd/resume-build/measure-memory-dirtying.sh), but the Usage comment does not document this requirement.

The fix is either: (a) update the Usage comment to require repo-root invocation, or (b) add cd "$(dirname "$0")/../../.." && ... at the top of the script to resolve all paths relative to the repo root regardless of where the caller is located. Option (b) is more robust for a developer utility.

This is a developer-facing benchmark script, not production code, so the severity is nit — it will cause an immediately obvious failure rather than silent data corruption.


BASE_ID="measure-base-$(date +%s)"
echo "=== Step 1: Build base template ==="
$CREATE_BUILD \
-to-build "$BASE_ID" \
-storage "$STORAGE" \
-hugepages \
-v

echo ""
echo "=== Step 2: Immediate pause (baseline — no activity) ==="
LAYER_IDLE="$BASE_ID-idle"
$RESUME_BUILD \
-from-build "$BASE_ID" \
-to-build "$LAYER_IDLE" \
-storage "$STORAGE" \
-pause

echo ""
echo "=== Step 3: Resume + sleep 2s + pause (idle drift) ==="
LAYER_SLEEP="$LAYER_IDLE-sleep2"
$RESUME_BUILD \
-from-build "$LAYER_IDLE" \
-to-build "$LAYER_SLEEP" \
-storage "$STORAGE" \
-cmd-pause "sleep 2"

echo ""
echo "=== Step 4: Resume + sleep 5s + pause (longer idle drift) ==="
LAYER_SLEEP5="$LAYER_SLEEP-sleep5"
$RESUME_BUILD \
-from-build "$LAYER_SLEEP" \
-to-build "$LAYER_SLEEP5" \
-storage "$STORAGE" \
-cmd-pause "sleep 5"

echo ""
echo "=== Step 5: Resume + write files via envd + pause ==="
LAYER_WRITE="$LAYER_SLEEP5-write"
$RESUME_BUILD \
-from-build "$LAYER_SLEEP5" \
-to-build "$LAYER_WRITE" \
-storage "$STORAGE" \
-cmd-pause "dd if=/dev/urandom of=/tmp/testfile bs=1K count=64 2>/dev/null && echo written"

echo ""
echo "=== Step 6: Resume + start process via envd + pause ==="
LAYER_PROC="$LAYER_WRITE-proc"
$RESUME_BUILD \
-from-build "$LAYER_WRITE" \
-to-build "$LAYER_PROC" \
-storage "$STORAGE" \
-cmd-pause "python3 -c 'print(sum(range(10000)))' || echo 'python not available, using echo'; echo done"

echo ""
echo "=== Step 7: Multi-iteration pause benchmark (10x immediate pause) ==="
$RESUME_BUILD \
-from-build "$LAYER_IDLE" \
-storage "$STORAGE" \
-pause \
-iterations 10

echo ""
echo "=== Done ==="
echo "Compare the '📦 Artifacts' memfile/rootfs diff sizes above."
echo "Smaller diffs = fewer dirty pages = faster snapshot restore."
9 changes: 9 additions & 0 deletions packages/orchestrator/pkg/template/build/core/rootfs/files/journald.conf.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{{- /*gotype:github.com/e2b-dev/infra/packages/orchestrator/pkg/template/build/core/rootfs.templateModel*/ -}}
{{ .WriteFile "etc/systemd/journald.conf.d/e2b.conf" 0o644 }}

[Journal]
Storage=none
MaxLevelConsole=warning
MaxLevelKMsg=warning
MaxLevelWall=emerg
ForwardToSyslog=no
6 changes: 5 additions & 1 deletion packages/orchestrator/pkg/template/build/core/rootfs/rootfs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@

keysIter := maps.Keys(actualFiles)
keys := slices.Collect(keysIter)
assert.Len(t, keys, 13)
assert.Len(t, keys, 14)
assert.Equal(t, "e2b.local", actualFiles["etc/hostname"])
assert.Equal(t, "nameserver 8.8.8.8", actualFiles["etc/resolv.conf"])

Expand All @@ -100,6 +100,10 @@
[Service]
WatchdogSec=0`)
assert.Equal(t, disabledContent, actualFiles["etc/systemd/system/systemd-journald.service.d/override.conf"])
assert.Equal(t, disabledContent, actualFiles["etc/systemd/system/systemd-networkd.service.d/override.conf"])

journaldConf := strings.TrimSpace(actualFiles["etc/systemd/journald.conf.d/e2b.conf"])
assert.Contains(t, journaldConf, "Storage=none")
assert.Contains(t, journaldConf, "MaxLevelConsole=warning")
})

Check warning on line 108 in packages/orchestrator/pkg/template/build/core/rootfs/rootfs_test.go

View check run for this annotation

Claude / Claude Code Review

rootfs_test.go: journald config test missing [Journal] header assertion 

The journald conf test only checks `Storage=none` and `MaxLevelConsole=warning`, omitting the `[Journal]` section header and `ForwardToSyslog=no`. Without the `[Journal]` header, systemd silently ignores the entire drop-in file — a regression that accidentally removes it would make `Storage=none` a complete no-op in production while the test still passes. Consider using `assert.Equal` to match the full config content, as done for `disabledContent` elsewhere in this test.
Comment on lines 103 to 108

@claude claude Bot Apr 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The journald conf test only checks Storage=none and MaxLevelConsole=warning, omitting the [Journal] section header and ForwardToSyslog=no. Without the [Journal] header, systemd silently ignores the entire drop-in file — a regression that accidentally removes it would make Storage=none a complete no-op in production while the test still passes. Consider using assert.Equal to match the full config content, as done for disabledContent elsewhere in this test.

Extended reasoning...

The new test assertions cover only two of the five settings produced by journald.conf.tpl: Storage=none and MaxLevelConsole=warning. Three settings go unchecked: [Journal] section header, ForwardToSyslog=no, and MaxLevelKMsg=warning.

The [Journal] section header is the most critical omission. systemd's drop-in parser uses an INI-style format where every directive must appear under a named section header. A .conf file that contains only bare key=value lines with no [Journal] header will be accepted by the parser without error but all its directives will be silently ignored. This means that if a future refactor accidentally removed the [Journal] line from journald.conf.tpl, the entire optimization (Storage=none, ForwardToSyslog=no, etc.) would become a no-op in production — journald would continue writing to disk and dirtying guest memory pages during every pause-resume cycle.

The current template (journald.conf.tpl) does correctly include [Journal] on line 4 and ForwardToSyslog=no on line 8, so production behaviour is correct today. This is purely a test coverage weakness: the assertions are not strict enough to catch a regression that removes the section header.

Step-by-step proof of the regression scenario:

  1. A future change accidentally removes [Journal] from journald.conf.tpl, leaving only bare key=value lines.
  2. The template still renders a file containing Storage=none and MaxLevelConsole=warning as literal strings.
  3. The test calls assert.Contains(t, journaldConf, "Storage=none") — this passes, because the string is present.
  4. The test calls assert.Contains(t, journaldConf, "MaxLevelConsole=warning") — this also passes.
  5. The test suite is green. But the drop-in file has no [Journal] section header.
  6. Inside the guest VM, systemd reads the file and discards all directives because there is no enclosing section. journald falls back to its default Storage=auto behaviour and continues writing to the journal, dirtying memory pages on every pause-resume cycle.

The fix is straightforward: either add assert.Contains(t, journaldConf, "[Journal]") (and for ForwardToSyslog=no), or use assert.Equal to match the full expected content — which is the pattern already established by the disabledContent check immediately above these lines.

}
Loading