Made fixes and optimizations to encryption GC by lumjjb · Pull Request #3430 · containerd/containerd

lumjjb · 2019-07-18T17:11:49Z

There are some fixes required for garbage collection, as well as some optimizations for removal of certain resources from the lease for the image encryption feature introduced in #3134.

This PR aims to fix these known GC issues.

Signed-off-by: Brandon Lum [email protected]

stefanberger · 2019-07-18T17:15:43Z

This PR fixes an issue I had seen with this command line:

./bin/ctr images rm ubuntuenc:1 ; ./bin/ctr images pull --all-platforms docker.io/library/ubuntu:16.04 ; sleep 0 ; while :; do ./bin/ctr images encrypt --recipient mypubkey.pem docker.io/library/ubuntu:16.04 ubuntuenc:1 && ./bin/ctr images export ubuntuenc.tgz ubuntuenc:1 && ./bin/ctr images rm ubuntuenc:1 || break; done

Tested-by: Stefan Berger [email protected]

theopenlab-ci · 2019-07-18T17:21:24Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 8m 50s (non-voting)

theopenlab-ci · 2019-07-18T17:45:40Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 8m 35s (non-voting)

dmcgowan · 2019-07-18T18:34:12Z

Ok, I think I see what is going wrong here. Normally the client is responsible for providing leases through the context and that lease will be used anywhere content is created. Explicitly adding to the lease here is not necessary (it is only needed in read case when you want to consume some content you did not create and want to ensure your operation completes without something else deleting it).

In this case, the lease should be created at the highest client level, ctr
Try adding

		ctx, done, err := client.WithLease(ctx)
		if err != nil {
			return err
		}
		defer done(ctx)

in your commands before calling down into your code which creates resources. This will ensure everything is kept around during the life of your client command.

codecov-io · 2019-07-18T18:38:57Z

Codecov Report

Merging #3430 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3430      +/-   ##
==========================================
+ Coverage   44.24%   44.24%   +<.01%     
==========================================
  Files         123      123              
  Lines       13675    13675              
==========================================
+ Hits         6050     6051       +1     
+ Misses       6694     6693       -1     
  Partials      931      931

Flag	Coverage Δ
#linux	`48.05% <ø> (ø)`	⬆️
#windows	`39.78% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
runtime/v2/shim/shim_windows.go	`43.5% <0%> (+0.64%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5631fe3...c118c45. Read the comment docs.

lumjjb · 2019-07-18T18:47:20Z

@dmcgowan That's nice! Let me make a change to that as well in this PR then!

Signed-off-by: Brandon Lum <[email protected]>

theopenlab-ci · 2019-07-18T19:28:38Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 8m 19s (non-voting)

stefanberger · 2019-07-18T19:33:34Z

Works for me...

theopenlab-ci · 2019-07-18T20:01:55Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 8m 49s (non-voting)

lumjjb · 2019-07-18T20:15:18Z

The test failing is

--- FAIL: TestImageIsUnpacked (0.35s)
...
    image_test.go:65: image should not be unpacked

I am able to make it go away by removing the use of the lease from the encryption test, but I'm not sure if this is the expected behavior for leases.
EDIT: unable -> able typo

    ctx, done, err := client.WithLease(ctx)
    if err != nil {
        t.Fatal(err)
    }
    defer done(ctx)

theopenlab-ci · 2019-07-18T20:30:26Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 8m 53s (non-voting)

dmcgowan · 2019-07-18T21:58:38Z

don't call cw.Digest() here, it should be called after Commit

dmcgowan · 2019-07-18T22:00:33Z

This test doesn't need a lease such as WithLease?

Ok - i think i figured out what the issue was. I think it is a bad gc label.

Pushing a fix 🤞 .

Signed-off-by: Brandon Lum <[email protected]>

theopenlab-ci · 2019-07-18T22:27:49Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 8m 46s (non-voting)

theopenlab-ci · 2019-07-18T22:49:26Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 9m 14s (non-voting)

lumjjb · 2019-07-18T23:03:15Z

~~@dmcgowan ready for review!~~ EDIT 2: @stefanberger figured out the underlying issue of test interaction. Potential fixes posted below. EDIT: nvm, that was not it, but it's really weird... can't explain the tests interacting with each other... We suspect it's that the images are not deleted properly across tests having something to do with the lease.

…

On Thu, Jul 18, 2019, 6:49 PM theopenlab-ci[bot] ***@***.***> wrote: Build succeeded. - containerd-build-arm64 <https://logs.openlabtesting.org/logs/30/3430/2c6c3b937d1b848a67777578b3b5674bc20519e3/check/containerd-build-arm64/05ceea9/> : SUCCESS in 9m 14s (non-voting) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3430?email_source=notifications&email_token=AAXLDBWWQO65QIWZA4OENTLQADXQJA5CNFSM4IE52CFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2KBFTY#issuecomment-513020623>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXLDBVQO2K7RRC4K45QT5LQADXQJANCNFSM4IE52CFA> .

theopenlab-ci · 2019-07-19T13:12:05Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 8m 26s (non-voting)

Signed-off-by: Brandon Lum <[email protected]> Signed-off-by: Stefan Berger <[email protected]>

theopenlab-ci · 2019-07-19T14:18:59Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 8m 29s (non-voting)

lumjjb · 2019-07-19T15:25:40Z

@stefanberger @dmcgowan

Yea, it does seem like the issue is that the tests are interacting with each other because certain things are not being deleted from the content store that were being leased. Since the client.WithLeases call, doesn't have the leases.SynchronousDelete option, if the order is

delete image
release lease

The content will not be deleted unless GC runs. We've found 2 patches that work, @dmcgowan let us know what your thoughts are.

diff --git a/lease.go b/lease.go
index d46b79d..2bdf878 100644
--- a/lease.go
+++ b/lease.go
@@ -41,6 +41,6 @@ func (c *Client) WithLease(ctx context.Context) (context.Context, func(context.C

        ctx = leases.WithLease(ctx, l.ID)
        return ctx, func(ctx context.Context) error {
-               return ls.Delete(ctx, l)
+               return ls.Delete(ctx, l, leases.SynchronousDelete)
        }, nil
 }

OR

diff --git a/image_enc_test.go b/image_enc_test.go
index 9749971..97c2429 100644
--- a/image_enc_test.go
+++ b/image_enc_test.go
@@ -161,10 +161,15 @@ func TestImageEncryption(t *testing.T) {
                        Parameters: dcparameters,
                },
        }
+       // Clean up function cancels lease before deleting the image so the images are
+       // properly deleted
+       defer func() {
+               done(ctx)
+               client.ImageService().Delete(ctx, imageName, images.SynchronousDelete())
+               client.ImageService().Delete(ctx, encImageName, images.SynchronousDelete())
+       }()

        // Perform decryption of image
-       defer client.ImageService().Delete(ctx, imageName, images.SynchronousDelete())
-       defer client.ImageService().Delete(ctx, encImageName, images.SynchronousDelete())
        lf = func(desc ocispec.Descriptor) bool {
                return true
        }

dmcgowan · 2019-07-22T17:54:02Z

+	// properly deleted
+	defer func() {
+		done(ctx)
+		client.ImageService().Delete(ctx, imageName, images.SynchronousDelete())


I would definitely encourage using test specific image names here. Also when doing a synchronous delete of 2 images, just add the flag on the second one.

Signed-off-by: Brandon Lum <[email protected]>

theopenlab-ci · 2019-07-22T20:16:35Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 9m 15s (non-voting)

estesp · 2019-07-22T20:22:56Z

I think it would make sense to just add some randomness to the encrypted image name; that way parallel runs of the suite won't overlap on name use. And we also pull a seed image at the start of test runs--busybox I think, so I'm not sure you need to absolutely pull or delete that image as you should be able to find it in the image service? @dmcgowan can correct me if I'm wrong on that.

lumjjb · 2019-07-22T20:30:46Z

I think it would make sense to just add some randomness to the encrypted image name;

@estesp I'm curious about that too - I was unsure about the implications for the unpacked image, seeing as how the actual unpacked image would have the same content hash even with a different tag... Let me try that out..

EDIT: I tried it out and it didn't work unfortunately... I tried it with this patch, that takes the pulled image and tags it as another image name.

diff --git a/image_enc_test.go b/image_enc_test.go
index 97c2429..d13853f 100644
--- a/image_enc_test.go
+++ b/image_enc_test.go
@@ -31,11 +31,12 @@ import (
        ocispec "github.com/opencontainers/image-spec/specs-go/v1"
 )

-func setupBusyboxImage(t *testing.T) {
+func setupBusyboxImage(t *testing.T, testImageName string) {
        if runtime.GOOS == "windows" {
                t.Skip()
        }

+       //const imageName = "docker.io/library/nginx:latest"
        const imageName = "docker.io/library/busybox:latest"
        ctx, cancel := testContext(t)
        defer cancel()
@@ -46,6 +47,8 @@ func setupBusyboxImage(t *testing.T) {
        }
        defer client.Close()

+       s := client.ImageService()
+
        // Cleanup
        err = client.ImageService().Delete(ctx, imageName)
        if err != nil && !errdefs.IsNotFound(err) {
@@ -58,6 +61,19 @@ func setupBusyboxImage(t *testing.T) {
                t.Fatal(err)
        }

+       img, err := s.Get(ctx, imageName)
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       img.Name = testImageName
+       _, err = s.Create(ctx, img)
+       if err != nil {
+               t.Fatalf("Unable to create image: %v", err)
+       }
+
+       image = NewImage(client, img)
+
        err = image.Unpack(ctx, DefaultSnapshotter)
        if err != nil {
                t.Fatal(err)
@@ -65,15 +81,16 @@ func setupBusyboxImage(t *testing.T) {
 }

 func TestImageEncryption(t *testing.T) {
-       setupBusyboxImage(t)
+       const imageName = "docker.io/library/encimgtest:enc-test"
+       const encImageName = "docker.io/library/encimgtest:enc-test-encrypted"
+
+       setupBusyboxImage(t, imageName)

        publicKey, privateKey, err := utils.CreateRSATestKey(2048, nil, true)
        if err != nil {
                t.Fatal(err)
        }

-       const imageName = "docker.io/library/busybox:latest"
-       const encImageName = "docker.io/library/busybox:enc"
        ctx, cancel := testContext(t)
        defer cancel()

@@ -164,9 +181,9 @@ func TestImageEncryption(t *testing.T) {
        // Clean up function cancels lease before deleting the image so the images are
        // properly deleted
        defer func() {
-               done(ctx)
-               client.ImageService().Delete(ctx, imageName, images.SynchronousDelete())
-               client.ImageService().Delete(ctx, encImageName, images.SynchronousDelete())
+               //done(ctx)
+               //client.ImageService().Delete(ctx, imageName, images.SynchronousDelete())
+               //client.ImageService().Delete(ctx, encImageName, images.SynchronousDelete())
        }()

        // Perform decryption of image

dmcgowan

LGTM

This change is better than what is in master...

However, I think there are still some improvements which could be made to improve reliability. We have a "seed" image which gets pulled at the beginning so we can attempt to avoid pulling as much as possible during tests. In this case, it is OK to use that seed image, encrypt it, then decrypt it into a different image name, the original image does not need to be deleted at the end, only the newly created ones.

lumjjb · 2019-07-22T22:20:48Z

Sounds good - I'll start checking out the seed image in the meantime!

estesp

LGTM

Made fixes and optimizations to encryption GC

c00517a

Signed-off-by: Brandon Lum <[email protected]>

dmcgowan reviewed Jul 18, 2019

View reviewed changes

Corrected lease implementation

c6d437f

Signed-off-by: Brandon Lum <[email protected]>

Change image_enc_test so that it more reliably delete images

c118c45

Signed-off-by: Brandon Lum <[email protected]> Signed-off-by: Stefan Berger <[email protected]>

dmcgowan reviewed Jul 22, 2019

View reviewed changes

Modified image_enc_test to use a different image from other tests

d531e78

Signed-off-by: Brandon Lum <[email protected]>

dmcgowan approved these changes Jul 22, 2019

View reviewed changes

estesp approved these changes Jul 22, 2019

View reviewed changes

estesp merged commit 49fdb9e into containerd:master Jul 22, 2019

Conversation

lumjjb commented Jul 18, 2019

Uh oh!

stefanberger commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci Bot commented Jul 18, 2019

Uh oh!

theopenlab-ci Bot commented Jul 18, 2019

Uh oh!

dmcgowan commented Jul 18, 2019

Uh oh!

codecov-io commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lumjjb commented Jul 18, 2019

Uh oh!

theopenlab-ci Bot commented Jul 18, 2019

Uh oh!

stefanberger commented Jul 18, 2019

Uh oh!

theopenlab-ci Bot commented Jul 18, 2019

Uh oh!

lumjjb commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci Bot commented Jul 18, 2019

Uh oh!

dmcgowan Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

lumjjb Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

dmcgowan Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

lumjjb Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

lumjjb Jul 18, 2019

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci Bot commented Jul 18, 2019

Uh oh!

theopenlab-ci Bot commented Jul 18, 2019

Uh oh!

lumjjb commented Jul 18, 2019 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci Bot commented Jul 19, 2019

Uh oh!

theopenlab-ci Bot commented Jul 19, 2019

Uh oh!

lumjjb commented Jul 19, 2019

Uh oh!

dmcgowan Jul 22, 2019

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci Bot commented Jul 22, 2019

Uh oh!

estesp commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lumjjb commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

lumjjb commented Jul 22, 2019

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

stefanberger commented Jul 18, 2019 •

edited

Loading

codecov-io commented Jul 18, 2019 •

edited

Loading

lumjjb commented Jul 18, 2019 •

edited

Loading

lumjjb commented Jul 18, 2019 via email •

edited

Loading

estesp commented Jul 22, 2019 •

edited

Loading

lumjjb commented Jul 22, 2019 •

edited

Loading