Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add documentation on imagestore and add a warning if set #1695

Merged
merged 1 commit into from Aug 29, 2023

Conversation

kannon92
Copy link
Contributor

Fixes #1694

During testing of imagestore in CRIO, I found this error. Added some documentation and add an error if set to the same value.

@@ -56,10 +56,13 @@ $ restorecon -R -v /NEWSTORAGEPATH
A common use case for this field is to provide a local storage directory when user home directories are NFS-mounted (podman does not support container storage over NFS).

**imagestore**=""
image storage path (default is assumed to be same as `graphroot`).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah when I am reading at to me this string looks unnecessary , imagestore is either used or not and if it is used then path must be different from graphRoot.

I think last line is okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I wanted to follow a similar pattern as other values. The first sentence describes what the field is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, graphroot:

graphroot="" container storage graph dir (default: "/var/lib/containers/storage")

storage.conf Outdated Show resolved Hide resolved
storage.conf-freebsd Outdated Show resolved Hide resolved
Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few nits otherwise LGTM. @giuseppe @rhatdan @nalind PTAL

storage.conf Outdated Show resolved Hide resolved
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of nits picked, otherwise LGTM

Path of imagestore different from `graphroot`, by default storage library stores all images in `graphroot` but if `imagestore` is provided it will store newly pulled images in provided `imagestore` but will keep using `graphroot` for everything else. If user is using `overlay` driver then images which were already part of `graphroot` will still be accessible ( Internally storage library will mount `graphroot` as an `additionalImageStore` to allow this behaviour ).

A common use case for this field is for the users who want to split the file-system in different parts i.e disk which stores images vs disk used by the container created by the image.

Imagestore must be different from `graphroot`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imagestor, if set, must be different from graphroot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

types/options.go Outdated
@@ -179,6 +179,10 @@ func defaultStoreOptionsIsolated(rootless bool, rootlessUID int, storageConf str
storageOpts.RootlessStoragePath = storagePath
}

if storageOpts.ImageStore != "" && storageOpts.ImageStore == storageOpts.GraphRoot {
return storageOpts, fmt.Errorf("imagestore if set must be a different location than graphroot")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is set, o error should be.
return fmt..Errorf("imagestore %s must either be not set or be different then graphroot", storageOpts.ImageStore)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

Signed-off-by: Kevin Hannon <kehannon@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Aug 29, 2023

LGTM
Thanks @kannon92

@rhatdan rhatdan merged commit 42f5e25 into containers:main Aug 29, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ImageStore and GraphRoot cannot be located in the same directory.
4 participants