Skip to content

Conversation

@zouyee
Copy link
Contributor

@zouyee zouyee commented Oct 12, 2021

Add a special handling for readonly overlay mounts from active snapshots that removes the upper directory.

xref:moby/buildkit#1100

fix: #6077

Signed-off-by: Zou Nengren [email protected]

@k8s-ci-robot
Copy link

Hi @zouyee. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@zouyee zouyee force-pushed the overlay branch 2 times, most recently from e5fb63b to 8936eef Compare October 12, 2021 09:17
@theopenlab-ci
Copy link

theopenlab-ci bot commented Oct 12, 2021

Build succeeded.

@thaJeztah
Copy link
Member

@tonistiigi @dmcgowan ptal

@theopenlab-ci
Copy link

theopenlab-ci bot commented Nov 1, 2021

Build succeeded.

@fuweid
Copy link
Member

fuweid commented Nov 1, 2021

For the #6077 , it can be fixed that the running container should be paused and use index=off for the overlayfs mount.

REF: https://github.com/containerd/containerd/blob/main/snapshots/overlay/overlay.go#L77

Copy link
Contributor

@qiutongs qiutongs left a comment

Choose a reason for hiding this comment

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

Please add a unit test

for i, m := range upper {
if m.Type == "overlay" {
upper[i].Options = readonlyOverlay(m.Options)
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

This "continue" seems unnecessary?

for _, o := range opt {
if strings.HasPrefix(o, "upperdir=") {
upper = strings.TrimPrefix(o, "upperdir=")
} else if !strings.HasPrefix(o, "workdir=") {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is basically "index" and "lowerdir"?

if upper != "" {
for i, o := range out {
if strings.HasPrefix(o, "lowerdir=") {
out[i] = "lowerdir=" + upper + ":" + strings.TrimPrefix(o, "lowerdir=")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it simpler to just append the upper instead of prepending?

Choose a reason for hiding this comment

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

Order of the dirs in lowerdir= option does matter. Leftmost dir represents the top layer of the mount. See multiple-lower-layers.

}
}

func readonlyOverlay(opt []string) []string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to add comments or rename this function to something like removeUpperdirForReadonlyOverlay, to be more descriptive.

@laurazard
Copy link
Member

Any news here? I'd be happy to take over it and make the requested changes.

@thaJeztah
Copy link
Member

I'll close this PR because the changes were carried as part of #8259. Thanks for doing the initial work, @zouyee !

@thaJeztah thaJeztah closed this Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Creating diff of a container running on overlayfs generates duplicate mounts

7 participants