c8d: Make sure the content isn't removed while we export #45963

rumpl · 2023-07-13T16:54:46Z

- What I did

This change adds leases for all the content that will be exported, once the image(s) are exported the lease is removed, thus letting containerd's GC to do its job if needed. This fixes the case where someone would remove an image that is still being exported.

This fixes the TestAPIImagesSaveAndLoad cli integration test.

- How I did it

Removed the creation of the lease on the context, it didn't do anything, a lease attached to a context is only added to the things that are created during the life of that context
We walk all the content that will be exported and add a lease to it, once finished we remove the lease.

- How to verify it

Run make DOCKER_GRAPHDRIVER=overlayfs TEST_FILTER=TestAPIImagesSaveAndLoad TEST_INTEGRATION_USE_SNAPSHOTTER=1 test-integration, the test should now pass.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

rumpl · 2023-07-13T17:06:00Z

@thaJeztah I see you added the 25.0.0 milestone, which is fine but this change is not essential to the 25 release

daemon/containerd/image_exporter.go

corhere

LGTM!

laurazard

LGTM, just left a couple suggestions/questions

laurazard · 2023-07-18T11:02:31Z

daemon/containerd/image_exporter.go

-	ctx, release, err := i.client.WithLease(ctx)
+	contentStore := i.client.ContentStore()
+	leasesManager := i.client.LeasesService()
+	lease, err := leasesManager.Create(ctx, leases.WithRandomID())


Suggested change

lease, err := leasesManager.Create(ctx, leases.WithRandomID())

lease, err := leasesManager.Create(ctx, leases.WithRandomID(), leases.WithExpiration(1*time.Hour))

For paranoid safety's sake, should we create the lease with an expiration (a la https://github.com/moby/moby/blob/0c6b61665668fa68538a630d341ad56e53817162/daemon/containerd/image_commit.go#L71C67-L71C101) ?

I think our best bet would be to add a cleanup routine at startup that would remove all dangling leases. This should only happen if there is a catastrophic failure and the daemon was killed mid-export. Adding a 1 hour lease would make exports that run more than 1h fail if someone removed the content while we were exporting. Both of the cases should be rare and yeah, a cleanup routine at startup would be best I think.

We could even add a check in all our tests checking that there are no dangling leases.

is there a relation between ctx and "expiration"? i.e., should leases also expire if the context is cancelled or timed out?

There isn't, not in the way you describe it no, would be a nice addition for sure. Containerd has only this:

ctx, release, err := i.client.WithLease(ctx)

In this case when release is called and the lease would be removed. This lease only works for content that is created during the life of that context. In the case of exporting we aren't creating any content, we are only reading them, that's why we need to pre-lease all the content and then safely read it.

I think our best bet would be to add a cleanup routine at startup that would remove all dangling leases.

I'm okay with this, and sounds like a better way to go than arbitrary 1-hour lease. I'll try to write this down to remember to do this somewhere, and when we do that we should go through the rest of the c8d integration code and remove the arbitrary 1-hour expirations in other places (so that an hour long docker commit won't break, even though I hope that's not happening frequently)

We should also make sure we always set options to the WithLease call, by default it will create a 24h lease.

Looks like we have a utility in one place;

moby/daemon/images/image_pull.go

Lines 136 to 150 in 058a6e9

func tempLease(ctx context.Context, mgr leases.Manager) (context.Context, func(context.Context) error, error) {

nop := func(context.Context) error { return nil }

_, ok := leases.FromContext(ctx)

if ok {

return ctx, nop, nil

}

// Use an expiration that ensures the lease is cleaned up at some point if there is a crash, SIGKILL, etc.

opts := []leases.Opt{

leases.WithRandomID(),

leases.WithExpiration(24 * time.Hour),

leases.WithLabels(map[string]string{

"moby.lease/temporary": time.Now().UTC().Format(time.RFC3339Nano),

}),

}

But given that we're wrapping the leases manager in our "namespaced" manager, would it make sense to apply defaults there?

Indeed! Forgot about that one

I opened a issue to track this one #46020

daemon/containerd/image_exporter.go

thaJeztah

one silly question on @laurazard's comment, and I agree with all other comments, but other than that, this LGTM

daemon/containerd/image_exporter.go

-	ctx, release, err := i.client.WithLease(ctx)
+	contentStore := i.client.ContentStore()
+	leasesManager := i.client.LeasesService()
+	lease, err := leasesManager.Create(ctx, leases.WithRandomID())


corhere · 2023-07-18T16:16:04Z

@thaJeztah scoping a lease to a context will be much cheaper to implement with Go 1.21's context.AfterFunc.

I think it's wise for containerd not to implement a WithScopedOnContext option or similar as it would have to be implemented by retaining a reference to the client used to create the lease. Without cooperation by the application there'd be no way to guarantee that the client is still valid (i.e. has not been closed) when the context becomes done.

This change add leases for all the content that will be exported, once the image(s) are exported the lease is removed, thus letting containerd's GC to do its job if needed. This fixes the case where someone would remove an image that is still being exported. This fixes the TestAPIImagesSaveAndLoad cli integration test. Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>

laurazard

re-LGTM

thaJeztah · 2023-07-19T09:52:44Z

as it would have to be implemented by retaining a reference to the client used to create the lease. Without cooperation by the application there'd be no way to guarantee that the client is still valid (i.e. has not been closed) when the context becomes done.

Also fair point. It was a "wild blurb" on my side, mostly considering what the proper / cleanest way would be to create a "transaction" (with some work during), which is expected to last for the duration of the context (possibly with a deadline, but I guess that'd be more for the context), but I definitely haven't given it a lot of thought yet.

thaJeztah

LGTM

rumpl added status/2-code-review process/cherry-pick area/images containerd-integration Issues and PRs related to containerd integration labels Jul 13, 2023

thaJeztah added this to the 25.0.0 milestone Jul 13, 2023

thaJeztah reviewed Jul 13, 2023

View reviewed changes

daemon/containerd/image_exporter.go Outdated Show resolved Hide resolved

thaJeztah reviewed Jul 13, 2023

View reviewed changes

daemon/containerd/image_exporter.go Outdated Show resolved Hide resolved

rumpl force-pushed the c8d-image-save-lease branch from a6f6128 to 2591d72 Compare July 13, 2023 17:14

thaJeztah reviewed Jul 13, 2023

View reviewed changes

daemon/containerd/image_exporter.go Outdated Show resolved Hide resolved

rumpl force-pushed the c8d-image-save-lease branch 2 times, most recently from 1757030 to 4fe7bb3 Compare July 13, 2023 17:51

corhere reviewed Jul 13, 2023

View reviewed changes

daemon/containerd/image_exporter.go Outdated Show resolved Hide resolved

daemon/containerd/image_exporter.go Outdated Show resolved Hide resolved

rumpl force-pushed the c8d-image-save-lease branch from 4fe7bb3 to 54f3f47 Compare July 17, 2023 08:24

vvoland reviewed Jul 17, 2023

View reviewed changes

daemon/containerd/image_exporter.go Outdated Show resolved Hide resolved

daemon/containerd/image_exporter.go Outdated Show resolved Hide resolved

daemon/containerd/image_exporter.go Outdated Show resolved Hide resolved

rumpl force-pushed the c8d-image-save-lease branch 3 times, most recently from 467cff5 to ab81658 Compare July 17, 2023 10:07

vvoland approved these changes Jul 17, 2023

View reviewed changes

corhere approved these changes Jul 17, 2023

View reviewed changes

rumpl requested a review from thaJeztah July 17, 2023 19:21

laurazard approved these changes Jul 18, 2023

View reviewed changes

thaJeztah reviewed Jul 18, 2023

View reviewed changes

daemon/containerd/image_exporter.go

ctx, release, err := i.client.WithLease(ctx)

contentStore := i.client.ContentStore()

leasesManager := i.client.LeasesService()

lease, err := leasesManager.Create(ctx, leases.WithRandomID())

This comment was marked as duplicate.

Sign in to view

thaJeztah mentioned this pull request Jul 18, 2023

c8d: some minor cleanups #46008

Merged

rumpl force-pushed the c8d-image-save-lease branch from ab81658 to f3a6b0f Compare July 19, 2023 07:59

laurazard approved these changes Jul 19, 2023

View reviewed changes

rumpl mentioned this pull request Jul 19, 2023

c8d: Cleanup leases on startup #46020

Open

thaJeztah approved these changes Jul 19, 2023

View reviewed changes

thaJeztah merged commit 9ba41b7 into moby:master Jul 19, 2023

thaJeztah deleted the c8d-image-save-lease branch July 19, 2023 09:53

thaJeztah mentioned this pull request Jul 19, 2023

[24.0 backport] c8d: Make sure the content isn't removed while we export #46021

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

c8d: Make sure the content isn't removed while we export #45963

c8d: Make sure the content isn't removed while we export #45963

rumpl commented Jul 13, 2023 •

edited

Loading

rumpl commented Jul 13, 2023

corhere left a comment

laurazard left a comment

laurazard Jul 18, 2023

This comment was marked as duplicate.

rumpl Jul 18, 2023

laurazard Jul 18, 2023

rumpl Jul 18, 2023

thaJeztah Jul 18, 2023

rumpl Jul 18, 2023

rumpl Jul 19, 2023

thaJeztah left a comment

This comment was marked as duplicate.

corhere commented Jul 18, 2023

laurazard left a comment

thaJeztah commented Jul 19, 2023

thaJeztah left a comment

	lease, err := leasesManager.Create(ctx, leases.WithRandomID())
	lease, err := leasesManager.Create(ctx, leases.WithRandomID(), leases.WithExpiration(1*time.Hour))

	func tempLease(ctx context.Context, mgr leases.Manager) (context.Context, func(context.Context) error, error) {
	nop := func(context.Context) error { return nil }
	_, ok := leases.FromContext(ctx)
	if ok {
	return ctx, nop, nil
	}

	// Use an expiration that ensures the lease is cleaned up at some point if there is a crash, SIGKILL, etc.
	opts := []leases.Opt{
	leases.WithRandomID(),
	leases.WithExpiration(24 * time.Hour),
	leases.WithLabels(map[string]string{
	"moby.lease/temporary": time.Now().UTC().Format(time.RFC3339Nano),
	}),
	}

c8d: Make sure the content isn't removed while we export #45963

c8d: Make sure the content isn't removed while we export #45963

Conversation

rumpl commented Jul 13, 2023 • edited Loading

rumpl commented Jul 13, 2023

corhere left a comment

Choose a reason for hiding this comment

laurazard left a comment

Choose a reason for hiding this comment

laurazard Jul 18, 2023

Choose a reason for hiding this comment

This comment was marked as duplicate.

rumpl Jul 18, 2023

Choose a reason for hiding this comment

laurazard Jul 18, 2023

Choose a reason for hiding this comment

rumpl Jul 18, 2023

Choose a reason for hiding this comment

thaJeztah Jul 18, 2023

Choose a reason for hiding this comment

rumpl Jul 18, 2023

Choose a reason for hiding this comment

rumpl Jul 19, 2023

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

corhere commented Jul 18, 2023

laurazard left a comment

Choose a reason for hiding this comment

thaJeztah commented Jul 19, 2023

thaJeztah left a comment

Choose a reason for hiding this comment

rumpl commented Jul 13, 2023 •

edited

Loading