[lxd.py] Fixed commands for snap + added missing commands#4217
[lxd.py] Fixed commands for snap + added missing commands#4217rmolkentin wants to merge 1 commit intososreport:mainfrom
Conversation
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
MggMuggins
left a comment
There was a problem hiding this comment.
Hey @rmolkentin - thanks for this. Two thoughts.
- I'm wondering if we can avoid duplicating the list of commands - I see that the SosPredicate is different between the snap and package installs. I wonder if we could declare
lxd_predoutside theif self.is_snapblock (just likelxd_pred = None) and then put theadd_cmd_outputafter the if/else block - What happens, if for some reason,
root'slxcconfiguration has a different remote thanlocalconfigured? My sense is thatsosshouldn't be trying to hit the API of a remote lxd server. I'm not remembering offhand what sos does for openstack (although I think it doesn't hit the API much at all). I think it's reasonable to scrape this data out of the local LXD, but I also think we should pass--remote localfor each command so that we're sure that we're not accidentally scraping information about a remote cluster that the user didn't intend to send (you might consider callingadd_cmd_outputin a loop to avoid writing--remote local20 times, but it's up to you what you think is more readable 🙂 ).
TurboTurtle
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Overall looks fine, just the note below. We can reduce maintenance overhead by only defining the list once, outside the existing if-block.
sos/report/plugins/lxd.py
Outdated
| self.add_cmd_output([ | ||
| "lxc image list", | ||
| "lxc list", | ||
| "lxc network list", | ||
| "lxc profile list", | ||
| "lxc storage list", | ||
| "lxc operation list", | ||
| "lxc info", | ||
| "lxc alias list", | ||
| "lxc config show", | ||
| "lxc remote list", | ||
| "lxc version", | ||
| "lxc warning list", | ||
| "lxc auth permission list", | ||
| "lxc cluster list", | ||
| "lxd cluster list-database" | ||
| ], pred=lxd_pred, snap_cmd=True) |
There was a problem hiding this comment.
As @MggMuggins alluded to, you can pull this block outside the if self.is_snap block, and then only define the list once. You can then use snap_cmd=self.is_snap.
MggMuggins
left a comment
There was a problem hiding this comment.
Looks nice! A few items
sos/report/plugins/lxd.py
Outdated
| "lxc warning list local", | ||
| "lxc auth permission list", | ||
| "lxc cluster list local", | ||
| "lxd cluster list-database local" |
There was a problem hiding this comment.
list-database doesn't exist in LXD 5.21; does it show anything different than cluster list? I'd recommend either just dropping it or making it conditional on the LXD version.
There was a problem hiding this comment.
That's fair enough, removing lxd cluster list-database
sos/report/plugins/lxd.py
Outdated
| def setup(self): | ||
|
|
||
| lxc_cmds = [ | ||
| "lxc image list local", |
There was a problem hiding this comment.
| "lxc image list local", | |
| "lxc image list local:", |
Missing a colon; same thing for all the other ones
There was a problem hiding this comment.
Thanks for your feedback! These commands work with or without the colon for me, is it necessary?
There was a problem hiding this comment.
From the --help output:
Usage:
lxc image list [<remote>:] [<filter>...] [flags]
It uses local as a filter instead of the remote, so you end up with only images with aliases containing local (compare the outputs with and without the filter 🙂 )
There was a problem hiding this comment.
Copy that, adding a colon to needed commands 👍
| "lxc storage list local", | ||
| "lxc operation list", | ||
| "lxc info", | ||
| "lxc alias list", |
There was a problem hiding this comment.
lxc alias list and lxc remote list are client-only so you're good for those ones, but everything else takes a [remote:] argument
TurboTurtle
left a comment
There was a problem hiding this comment.
Code LGTM.
However, please squash your fixup commits into the base commit. You can do this with git rebase -i HEAD~5, then in your editor change pick to f (or fixup) for all commits except for the original first commit, which should remain pick. Save and exit, and everything should be squashed. Then you'll just need to git push origin $your_branch --force
|
Oh, also, please add a DCO line. You can do this with |
6290680 to
097367d
Compare
sos/report/plugins/lxd.py
Outdated
| "lxc operation list local:", | ||
| "lxc info local:", | ||
| "lxc alias list", | ||
| "lxc config show local:", |
There was a problem hiding this comment.
Hey Ryan, sorry for the additional comment, but config show might grab a few secrets that live in the server config:
oidc.client.secretloki.auth.passwordmaas.api.keyandnetwork.ovn.client_keyfrom https://documentation.ubuntu.com/lxd/latest/server/#miscellaneous-options
And unfortunately those aren't necessarily exhaustive if the LXD team decides to add more integrations with more secrets in the future. Does lxd.buginfo (source code) collect the server config (and correctly redact the secrets)? I wonder if they avoided doing that for the same reason...
I think it's probably better to try and get the buginfo script updated than try to keep sos up to date with all the possible config values - could you drop config show from here for now (so we can merge the rest) and then we'll open an issue/PR in LXD?
There was a problem hiding this comment.
Hey, no worries. Looks like lxd.buginfo does collect the config but I'm not sure if it redacts out that information. That's fine with me, I'll drop that command.
The LXD plugin for sos had 2 main issues: 1. It was lacking in troubleshooting commands. For verifying cluster quorum & health and for more information I have since added: - lxc operation list - lxc info - lxc alias list - lxc remote list - lxc version - lxc warning list - lxc auth permission list - lxc cluster list All command use the local: remote. 2. The ‘snap’ section of the plugin was broken and did not capture much of anything except lxd.buginfo and the few lxc commands included only worked on debs. This has been changed to work on both the debian package and snap version of LXD. Signed-off-by: Ryan Molkentin <ryan.molkentin@canonical.com>
I added some missing LXD commands such as:
The ‘snap’ section did not capture any LXD commands, this has been fixed.
Signed-off-by: Ryan Molkentin ryan.molkentin@canonical.com
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines