Add a configuration file #2
Comments
|
I think that since this is a library, it makes sense to have the config in OCID. The config paramaters could be passed to the storage library instantiation code. |
|
I'd lean that way as well, but I opened the issue so that we could try to figure out how that's supposed to work when OCID's not the only logic that might want to be poking at the layer storage. Expecting them to parse OCID's configuration file to get this information would feel a bit odd. |
|
Yes that true, but wouldn't the other tool have similar config? |
|
Applications utilizing containers/storage will differ in what file format they want to use (toml, yaml, json, etc). We could just provide an API that takes in a config struct and leave it up to the application to populate the config struct which should be relatively simple glue code using the appropriate parsing library for the format they use. // containers/storage
type StorageConfig struct {
type string,
rootDir string,
...
}
// application
func parseConfig(path string) (*StorageConfig, error) {
...
}
err, config := parseConfig("/path/to/config") // parseConfig could be left to the application
st := storage.New(config) // This API could be in containers/storage |
|
I'd be wary that this would lead to requiring the same information to be written to multiple places by something like docker-storage-setup, in order to get OCID and, for example, skopeo on the same page, but it's not a deal-breaker. The current MakeStore() function accepts its configuration as multiple arguments, but passing in a single structure that can have fields added, and I suppose tagged to make encoding/decoding easier, is probably better because it would survive things like the addition of UID and GID maps without requiring |
|
I agree lets more to a structure passing and leave the configuration tool to something else. We are going to have to figure out what we can do with @rhvgoyal docker-storage-setup. |
Switch MakeStore() to accepting an options structure instead of individual arguments, so that adding options in the future won't require rewriting logic that calls it, per discussion in issue #2. This also lets the CLI wrapper pick up its defaults from the library, so it no longer has to have them hard-coded. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
|
Okay, moved to have the initialization function take a structure, to hopefully require fewer changes in consumers of the library if/when new settings are added. |
This subtle bug keeps lurking in because error checking for `Mkdir()` and `MkdirAll()` is slightly different wrt `EEXIST`/`IsExist`: - for `Mkdir()`, `IsExist` error should (usually) be ignored (unless you want to make sure directory was not there before) as it means "the destination directory was already there"; - for `MkdirAll()`, `IsExist` error should NEVER be ignored. This commit removes ignoring the IsExist error, as it should not be ignored. For more details, a quote from my opencontainers/runc#162 (July 2015): -quote- TL;DR: check for IsExist(err) after a failed MkdirAll() is both redundant and wrong -- so two reasons to remove it. Quoting MkdirAll documentation: MkdirAll creates a directory named path, along with any necessary parents, and returns nil, or else returns an error. If path is already a directory, MkdirAll does nothing and returns nil. This means two things: If a directory to be created already exists, no error is returned. If the error returned is IsExist (EEXIST), it means there exists a non-directory with the same name as MkdirAll need to use for directory. Example: we want to MkdirAll("a/b"), but file "a" (or "a/b") already exists, so MkdirAll fails. The above is a theory, based on quoted documentation and my UNIX knowledge. In practice, though, current MkdirAll implementation [1] returns ENOTDIR in most of cases described in containers#2, with the exception when there is a race between MkdirAll and someone else creating the last component of MkdirAll argument as a file. In this very case MkdirAll() will indeed return EEXIST. Because of containers#1, IsExist check after MkdirAll is not needed. Because of containers#2 and containers#3, ignoring IsExist error is just plain wrong, as directory we require is not created. It's cleaner to report the error now. Note this error is all over the tree, I guess due to copy-paste, or trying to follow the same usage pattern as for Mkdir(), or some not quite correct examples on the Internet. [1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go -end-quote- Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This subtle bug keeps lurking in because error checking for `Mkdir()` and `MkdirAll()` is slightly different wrt `EEXIST`/`IsExist`: - for `Mkdir()`, `IsExist` error should (usually) be ignored (unless you want to make sure directory was not there before) as it means "the destination directory was already there"; - for `MkdirAll()`, `IsExist` error should NEVER be ignored. This commit removes ignoring the IsExist error, as it should not be ignored. For more details, a quote from my opencontainers/runc#162 (July 2015): -quote- TL;DR: check for IsExist(err) after a failed MkdirAll() is both redundant and wrong -- so two reasons to remove it. Quoting MkdirAll documentation: MkdirAll creates a directory named path, along with any necessary parents, and returns nil, or else returns an error. If path is already a directory, MkdirAll does nothing and returns nil. This means two things: If a directory to be created already exists, no error is returned. If the error returned is IsExist (EEXIST), it means there exists a non-directory with the same name as MkdirAll need to use for directory. Example: we want to MkdirAll("a/b"), but file "a" (or "a/b") already exists, so MkdirAll fails. The above is a theory, based on quoted documentation and my UNIX knowledge. In practice, though, current MkdirAll implementation [1] returns ENOTDIR in most of cases described in containers#2, with the exception when there is a race between MkdirAll and someone else creating the last component of MkdirAll argument as a file. In this very case MkdirAll() will indeed return EEXIST. Because of containers#1, IsExist check after MkdirAll is not needed. Because of containers#2 and containers#3, ignoring IsExist error is just plain wrong, as directory we require is not created. It's cleaner to report the error now. Note this error is all over the tree, I guess due to copy-paste, or trying to follow the same usage pattern as for Mkdir(), or some not quite correct examples on the Internet. [1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go -end-quote- Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Defaults are nice and all, but we probably need a configuration file that we can consult by default to fill in missing information when a calling application doesn't want to care about locations or which driver to use.
A first guess is a TOML file with each section containing a tuple of the startup parameters (root directory, runtime root, driver name, driver options) that it will match against if some of the parameters aren't specified, so that it could fill in the blanks if it was given only the first of those, and we could hard-code a default for just that one value.
This would make it much easier for external code to dictate the settings for a given storage location, since we're not all that interested in setting that policy here, either.
The text was updated successfully, but these errors were encountered: