Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Only allocate a zpool that will occupy 90% of the free space on '/' for LXDs storage #6406
Conversation
babbageclunk
approved these changes
Oct 7, 2016
Looks good!
There are lots of other uses of syscall in the codebase, I guess they pre-date that note in the docs.
| @@ -21,6 +21,8 @@ import ( | ||
| "github.com/juju/utils/packaging/manager" | ||
| "github.com/juju/utils/proxy" | ||
| + "syscall" |
babbageclunk
Oct 7, 2016
Member
Should we be using this package? It's deprecated, sort of, in favour of https://godoc.org/golang.org/x/sys/unix according to https://golang.org/pkg/syscall/
If we are using it it should be in the top block of imports.
| @@ -122,19 +124,36 @@ func configureLXDProxies(setter configSetter, proxies proxy.Settings) error { | ||
| return nil | ||
| } | ||
| -var execCommand = exec.Command | ||
| +// df returns the number of free bytes on the file system at the given path | ||
| +var df = func(path string) (uint64, error) { |
hoenirvili
Oct 7, 2016
Contributor
Why not make a simple function and make it consistent with the rest of the code, instead of using this lambda?
| + if err != nil { | ||
| + logger.Errorf("configuring zfs failed - unable to find file system size: %s", err) | ||
| + } | ||
| + freeBytes = freeBytes * 9 / 10 |
babbageclunk
Oct 7, 2016
Member
Maybe call it bytesToUse? Reusing freeBytes makes the bit below read like it's grabbing all the space.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
$$merge$$ |
| + } | ||
| + | ||
| + return uint64(statfs.Bsize) * statfs.Bfree, nil | ||
| +} | ||
| var configureZFS = func() { |
hoenirvili
Oct 7, 2016
Contributor
if you are already touching the code you could also modify this into a function.
|
$$merge$$ |
dooferlad
added some commits
Oct 6, 2016
|
Build failed: Last attempt never reported back |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
| @@ -1,7 +1,7 @@ | ||
| // Copyright 2016 Canonical Ltd. | ||
| // Licensed under the AGPLv3, see LICENCE file for details. | ||
| -// +build go1.3 | ||
| +// +build go1.3, linux |
frobware
Oct 11, 2016
Contributor
If we are excluding windows then use, !windows - that's way more explicit.
dooferlad
Oct 11, 2016
Contributor
While I think that is fair, the only platform that LXD runs on is Linux, so perhaps what I should do is rename initialisation.go --> initialisation_linux.go and have initialisation.go do !linux?
dooferlad
Oct 11, 2016
Contributor
All options are as simple as each other. Going for the Linux only option.
| +) | ||
| + | ||
| +type containerInitialiser struct { | ||
| + series string |
| } | ||
| -// Initialise is specified on the container.Initialiser interface. | ||
| +// Initialise - on Windows this is a NOP |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
dooferlad commentedOct 7, 2016
Fixes https://bugs.launchpad.net/juju/+bug/1617460
Improves https://bugs.launchpad.net/juju/+bug/1626725
Previously Juju would perform 'lxd init' with arguments that told LXD to create a 100GB sparse file for storage. The problem with this is that if the host disk can't deliver on that promise of space then the ZFS driver will panic. This takes the zpool offline and LXD gets stuck.
Juju still doesn't make any decisions about how many containers it can deploy based on resource usage so you can still get stuck with containers pending/down because they ran out of space, but at least we don't have errors from the kernel and you can log into the host machine to debug the problem.