Skip to content
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

fix: check if lxd snap is installed #585

Merged
merged 12 commits into from
Jun 21, 2024

Conversation

cmatsuoka
Copy link
Collaborator

Check if the lxd snap is installed by querying the snapd socket
instead of looking for an executable in the path, as lxd can use
auto-installer stubs.

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

Check if the lxd snap is installed by querying the snapd socket
instead of looking for an executable in the path, as lxd can use
auto-installer stubs.

Co-authored-by: Callahan Kovacs <callahan.kovacs@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka cmatsuoka changed the base branch from main to hotfix/1.23.2 June 20, 2024 23:19
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka cmatsuoka requested review from mr-cal and tigarmo June 21, 2024 14:56
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Copy link
Collaborator

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, nice unit test!

logger.debug(f"LXD snap status: {status}")
# snap status can be "installed" or "active" - "installed" revisions
# are filtered from this API call with `select: enabled`
return bool(status == "active")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about retaining the old check as well? It's unlikely to fail but may still be useful:

return bool(status == "active) and shutil.which("lxd") is not None

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dariuszd21 also raised the issue about lxd on arch and nixos not being snap-packaged, we may need a dual check to cover these cases too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, that's a good point.

Does that stubbed LXD exec file on Ubuntu come in as an apt package? Or is that just part of the base image?

I'm wondering if craft-providers could check if LXD is installed as a snap only on Ubuntu with is_ubuntu_like.

Making an API call to LXC may be the most robust check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered this for now by verifying the presence of a non-snap lxd socket in the standard location, but a more robust verification would be always welcome.

@mr-cal
Copy link
Collaborator

mr-cal commented Jun 21, 2024

You'll need to cherry-pick #572 for the failing integration tests

craft_providers/lxd/installer.py Outdated Show resolved Hide resolved
craft_providers/lxd/installer.py Outdated Show resolved Hide resolved
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@cmatsuoka
Copy link
Collaborator Author

Re-requesting reviews because Dariusz raised a concern about a new scenario where non-snap LXD can be installed on the system.

Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
@sergiusens
Copy link
Collaborator

I am a bit conflicted by this as we can also be checking for /var/snap/lxd/common/lxd/unix.socket as it is their documented entrypoint https://github.com/canonical/lxd/blob/65de71d1fd90e71d84ad9bc238e9ea33bd2d1683/doc/howto/images_manage.md?plain=1#L295

@cmatsuoka
Copy link
Collaborator Author

I am a bit conflicted by this as we can also be checking for /var/snap/lxd/common/lxd/unix.socket as it is their documented entrypoint

By asking snapd we can have additional system health assurances (such as that snapd is up and running). Checking the presence of the socket file is what was possible in the non-snap case.

@cmatsuoka cmatsuoka merged commit b087d64 into hotfix/1.23.2 Jun 21, 2024
9 of 13 checks passed
@cmatsuoka cmatsuoka deleted the CRAFT-3038-Improve-the-LXD-init-experience branch June 21, 2024 17:26
cmatsuoka added a commit that referenced this pull request Jun 21, 2024
Check if the lxd snap is installed by querying the snapd socket
instead of looking for an executable in the path, as lxd can use
auto-installer stubs.

Co-authored-by: Callahan Kovacs <callahan.kovacs@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
tigarmo pushed a commit that referenced this pull request Jul 2, 2024
Check if the lxd snap is installed by querying the snapd socket
instead of looking for an executable in the path, as lxd can use
auto-installer stubs.

Co-authored-by: Callahan Kovacs <callahan.kovacs@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
tigarmo pushed a commit that referenced this pull request Jul 3, 2024
Check if the lxd snap is installed by querying the snapd socket
instead of looking for an executable in the path, as lxd can use
auto-installer stubs.

Co-authored-by: Callahan Kovacs <callahan.kovacs@canonical.com>
Signed-off-by: Claudio Matsuoka <claudio.matsuoka@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants