fix(core): fall back to podman CLI when no API socket is found#1858
fix(core): fall back to podman CLI when no API socket is found#1858russellb wants to merge 1 commit into
Conversation
|
/ok to test e068662 |
elezar
left a comment
There was a problem hiding this comment.
One question I have: This seems to fix detection, but doesn't change how the gateway interacts with the podman driver? Why are no changes needed there?
It's a fair question. I have podman on mac, but I don't have this problem. I was hoping the reporter would test this and see if it was enough. It seems like a reasonable change on the detection side. We go from "podman not detected at all" to either:
In either case, it's more consistent with docker in this part of the code. Docker does the same thing with falling back to a CLI check at this stage. A next improvement could be to discover the socket using cc @r3v5, reporter of the issue |
Hey @russellb ! Sure, I will test your fix, no worries. |
|
@r3v5 thanks. The output of that podman info command would be helpful too |
|
I have a couple of concerns / questions here. The first is the one that I've already mentioned. This change checks that Then, although this seems to align Podman functionality with Docker, there are subtle differences between the two paths. Although it is a slightly larger change than originally proposed, I think there is some benefit in trying to better align the detection paths for Podman and Docker. Ideally these would return a usable driver config (including, for example socket information) and not just a boolean. This config could then be used directly when instantiating the driver(s) instead of rediscovering the relevant config (as is done in the Docker case). |
|
Thanks, @elezar. I'm happy to work on the changes you described. |
|
I made a change to podman auto-detection recently (#1536), to avoid using just the existence of the CLI to determine that podman was available. In that change I made sure to align the auto-detection with the actual client socket choice mechanism. Podman unfortunately has different behavior on macos depending on how you install it, which I talked about in #1690 (comment) This PR could supersede #1690 (which is scoped to documentation), but it needs to keep the auto-detection mechanism and what the client uses to make the actual connection be aligned, like @elezar has raised. |
|
Great feedback, thanks @krishicks. I'll iterate on this. |
Hey @russellb ! I am coming back with results from local testing on my machine. Tested on macOS (Apple Silicon, M3 Pro RAM 36 GB), Podman 5.7.1 via Homebrew.Detection fix works — gateway now finds podman (Using compute driver driver=podman). Without this PR, it crashes with: I ran Gateway output with the fix: Socket mismatch — after detection, driver construction fails because default_socket_path() returns podman info output |
|
Perfect, thanks. This confirms the non-standard socket location and that discovery needs to include determining socket location to use. |
|
@r3v5 I've pushed follow-up commits that address the socket mismatch you confirmed. Changes:
Precedence for the socket path is: Could you re-test on your Homebrew Podman setup? The retry loop against the missing |
| let driver = configured_compute_driver(config)?; | ||
| info!(driver = %driver, "Using compute driver"); | ||
| warn_if_kubernetes_sandbox_jwt_expiry_disabled(config, driver); | ||
| let detected = configured_compute_driver(config)?; | ||
| info!(driver = %detected.kind(), "Using compute driver"); |
There was a problem hiding this comment.
A nit, but I don't think this driver -> detected rename here is required.
There was a problem hiding this comment.
Reverted — back to driver.
| Some(driver) => Ok(driver), | ||
| Some(detected) => Ok(detected), |
| ComputeDriverKind::Podman => DetectedDriver::Podman { | ||
| socket_path: openshell_driver_podman::PodmanComputeConfig::default_socket_path(), | ||
| }, |
There was a problem hiding this comment.
Why are we using the default path again here?
There was a problem hiding this comment.
Good catch — this would re-introduce the same problem for explicit --drivers podman on Homebrew macOS. Fixed: explicit_driver() now calls detect_podman() to discover the socket first, falling back to default_socket_path() only if discovery fails.
Hey @russellb ! Your fix is working, thanks! 1. No socket at the hardcoded path (confirms the bug) Input: Output: 2. Real socket discovered via podman machine inspect (confirms discovery works) Input: Output: 3. Gateway starts successfully with auto-detected Podman driver (fix complete) Input: Output: |
| /// Result of [`detect_driver`] or an explicitly configured driver, carrying | ||
| /// any driver-specific connection metadata discovered during probing. |
There was a problem hiding this comment.
I'm not a fan of comments that assume usage of a type; the referenced function may be renamed or this may be used in a different way. I'd remove the comment entirely.
| /// VM is never auto-detected but flows through the same path when | ||
| /// explicitly configured via `--drivers vm`. |
There was a problem hiding this comment.
This also assumes usage; this type knows nothing about auto-detection, and shouldn't reference it.
| /// explicitly configured via `--drivers vm`. | ||
| Vm, | ||
| Podman { | ||
| /// API socket path verified during detection. |
There was a problem hiding this comment.
Again, this shouldn't know about auto-detection, so I'd remove this comment.
| /// Result of [`detect_driver`] or an explicitly configured driver, carrying | ||
| /// any driver-specific connection metadata discovered during probing. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum DetectedDriver { |
There was a problem hiding this comment.
This is basically ComputeDriverKind with additional config for the driver, right? It might be better for this to be ComputeDriverConfig, or for ComputeDriverKind to be expanded so that it could be used instead. Then you wouldn't need the DetectedDriver impl below.
d9a1022 to
25e6db7
Compare
| if let Ok(p) = std::env::var("OPENSHELL_PODMAN_SOCKET") { | ||
| podman.socket_path = PathBuf::from(p); | ||
| } else { | ||
| podman.socket_path = socket_path; |
There was a problem hiding this comment.
@russellb If I understand this correctly, we're overriding the entry in the config file for [openshell.drivers.podman].socket_path with an auto discovered socket path and that likely isn't what we want. I'm thinking of a scenario where the user has multiple podman machines and the autodetecting the first but the user wants to point at one of their other ones.
There was a problem hiding this comment.
Good catch — confirmed. The else branch overwrote socket_path with the auto-detected value unconditionally, so an explicit [openshell.drivers.podman].socket_path was always discarded (unless OPENSHELL_PODMAN_SOCKET was set). Multiple-machine users couldn't pin one.
Root cause: socket_path is a plain PathBuf with #[serde(default)], so after deserialization there's no way to tell an explicit value from the filled-in default.
Fixed in ecb5e4f. Precedence is now env var > config-file value > auto-detected/default. A new podman_socket_path_set_in_file helper checks the raw driver table for the socket_path key, and we only fall back to auto-detection when the file didn't set it. Added unit tests and documented the precedence in docs/reference/gateway-config.mdx.
|
This pull request has had no activity for 14 days and is now marked stale. It may be closed in 7 days if there is no further activity. |
|
@russellb @maxamillion Any updates or discussion needed further on this PR? |
|
I was waiting for hear back from @russellb on this one. I thought maybe he was on PTO or something 🙂 |
|
Sorry, just missed the notification. I'm responding and rebasing now. |
ecb5e4f to
2a24776
Compare
|
I'm letting my agents fight over this for a bit. I'll comment again when it's ready for review. |
|
Sounds good @russellb - thanks for picking this one back up! |
Auto-detection previously only checked well-known Podman socket paths, so a Podman machine exposing its API socket at a non-standard location went undetected and the gateway fell back to the default socket path. Add detect_podman(), which probes the well-known socket candidates and then falls back to podman CLI discovery: podman info --format json to determine local vs remote service, and podman machine inspect to resolve the host-side forwarded socket for VM-backed machines. Auto-detection and the Podman driver config both consume the discovered path. Resolve the driver socket_path with clear precedence, highest to lowest: OPENSHELL_PODMAN_SOCKET env var, explicit [openshell.drivers.podman].socket_path in the config file, auto-detected socket, then the built-in default. An explicit config-file value is no longer clobbered by auto-detection, so operators running multiple Podman machines can pin a specific one. Select the machine backing the active Podman connection instead of the first entry from podman machine inspect, honoring podman's connection precedence: CONTAINER_CONNECTION, then CONTAINER_HOST (mapped to a connection by URI), then the containers.conf default. An explicit endpoint that maps to no known machine is left unresolved rather than guessing an unrelated machine. When CONTAINER_HOST is an explicit unix:// socket, use that path directly since podman info connects through it. Document the socket_path precedence in the gateway config reference. Signed-off-by: Russell Bryant <rbryant@redhat.com>
e5b3f19 to
8329698
Compare
I think it's OK now. |
Summary
Auto-detection fails to discover Podman on macOS because the API socket symlink is not always present — it varies by Podman version, machine provider, and platform. This adds a
podman infoCLI fallback so auto-detection succeeds when Podman is functional but the socket isn't at a well-known path.Related Issue
Closes #1834
Changes
podman_cli_responds()fallback tois_podman_available()inopenshell-coreconfigpodman infowhich uses Podman's own internal discoveryTesting
cargo test -p openshell-core— 164 tests passChecklist