Remove overlayfs volatile option on temp mounts

[KodieGlosserIBM]

Will address #6406. Which will allow us to use volatile w/overlay mounts.

[k8s-ci-robot]

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

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.

[fuweid]

Would you please add that reference https://docs.kernel.org/filesystems/overlayfs.html#volatile-mount in that comment and add that test in mount package? thanks

NOTE: Currently, containerd uses read-only mount to get uid/gid. But containerd still needs to strip out volatile option on temp mounts.

• https://github.com/containerd/containerd/pull/6478/files
• Add ReadonlyMounts to make overlay mounts readonly #8259

[KodieGlosserIBM]

@fuweid addressed your comments! PTAL

[ktock]

It seems that the function doc comment needs to be updated to add the explanation.

[KodieGlosserIBM]

@ktock what do you mean by function doc comment?

[ktock]

I meant the comment for the function itself:

containerd/mount/temp.go

Lines 29 to 32 in c950537

	// WithTempMount mounts the provided mounts to a temp dir, and pass the temp dir to f.
	// The mounts are valid during the call to the f.
	// Finally we will unmount and remove the temp dir regardless of the result of f.
	func WithTempMount(ctx context.Context, mounts []Mount, f func(root string) error) (err error) {

[fuweid]

Hi @KodieGlosserIBM Please check out @ktock's comment. The comment should be for WithTempMount. And please squash into one commit. Thanks

[KodieGlosserIBM]

done!

[samuelkarp]

/ok-to-test

[KodieGlosserIBM]

@fuweid @samuelkarp anything still needed to push this one through? Thanks!

[dmcgowan]

This is changing the input slice, if the "volatile" option is detected, the original slice and options should be copied to avoid the mount call input slice.

[KodieGlosserIBM]

@dmcgowan I am not sure I follow. The original slice are still copied over. My test here should cover that TestRemoveVolatileTempMount. Maybe I'm missing something?

[dmcgowan]

I should have posted the test update with my comment, sorry. The input slice is never copied.

diff --git a/mount/mount_test.go b/mount/mount_test.go
index 9fc7e0165..078c6c300 100644
--- a/mount/mount_test.go
+++ b/mount/mount_test.go
@@ -205,9 +205,24 @@ func TestRemoveVolatileTempMount(t *testing.T) {
        }

        for _, tc := range testCases {
+               original := copyMounts(tc.input)
                actual := removeVolatileTempMount(tc.input)
                if !reflect.DeepEqual(actual, tc.expected) {
                        t.Fatalf("incorrectly modified mounts: %s.\n\n Expected: %v\n\n, Actual: %v", tc.desc, tc.expected, actual)
                }
+               if !reflect.DeepEqual(original, tc.input) {
+                       t.Fatalf("modified original mounts: %s.\n\n Expected: %v\n\n, Actual: %v", tc.desc, original, tc.input)
+               }
+       }
+}
+
+func copyMounts(in []Mount) []Mount {
+       out := make([]Mount, len(in))
+       for i := range in {
+               out[i] = in[i]
+               if len(out[i].Options) > 0 {
+                       out[i].Options = append([]string{out[i].Options[0]}, out[i].Options[1:]...)
+               }
        }
+       return out
 }

[KodieGlosserIBM]

ahh that makes sense! I've adjusted the code to not modify the original slice! Please take another look. And it looks like I need to rebase so I will do that too :)

[KodieGlosserIBM]

/test pull-containerd-node-e2e

[dmcgowan]

One more optimization here, don't have copyMounts copy the Options slice. Just copy the option slice here when removing the element. Its fine to keep the original string slice until it is modified.

out[i].Options = append(out[i].Options[:j], out[i].Options[j+1:]...)

[KodieGlosserIBM]

Great idea! Made those adjustments. Really appreciate the help you are providing!

[dmcgowan]

This block is no longer necessary