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

[JUJU-974] Detect LXD "not found" errors #13957

Merged
merged 1 commit into from Apr 14, 2022

Conversation

manadart
Copy link
Member

@manadart manadart commented Apr 13, 2022

LXD 5.0.0 introduced this change which breaks not-found error detection in the LXD container manager.

Here, we change the detection to work on a sub-string instead of verbatim "not found".

QA steps

See unit tests.

Documentation changes

None.

Bug reference

https://bugs.launchpad.net/juju/+bug/1968849

just "not found".
Upstream dependency (LXD 5.0) now returns custom errors instead of
os.ErrNotExist.
@manadart manadart changed the title Detects LXD "not found" errors using substring [JUJU-974] Detect LXD "not found" errors Apr 13, 2022
Copy link
Contributor

@arnodel arnodel left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@hmlanigan hmlanigan left a comment

Choose a reason for hiding this comment

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

LGTM, the "already exists" has been taken care of, verified that no other error checking exists for container/LXD. I did check the lxd provider and found some errors, we check on string.Contains, but do not use ToLower as well.

@freyes
Copy link
Contributor

freyes commented Apr 14, 2022

hi, is anything blocking this change?, I'm not able to deploy lxd containers and using an older version of lxd is not possible for me since I need fixes only available in lxd's latest/edge .

Copy link
Member

@jameinel jameinel left a comment

Choose a reason for hiding this comment

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

This isn't the ideal fix (the ideal would be to update our LXD client lib), but it is a very good stopgap for now.

@jameinel
Copy link
Member

$$merge$$

@stgraber
Copy link
Contributor

Yeah, you really really need to find a way to get on a vaguely modern version of the LXD Go client and get out of string parsing for errors. We really cannot guarantee that we won't be making more changes to those strings as we're actively working on improving errors for our users, moving away from generic error strings.

The HTTP return codes are really what you ought to be checking and our client has exposed those since LXD 4.18 (September 2021).

@manadart manadart deleted the 2.9-lxd-error-types branch April 19, 2022 09:33
jujubot added a commit that referenced this pull request Apr 20, 2022
#13964

New code to fix tests:
- state/logs.go InitDbLogsForModel() - issues with getting capped collection info when the database doesn't yet exist.

Conflicts:
- apiserver/errors/errors.go
- apiserver/facades/client/application/application.go
- apiserver/facades/client/charmhub/convert.go
- apiserver/testserver/server.go
- caas/kubernetes/provider/k8s.go
- caas/kubernetes/provider/k8s_test.go
- cmd/juju/application/bind.go
- cmd/juju/application/deployer/bundle.go
- cmd/juju/application/deployer/bundlehandler_test.go
- cmd/juju/application/deployer/charm_test.go
- cmd/juju/application/refresh.go
- worker/apiserver/manifold_test.go
- worker/apiserver/worker.go

Includes:
- #13942
- #13888
- #13940
- #13934
- #13939
- #13948
- #13954
- #13933
- #13949
- #13947
- #13956
- #13957
- #13951
- #13958
- #13946
- #13952
- #13950
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants