[release/1.6 backport] Add cleanup package for context management during cleanup by thaJeztah · Pull Request #8822 · containerd/containerd

thaJeztah · 2023-07-14T10:35:12Z

[release/1.6] client.newUnpacker: call abort() on ctx.Cancel() and fetchErr

This code did not call "abort" on ctx.Cancel() and fetchErr before
commit 8017daa

Lines 215 to 241 in 030c1ac

    
           select { 
        
           case <-ctx.Done(): 
        
           	return ctx.Err() 
        
           case err := <-fetchErr: 
        
           	if err != nil { 
        
           		return err 
        
           	} 
        
           case <-fetchC[i-fetchOffset]: 
        
           } 
        
           diff, err := a.Apply(ctx, desc, mounts, u.config.ApplyOpts...) 
        
           if err != nil { 
        
           	abort() 
        
           	return fmt.Errorf("failed to extract layer %s: %w", diffIDs[i], err) 
        
           } 
        
           if diff.Digest != diffIDs[i] { 
        
           	abort() 
        
           	return fmt.Errorf("wrong diff id calculated on extraction %q", diffIDs[i]) 
        
           } 
        
           if err = sn.Commit(ctx, chainID, key, opts...); err != nil { 
        
           	abort() 
        
           	if errdefs.IsAlreadyExists(err) { 
        
           		return nil 
        
           	} 
        
           	return fmt.Errorf("failed to commit snapshot %s: %w", key, err) 
        
           }

But after the code was moved to pkg/unpacker, abort calls were added;

containerd/pkg/unpack/unpacker.go

Lines 353 to 381 in 8017daa

    
           select { 
        
           case <-ctx.Done(): 
        
           	abort() 
        
           	return ctx.Err() 
        
           case err := <-fetchErr: 
        
           	if err != nil { 
        
           		abort() 
        
           		return err 
        
           	} 
        
           case <-fetchC[i-fetchOffset]: 
        
           } 
        
           diff, err := a.Apply(ctx, desc, mounts, unpack.ApplyOpts...) 
        
           if err != nil { 
        
           	abort() 
        
           	return fmt.Errorf("failed to extract layer %s: %w", diffIDs[i], err) 
        
           } 
        
           if diff.Digest != diffIDs[i] { 
        
           	abort() 
        
           	return fmt.Errorf("wrong diff id calculated on extraction %q", diffIDs[i]) 
        
           } 
        
           if err = sn.Commit(ctx, chainID, key, opts...); err != nil { 
        
           	abort() 
        
           	if errdefs.IsAlreadyExists(err) { 
        
           		return nil 
        
           	} 
        
           	return fmt.Errorf("failed to commit snapshot %s: %w", key, err) 
        
           }

That code is not part of the 1.6 branch, so applying it here.

and backport of

⚠️ Conflicts in:

metadata/db.go
pkg/unpack/unpacker.go (due to dmcgowan@8017daa missing)
runtime/v2/shim.go

Conflicts:

diff --cc metadata/db.go
index 2d9cbf31a,60a65b5b4..000000000
--- a/metadata/db.go
+++ b/metadata/db.go
@@@ -26,9 -26,13 +26,14 @@@ import
  	"sync/atomic"
  	"time"

 -	eventstypes "github.com/containerd/containerd/api/events"
  	"github.com/containerd/containerd/content"
 -	"github.com/containerd/containerd/events"
  	"github.com/containerd/containerd/gc"
  	"github.com/containerd/containerd/log"
++<<<<<<< HEAD
++=======
+ 	"github.com/containerd/containerd/namespaces"
+ 	"github.com/containerd/containerd/pkg/cleanup"
++>>>>>>> b550526cc (Use cleanup.Background instead of context.Background for cleanup)
  	"github.com/containerd/containerd/snapshots"
  	bolt "go.etcd.io/bbolt"
  )
diff --cc runtime/v2/shim.go
index 456ffb440,3f1c84b87..000000000
--- a/runtime/v2/shim.go
+++ b/runtime/v2/shim.go
@@@ -32,15 -32,13 +32,13 @@@ import
  	"github.com/containerd/containerd/events/exchange"
  	"github.com/containerd/containerd/identifiers"
  	"github.com/containerd/containerd/log"
- 	"github.com/containerd/containerd/namespaces"
  	"github.com/containerd/containerd/pkg/timeout"
 -	"github.com/containerd/containerd/protobuf"
 -	ptypes "github.com/containerd/containerd/protobuf/types"
  	"github.com/containerd/containerd/runtime"
  	client "github.com/containerd/containerd/runtime/v2/shim"
 +	"github.com/containerd/containerd/runtime/v2/task"
  	"github.com/containerd/ttrpc"
 +	ptypes "github.com/gogo/protobuf/types"
  	"github.com/hashicorp/go-multierror"
- 	"github.com/sirupsen/logrus"
  )

  const (
@@@ -131,11 -126,10 +129,15 @@@ func loadShim(ctx context.Context, bund
  	if _, err := s.PID(ctx); err != nil {
  		return nil, err
  	}
 -	return shim, nil
 +	return s, nil
  }

++<<<<<<< HEAD
 +func cleanupAfterDeadShim(ctx context.Context, id, ns string, rt *runtime.TaskList, events *exchange.Exchange, binaryCall *binary) {
 +	ctx = namespaces.WithNamespace(ctx, ns)
++=======
+ func cleanupAfterDeadShim(ctx context.Context, id string, rt *runtime.NSMap[ShimInstance], events *exchange.Exchange, binaryCall *binary) {
++>>>>>>> b550526cc (Use cleanup.Background instead of context.Background for cleanup)
  	ctx, cancel := timeout.WithContext(ctx, cleanupTimeout)
  	defer cancel()

* Unmerged path pkg/unpack/unpacker.go

I applied the pkg/unpack/unpacker.go changes to /unpacker.go instead

I also noticed that not all calls to cleanupAfterDeadShim use cleanup.Background (not sure if that's intentional?)

git grep cleanupAfterDeadShim
runtime/v1/linux/runtime.go:                    if err = r.cleanupAfterDeadShim(cleanup.Background(ctx), bundle, namespace, id); err != nil {
runtime/v1/linux/runtime.go:                    if err := r.cleanupAfterDeadShim(ctx, bundle, ns, id); err != nil {
runtime/v1/linux/runtime.go:                    err := r.cleanupAfterDeadShim(ctx, bundle, ns, id)
runtime/v2/manager.go:          cleanupAfterDeadShim(cleanup.Background(ctx), id, m.shims, m.events, b)
runtime/v2/shim.go:func cleanupAfterDeadShim(ctx context.Context, id string, rt *runtime.TaskList, events *exchange.Exchange, binaryCall *binary) {
runtime/v2/shim_load.go:                        cleanupAfterDeadShim(cleanup.Background(ctx), id, m.shims, m.events, binaryCall)
runtime/v2/shim_load.go:                        cleanupAfterDeadShim(ctx, id, m.shims, m.events, binaryCall)

I'll keep this one in draft for now, perhaps @dmcgowan should have a look

k8s-ci-robot · 2023-07-14T10:35:14Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

thaJeztah · 2023-07-14T11:47:31Z

Timestamps / timing and Windows.. always fun. Guess this is a flaky test that may need to be adjusted to be a bit more relaxed;

=== FAIL: metadata TestContainersCreateUpdateDelete/UpdateExtensionsFieldPath (0.02s)
    containers_test.go:696: timestamp for updatedat not after createdat: 2023-07-14 11:07:00.1507916 +0000 UTC <= 2023-07-14 11:07:00.1507916 +0000 UTC
    --- FAIL: TestContainersCreateUpdateDelete/UpdateExtensionsFieldPath (0.02s)

thaJeztah · 2023-07-14T11:49:21Z

Good news; looks like there's a PR on main that we can cherry-pick;

Make checkContainerTimestamps less strict on Windows #7350

…tchErr This code did not call "abort" on ctx.Cancel() and fetchErr before commit 8017daa https://github.com/containerd/containerd/blob/030c1ac1caa49f88cd572a43a053fb3b7007f94b/unpacker.go#L215-L241 But after the code was moved to pkg/unpacker, abort calls were added; https://github.com/containerd/containerd/blob/8017daa12dd4159e90701bedd1caaa9be5fb0357/pkg/unpack/unpacker.go#L353-L381 That code is not part of the 1.6 branch, so applying it here. Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Derek McGowan <[email protected]> (cherry picked from commit 0bc9f7b) Signed-off-by: Sebastiaan van Stijn <[email protected]>

Provides a couple helper functions that provide a background context for running cleanup jobs while preserving the original context values. The new contexts will not inherit the errors or cancellations. Signed-off-by: Derek McGowan <[email protected]> (cherry picked from commit f606c4e) Signed-off-by: Sebastiaan van Stijn <[email protected]>

Use the cleanup context to re-use values from the original context Signed-off-by: Derek McGowan <[email protected]> (cherry picked from commit b550526) Signed-off-by: Sebastiaan van Stijn <[email protected]>

k8s-ci-robot · 2024-05-28T23:19:26Z

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-sigs/prow repository.

github-actions · 2024-08-27T00:10:31Z

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-09-03T00:10:37Z

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

thaJeztah force-pushed the 1.6_backport_cleanup-context branch from 6ad4c4f to f887aa0 Compare July 14, 2023 10:53

thaJeztah mentioned this pull request Jul 14, 2023

[release/1.6 backport] Make checkContainerTimestamps less strict on Windows #8827

Merged

thaJeztah and others added 4 commits November 9, 2023 13:28

Avoid using canceled context in unpacker cleanup

7e0e76f

Signed-off-by: Derek McGowan <[email protected]> (cherry picked from commit 0bc9f7b) Signed-off-by: Sebastiaan van Stijn <[email protected]>

Use cleanup.Background instead of context.Background for cleanup

3d274f2

Use the cleanup context to re-use values from the original context Signed-off-by: Derek McGowan <[email protected]> (cherry picked from commit b550526) Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the 1.6_backport_cleanup-context branch from f887aa0 to 3d274f2 Compare November 9, 2023 12:28

thaJeztah marked this pull request as ready for review November 9, 2023 12:28

k8s-ci-robot added the needs-rebase label May 28, 2024

github-actions bot added the Stale label Aug 27, 2024

github-actions bot closed this Sep 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release/1.6 backport] Add cleanup package for context management during cleanup#8822

[release/1.6 backport] Add cleanup package for context management during cleanup#8822
thaJeztah wants to merge 4 commits intocontainerd:release/1.6from
thaJeztah:1.6_backport_cleanup-context

thaJeztah commented Jul 14, 2023 •

edited

Loading

Uh oh!

k8s-ci-robot commented Jul 14, 2023

Uh oh!

thaJeztah commented Jul 14, 2023

Uh oh!

thaJeztah commented Jul 14, 2023

Uh oh!

k8s-ci-robot commented May 28, 2024

Uh oh!

github-actions bot commented Aug 27, 2024

Uh oh!

github-actions bot commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	select {
	case <-ctx.Done():
	return ctx.Err()
	case err := <-fetchErr:
	if err != nil {
	return err
	}
	case <-fetchC[i-fetchOffset]:
	}

	diff, err := a.Apply(ctx, desc, mounts, u.config.ApplyOpts...)
	if err != nil {
	abort()
	return fmt.Errorf("failed to extract layer %s: %w", diffIDs[i], err)
	}
	if diff.Digest != diffIDs[i] {
	abort()
	return fmt.Errorf("wrong diff id calculated on extraction %q", diffIDs[i])
	}

	if err = sn.Commit(ctx, chainID, key, opts...); err != nil {
	abort()
	if errdefs.IsAlreadyExists(err) {
	return nil
	}
	return fmt.Errorf("failed to commit snapshot %s: %w", key, err)
	}

Conversation

thaJeztah commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

[release/1.6] client.newUnpacker: call abort() on ctx.Cancel() and fetchErr

Uh oh!

k8s-ci-robot commented Jul 14, 2023

Uh oh!

thaJeztah commented Jul 14, 2023

Uh oh!

thaJeztah commented Jul 14, 2023

Uh oh!

k8s-ci-robot commented May 28, 2024

Uh oh!

github-actions bot commented Aug 27, 2024

Uh oh!

github-actions bot commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thaJeztah commented Jul 14, 2023 •

edited

Loading