Add log rotation for swtpm#5838
Conversation
|
@naiming-zededa could you please review 1e74fd2? I fixed a similar issue for vtpm in this PR, so I added edgeview to this PR too. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5838 +/- ##
==========================================
- Coverage 19.52% 17.07% -2.46%
==========================================
Files 19 478 +459
Lines 3021 85863 +82842
==========================================
+ Hits 590 14659 +14069
- Misses 2310 69688 +67378
- Partials 121 1516 +1395 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
eriknordmark
left a comment
There was a problem hiding this comment.
Run tests.
Please add text to the "How to test and validate this PR" section in the description
It is a bit complicated, I'm working on a easy way to test it, I'll add the instruction today. |
@eriknordmark done. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a FIFO-based log forwarder with size-bounded rotation for swtpm logs, and updates build/test plumbing so the vTPM (pkg/vtpm) unit tests are executed by the repository’s main make test target.
Changes:
- Add a per-instance SWTPM log forwarder that writes FIFO output to
swtpm.logand rotates/compresses when it exceeds 10MB. - Refactor vTPM runtime state tracking from a PID map to an
instancesmap holding PID + log forwarder, and adjust paths. - Add
pkg/vtpmtest target + Docker test stage; wirepkg/vtpm testinto the rootmake testtarget.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/vtpm/swtpm-vtpm/src/main.go | Introduces per-instance log-forwarder lifecycle and refactors instance tracking/paths. |
| pkg/vtpm/swtpm-vtpm/src/fifolog.go | Adds FIFO reader, log writer, rotation + gzip compression implementation. |
| pkg/vtpm/swtpm-vtpm/src/vtpm_test.go | Updates tests for new paths/behavior and adds backup capability gating + VM-attach simulation. |
| pkg/vtpm/Makefile | Adds a docker-based make test harness for vTPM unit tests with coverage output. |
| pkg/vtpm/Dockerfile | Adds a test build stage used by the new vTPM test harness. |
| Makefile | Runs pkg/vtpm tests under the top-level make test target (and also adds an edgeview test invocation). |
Comments suppressed due to low confidence (1)
pkg/vtpm/swtpm-vtpm/src/vtpm_test.go:52
TestMaincallsm.Run()but does not pass its result toos.Exit. With a customTestMain, failing tests won’t reliably propagate a non-zero exit status unlessos.Exit(m.Run())is used.
func TestMain(m *testing.M) {
log = base.NewSourceLogObject(logrus.StandardLogger(), "vtpm", os.Getpid())
maxPidWaitTime = 15
maxInstances = 3
os.MkdirAll(baseDir, 0755)
stateEncryptionKey = baseDir + "/%s.binkey"
stateIsEncryptedPath = baseDir + "/%s.encrypted"
workDir = baseDir + "/tpm-state-%s"
instanceLogFifoPath = baseDir + "/%s.log.fifo"
swtpmCtrlSockPath = baseDir + "/%s.ctrl.sock"
swtpmPidPath = baseDir + "/%s.pid"
vtpmdCtrlSockPath = baseDir + "/vtpmd.ctrl.sock"
client = &http.Client{
Transport: UnixSocketTransport(vtpmdCtrlSockPath),
Timeout: 60 * time.Second,
}
go startServing()
time.Sleep(1 * time.Second)
m.Run()
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@shjala makes sense to review the copilot comments. The one about reading from the fifo seems particularly pertinent. |
Every single copilot review comment, except the typo, was incorrect. |
Add logForwarder to handle swtpm log output through a FIFO interface with automatic rotation and compression when logs exceed 10MB. Signed-off-by: Shahriyar Jalayeri <shahriyar@posteo.de>
Update test setup to work with the new changes. Signed-off-by: Shahriyar Jalayeri <shahriyar@posteo.de>
|
Rebased on master and fixed the merge conflict. |
Make sure vtpm unit tests run as part of make test. Signed-off-by: Shahriyar Jalayeri <shahriyar@posteo.de>
The edgeview package has unit tests but they were never included in the make test target, so CI was not running them. Signed-off-by: Shahriyar Jalayeri <shahriyar@posteo.de>
|
|
||
| func newLogForwarder(fifoPath, logPath string) (*logForwarder, error) { | ||
| // remove stale FIFO from a previous run, if any | ||
| os.Remove(fifoPath) |
There was a problem hiding this comment.
Wouldn't it be easier to just let swtpm write the log to stdout and capture it from there? Then no code is needed to remove, create, ... this file.
There was a problem hiding this comment.
well, too late I guess :D
Description
This PR adds log rotation for swtpm instances and and the necessary build and packaging changes to make sure vtpm unit tests are run in CI.
The swtpm process writes its logs to a FIFO, and a log forwarder reads from that FIFO and writes to a log file on disk. When the log file exceeds 10MB it is automatically rotated and the old copy is compressed with gzip. This prevents swtpm logs from growing without bound on long-running systems. A simpler approach using
logrotatewas considered first but swtpm never resets its file position after rotation, so it would seek to the last offset and leave the new log file with a large gap of zeros. This could still cause issues with the disk monitoring service.The vtpm unit tests were never included in the
make testtarget, so they were never picked up by CI either. Commit 688a02c fixes that.PR dependencies
None
How to test and validate this PR
Manual test
Deploy an Ubuntu VM with a virtual TPM enabled (this is the default).
Inside the VM, generate continuous TPM traffic:
While that loop is running, SSH into the EVE host and inspect the swtpm log directory:
Verify:
swtpm.logexists and is actively being written to (size and timestamp change).swtpm.log.1,swtpm.log.2.gz, etc.).Automated test
There is a self-contained test harness. It builds EVE from a PR branch, and runs the test automatically:
The
vtpm-log-rotationtest:CMD_INIT) using ctrl socket (like what qemu does).CMD_GET_CAPABILITYto generate log output.Changelog notes
Add log rotation for swtpm to prevent unbounded log growth.
PR Backports
Checklist
For backport PRs (remove it if it's not a backport):
And the last but not least:
check them.
Please, check the boxes above after submitting the PR in interactive mode.