Skip to content

Conversation

@rumpl
Copy link
Member

@rumpl rumpl commented Apr 5, 2023

The #8259 change added a new WithReadonlyTempMount that will force the mount to be readonly.

This change removes the tryReadonlyMounts and uses that function.

Relates to #8259
Related: rumpl/moby#92 and moby/moby#45267

Note: once this PR is good to go I will backport these to 1.6 and 1.7, this is needed for moby's migration to the containerd image store and moby uses 1.6.x

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah added cherry-pick/1.6.x cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Apr 5, 2023
@thaJeztah
Copy link
Member

thaJeztah commented Apr 5, 2023

I added cherry-pick labels for this, as it reduces the risk of code diverging (and the new code should do exactly the same, so should be "safe" to take). Of course that's my $0.02 (but there's still review on backports to decide on such things)

@thaJeztah
Copy link
Member

derp; new code isn't doing the same, but actually fixing the overlay mounts (updated my previous comment)

@thaJeztah thaJeztah added cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch and removed cherry-pick/1.6.x cherry-pick/1.7.x Change to be cherry picked to release/1.7 branch labels Apr 5, 2023
// tryReadonlyMounts is used by the options which are trying to get user/group
// information from container's rootfs. Since the option does read operation
// only, this helper will append ReadOnly mount option to prevent linux kernel
// from syncing whole filesystem in umount syscall.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep the comments here to know why we use readonly mount .... no just remove it

Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants