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

lxd: hotplug mounts #11336

Merged
merged 2 commits into from
Feb 9, 2023
Merged

lxd: hotplug mounts #11336

merged 2 commits into from
Feb 9, 2023

Conversation

brauner
Copy link
Contributor

@brauner brauner commented Feb 7, 2023

For centuries we've worked ourselves to the bone with mount propagation
to hotplug mounts. No more! The new mount api does have proper support
for moving a mount into a container. I've had that on my ToDo for a
while but due to recent events this has been bumped up on my priority
list.

So here it is. A way to simply have LXD prepare a mount with exactly the
options we want and then to hand that fd to the container and the
container can just attach it to the correct place. No more pointless
mount propagation shenaningans.

I would also like to call out that this has some elegant properties. For
example, every filesystem that supports idmapped mounts can be exposed
to a container completely idmapped. The filesystem can be directly
created with the idmap applied. Neither does the filesystem have to
appear anywhere before applying the idmapping nor are any additional
mounts required before applying the idmapping.

Signed-off-by: Christian Brauner (Microsoft) brauner@kernel.org

@brauner brauner force-pushed the master branch 3 times, most recently from 499dbfa to a7fa585 Compare February 7, 2023 16:27
@lxc-jenkins
Copy link

Testsuite passed

@tomponline
Copy link
Member

The static analysis checks are failing on:

Error: lxd/cgo.go:14:8: could not import C (cgo preprocessing failed) (typecheck)
import "C"
^
make: *** [Makefile:267: static-analysis] Error 1
Error: Process completed with exit code 2.

I'm not sure how this was avoided for the other parts of cgo in LXD, but it doesn't normally occur.

So far we failed to correctly handle non-bind mounts. Add the basic
ability.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
@brauner
Copy link
Contributor Author

brauner commented Feb 7, 2023

The static analysis checks are failing on:

Error: lxd/cgo.go:14:8: could not import C (cgo preprocessing failed) (typecheck)
import "C"
^
make: *** [Makefile:267: static-analysis] Error 1
Error: Process completed with exit code 2.

I'm not sure how this was avoided for the other parts of cgo in LXD, but it doesn't normally occur.

I asked @stgraber and he suspected that it's something to do with the go static checker not built with cgo support. I didn't even touch that file so it's weird in the first place.

@tomponline
Copy link
Member

The static analysis checks are failing on:

Error: lxd/cgo.go:14:8: could not import C (cgo preprocessing failed) (typecheck)
import "C"
^
make: *** [Makefile:267: static-analysis] Error 1
Error: Process completed with exit code 2.

I'm not sure how this was avoided for the other parts of cgo in LXD, but it doesn't normally occur.

I asked @stgraber and he suspected that it's something to do with the go static checker not built with cgo support. I didn't even touch that file so it's weird in the first place.

I would have expected the static checker to fail previously if that were the case. As this isn't the only cgo in LXD.
Unless the other places have been ignored somehow. Any ideas @markylaing ?

@brauner
Copy link
Contributor Author

brauner commented Feb 7, 2023

The static analysis checks are failing on:

Error: lxd/cgo.go:14:8: could not import C (cgo preprocessing failed) (typecheck)
import "C"
^
make: *** [Makefile:267: static-analysis] Error 1
Error: Process completed with exit code 2.

I'm not sure how this was avoided for the other parts of cgo in LXD, but it doesn't normally occur.

I asked @stgraber and he suspected that it's something to do with the go static checker not built with cgo support. I didn't even touch that file so it's weird in the first place.

I would have expected the static checker to fail previously if that were the case. As this isn't the only cgo in LXD. Unless the other places have been ignored somehow. Any ideas @markylaing ?

Please see golangci/golangci-lint#3289

@brauner brauner changed the title lxd: elegantly hotplug mounts lxd: hotplug mounts Feb 7, 2023
@lxc-jenkins
Copy link

Testsuite passed

@markylaing
Copy link
Contributor

The static analysis checks are failing on:

Error: lxd/cgo.go:14:8: could not import C (cgo preprocessing failed) (typecheck)
import "C"
^
make: *** [Makefile:267: static-analysis] Error 1
Error: Process completed with exit code 2.

I'm not sure how this was avoided for the other parts of cgo in LXD, but it doesn't normally occur.

I asked @stgraber and he suspected that it's something to do with the go static checker not built with cgo support. I didn't even touch that file so it's weird in the first place.

I would have expected the static checker to fail previously if that were the case. As this isn't the only cgo in LXD. Unless the other places have been ignored somehow. Any ideas @markylaing ?

Please see golangci/golangci-lint#3289

The static analysis is working locally for me with the same version of golangci-lint, so I wonder if this is related to golangci/golangci-lint#1176 and we are missing a C dependency in the action?

@tomponline
Copy link
Member

Yes sounds like this change has introduced an additional build dependency not present in the github runner.

@brauner brauner force-pushed the master branch 3 times, most recently from addef80 to 87b5472 Compare February 8, 2023 18:31
@brauner
Copy link
Contributor Author

brauner commented Feb 8, 2023

I think I fixed this but I'm now getting download errors for some packages:

# get https://proxy.golang.org/github.com/@v/list: 404 Not Found (0.168s)
# get https://proxy.golang.org/github.com/go-swagger/@v/list: 404 Not Found (0.255s)
# get https://proxy.golang.org/github.com/go-swagger/go-swagger/cmd/@v/list: 404 Not Found (0.256s)
# get https://proxy.golang.org/github.com/go-swagger/go-swagger/cmd/swagger/@v/list: 404 Not Found (0.261s)
# get https://proxy.golang.org/github.com/go-swagger/go-swagger/@v/list: 200 OK (0.279s)

For centuries we've worked ourselves to the bone with mount propagation
to hotplug mounts. No more! The new mount api does have proper support
for moving a mount into a container. I've had that on my ToDo for a
while but due to recent events this has been bumped up on my priority
list.

So here it is. A way to simply have LXD prepare a mount with exactly the
option we want and then to hand that fd to the container and the
container can just attach it to the correct place. No more pointless
mount propagation shenaningans.

I would also like to call out that this has some elegant properties. For
example, every filesystem that supports idmapped mounts can be exposed
to a container completely idmapped. The filesystem can be directly
created with the idmap applied. Neitehr does the filesystem have to
appear anywhere before applying the idmapping nor are any additional
mounts required before applying the idmapping.

Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
@brauner
Copy link
Contributor Author

brauner commented Feb 8, 2023

I think I fixed this but I'm now getting download errors for some packages:

# get https://proxy.golang.org/github.com/@v/list: 404 Not Found (0.168s)
# get https://proxy.golang.org/github.com/go-swagger/@v/list: 404 Not Found (0.255s)
# get https://proxy.golang.org/github.com/go-swagger/go-swagger/cmd/@v/list: 404 Not Found (0.256s)
# get https://proxy.golang.org/github.com/go-swagger/go-swagger/cmd/swagger/@v/list: 404 Not Found (0.261s)
# get https://proxy.golang.org/github.com/go-swagger/go-swagger/@v/list: 200 OK (0.279s)

Hmkay, that isn't the failure. The failure is apparently earlier.

@stgraber stgraber merged commit d4cf267 into canonical:master Feb 9, 2023
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.

5 participants