Make LXD refresh optional (speed reason)

[taurus-forever]

Hi,

After migration from 'whywaita/setup-lxd' to 'canonical/setup-lxd' we noticed significant performance slows down (90x times), see https://github.com/canonical/mongodb-rock/pull/2#issuecomment-1377338493

TL;DR. LXD is already installed on GitHub 'ubuntu-latest' image, so 'whywaita/setup-lxd' did nothing spending ~10 seconds but 'canonical/setup-lxd' is always upgrading LXD which tooks ~15 minutes.

The PR proposes to make 'LXD refresh' optional (in a backward compatible way). Tnx!

[taurus-forever]

The failing test cannot install juju 3.0:

juju (3.0/stable) 3.0.2 from Canonical** installed
error: snap "juju" has no plug named "ssh-public-keys"

which looks unrelated to the PR and no re-trigger button on my side :-(

[jnsgruk]

@barrettj12 - this was initially your work, I'll leave this to you to merge if/when you're happy! Thanks!

[barrettj12]

@taurus-forever: I've opened a new PR #5 which does this using the channel: '' option as I suggested. Would like to get your feedback on this.

[taurus-forever]

@taurus-forever: I've opened a new PR #5 which does this using the channel: '' option as I suggested. Would like to get your feedback on this.

Shared my vision in PR#5, I can close this PR if you as a repo owner would like to go PR#5 direction. Tnx for taking care!

[taurus-forever]

I definitely support the option to disable the snap install/refresh, however I'm not sure if this is the right way to do it. I think your example in the README shows the issue here:

    - name: Setup LXD
      uses: canonical/setup-lxd@[sha]
      with:
        channel: 5.0/stable
        refresh: false

This seems like it should ensure LXD version 5.0.x, however we have set refresh: false, so we could end up with a different LXD version to what we've specified.

This also doesn't handle the case where LXD is installed not using snap, and we want to disable the snap install.

I'd prefer either of the following options:

• The channel is meaningless if we're not doing a snap install/refresh, so we could set the input channel: '' to disable the snap install/refresh. Then we can do a check if [[ -n ${{ inputs.channel }} ]] before the snap install refresh.
• If we want to be more explicit, we could have a new input install-snap which is true by default (or the converse use-existing-lxd, false by default). Then we'd have to document that install-snap: false (resp. use-existing-lxd: true) will disable the snap install/refresh and override any value provided to channel.

Good points. I have refactored the last commits to match your expectations. Please share your comments. Tnx!

[jnsgruk]

FWIW, I think the following approach is probably the simplest:

1. If the user specifies the following:

uses: canonical/setup-lxd

First check if LXD is installed. If it is, move on. If it isn't then run sudo snap install lxd and just install the default track at the time the action runs

2. If the user runs:

uses: canonical/setup-lxd
with:
  channel: 5.2/candidate

Then check if LXD is installed, and either snap install or snap refresh to the version specified.

There is a little bit of magic here, but it means that existing workflows that specify a version can be left alone and will continue to work as expected, and people who are happy to use whatever is installed on the latest Ubuntu Github Actions runner will have a (very simple) way to just get up and running.

While your preferred option is more explicit, the idea of having a setup-lxd action that has a valid/expected code path where it could fail because LXD isn't installed seems like a poor experience to me. Similarly, while explicit values are often better - in this case it would be very easy to specify a version that isn't already installed, and refresh: false which immediately puts it in a sort of non-deterministic state. Perhaps if this action grows more functionality later (unlikely?) we could expand and add more options, but right now there just aren't many outcomes.

[taurus-forever]

2 Jon: "Then check if LXD is installed" is a tricky definition. Installed from snap or whatever?

IMHO: 1) do one thing, but do it right. 2) do not harm.

Currently, the action setup-lxd is about to install LXD from snap only (README):

A GitHub Action which installs and configures the LXD snap on a runner.

The current main branch doesn't support any other LXD sources.

To do not harm: avoid changes if LXD exist from unsupported sources. Consider to add 'force: true' if you want to remove current LXD and force snap version. TODO: support all possible sources. Not a topic for this PR.

Do it right: make sure proper version of LXD from snap is installed (if LXD is installed from snap only).

The question is what should happen if we have refresh: false but the LXD snap is not installed.

It should be installed, as the option called refresh and applicable for refreshing app from snap only.

If you want to conquire the entire world:

uses: canonical/setup-lxd
with:
  source: snap            # (optional) Application source, snap only for the moment. TODO: apt, flatpack, ...
  channel: latest/stable  # (optional) Channel to track (the latest version will be installed if LXD is missing)
  force: true             # (optional) Force installing from $source even if installed from another place.
  refresh: true           # (optional) Force refresh to the latest version from $channel. Supported sources: snap, apt.

... but I would avoid this direction as it looses K.I.S.S.

Let's focus on the reported issue: avoid long update if currently installed LXD is fine for you. I will address all comments, but IMHO it follows 1) + 2) above right now.

[jnsgruk]

Just to clarify: I think we should only consider "checking for install" as "is LXD installed with a snap" -- I don't think we should cater for LXD from other sources. This is a Canonical Github Action, and we only really distribute through snap, and thus we should only be supporting that in this action :)

[barrettj12]

I like @jnsgruk's approach: simple, elegant, and does everything we need to do at the moment. I'll update my PR #5 to match. We can always revisit it if we need to later.

This will break current default behaviour, but this action is in early stages so we make no guarantee of compatibility here. In fact, I'll add a note about that to the README.

[taurus-forever]

Resolving the PR as the proper fix should be addressed in https://github.com/canonical/setup-lxd/pull/5 (as mentioned https://github.com/canonical/setup-lxd/pull/4#issuecomment-1386150693). Tnx!