Add support for overlay volatile option #8402

natefive · 2023-04-17T14:11:00Z

This PR enables volatile overlayfs mounts following the same implementation as #4785

Updates the code inline with surrounding code changes since the PR was originally proposed
Fixes a comment typo 'ommit' to 'omit' as raised on the original PR
Add a SupportsVolatileMount func in overlayutils to test if volatile mounts are available from the NewSnapshotter func

k8s-ci-robot · 2023-04-17T14:11:10Z

Hi @natefive. 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.

Signed-off-by: Nate <[email protected]>

natefive · 2023-05-10T15:25:29Z

any thoughts @fuweid :)?

fuweid · 2023-05-14T04:48:25Z

mount/temp.go

 		}
 	}()
+
+	// The volatile option of overlayfs doesn't allow to mount again using the


I don't think we should change the option here. If there is conflict, we should modify the option before call the WithTempMount, for instance, https://github.com/containerd/containerd/pull/8259/files.

there are lots of calls to WithTempMount? should I create another function which modifies the options calls before calling WithTempMount then update those other calls to call the new function? Feels like pointless nesting maybe I am misunderstanding, would be helpful if that link has some line numbers :) ty

I think this is probably the right place to strip the option. Need to ensure the input mount array is not overwritten though.

@natefive Sorry for late reply.

should I create another function which modifies the options calls before calling WithTempMount then update those other calls to call the new function?

For my original comment, yes. But recently, I checked the code about starting container. It's unlikely to change the mount since there is no entrypoint for us to update it before passing to shim.

containerd/container.go

Lines 244 to 267 in b6abda7

s, err := c.client.getSnapshotter(ctx, r.Snapshotter)

if err != nil {

return nil, err

}

mounts, err := s.Mounts(ctx, r.SnapshotKey)

if err != nil {

return nil, err

}

spec, err := c.Spec(ctx)

if err != nil {

return nil, err

}

for _, m := range mounts {

if spec.Linux != nil && spec.Linux.MountLabel != "" {

if ml := label.FormatMountLabel("", spec.Linux.MountLabel); ml != "" {

m.Options = append(m.Options, ml)

}

}

request.Rootfs = append(request.Rootfs, &types.Mount{

Type: m.Type,

Source: m.Source,

Target: m.Target,

Options: m.Options,

})

I think we should introduce a label like #6528.

Maybe we don't need to persist the label in boltdb. We can pass the label by gRPC context and get mount with the option when we want to get the mount.

cc @dmcgowan

fuweid · 2023-05-14T05:08:25Z

snapshots/overlay/overlay.go

 	}

+	if o.volatile {
+		options = append(options, "volatile")


overlayfs snapshot is service and this option is global setting. I think there are existing cases which required the rootfs is synced after umount. Currently, the kubernetes container is temporary and we can't restart a exited container. kubernetes always recreates container for pod. So, I would like to add the volatile option from caller, for example, CRI plugin.

I was thinking that we can go in this way:

a. When the overlayfs snapshot inits, it use SupportsVolatileMount to check and add the capability in Meta.

containerd/snapshots/overlay/plugin/plugin.go

Lines 41 to 61 in e52fbfd

InitFn: func(ic *plugin.InitContext) (interface{}, error) {

ic.Meta.Platforms = append(ic.Meta.Platforms, platforms.DefaultSpec())

config, ok := ic.Config.(*Config)

if !ok {

return nil, errors.New("invalid overlay configuration")

}

root := ic.Root

if config.RootPath != "" {

root = config.RootPath

}

var oOpts []overlay.Opt

if config.UpperdirLabel {

oOpts = append(oOpts, overlay.WithUpperdirLabel)

}

ic.Meta.Exports["root"] = root

return overlay.NewSnapshotter(root, append(oOpts, overlay.AsynchronousRemove)...)

},

b. Add the mount option to add volatile if snapshot has the capability. Currently, the CRI UserNS feature is working with capability. You can check the main branch for this. And just in case, we should add CRI option to turn this feature on.

mcginne · 2023-10-24T10:29:59Z

Hi folks, do we have any path or outlook on when this could get merged? We see significantly better IO performance on RedHat/CRIO based systems which already have the volatile option support, and would like to get the same on containerd systems.

k8s-ci-robot · 2023-11-29T05:40:14Z

PR needs rebase.

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.

github-actions · 2024-03-10T00:09:49Z

This PR is stale because it has been open 90 days with no activity. This PR will be closed in 7 days unless new comments are made or the stale label is removed.

github-actions · 2024-03-18T00:09:49Z

This PR was closed because it has been stalled for 7 days with no activity.

k8s-ci-robot added the needs-ok-to-test label Apr 17, 2023

natefive mentioned this pull request Apr 17, 2023

Add support for overlay volatile option #4785

Closed

natefive force-pushed the volatile_mounts_global_option branch 2 times, most recently from 3114067 to dfbd2a7 Compare April 18, 2023 10:24

Add support for overlay volatile option

0f42f4d

Signed-off-by: Nate <[email protected]>

natefive force-pushed the volatile_mounts_global_option branch from dfbd2a7 to 0f42f4d Compare April 18, 2023 16:16

natefive changed the title ~~Draft: Add support for overlay volatile option~~ Add support for overlay volatile option Apr 18, 2023

fuweid self-requested a review April 20, 2023 05:28

fuweid requested changes May 14, 2023

View reviewed changes

fuweid mentioned this pull request May 19, 2023

Enable to configure overlayfs volatile mount option within CRI client #8541

Closed

dmcgowan added the status/needs-discussion Needs discussion and decision from maintainers label Jun 6, 2023

k8s-ci-robot added the needs-rebase label Nov 29, 2023

github-actions bot added the Stale label Mar 10, 2024

github-actions bot closed this Mar 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add support for overlay volatile option #8402

Add support for overlay volatile option #8402

Uh oh!

natefive commented Apr 17, 2023 •

edited

Loading

Uh oh!

k8s-ci-robot commented Apr 17, 2023

Uh oh!

natefive commented May 10, 2023

Uh oh!

fuweid May 14, 2023

Uh oh!

natefive Jun 5, 2023

Uh oh!

dmcgowan Jun 11, 2023

Uh oh!

fuweid Jun 25, 2023

Uh oh!

fuweid May 14, 2023

Uh oh!

mcginne commented Oct 24, 2023

Uh oh!

k8s-ci-robot commented Nov 29, 2023

Uh oh!

github-actions bot commented Mar 10, 2024

Uh oh!

github-actions bot commented Mar 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	s, err := c.client.getSnapshotter(ctx, r.Snapshotter)
	if err != nil {
	return nil, err
	}
	mounts, err := s.Mounts(ctx, r.SnapshotKey)
	if err != nil {
	return nil, err
	}
	spec, err := c.Spec(ctx)
	if err != nil {
	return nil, err
	}
	for _, m := range mounts {
	if spec.Linux != nil && spec.Linux.MountLabel != "" {
	if ml := label.FormatMountLabel("", spec.Linux.MountLabel); ml != "" {
	m.Options = append(m.Options, ml)
	}
	}
	request.Rootfs = append(request.Rootfs, &types.Mount{
	Type: m.Type,
	Source: m.Source,
	Target: m.Target,
	Options: m.Options,
	})

	InitFn: func(ic *plugin.InitContext) (interface{}, error) {
	ic.Meta.Platforms = append(ic.Meta.Platforms, platforms.DefaultSpec())

	config, ok := ic.Config.(*Config)
	if !ok {
	return nil, errors.New("invalid overlay configuration")
	}

	root := ic.Root
	if config.RootPath != "" {
	root = config.RootPath
	}

	var oOpts []overlay.Opt
	if config.UpperdirLabel {
	oOpts = append(oOpts, overlay.WithUpperdirLabel)
	}

	ic.Meta.Exports["root"] = root
	return overlay.NewSnapshotter(root, append(oOpts, overlay.AsynchronousRemove)...)
	},

Add support for overlay volatile option #8402

Add support for overlay volatile option #8402

Uh oh!

Conversation

natefive commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 17, 2023

Uh oh!

natefive commented May 10, 2023

Uh oh!

fuweid May 14, 2023

Choose a reason for hiding this comment

Uh oh!

natefive Jun 5, 2023

Choose a reason for hiding this comment

Uh oh!

dmcgowan Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

fuweid Jun 25, 2023

Choose a reason for hiding this comment

Uh oh!

fuweid May 14, 2023

Choose a reason for hiding this comment

Uh oh!

mcginne commented Oct 24, 2023

Uh oh!

k8s-ci-robot commented Nov 29, 2023

Uh oh!

github-actions bot commented Mar 10, 2024

Uh oh!

github-actions bot commented Mar 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

natefive commented Apr 17, 2023 •

edited

Loading