store,imagestore: treat graphRoot as rwImageStore to support Delete
#1578
Conversation
flouthoc
force-pushed
the
pull-store
branch
2 times, most recently
from
4102432
to
7eb8e4e
Compare
| @@ -89,9 +89,6 @@ load helpers | |||
| [[ "$output" =~ "Name: $name" ]] | |||
| [[ "$output" =~ "Data: random1" ]] | |||
| [[ "$output" =~ "Data: random2" ]] | |||
| # Since this image is being read from the readonly graph root | |||
| # so it must show that | |||
| [[ "$output" =~ "Read Only: true" ]] | |||
There was a problem hiding this comment.
Could we test it is writeable instead?
There was a problem hiding this comment.
Modified the test to verify if we can actually delete image from graphRoot while --image-store is still set.
PR containers#1549 added support for split-filesystem using `--imagestore` and when `--imagestore` is used current `graphRoot` is mounted as an additional store. In such case the `graphRoot` must be mounted as an `rw` additional image store so users can still `Delete` images from the `graphRoot` if they want to . Signed-off-by: Aditya R <arajan@redhat.com>
| if !imageStore.Exists(id) { | ||
| primaryImageStore = false | ||
| continue | ||
| } |
There was a problem hiding this comment.
If we go through this for loop 3 times, and the 2nd iteration we hit the continue , but the third we find it, imageFound will be set to true. Is that valid in that circumstance? (Stretching my store memory greatly).
There was a problem hiding this comment.
Yes that sounds valid use-case, its not an issue :) . Since if we find image even in third iteration its a valid find.
|
LGTM |
When we added splitstore support via `--imagestore` the `graphRoot` was added as an `rw` additionalImageStore so lets do same for the `vfs` driver. See PR for more details: * containers#1549 * containers#1578 Signed-off-by: Aditya R <arajan@redhat.com>
There was a problem hiding this comment.
I don’t understand why this behavior is desirable. The documentation added in #1549 says that graphRoot (only if the driver is overlay, which is a surprising condition, but, fine) is treated as an additional image store. This explicitly does not treat it as an additional image store, but it adds an (undocumented) special case.
If users set options.ImageStore, why would they expect to be able to edit the graphRoot-based store? And why would that work only for deletes, not for tags/untags and the like?
My instinctive response to all this complexity is “let’s not do that without a good reason”, and this PR just says “if they want to”. There might be a good reason but this PR does not document one, at least.
(I would also question why we need the “graphRoot is an automatic additional image store” behavior in the first place — if users want to split the container and image store, fine, let them; but why would we make it easy to use the same store in both modes? That seems to me quite likely to cause confusion as users switch between one and the other configuration. Why do users need to do that?
Overall #1549 is adding very many conditions and to me, that intuitively suggests that. there must be a better way to model the feature in code.
| // If --imagestore was set and current store | ||
| // is `graphRoot` then mount it as a `rw` additional | ||
| // store instead of `readonly` additional store. |
There was a problem hiding this comment.
The code says this already.
It would be much more useful to have a comment saying why this is necessary.
| @@ -676,6 +676,7 @@ type store struct { | |||
| autoNsMinSize uint32 | |||
| autoNsMaxSize uint32 | |||
| imageStore rwImageStore | |||
| rwImageStores []rwImageStore | |||
There was a problem hiding this comment.
If we need to special-case this situation, fine.
This rwImageStores:
- has only zero or one element
- does not directly correspond to any user-visible UI feature
So to me, that’s an a surprising way to record the data. It seems to me to be an unnecessary generalization just complicating the code right now.
At the very least this field needs documentation as to how it relates to imageStore/roImageStores (it looks as if it is some kind of direct sibling to imageStore/roImageStore, which is not the case). And I’d prefer the relevant stores to be somehow directly modeled here — “primary image store” / “graph-root image store” / “additional image stores as configured by user”, or something like that.
To my way of thinking, it’s surprising, inconsistent and hard to follow that the responsibility for options.ImageStore is so diffuse. This store object is responsible for creating the directories based on options.ImageStore, but somehow the individual graph driver is responsible for turning options.ImageStore into an entry in AdditionalImageStores, but it must use precisely the s.graphRoot path, or the upper-level store breaks.
This is not at all a clean division of responsibility between the top-level store and the graph driver. I feel rather confident in predicting that it will lead to bugs.
(To be fair, all of the “additional * store” has similar design entanglement between store and the overlay driver; but this is expanding it to a much larger degree.)
Either this whole image store feature is a UI convenience over the existing “additional read-only stores” feature, and in that case it should exist ~entirely in the top-level objects close to the UI. (I think this should be the case, but that’s not true after this PR.) And then the graph driver should be completely unaware of the options.ImageStore option.
Or it is really a cross-cutting feature across the stores and the drivers, and in that case I think it should be explicitly modeled as Go struct fields, and methods that can fail. The top-level store should, at the very least, know whether the graph driver implements the “graphRoot additional image store” (maybe by the driver implementing a method that returns a value about that). And then, again, the store can just instruct the driver to use an additional image store, without the driver explicitly checking options.ImageStore.
It’s a strong “smell” of unclean design to me that store and the drivers (only one of them!), within the same codebase, communicate via fmt.Sprintf("AdditionalImageStore=%s", options.ImageStore). Why use text when we we have Go struct fields?
| // Perform delete image on primary imageStore but if image | ||
| // is not found on primary store then try delete on any additional | ||
| // write-able image store. |
There was a problem hiding this comment.
This comment would, typically, also be of the “code says it already” kind — except that this one is factually incorrect: the code, as is, deletes the image from all image stores where it is found.
|
I'll open a new PR to address these refactor comments. |
* We must lock store before doing any operation if its not the default imagestore. * Refactor and address some comments from: containers#1578 Signed-off-by: Aditya R <arajan@redhat.com>
* We must lock store before doing any operation if its not the default imagestore. * Refactor and address some comments from: containers#1578 Signed-off-by: Aditya R <arajan@redhat.com>
* We must lock store before doing any operation if its not the default imagestore. * Refactor and address some comments from: containers#1578 Signed-off-by: Aditya R <arajan@redhat.com>
PR #1549 added support for split-filesystem using
--imagestoreand when--imagestoreis used currentgraphRootis mounted as an additional store.In such case the
graphRootmust be mounted as anrwadditional image store so users can stillDeleteimages from thegraphRootif they want to .