Skip to content

Check LXD version before using it as a backend#473

Merged
dmitry-lyfar merged 1 commit into
mainfrom
chore/check-lxd-version
Sep 5, 2025
Merged

Check LXD version before using it as a backend#473
dmitry-lyfar merged 1 commit into
mainfrom
chore/check-lxd-version

Conversation

@dmitry-lyfar
Copy link
Copy Markdown
Collaborator

@dmitry-lyfar dmitry-lyfar commented Sep 4, 2025

Description

Check LXD version before using it as a backend.

Self-review quick check

  • Make decisions that cost a lot to reverse explicit in the PR description.
  • Avoid nested conditions.
  • Delete dead code and redundant comments.
  • Normalise symmetries by sticking to doing identical things identically.
// one way to handle errors
if err := f(); err != nil {
   ...
}

// one way to handle multiple returns
val, err := f()
if err != nil {
   ...
}
...
  • Check that coupled code elements, files, and directories are adjacent. For example, test data is stored as close as possible to a test.
  • Put variable declaration and initialisation together.
  • Divide large expressions into digestable and self-explanatory ones. Use multiple variables if required.
  • Put a blank line between two logically different chunks of code.
  • Follow the style guide for new error messages.

Docs

  • I have checked and added or updated relevant documentation.
  • I have checked and added or updated relevant release notes.
  • I have included the technical author in the review.

Or:

  • I confirm the PR has no implications for documentation.

@dmitry-lyfar dmitry-lyfar requested a review from Copilot September 4, 2025 00:20
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds LXD version compatibility checking to ensure the workshop daemon only works with LXD versions 6.3 and above. The change prevents the daemon from starting with incompatible LXD versions and provides clear error messages to users.

Key changes:

  • Implements version checking logic that validates LXD server version against minimum requirements (6.3+)
  • Adds comprehensive test coverage for version validation edge cases
  • Integrates version checking into the LXD backend initialization process

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/workshop/lxd/lxd_backend.go Adds version checking functions and integrates them into backend initialization
internal/workshop/lxd/lxd_backend_test.go Comprehensive test cases for version validation logic
internal/workshop/lxd/export_test.go Exports the version checking function for testing
internal/workshop/backend.go Adds new error type for incompatible backend scenarios
tests/main/compatible-backend/ Integration test files to verify LXD compatibility checking
internal/overlord/overlord.go Minor code cleanup removing redundant error check
internal/daemon/daemon.go Refactors error handling to use switch statement for better readability

Comment thread internal/workshop/lxd/lxd_backend.go
@dmitry-lyfar dmitry-lyfar force-pushed the chore/check-lxd-version branch from 88fc317 to 611d495 Compare September 4, 2025 00:25
@dmitry-lyfar dmitry-lyfar marked this pull request as ready for review September 4, 2025 00:29
@dmitry-lyfar dmitry-lyfar requested review from jonathan-conder and removed request for jonathan-conder September 4, 2025 00:29
Copy link
Copy Markdown
Contributor

@jonathan-conder jonathan-conder left a comment

Choose a reason for hiding this comment

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

Couple of minor things, the main thing is I don't think it's safe to refresh to an older version of LXD

Comment thread internal/overlord/overlord.go Outdated
Comment thread internal/workshop/lxd/lxd_backend.go Outdated
Comment thread tests/main/compatible-backend/task.yaml Outdated
@dmitry-lyfar dmitry-lyfar force-pushed the chore/check-lxd-version branch 2 times, most recently from 6455647 to f423919 Compare September 4, 2025 23:00
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 5, 2025

TICS Quality Gate

✔️ Passed

workshop

All conditions passed

See the results in the TICS Viewer

The following files have been checked for this project
  • cmd/workshopd/run.go
  • internal/daemon/daemon.go
  • internal/overlord/overlord.go
  • internal/syscheck/check.go
  • internal/workshop/backend.go
  • internal/workshop/lxd/lxd_backend.go

Automatic-tests / code-coverage / TICS GitHub Action

@dmitry-lyfar dmitry-lyfar force-pushed the chore/check-lxd-version branch 3 times, most recently from 33e9a74 to 2649842 Compare September 5, 2025 01:42
@dmitry-lyfar dmitry-lyfar force-pushed the chore/check-lxd-version branch from 2649842 to 6904f5e Compare September 5, 2025 01:45
Copy link
Copy Markdown
Contributor

@jonathan-conder jonathan-conder left a comment

Choose a reason for hiding this comment

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

Yeah I think this makes sense. My main concern is that being in degraded mode doesn't seem to block GET requests, or prevent the backend from being initialised. I think it works for the purpose of improving error messages for new users, at least.

Comment thread internal/workshop/lxd/lxd_backend.go Outdated
@dmitry-lyfar
Copy link
Copy Markdown
Collaborator Author

@jonathan-conder the only command which does not start with POST and uses the backed is workshop list --global. It is going to return an empty list. Hence, I considered it safe to not touch the original condition on GET so a user can see tasks, changes and warnings.

@dmitry-lyfar dmitry-lyfar reopened this Sep 5, 2025
@dmitry-lyfar dmitry-lyfar force-pushed the chore/check-lxd-version branch from 6904f5e to 899c46a Compare September 5, 2025 04:05
@dmitry-lyfar dmitry-lyfar merged commit 526caf4 into main Sep 5, 2025
14 of 17 checks passed
@dmitry-lyfar dmitry-lyfar deleted the chore/check-lxd-version branch September 5, 2025 05:23
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.

3 participants