Skip to content

overlay: use private directory for composefs mounts#2148

Merged
openshift-merge-bot[bot] merged 2 commits intocontainers:mainfrom
giuseppe:make-composefs-mount-dirs-private
Oct 24, 2024
Merged

overlay: use private directory for composefs mounts#2148
openshift-merge-bot[bot] merged 2 commits intocontainers:mainfrom
giuseppe:make-composefs-mount-dirs-private

Conversation

@giuseppe
Copy link
Member

similarly to what we are doing for the merged directory we need to use private directories for the composefs temporary mounts. Otherwise when the id is in an additional store, we attempt to perform the mount without holding an exclusive lock there.

Could help with containers/container-libs#124. Can't confirm as I wasn't able to find a reproducer yet.

@cgwalters PTAL, another fix for composefs

@rhatdan
Copy link
Member

rhatdan commented Oct 23, 2024

LGTM

Copy link
Contributor

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

I can believe the patch fixes or helps with the reported bug, but I don't know this code well enough to just drop a lgtm myself yet, and I have some suggested cleanups.

@giuseppe giuseppe force-pushed the make-composefs-mount-dirs-private branch from a554144 to 310a64b Compare October 24, 2024 08:23
@cgwalters
Copy link
Contributor

/lgtm

@vrothberg
Copy link
Member

@giuseppe lint is failing

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
similarly to what we are doing for the merged directory we need to use private
directories for the composefs temporary mounts.  Otherwise when the id is in
an additional store, we attempt to perform the mount without holding an
exclusive lock there.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the make-composefs-mount-dirs-private branch from 310a64b to cfd7619 Compare October 24, 2024 12:49
@openshift-ci openshift-ci bot removed the lgtm label Oct 24, 2024
@giuseppe
Copy link
Member Author

rebased, it passes now

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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 11fcb78 into containers:main Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants