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
On Read/only additional stores, ignore Read/Only errors #1763
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really work when the additional store is truly read-only but fully populated with a lockfile? At the moment we just ignore when trying to create the lockfile.
@@ -961,6 +962,9 @@ func (s *store) load() error { | |||
} else { | |||
ris, err = newROImageStore(gipath) | |||
if err != nil { | |||
if errors.Is(err, syscall.EROFS) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a debug line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added and it seems to when there are images in the additional store and it is mounted read-only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you repush? I don't see the change yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it also make sense to handle ENOENT here? Like, to support configuring an optional directory you can easily create when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wondered about ENOENT. One issue with ignoring ENOENT is detecting misconfiguration. An admin may introduce a typo and be tricked into believing everything's 0K.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the underlying lock code creates missing directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR LGTM Just a general question: c/storage
should be aware of read-only
stores and should not attempt to create lockfiles
at all right ? If its read-only then why is storage
maintaining a lockfile.
It may be readable for this user, but writable for others. |
@vrothberg Ah I meant if other instances of |
We want to setup empty additonal stores which could potentially be stored on a read-only mount point (ostree). Currently empty stores work fine as long as the directory is read/write, because when we attempt to lock them, the code wants to create a lock file directory. This returns an error on read-only directories. This patch ignores read-only addiitonal stores with no content. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
554df46
to
2472af8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, rhatdan, vrothberg 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 |
We want to setup empty additonal stores which could potentially be stored on a read-only mount point (ostree). Currently empty stores work fine as long as the directory is read/write, because when we attempt to lock them, the code wants to create a lock file directory. This returns an error on read-only directories.
This patch ignores read-only addiitonal stores with no content.