-
Notifications
You must be signed in to change notification settings - Fork 42
Switch containers/[image,storage,common] to go.podman.io. #353
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.
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
Open
jankaluza
wants to merge
1
commit into
crc-org:main
Choose a base branch
from
jankaluza:monorepo
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public API break: exported strongunits type changed module path
Switching
strongunits
togo.podman.io/common
changes the type identity ofstrongunits.B
used in the exportedVirtualMachine.Memory
field. Downstream users constructingconfig.VirtualMachine
with the oldgithub.com/containers/common/pkg/strongunits.B
will no longer compile.strongunits
in the public surface (e.g., store memory internally asuint64
bytes; provide constructors/getters likeNewVirtualMachine(vcpus, memoryMiB)
andSetMemoryMiB
), keeping JSON stable.I can draft a minimal refactor to internalize the unit type while preserving behavior and JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment needs a closer look as podman/crc/… are using this code
/hold
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
@cfergeau I'll investigate the downstream usage impact of this change. Let me analyze how this package is consumed and what the actual breakage risk is.
🏁 Script executed:
Length of output: 15455
🏁 Script executed:
Length of output: 708
🏁 Script executed:
Length of output: 54928
🏁 Script executed:
Length of output: 10869
@cfergeau You're absolutely right to flag this for closer examination. This change does introduce a Go API breaking change for downstream consumers. Let me investigate the actual usage patterns to understand the full impact.
🌐 Web query:
💡 Result:
Short answer: vfkit's VirtualMachine.Memory is typed as strongunits.B (a bytes type) and crc passes the configured VM memory (in MiB) into vfkit, which stores/serializes it as that strongunits.B value (i.e., raw bytes). strongunits provides typed byte/size helpers and conversions used by vfkit's config layer.
Sources:
References:
[1] vfkit pkg config (VirtualMachine.Memory = strongunits.B).
[2] vfkit usage (--memory option, memory in MiB).
[3] containers/common/pkg/strongunits (type B represents bytes).
[4] crc Apple Silicon / vfkit docs.
🌐 Web query:
💡 Result:
Use the config package to build a config.VirtualMachine, then call vm.Cmd(vfkitPath) to get an *exec.Cmd to run vfkit. Example (minimal):
Imports:
Example snippet:
package main
import (
"fmt"
"os"
"os/exec"
)
func main() {
// create a Linux bootloader (kernel path, cmdline, optional initrd)
boot := config.NewLinuxBootloader("/path/to/vmlinuz", "console=ttyAMA0", "/path/to/initrd")
}
Notes:
Sources:
[1] crc-org/vfkit GitHub repository (README & usage).
[2] pkg.go.dev: github.com/crc-org/vfkit/pkg/config (VirtualMachine, NewVirtualMachine, Cmd, bootloader helpers).
🌐 Web query:
💡 Result:
Short answer: Yes — Podman 5's rewrite of the podman machine backend introduced breaking changes in machine config / unit handling that can affect how memory is interpreted (causing silent downgrades). See the Podman issue tracking the "silently downgrade memory" problem and the recent Podman v5 breaking-change notes; vfkit is also referenced as the Apple hypervisor provider involved. [1][2][3]
Sources
If you want, I can fetch the specific issue/PR that links vfkit (#297) and the exact commit or workaround steps.
Based on my analysis of the code and web research, I can now provide you with a comprehensive assessment of this breaking change:
@cfergeau You're absolutely correct to be concerned about this change. This is a Go API breaking change that will affect downstream consumers like CRC and Podman. Here's the detailed impact:
Breaking Change Analysis
What's Breaking:
VirtualMachine.Memory
field changes fromgithub.com/containers/common/pkg/strongunits.B
togo.podman.io/common/pkg/strongunits.B
config.VirtualMachine{Memory: strongunits.B(...)}
will break at compile timeWhat's Safe:
memoryBytes
numeric field)config.NewVirtualMachine(vcpus, memoryMiB, bootloader)
constructor should continue workingMemory
field type should be unaffectedDownstream Impact:
github.com/crc-org/vfkit/pkg/config
and working with theVirtualMachine
typeRecommendations
Since this affects multiple downstream projects, I'd suggest:
Would you like me to:
The migration itself is straightforward (update import paths), but the coordination across the ecosystem is the key challenge here.