overlay, composefs: use data-only lower layers #1713

giuseppe · 2023-09-13T14:54:43Z

use the new overlay data-only feature to mount the composefs data directory so there is no need for upper layers to create whiteouts to hide payload files.

The feature was added to Linux 6.5.

Signed-off-by: Giuseppe Scrivano [email protected]

drivers/overlay/overlay.go

nalind · 2023-09-13T20:15:39Z

How is whether or not we can use a data-only lower layer determined? Is there a run-time feature test for this?

giuseppe · 2023-09-14T09:04:46Z

How is whether or not we can use a data-only lower layer determined? Is there a run-time feature test for this?

I've added a run-time feature test. Without the data-only lower layers feature we are limited to mount only images with a single layer when using composefs. I don't think it is worth the extra complexity, so composefs is disabled when the feature is not present.

@alexlarsson PTAL

giuseppe · 2023-09-14T09:27:30Z

we also need: #1712

rhatdan · 2023-09-14T19:00:27Z

#1712 is merged

flouthoc · 2023-09-15T09:54:15Z

drivers/overlay/overlay.go

+	}
+	// absLowers is not valid anymore now as we have added composeFsLayers to it, so prevent
+	// its usage.
+	absLowers = nil


linter says: ineffectual assignment about this

giuseppe · 2023-09-15T16:13:37Z

ready for review

rhatdan · 2023-09-17T13:42:52Z

LGTM
@nalind @mtrmac @alexlarsson @flouthoc PTAL

nalind

I'd really like a test that ensures that the driver does the right thing when the feature is available and asked for, even if it has to be skipped in CI in the short-term.

giuseppe · 2023-09-18T19:29:50Z

I'd really like a test that ensures that the driver does the right thing when the feature is available and asked for, even if it has to be skipped in CI in the short-term.

Sure, I can add some tests. Do you think there is anything in particular we should test for? Once composefs is enabled, there should not be any visible difference for the users and the existing tests should still work

nalind · 2023-09-18T19:35:47Z

I'd really like a test that ensures that the driver does the right thing when the feature is available and asked for, even if it has to be skipped in CI in the short-term.

Sure, I can add some tests. Do you think there is anything in particular we should test for? Once composefs is enabled, there should not be any visible difference for the users and the existing tests should still work

Do we have tests that exercise using composefs layers as read-only layers?

giuseppe · 2023-09-18T19:46:29Z

I'd really like a test that ensures that the driver does the right thing when the feature is available and asked for, even if it has to be skipped in CI in the short-term.

Sure, I can add some tests. Do you think there is anything in particular we should test for? Once composefs is enabled, there should not be any visible difference for the users and the existing tests should still work

Do we have tests that exercise using composefs layers as read-only layers?

I don't think it would be any different than the tests we already have, the only difference with composefs is that there will be more 2x lower layers (for each layer there is one overlay layer with the erofs file system and one for the data)

rhatdan · 2023-09-20T10:27:05Z

@mtrmac @nalind PTAL

docs/containers-storage-applydiff-using-staging-dir.md

cmd/containers-storage/diff.go

giuseppe · 2023-09-22T07:30:12Z

thanks, comments addressed in the current version

giuseppe · 2023-09-25T08:50:49Z

can I get another review?

alexlarsson

Minor comments, but looks good to me.

drivers/overlay/check.go

cmd/containers-storage/diff.go

rhatdan · 2023-09-26T13:46:34Z

@alexlarsson @nalind PTANL

alexlarsson

lgtm

openshift-ci · 2023-09-27T10:05:37Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe]

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

rhatdan · 2023-09-27T10:42:18Z

@mtrmac @nalind another review?

if d.options.mountOptions is the empty string, we should not append an empty option. Signed-off-by: Giuseppe Scrivano <[email protected]>

use the new overlay data-only feature to mount the composefs data directory so there is no need for upper layers to create whiteouts to hide payload files. The feature was added to Linux 6.5. Signed-off-by: Giuseppe Scrivano <[email protected]>

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2023-10-02T07:53:35Z

rebased, can you please take another look?

giuseppe · 2023-10-02T20:15:58Z

@nalind could you please take another look?

nalind · 2023-10-02T21:46:04Z

drivers/overlay/mount.go

+		dataOnly := false
+		for _, lowerPath := range lowers {
+			if lowerPath == "" {
+				dataOnly = true


Should this loop be enforcing the "once we see a data-only layer, they should all be data-only layers" requirement that the kernel has?

not sure, if the input is not correct we'd get a similar error from Mount as we do if we call directly the syscall and we are not doing any validation in the mountOverlayFromMain function.

I don't think we need it, but if you prefer to have it, I'll add it

It's not a blocker for me so long as it's something we thought about, and not a "we didn't think of that" that becomes a problem later on.

tests/apply-diff.bats

docs/containers-storage-applydiff-using-staging-dir.md

add a new command to exercise the ApplyDiff from a staging directory. Signed-off-by: Giuseppe Scrivano <[email protected]>

Signed-off-by: Giuseppe Scrivano <[email protected]>

nalind · 2023-10-03T13:19:05Z

LGTM

mtrmac · 2023-10-03T19:01:42Z

/lgtm

(reflecting others’ reviews, really)

openshift-ci bot added do-not-merge/work-in-progress approved labels Sep 13, 2023

giuseppe marked this pull request as ready for review September 13, 2023 15:33

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 13, 2023

giuseppe marked this pull request as draft September 13, 2023 18:32

openshift-ci bot added the do-not-merge/work-in-progress label Sep 13, 2023

giuseppe force-pushed the dataonly-overlay-composefs branch from 19118f6 to 5ff2951 Compare September 13, 2023 19:43

giuseppe marked this pull request as ready for review September 13, 2023 19:44

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 13, 2023

nalind reviewed Sep 13, 2023

View reviewed changes

drivers/overlay/overlay.go Show resolved Hide resolved

giuseppe force-pushed the dataonly-overlay-composefs branch from 5ff2951 to 514dbc0 Compare September 14, 2023 08:59

giuseppe force-pushed the dataonly-overlay-composefs branch from 514dbc0 to b83bd45 Compare September 15, 2023 08:41

flouthoc reviewed Sep 15, 2023

View reviewed changes

giuseppe force-pushed the dataonly-overlay-composefs branch from b83bd45 to 210c31a Compare September 15, 2023 10:44

nalind requested changes Sep 18, 2023

View reviewed changes

giuseppe force-pushed the dataonly-overlay-composefs branch from 25e0478 to fbe981b Compare September 18, 2023 19:21

giuseppe force-pushed the dataonly-overlay-composefs branch from fbe981b to 64284da Compare September 19, 2023 08:48

giuseppe marked this pull request as draft September 19, 2023 10:57

openshift-ci bot added the do-not-merge/work-in-progress label Sep 19, 2023

giuseppe force-pushed the dataonly-overlay-composefs branch from 8803c74 to 027416c Compare September 19, 2023 12:25

giuseppe marked this pull request as ready for review September 19, 2023 12:25

nalind reviewed Sep 21, 2023

View reviewed changes

docs/containers-storage-applydiff-using-staging-dir.md Outdated Show resolved Hide resolved

nalind reviewed Sep 21, 2023

View reviewed changes

cmd/containers-storage/diff.go Outdated Show resolved Hide resolved

giuseppe force-pushed the dataonly-overlay-composefs branch from ebcf14d to 0287143 Compare September 22, 2023 07:29

alexlarsson approved these changes Sep 25, 2023

View reviewed changes

drivers/overlay/check.go Show resolved Hide resolved

cmd/containers-storage/diff.go Outdated Show resolved Hide resolved

giuseppe force-pushed the dataonly-overlay-composefs branch from 0287143 to c34d862 Compare September 25, 2023 11:33

alexlarsson approved these changes Sep 27, 2023

View reviewed changes

giuseppe added 3 commits October 2, 2023 09:53

overlay: do not append empty option

b024e30

if d.options.mountOptions is the empty string, we should not append an empty option. Signed-off-by: Giuseppe Scrivano <[email protected]>

overlay, composefs: include file path in the error

1dd7a55

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the dataonly-overlay-composefs branch from c34d862 to 77fa03d Compare October 2, 2023 07:53

nalind reviewed Oct 2, 2023

View reviewed changes

tests/apply-diff.bats Outdated Show resolved Hide resolved

nalind reviewed Oct 2, 2023

View reviewed changes

docs/containers-storage-applydiff-using-staging-dir.md Show resolved Hide resolved

giuseppe added 2 commits October 3, 2023 09:38

cmd: add applydiff-using-staging-dir

c40dde3

add a new command to exercise the ApplyDiff from a staging directory. Signed-off-by: Giuseppe Scrivano <[email protected]>

test: test create layer from staging directory

42fa4a9

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the dataonly-overlay-composefs branch from 77fa03d to 42fa4a9 Compare October 3, 2023 07:46

openshift-ci bot assigned mtrmac Oct 3, 2023

openshift-ci bot added the lgtm label Oct 3, 2023

rhatdan merged commit 19c3c10 into containers:main Oct 3, 2023
18 checks passed

cgwalters added the area/composefs composefs related changes label Dec 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

overlay, composefs: use data-only lower layers #1713

overlay, composefs: use data-only lower layers #1713

giuseppe commented Sep 13, 2023

nalind commented Sep 13, 2023

giuseppe commented Sep 14, 2023

giuseppe commented Sep 14, 2023

rhatdan commented Sep 14, 2023

flouthoc Sep 15, 2023

giuseppe Sep 15, 2023

giuseppe commented Sep 15, 2023

rhatdan commented Sep 17, 2023

nalind left a comment

giuseppe commented Sep 18, 2023

nalind commented Sep 18, 2023

giuseppe commented Sep 18, 2023

rhatdan commented Sep 20, 2023

giuseppe commented Sep 22, 2023

giuseppe commented Sep 25, 2023

alexlarsson left a comment

rhatdan commented Sep 26, 2023

alexlarsson left a comment

openshift-ci bot commented Sep 27, 2023

rhatdan commented Sep 27, 2023

giuseppe commented Oct 2, 2023

giuseppe commented Oct 2, 2023

nalind Oct 2, 2023

giuseppe Oct 3, 2023

nalind Oct 3, 2023

nalind commented Oct 3, 2023

mtrmac commented Oct 3, 2023

overlay, composefs: use data-only lower layers #1713

overlay, composefs: use data-only lower layers #1713

Conversation

giuseppe commented Sep 13, 2023

nalind commented Sep 13, 2023

giuseppe commented Sep 14, 2023

giuseppe commented Sep 14, 2023

rhatdan commented Sep 14, 2023

flouthoc Sep 15, 2023

Choose a reason for hiding this comment

giuseppe Sep 15, 2023

Choose a reason for hiding this comment

giuseppe commented Sep 15, 2023

rhatdan commented Sep 17, 2023

nalind left a comment

Choose a reason for hiding this comment

giuseppe commented Sep 18, 2023

nalind commented Sep 18, 2023

giuseppe commented Sep 18, 2023

rhatdan commented Sep 20, 2023

giuseppe commented Sep 22, 2023

giuseppe commented Sep 25, 2023

alexlarsson left a comment

Choose a reason for hiding this comment

rhatdan commented Sep 26, 2023

alexlarsson left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 27, 2023

rhatdan commented Sep 27, 2023

giuseppe commented Oct 2, 2023

giuseppe commented Oct 2, 2023

nalind Oct 2, 2023

Choose a reason for hiding this comment

giuseppe Oct 3, 2023

Choose a reason for hiding this comment

nalind Oct 3, 2023

Choose a reason for hiding this comment

nalind commented Oct 3, 2023

mtrmac commented Oct 3, 2023