Restore backwards compatibility for VirtioNet:ToCmdline() #419
Conversation
`builder.WriteString(fmt.Sprintf(…))` can be written as `fmt.Fprintf(&builder, …)` Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
WalkthroughEmit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
🧹 Nitpick comments (1)
pkg/config/virtio_test.go (1)
263-278: Tests correctly track the new canonicalToCmdLine()output (unixSocketPath=whenVfkitMagicdefaults/on).These updates look consistent with the intended “parse either syntax, emit legacy syntax by default” behavior, and they exercise the roundtrip through
deviceFromCmdLine()well.Optional: consider adding one regression case for
UnixSocketPath + MacAddressto ensureToCmdLine()continues emitting...,unixSocketPath=...,mac=...as expected (since mac emission happens after the switch).Also applies to: 393-406, 407-416, 417-426, 461-470
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/config/virtio.gopkg/config/virtio_test.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: nirs
Repo: crc-org/vfkit PR: 0
File: :0-0
Timestamp: 2025-10-12T15:51:07.560Z
Learning: In the vfkit virtio-net code (pkg/vf/virtionet.go), the connectUnixPath function change was about closing the connection after duplication (since conn.File() already duplicates the fd), not about introducing the duplication itself.
📚 Learning: 2025-10-12T15:51:07.560Z
Learnt from: nirs
Repo: crc-org/vfkit PR: 0
File: :0-0
Timestamp: 2025-10-12T15:51:07.560Z
Learning: In the vfkit virtio-net code (pkg/vf/virtionet.go), the connectUnixPath function change was about closing the connection after duplication (since conn.File() already duplicates the fd), not about introducing the duplication itself.
Applied to files:
pkg/config/virtio.gopkg/config/virtio_test.go
📚 Learning: 2025-10-12T15:51:07.560Z
Learnt from: nirs
Repo: crc-org/vfkit PR: 0
File: :0-0
Timestamp: 2025-10-12T15:51:07.560Z
Learning: In Go, calling `conn.File()` on a net.Conn already duplicates the underlying file descriptor. When reviewing changes in virtio-net or similar networking code, if conn.File() is being used, recognize that duplication is already happening - don't characterize it as a new duplication.
Applied to files:
pkg/config/virtio.go
📚 Learning: 2025-09-04T08:44:20.871Z
Learnt from: vyasgun
Repo: crc-org/vfkit PR: 348
File: Makefile:16-18
Timestamp: 2025-09-04T08:44:20.871Z
Learning: In the vfkit project Makefile, the original test target intentionally only ran tests under ./pkg/... and excluded tests in other directories like cmd/. When splitting tests into unit and integration targets, the project maintainers want to preserve this original test scope rather than expanding it.
Applied to files:
pkg/config/virtio_test.go
🔇 Additional comments (2)
pkg/config/virtio.go (2)
483-485: Nice micro-simplification usingfmt.Fprintffor mac emission.
455-478: The backwards-compatibility approach is correct: emittingunixSocketPath=<path>by default preserves compatibility with older vfkit versions. The inline comment accurately describes the "old commandline syntax," though it could be more explicit by naming both syntaxes (unixSocketPath=<path>vs.type=unixgram,path=<path>). The existing test suite validates both syntaxes work correctly during parsing and serialization.
| // that the `vfkit` binary these projects are using is the latest | ||
| // one with support for the new syntax. | ||
| // https://github.com/containers/podman/issues/27873 | ||
| fmt.Fprintf(&builder, ",unixSocketPath=%s", dev.UnixSocketPath) |
There was a problem hiding this comment.
nit: 3199a66#diff-76ecb88d3d9af7dfcd333d56014410f18e891dbdec26c909a586b253b79e67ea was what changed this according to VfkitMagic so may be mention that as part of commit log or comment so we know fmt.Fprintf(&builder, ",unixSocketPath=%s", dev.UnixSocketPath) was there before.
other than that lgtm.
The pkg/config code is used by other projects as a go module to generate the command line to start vfkit. There is no guarantee that the `vfkit` binary these projects are using is the latest one with support for the new syntax. This reverts part of efda81c and 3199a66 which introduced support for `vfkitMagic=on/off` This should fix containers/podman#27873 Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
When go-version-file is used, actions/setup-go will use the exact version specified in go.mod, if go.mod specifies 1.25.0, it will use this and not the latest 1.25.x release. See actions/setup-go#561 This is problematic as 1.25.0 is likely to have unfixed CVEs if there has been subsequent minor releases. Given this caveat, this commit removes the use of go-version-file from our GitHub Actions workflows. Using the latest one should not be a problem. Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>
cfergeau
force-pushed
the
backwards-compat
branch
from
126e7e5 to
69fd7b3
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: praveenkumar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/compile.yml:
- Line 17: The workflow uses go-version: 'stable' which diverges from the
project's declared Go version in go.mod (1.24.2); change the two occurrences of
go-version: 'stable' to use go-version-file: go.mod (or explicitly to '1.24.2')
so CI uses the exact Go version declared in go.mod, or if you intentionally need
a different version add a clear justification in the PR and update go.mod to
match; update both instances in .github/workflows/compile.yml (the go-version
entries on the workflow) accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/compile.ymlpkg/config/virtio.gopkg/config/virtio_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/virtio.go
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: nirs
Repo: crc-org/vfkit PR: 0
File: :0-0
Timestamp: 2025-10-12T15:51:07.560Z
Learning: In the vfkit virtio-net code (pkg/vf/virtionet.go), the connectUnixPath function change was about closing the connection after duplication (since conn.File() already duplicates the fd), not about introducing the duplication itself.
📚 Learning: 2025-10-12T15:51:07.560Z
Learnt from: nirs
Repo: crc-org/vfkit PR: 0
File: :0-0
Timestamp: 2025-10-12T15:51:07.560Z
Learning: In the vfkit virtio-net code (pkg/vf/virtionet.go), the connectUnixPath function change was about closing the connection after duplication (since conn.File() already duplicates the fd), not about introducing the duplication itself.
Applied to files:
pkg/config/virtio_test.go
📚 Learning: 2025-09-04T08:44:20.871Z
Learnt from: vyasgun
Repo: crc-org/vfkit PR: 348
File: Makefile:16-18
Timestamp: 2025-09-04T08:44:20.871Z
Learning: In the vfkit project Makefile, the original test target intentionally only ran tests under ./pkg/... and excluded tests in other directories like cmd/. When splitting tests into unit and integration targets, the project maintainers want to preserve this original test scope rather than expanding it.
Applied to files:
pkg/config/virtio_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (macos-26)
- GitHub Check: lint
- GitHub Check: build (macos-14)
- GitHub Check: build (macos-15-intel)
- GitHub Check: build (macos-15)
🔇 Additional comments (2)
pkg/config/virtio_test.go (2)
30-55: LGTM: Test structure provides thorough validation.The test framework effectively validates both command-line generation and parsing with round-trip consistency checks. The inclusion of
alternateCmdLinefor testing different parameter orderings is a good practice.
277-277: Test expectations correctly reflect the VfkitMagic behavior: confirm the tests cover both output formats.The test changes are correct. When
VfkitMagic=true(default), the command line usesunixSocketPath=<path>format; whenVfkitMagic=false, it uses the oldertype=unixgram,path=<path>,vfkitMagic=offformat. Downstream projects can setVfkitMagic=falseexplicitly if they need to support older vfkit binaries. The tests appropriately validate both code paths.Note: The comment in
virtio.goline 467 saying "Use the old commandline syntax" is misleading—unixSocketPath=is actually the newer, simpler syntax. The verbosetype=unixgram,path=format is the older one. Consider clarifying this comment in the implementation.
| - uses: actions/setup-go@v6 | ||
| with: | ||
| go-version-file: go.mod | ||
| go-version: 'stable' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Go version in go.mod and examine the workflow file
echo "=== Checking Go version in go.mod ==="
if [ -f go.mod ]; then
grep -E "^go " go.mod
else
echo "go.mod not found"
fi
echo ""
echo "=== Examining workflow file around line 17 ==="
if [ -f .github/workflows/compile.yml ]; then
sed -n '10,25p' .github/workflows/compile.yml
else
echo ".github/workflows/compile.yml not found"
fi
echo ""
echo "=== Examining workflow file around line 50 ==="
if [ -f .github/workflows/compile.yml ]; then
sed -n '43,57p' .github/workflows/compile.yml
else
echo ".github/workflows/compile.yml not found"
fiRepository: crc-org/vfkit
Length of output: 929
Address Go version mismatch in workflow.
The project declares go 1.24.2 in go.mod, but the workflow uses go-version: 'stable' (lines 17 and 50). This change:
- Creates version drift: 'stable' will eventually advance beyond 1.24.2
- Reduces reproducibility: CI builds will use different Go versions over time
- Diverges from the declared version: inconsistency between go.mod and CI environment
This workflow change appears unrelated to the PR's stated objectives (VirtioNet backwards compatibility). Either clarify why this version change is necessary, or revert to go-version-file: go.mod to ensure CI tests against the project's declared Go version.
🤖 Prompt for AI Agents
In @.github/workflows/compile.yml at line 17, The workflow uses go-version:
'stable' which diverges from the project's declared Go version in go.mod
(1.24.2); change the two occurrences of go-version: 'stable' to use
go-version-file: go.mod (or explicitly to '1.24.2') so CI uses the exact Go
version declared in go.mod, or if you intentionally need a different version add
a clear justification in the PR and update go.mod to match; update both
instances in .github/workflows/compile.yml (the go-version entries on the
workflow) accordingly.
2 participants
The pkg/config code is used by other projects as a go module to
generate the command line to start vfkit. There is no guarantee
that the
vfkitbinary these projects are using is the latestone with support for the new syntax.
This should fix containers/podman#27873
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.