Store: add ImageDirectory() and ImageRunDirectory() #1807
Conversation
Add ImageDirectory() and ImageRunDirectory(), which return the paths of directories which the caller can use to store image-specific data which will be cleaned up automatically when the image is removed or the system is restarted, respectively. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, nalind, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| middleDir := s.graphDriverName + "-images" | ||
| gipath := filepath.Join(s.GraphRoot(), middleDir, id, "userdata") | ||
| if err := os.MkdirAll(gipath, 0o700); err != nil { | ||
| return "", true, err | ||
| } |
There was a problem hiding this comment.
I’d expect the responsibility for creating these directories to be in the same object that is responsible for deleting them — so I guess inside imageStore, and calling imageStore.datadir instead of hard-coding the structure.
(Applies to both.)
| // ImageDirectory returns a path of a directory which the caller can | ||
| // use to store data, specific to the image, which the library does not | ||
| // directly manage. The directory will be deleted when the image is | ||
| // deleted. |
There was a problem hiding this comment.
In both cases, a small warning that the user is responsible for dealing with concurrency (including concurrency with image deletion, deleting the stored files in unpredictable order) might be valuable.
| } | ||
|
|
||
| middleDir := s.graphDriverName + "-images" | ||
| rcpath := filepath.Join(s.RunRoot(), middleDir, id, "userdata") |
There was a problem hiding this comment.
(Yes the API documents that these directories are not explicitly deleted. A small comment here might help, e.g. for refactoring efforts which don’t naturally end up reading that API documentation.)
| }); done { | ||
| return res, err | ||
| } | ||
| if foundImage { |
There was a problem hiding this comment.
AFAICS this is dead code; done is always set to true on the first iteration of readAllImageStores.
Also this code seems to read all image stores, but in fact only ever reads the first one and terminates; right now that loop could be dropped entirely.
(Applies to both.)
| if store.Exists(id) { | ||
| foundImage = true | ||
| } |
There was a problem hiding this comment.
This is surprising, is this right? Besides foundImage being dead, as noted elsewhere, the effect is that for images in an additional store, we create these data directories in the primary store which doesn’t store the image at all.
That might be right (assuming the additional stores are read-only and we simply can’t write them to additional stores — I don’t know what the use case is and whether we need to write this data to read-only images), but then we probably need some new image cleanup code to remove this data in the otherwise-unrelated store when the image is being deleted.
If that was not the intent, I’d expect this to look more like the image part of store.Metadata:
if res, done, err := readAllImageStores … {
if store.Exists(id) {
res, err := …
return res, true, err
}
return N/A, false, nil
}; done {
return res, err
}
return "", “not found”(Applies to both.)
| // directly manage. The directory will be deleted when the host system | ||
| // is restarted. |
There was a problem hiding this comment.
Do we want to commit to this behavior? Or should this allow the directory to be deleted as soon as the image is deleted, even if it isn’t currently implemented?
Add ImageDirectory() and ImageRunDirectory(), which return the paths of directories which the caller can use to store image-specific data which will be cleaned up automatically when the image is removed or the system is restarted, respectively.