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

Create readonly locks if the lockfile does not exists #1030

Merged
merged 1 commit into from Sep 29, 2021

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Sep 29, 2021

Users are setting up readonly shares and sometimes they forget, or do
not even know that they need to create the lockfile.

Even in the quay.io/podman/stable images Dockerfile, I have to manually
create these files.

Fixes: #1029

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

Users are setting up readonly shares and sometimes they forget, or do
not even know that they need to create the lockfile.

Even in the quay.io/podman/stable images Dockerfile, I have to manually
create these files.

Fixes: containers#1029

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Member Author

rhatdan commented Sep 29, 2021

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM but want a head nod from @nalind

@nalind
Copy link
Member

nalind commented Sep 29, 2021

If the underlying filesystem is mounted read-only, we're not going to be able to create lock files on it, but otherwise, sure.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 29, 2021

Well in that case we will return a more clear error.

@flouthoc
Copy link
Collaborator

LGTM , I might be completely wrong but since we are trying to create file in a readonly fs so i doubt if this solves the reported issue since this would just report different error unable to create: permission denied to the user however user wants if imagestore is empty and readonly podman should not try to acquire lock and simply bypass.

@flouthoc
Copy link
Collaborator

I am assuming ro here stands for readonly https://github.com/containers/storage/blob/main/pkg/lockfile/lockfile_unix.go#L38

@flouthoc
Copy link
Collaborator

LGTM, nvm that should be only when underlying FS is readonly.

@umohnani8
Copy link
Member

/lgtm

@rhatdan rhatdan merged commit fdc780e into containers:main Sep 29, 2021
@rhatdan
Copy link
Member Author

rhatdan commented Sep 29, 2021

Right additional stores are only treated as read/only from container/storage point of view. There is nothing that requires them to be actually on read/only file systems.

The create attempt when the file actually does not exist.

@apollo13
Copy link

Looking through the change, does this actually fix the issue? After all the overlay-images won't exist either in an empty store…

@rhatdan
Copy link
Member Author

rhatdan commented Sep 29, 2021

Maybe not, you would want it to create the entire path?

@rhatdan
Copy link
Member Author

rhatdan commented Sep 29, 2021

Ok not fail on additonal stores that return errors.

@apollo13
Copy link

Yeah it would be great if it would simply skip the store if there is nothing in there. Think of it like that: I can mount an empty store in and fill it at a later time for instance… Currently it requires it to have at least one image there (or at least run any other command that fills up that directory & lock structure).

@rhatdan
Copy link
Member Author

rhatdan commented Sep 29, 2021

This patch in Podman worked for me.

I added "/var/lib/shared", to /etc/containers/storage.conf

# mkdir /var/lib/shared 
# ./bin/podman info | grep shared
    overlay.imagestore: /var/lib/shared
#  find /var/lib/shared/
/var/lib/shared/
/var/lib/shared/overlay-images
/var/lib/shared/overlay-images/images.lock

Seems like containers storage creates the /var/lib/shared/overlay-images now, and with my fix it now creates the images.lock file.

@apollo13
Copy link

Ah well it probably did not create overlay-images for me since I mounted the volume ro (I mostly followed your article https://www.redhat.com/sysadmin/image-stores-podman and thought that it seemed like a nice idea to provide a read only cache for commonly used images that clients cannot modify).

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.

Empty additional image stores are not ignored.
6 participants