-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: finch linux rootless containerd detection #8650
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
robbrad
wants to merge
11
commits into
aws:develop
Choose a base branch
from
robbrad:fix/finch-linux-rootless-containerd-detection
base: develop
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
Show all changes
11 commits
Select commit
Hold shift + click to select a range
2a8375b
fix: Improve Finch socket detection on Linux for rootless containerd
robbrad f8d0c45
test: Update tests for improved Finch socket detection
robbrad 58660b5
fix: Black formatting
robbrad d75e765
fix: Black formatting
robbrad f50eb00
fix: Update socket path comments and logging in platform_config
robbrad a5e143d
Merge branch 'develop' into fix/finch-linux-rootless-containerd-detec…
robbrad dfc9132
fix: Refactor socket path retrieval logic in platform_config.py
robbrad a4e2969
fix: Implement test for finch socket path priority
robbrad c24b7f9
Merge branch 'develop' into fix/finch-linux-rootless-containerd-detec…
robbrad 123ef89
fix: Log warn deprecation
robbrad 02b392b
Merge branch 'develop' into fix/finch-linux-rootless-containerd-detec…
vicheey 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,11 +123,61 @@ def _read_config(self) -> Optional[str]: | |
|
|
||
| def get_finch_socket_path(self) -> Optional[str]: | ||
| """ | ||
| Returns the socket path for Linux. | ||
| """ | ||
|
|
||
| # Default fallback to system socket | ||
| return "unix:///var/run/finch.sock" | ||
| Returns the socket path for Linux, checking multiple locations. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. needs integ test to check builds work too in rootless mode? |
||
|
|
||
| On Linux, Finch can use either a Finch-specific socket or the underlying | ||
| containerd socket via nerdctl. | ||
|
|
||
| Priority order: | ||
| 1. XDG_RUNTIME_DIR/finch.sock (Finch-specific socket) | ||
| 2. ~/.finch/finch.sock (user home directory) | ||
| 3. /var/run/finch.sock (system-wide) | ||
| 4. XDG_RUNTIME_DIR/containerd/containerd.sock (rootless containerd fallback) | ||
|
|
||
| Note: The containerd socket is checked last as a fallback since it may be | ||
| shared by multiple container tools. Finch-specific sockets are preferred | ||
| for accurate telemetry reporting. | ||
|
|
||
| Returns: | ||
| Optional[str]: Socket path if found, None otherwise | ||
| """ | ||
|
|
||
| # Check XDG_RUNTIME_DIR for Finch-specific socket first | ||
| xdg_runtime_dir = os.environ.get("XDG_RUNTIME_DIR") | ||
| if xdg_runtime_dir: | ||
| # Finch-specific socket in XDG_RUNTIME_DIR | ||
| finch_sock = os.path.join(xdg_runtime_dir, "finch.sock") | ||
| if os.path.exists(finch_sock): | ||
| LOG.debug(f"Found Finch socket at XDG_RUNTIME_DIR: {finch_sock}") | ||
| return f"unix://{finch_sock}" | ||
|
|
||
| # Check user home directory for Finch VM socket | ||
| home_dir = os.path.expanduser("~") | ||
| home_finch_sock = os.path.join(home_dir, ".finch", "finch.sock") | ||
| if os.path.exists(home_finch_sock): | ||
| LOG.debug(f"Found Finch socket in home directory: {home_finch_sock}") | ||
| return f"unix://{home_finch_sock}" | ||
|
|
||
| # System-wide socket | ||
| system_sock = "/var/run/finch.sock" | ||
| if os.path.exists(system_sock): | ||
| LOG.debug(f"Found Finch socket at system location: {system_sock}") | ||
| return f"unix://{system_sock}" | ||
|
|
||
| # Fallback: Check for rootless containerd socket | ||
| # This is checked last since containerd may be used by other tools | ||
| if xdg_runtime_dir: | ||
| containerd_sock = os.path.join(xdg_runtime_dir, "containerd", "containerd.sock") | ||
| if os.path.exists(containerd_sock): | ||
| LOG.debug( | ||
| f"Found containerd socket at XDG_RUNTIME_DIR (fallback): {containerd_sock}. " | ||
| "Note: This socket may be shared with other containerd-based tools." | ||
| ) | ||
| return f"unix://{containerd_sock}" | ||
|
|
||
| # No socket found - return None to enable future CLI fallback | ||
| LOG.warning("No Finch socket found in standard locations") | ||
| return None | ||
|
|
||
| def supports_finch(self) -> bool: | ||
| """ | ||
|
|
||
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
Oops, something went wrong.
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.
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.
we should add a basic e2e test with rootless setup to see if it actually works.
Unit test i feel is not sufficient to check system related configs.
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 will also check the networking works as we probably will need to pull some container.
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.
My one concern is that an added integration test for this situation will end up adding a lot of complexity to our testing structure, especially because this is a niche situation that requires a non-trivial amount of changes to the environment. I can discuss this further with the team.