Make LXD refresh optional (speed reason) #4

taurus-forever · 2023-01-11T09:36:24Z

Hi,

After migration from 'whywaita/setup-lxd' to 'canonical/setup-lxd' we noticed significant performance slows down (90x times),
see canonical/charmed-mongodb-rock#2 (comment)

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!

> > time snap info lxd | grep "installed" > ... > real 0m0.430s > > > time snap info lxd | grep "^installed" > ... > real 0m0.341s

taurus-forever · 2023-01-11T09:40:22Z

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 :-(

README.md

jnsgruk

LGTM - one minor grammatical error

jnsgruk · 2023-01-11T12:01:40Z

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

barrettj12

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.

.github/workflows/integration.yml

barrettj12 · 2023-01-11T21:48:06Z

README.md

-By default, this action will install LXD from the `latest/stable` snap channel. You can specify a different channel using the `channel` input. (See `snap info lxd` for a list of available channels).
+It will default install LXD from the `latest/stable` snap channel.
+You can specify a different channel using the `channel` input
+(see `snap info lxd` for a list of available channels).


No need to change the text here.

ACK, I will revert it. Old-school... 80 symbols length for the line in a source code. :-)

action.yml

barrettj12 · 2023-01-11T21:53:11Z

action.yml

+    - name: Refresh LXD snap
+      shell: bash
+      run: |
+        if ( "${{ inputs.refresh }}" == "true" ) ; then


I think this check should be on the previous step. If LXD is installed another way, e.g. source, apt repo, then snap info lxd | grep "installed" will return false and we will install the snap anyway.

We should really be checking if LXD is on PATH by running e.g. lxc version.

It is a question how deep should we follow the rabbit hole.
We can declare compatibility with 'ubuntu-latest' as a base only (lxd is installed from snap) or provide support for all kind of installations. Sent a new commit checking all the mentioned scenarios.

barrettj12 · 2023-01-16T05:13:45Z

@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 · 2023-01-16T15:43:17Z

@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 · 2023-01-16T16:00:29Z

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!

barrettj12

I've changed my mind on the approach in #5. I think you're right that we should have an explicit configuration option to disable the snap refresh. We should think it through a bit to make sure we're doing something sensible, and won't have to go back and change it later.

My use case: make sure a certain version of LXD is installed. We do this by specifying a channel input (as per current behaviour)

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

Your use case: snap refresh takes too long, we just want to use the LXD pre-installed on the runners. Great, we can add another input to do this.

uses: canonical/setup-lxd
with:
  refresh: false

Now if we have refresh: false and the LXD snap is pre-installed, don't snap install/refresh anything. refresh defaults to true.

The question is what should happen if we have refresh: false but the LXD snap is not installed. There are a few options and I'm not sure which one is best:

install LXD anyway. This means users don't have a way to disable the snap install, which they might want to do if e.g. they're installing LXD some other way.
don't install LXD. This will cause the action to fail unless the user installs LXD another way.
check $PATH/dpkg for LXD, but this quickly goes down a rabbit hole as you say (environment issues, might match a different script called lxd, etc).

I'm probably favouring option 2 cause:

it allows users to disable the snap install if they want to install from source
it makes the logic simpler (if refresh = false, skip the snap install/refresh step)
In this case it is probably more sensible to call the new input install-snap or something, instead of refresh.

The only downside here is it assumes (want to install LXD) == (want to refresh LXD). I'm not sure if a user might want one, but not the other.

README.md

action.yml

jnsgruk · 2023-01-17T08:11:47Z

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

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

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 · 2023-01-17T12:18:22Z

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 · 2023-01-17T13:27:08Z

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 · 2023-01-17T22:11:40Z

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 · 2023-01-18T08:38:39Z

Resolving the PR as the proper fix should be addressed in #5 (as mentioned #4 (comment)). Tnx!

taurus-forever added 2 commits January 11, 2023 10:12

Speedup 'snap info lxp' a bit

e45994e

> > time snap info lxd | grep "installed" > ... > real 0m0.430s > > > time snap info lxd | grep "^installed" > ... > real 0m0.341s

Make LXD upgrade optional

6dac6d8

taurus-forever mentioned this pull request Jan 11, 2023

Add Firewall Rules to Fix LXD in CI canonical/charmed-mongodb-rock#2

Merged

taurus-forever added 2 commits January 11, 2023 10:47

Fix juju=3.0 test. ssh-public-keys has been renamed to ssh-keys

5c337f7

Describe refresh option in README file

fcd478c

jnsgruk reviewed Jan 11, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

jnsgruk approved these changes Jan 11, 2023

View reviewed changes

Reverted English typo caused by Grammarly hint.

b11399a

taurus-forever mentioned this pull request Jan 11, 2023

Run zookeeper in rocks from non-root user canonical/charmed-zookeeper-rock#5

Merged

barrettj12 requested changes Jan 11, 2023

View reviewed changes

Merge install/refresh logic and check for already installed LXD.

cd9d65c

taurus-forever added 2 commits January 16, 2023 16:45

Restore original README wording as requested.

ccb2b00

Restore original step name

ad0e2b7

taurus-forever requested review from barrettj12 and jnsgruk and removed request for barrettj12 and jnsgruk January 16, 2023 15:47

barrettj12 reviewed Jan 17, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

action.yml Outdated Show resolved Hide resolved

action.yml Show resolved Hide resolved

English typos fixed

503702f

taurus-forever closed this Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make LXD refresh optional (speed reason) #4

Make LXD refresh optional (speed reason) #4

taurus-forever commented Jan 11, 2023

taurus-forever commented Jan 11, 2023

jnsgruk left a comment

jnsgruk commented Jan 11, 2023

barrettj12 left a comment

barrettj12 Jan 11, 2023

taurus-forever Jan 16, 2023

barrettj12 Jan 11, 2023

barrettj12 Jan 11, 2023

taurus-forever Jan 16, 2023

barrettj12 commented Jan 16, 2023

taurus-forever commented Jan 16, 2023

taurus-forever commented Jan 16, 2023

barrettj12 left a comment

jnsgruk commented Jan 17, 2023 •

edited

Loading

taurus-forever commented Jan 17, 2023 •

edited

Loading

jnsgruk commented Jan 17, 2023

barrettj12 commented Jan 17, 2023

taurus-forever commented Jan 18, 2023

Make LXD refresh optional (speed reason) #4

Make LXD refresh optional (speed reason) #4

Conversation

taurus-forever commented Jan 11, 2023

taurus-forever commented Jan 11, 2023

jnsgruk left a comment

Choose a reason for hiding this comment

jnsgruk commented Jan 11, 2023

barrettj12 left a comment

Choose a reason for hiding this comment

barrettj12 Jan 11, 2023

Choose a reason for hiding this comment

taurus-forever Jan 16, 2023

Choose a reason for hiding this comment

barrettj12 Jan 11, 2023

Choose a reason for hiding this comment

barrettj12 Jan 11, 2023

Choose a reason for hiding this comment

taurus-forever Jan 16, 2023

Choose a reason for hiding this comment

barrettj12 commented Jan 16, 2023

taurus-forever commented Jan 16, 2023

taurus-forever commented Jan 16, 2023

barrettj12 left a comment

Choose a reason for hiding this comment

jnsgruk commented Jan 17, 2023 • edited Loading

taurus-forever commented Jan 17, 2023 • edited Loading

jnsgruk commented Jan 17, 2023

barrettj12 commented Jan 17, 2023

taurus-forever commented Jan 18, 2023

jnsgruk commented Jan 17, 2023 •

edited

Loading

taurus-forever commented Jan 17, 2023 •

edited

Loading