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

Ubuntu Pro auto-attachment of guests #13953

Merged
merged 10 commits into from
Sep 23, 2024
Merged

Conversation

markylaing
Copy link
Contributor

@markylaing markylaing commented Aug 20, 2024

This PR implements the LXD side of things for auto-attachment of ubuntu guests:

  • Adds a lxd/ubuntupro package containing a Client which contains an fsmonitor for LXD related configuration.
  • Adds the ubuntupro.Client to Daemon and State.
  • Adds a new config option ubuntu_pro.guest_attach with allowed values on, off, or available.
  • Adds two devlxd endpoints: GET /1.0/ubuntu-pro and POST /1.0/ubuntu-pro/token. The first returns the value of ubuntu_pro.guest_attach (if enabled on the host, otherwise it returns "off"). The second executes a pro command on the host to request a guest token, which is returned to the guest.
  • Adds the devlxd endpoints to the LXD agent.

To use the fsmonitor package for this, I needed to refactor it somewhat. This makes the fsmonitor package much more flexible, but we do need to watch for regressions in unix devices, as I'm not sure if our tests cover all the edge cases here.

See https://docs.google.com/document/d/1MJA1LcTcd1OGHweyiahxAJ-UjrjXKzGtslrNqHnc9uc/edit for more details.

Opening as draft while I conduct more testing.

@markylaing markylaing self-assigned this Aug 20, 2024
@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Aug 20, 2024
Copy link

Heads up @mionaalex - the "Documentation" label was applied to this issue.

@markylaing markylaing force-pushed the pro-attach branch 2 times, most recently from 6e6e757 to ce648d0 Compare August 21, 2024 09:40
@markylaing
Copy link
Contributor Author

@tomponline RE these lint failures: https://github.com/canonical/lxd/actions/runs/10487443271/job/29047794191#step:14:78

They seem to have appeared due to a new version of golangci-lint. It is to do with using non- string format values in a format string. I think govet has now learned to pick these up in api.StatusErrorf.

This means that we'll need to replace things like

api.StatusErrorf(http.StatusBadRequest, http.StatusText(http.StatusBadRequest)

with

api.StatusErrorf(http.StatusBadRequest, "%s", http.StatusText(http.StatusBadRequest)

Which seems a bit redundant. I couldn't see anything in golangci-lint about this change. But this CodeQL section makes sense of it a bit better. I'll make the change.

@tomponline
Copy link
Member

Which seems a bit redundant. I couldn't see anything in golangci-lint about this change. But this CodeQL section makes sense of it a bit better. I'll make the change.

Yeah, it makes sense to change these because, in theory, if there was a '%' character in the string variable passed in, it would be considered a placeholder rather than a literal and cause incorrect output

@markylaing markylaing force-pushed the pro-attach branch 4 times, most recently from ca95e86 to 26e3a79 Compare August 21, 2024 17:20
@markylaing
Copy link
Contributor Author

@tomponline I've marked the JIRA card as blocked as we're waiting for pro API and CLI changes. I've done all I can do on this for now. Feel free to review when you have a moment.

doc/api-extensions.md Outdated Show resolved Hide resolved
lxd/devlxd.go Outdated Show resolved Hide resolved
lxd/ubuntupro/client.go Outdated Show resolved Hide resolved
lxd/ubuntupro/client.go Outdated Show resolved Hide resolved
lxd/ubuntupro/client.go Outdated Show resolved Hide resolved
lxd/ubuntupro/client.go Outdated Show resolved Hide resolved
lxd/ubuntupro/client.go Outdated Show resolved Hide resolved
lxd/ubuntupro/client.go Outdated Show resolved Hide resolved
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

This is looking good overall, thanks!

My two main requests are:

  1. Move client facing API structs used in /dev/lxd responses into shared/api/devlxd.go and rename appropriately to align with existing conventions.
  2. Review lxd-agent fsmonitor and inotify imports and double check whether lxd-agent actually needs them or whether they are being imported accidentally due to a package not being sufficiently separated into server and client roles.

@markylaing
Copy link
Contributor Author

This is looking good overall, thanks!

My two main requests are:

  1. Move client facing API structs used in /dev/lxd responses into shared/api/devlxd.go and rename appropriately to align with existing conventions.

Yep easy fix.

  1. Review lxd-agent fsmonitor and inotify imports and double check whether lxd-agent actually needs them or whether they are being imported accidentally due to a package not being sufficiently separated into server and client roles.

Have done an initial investigation. A huge chunk of imports including fsmonitor are coming from the import of the operations package, which in turn imports state. state brings in lots and lots of imports and it explodes from there.

@tomponline
Copy link
Member

Have done an initial investigation. A huge chunk of imports including fsmonitor are coming from the import of the operations package, which in turn imports state. state brings in lots and lots of imports and it explodes from there.

I wonder if we can decouple either lxd-agent and operations or operations and state?

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
…configuration.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
The agent just needs to proxy these requests via vsock.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
Copy link
Contributor Author

@tomponline this PR now doesn't add any dependencies to the LXD agent, but it is still too large: https://github.com/canonical/lxd/actions/runs/10945290233/job/30389176932?pr=13953#step:16:71

This is from a recent run and we can see there is very little headroom in the check: https://github.com/canonical/lxd/actions/runs/10940393014/job/30372708033#step:16:71

@tomponline
Copy link
Member

@tomponline this PR now doesn't add any dependencies to the LXD agent, but it is still too large: https://github.com/canonical/lxd/actions/runs/10945290233/job/30389176932?pr=13953#step:16:71

This is from a recent run and we can see there is very little headroom in the check: https://github.com/canonical/lxd/actions/runs/10940393014/job/30372708033#step:16:71

Ok lets bump it a little then

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@tomponline tomponline merged commit 75a5d9e into canonical:main Sep 23, 2024
28 of 30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants