Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Charm storage metadata #77
Conversation
wallyworld
reviewed
Nov 18, 2014
| + // Shared indicates that the storage is shared between all units of | ||
| + // a service deployed from the charm. It is an error to attempt to | ||
| + // assign non-shareable storage to a "shared" storage requirement. | ||
| + Shared bool |
wallyworld
Nov 18, 2014
Owner
I'd personally like to see, for each of these attributes and where applicable, a comment indicating what the default value is if not explicitly specified in the metadata. The defaults are specified in the schema definition elsewhere, but this is where the main doc for the meaning of each attribute is, so someone reading the code may find it useful. YMMV.
|
provisional LGTM pending william's approval |
fwereade
reviewed
Dec 2, 2014
| +) | ||
| + | ||
| +// Storage represents a charm's storage requirement. | ||
| +type Storage struct { |
axw
Dec 2, 2014
Member
Can do, though I'm reluctant to increase the use of bson tags here. I'll whip up a patch tomorrow to duplicate the Meta structure in juju/state.
axw
Dec 5, 2014
Member
I asked Rog about this, and he said that Meta is being used directly in the charm store code. So I went ahead and added bson tags with names to everything.
fwereade
reviewed
Dec 2, 2014
| + // create, in order of most to least preferred. | ||
| + // | ||
| + // Filesystem has no default, and is option. | ||
| + Filesystem []Filesystem `bson:",omitempty"` |
fwereade
Dec 2, 2014
Contributor
hmm, not sure we want bson serialization here, do we? Yaml, yes, because that's the native format for charms -- but we don't want our DB "schema" defined outside state, lest we change it accidentally.
axw
Dec 2, 2014
Member
hmm, not sure we want bson serialization here, do we? Yaml, yes, because that's the native format for charms -- but we don't want our DB "schema" defined outside state, lest we change it accidentally.
I took my cue from the existing Meta structure. state/charm.go already directly uses charm.Meta as a document struct field. This can be changed of course.
fwereade
reviewed
Dec 2, 2014
| + | ||
| + // MkfsOptions is any options to use when creating the filesystem. | ||
| + // MkfsOptions will be passed directly to "mkfs". | ||
| + MkfsOptions []string `bson:",omitempty"` |
fwereade
Dec 2, 2014
Contributor
Not quite convinced about putting "mkfs" or "mount" explicitly into the names here -- windows is a thing :-/. Do these definitely have to be in phase 1?
axw
Dec 2, 2014
Member
I know, I struggled with this a bit. Specifying preferred filesystems is part of the spec, and Mark was quite adamant about Juju not being opinionated in this case. We could do without the options in phase 1 I think, but I don't really see a way to avoid them. Do you have some ideas?
axw
Dec 5, 2014
Member
I've taken them out. This was something suggested by dpb in Mark's spec, but there was never any formal agreement. I think it's a good idea to have it, but we can do it later on.
BTW: I'm not really convinced that having names without mkfs/mount in them would be very useful. The values are not abstract, they're directly tied to those commands. Windows charms are unlikely even to specify a file system type, let alone options.
fwereade
reviewed
Dec 2, 2014
| @@ -141,8 +227,8 @@ func parseStringList(list interface{}) []string { | ||
| } | ||
| slice := list.([]interface{}) | ||
| result := make([]string, 0, len(slice)) | ||
| - for _, cat := range slice { | ||
| - result = append(result, cat.(string)) | ||
| + for _, elem := range slice { |
fwereade
reviewed
Dec 2, 2014
| + "store0": charm.Storage{ | ||
| + Name: "store0", | ||
| + Type: charm.StorageBlock, | ||
| + CountMax: 1, |
axw
Dec 2, 2014
Member
CountMin is 0 because the storage is not marked "required". CountMax should be -1 (unbounded); this test is wrong.
fwereade
reviewed
Dec 2, 2014
| + "store1": charm.Storage{ | ||
| + Name: "store1", | ||
| + Type: charm.StorageFilesystem, | ||
| + CountMax: 1, |
fwereade
reviewed
Dec 2, 2014
| @@ -424,6 +534,138 @@ var ifaceSchema = schema.FieldMap( | ||
| }, | ||
| ) | ||
| +func parseStorage(stores interface{}) map[string]Storage { |
fwereade
Dec 2, 2014
Contributor
I'm wondering whether we can usefully pack some of this code into a schema.Checker? Having something that returns a validated Storage would be really nice -- is there some drawback to that approach that I'm not thinking of?
axw
Dec 3, 2014
Member
I think the main reason for meta.Check() is that we want to check metadata de/serialised to bson in state (see state.State.Charm).
|
@fwereade PTAL. |
axw
added some commits
Oct 29, 2014
|
PTAL. I've updated again to match recent changes to the spec.
I have not yet added minimum size or properties: these can be added later. |
|
LGTM |
added a commit
that referenced
this pull request
Jan 9, 2015
axw commentedNov 17, 2014
We introduce a new "storage" attribute to charm metadata, which describes required and optional storage for a charm. Each may be raw block devices, or file systems. The charm declares the name, type and some restrictions on the storage, and the administrator specifies how to fulfil that storage when deploying the charm.
The metadata format is based on the storage phase 1 scope document:
https://docs.google.com/a/canonical.com/document/d/1-9ZPfdgpkj2R9mBG_tlSclGGyK3tRpMf2L4C37mzYD8/edit#bookmark=id.6abw2ee0ebgm
which is mostly the same as described in the spec: https://docs.google.com/a/canonical.com/document/d/1OhaLiHMoGNFEmDJTiNGMluIlkFtzcYjezC8Yq4nAX3Q/edit
Note that the spec was never signed off, and the scope document is yet to be reviewed. This metadata format is subject to change, and should not be documented until it is finalised. In the short term, this unblocks further work on storage in Juju (which should be uncontroversial).
There are also two new hook kinds defined for notifying units of storage attachment and detachment.