fix(firecracker): make vsock field a pointer so omitempty drops it without vAccel#814
Open
naman79820 wants to merge 1 commit into
Open
fix(firecracker): make vsock field a pointer so omitempty drops it without vAccel#814naman79820 wants to merge 1 commit into
naman79820 wants to merge 1 commit into
Conversation
The vsock config field was a struct value rather than a pointer, so it was always serialized into the generated Firecracker config even when vAccel was not in use, producing a vsock device with a zero guest CID and an empty socket path. Go's JSON encoder only drops empty pointers, slices and maps, not zero-value structs. Make the field a pointer and populate it only in the vAccel branch, so the device is emitted solely when vsock-based vAccel is requested. Also add a regression test asserting the config omits vsock without vAccel. Fixes: #881 Signed-off-by: naman79820 <naman79820@gmail.com>
✅ Deploy Preview for urunc canceled.
|
Author
|
@cmainas ready for review :)) |
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.
Description
Firecracker's JSON config is generated in
BuildExecCmd. Thevsockdevice field (FirecrackerConfig.VSock) was declared as a struct value but taggedjson:"vsock,omitempty". In Go'sencoding/json,omitemptyhas no effect on struct values (a struct is never considered empty), so the key was always emitted. As a result every non-vAccel Firecracker boot wrote a meaninglessvsock device into the config:
The code's intent is the opposite — the field is guarded by
if args.VAccelType == "vsock"and taggedomitemptyso the section should only appear when vAccel/vsock is in use. Guest CID 0 is reserved (valid CIDs start at 3) and the socket path is empty, so this is an invalid device entry.This mirrors the same class of issue already fixed for the network-interfaces section in #310 (don't emit config that isn't needed).
Fix: make
VSocka pointer (*FirecrackerVSockDev) soomitemptytakes effect, and populate it only in the vAccel branch. A regression test asserts the generated config omitsvsockwithout vAccel and includes it (with the configured guest CID) when vAccel is requested.Related issues
vsockdevice (omitempty no-op on struct field) #811How was this tested?
TestFirecrackerBuildExecCmdVSock(table test). Confirmed it fails onthe current code (generated config contains the empty
vsockdevice) andpasses after the fix true red→green.
make unittest GO=$(which go)all unit packages pass.gofmt -l cmd/ pkg/ internal/clean;go vet ./pkg/unikontainers/hypervisors/clean.golangci-lintv2.9.0 on the changed package 0 issues.go build ./...builds cleanly.LLM usage
Claude Opus 4.8
Checklist
golangci-lintv2.9.0 the versionmake lintuses; Docker was unavailable so I ran the binary directly).make test_ctr,make test_nerdctl,make test_docker,make test_crictl).