Fix optional EROFS differ setup in transfer plugin by cpuguy83 · Pull Request #13328 · containerd/containerd

cpuguy83 · 2026-05-01T01:12:52Z

closes Since 2.3.0, transfer service refuses to initialize without erofs-utils #13346

Found this during some smoke tests for containerd 2.3.
On my system I have erofs available but I was running tests in a container which didn't have mkfs.erofs.
This got containerd into a weird state.
Transfer service completely failed to load, a few other things as well, and I couldn't even ctr plugin ls, subscribe to events, nothing despite the grpc service running (returning Unavailable for the things I tried).

Refactor transfer unpack platform setup into helpers for targeted testing.
Skip optional unpack configs when their explicit differ is unavailable or skipped.
Add regression coverage for EROFS defaults when the EROFS snapshotter is available but the EROFS differ skips due to missing mkfs.erofs.

Copilot

Pull request overview

Refactors transfer plugin unpack-platform configuration to correctly handle unavailable (skipped) diff plugins—especially the EROFS differ—so optional configurations don’t block transfers when a differ is not usable on the host.

Changes:

Extracted unpack platform configuration into configureUnpackPlatforms and centralized differ selection in getApplier, adding logic to skip optional differs when plugin init returns errors (including plugin.ErrSkipPlugin).
Added unit tests covering optional/required explicit differ behavior and auto-selection skipping skipped candidates.
Added a Linux-specific test verifying the default config skips an unavailable EROFS differ and still configures the default unpack platform.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
plugins/transfer/plugin.go	Refactors unpack-platform setup and differ selection; adds skip/optional handling for unavailable differs.
plugins/transfer/plugin_test.go	Adds focused unit tests for `configureUnpackPlatforms`/`getApplier` skip and selection behavior.
plugins/transfer/plugin_linux_test.go	Adds Linux-only test validating default config behavior when EROFS differ is unavailable.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

This PR fixes transfer plugin initialization/unpack platform configuration so that optional EROFS unpack entries don’t block transfers when the EROFS differ plugin is unavailable (e.g., returns plugin.ErrSkipPlugin).

Changes:

Refactors unpack platform setup into configureUnpackPlatforms and moves diff-applier selection into getApplier, adding explicit handling for ErrSkipPlugin and optional configs.
Ensures optional unpack configs are skipped when their explicit differ is missing or fails to initialize.
Adds unit tests (including a Linux-specific test) covering optional differ skipping and auto-selection behavior.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
plugins/transfer/plugin.go	Refactors unpack platform configuration and fixes differ selection to skip unavailable/skip-plugin differs for optional configs.
plugins/transfer/plugin_test.go	Adds unit tests and helpers for plugin set initialization and unpack platform configuration scenarios.
plugins/transfer/plugin_linux_test.go	Adds Linux-only regression test ensuring default config skips unavailable EROFS differ and keeps only the default unpack platform.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+		if p := ic.Plugins().Get(plugins.SnapshotPlugin, uc.Snapshotter); p != nil {
+			snExports = p.Meta.Exports
+			snCapabilities = p.Meta.Capabilities


prakleumas

^{beep boop}
Hello! I am @samuelkarp's friendly code review robot 🤖! Sam asked me to take a look and post my comments directly on GitHub.

Please treat my comments as suggestions and advisory, not mandates. Since I am an LLM and my output is used here directly, you should evaluate carefully and see if my suggestions make sense. Feel free to directly resolve any you disagree with.

^{This is not an automated action. This bot does not read or reply to comments.}

prakleumas · 2026-05-01T04:47:18Z

+		if err != nil {
+			return fmt.Errorf("%s: platform configuration %v invalid", plugins.TransferPlugin, uc.Platform)
+		}


It is generally safer to preserve the underlying error when returning a new error so that the original cause is not lost, which aids in debugging and auditability.

Suggested change

if err != nil {

return fmt.Errorf("%s: platform configuration %v invalid", plugins.TransferPlugin, uc.Platform)

}

if err != nil {

return fmt.Errorf("%s: platform configuration %v invalid: %w", plugins.TransferPlugin, uc.Platform, err)

}

Agree, as it is though this is moved untouched.
I was already kind of iffy about including the refactor without the actual patch (as it is in the first commit) instead of just doing the patch and doing a separate change to refactor.
But I also wanted to unit test it so, here we are.

prakleumas · 2026-05-01T04:47:18Z

+			}
+			return nil, false, fmt.Errorf("failed to get instance for diff plugin %q: %w", uc.Differ, err)
+		}
+		return inst.(diff.Applier), false, nil


It is generally safer to protect type assertions with an ok check to prevent unrecoverable panics during initialization. If a misconfigured plugin returns an instance that does not implement diff.Applier, this introduces the opportunity for a panic.

prakleumas · 2026-05-01T04:47:18Z

+				skippedApplier = applierID
+			}
+
+			log.G(ic.Context).Warnf("multiple differs match for platform, set `differ` option to choose, skipping %q", skippedApplier)


Structured logging is generally preferred over printf-style formatting when injecting dynamic variables into log messages, as it preserves the fields for easier querying in log aggregators.

Suggested change

log.G(ic.Context).Warnf("multiple differs match for platform, set `differ` option to choose, skipping %q", skippedApplier)

log.G(ic.Context).WithField("skipped_applier", skippedApplier).Warn("multiple differs match for platform, set `differ` option to choose")

prakleumas · 2026-05-01T04:47:18Z

+		applier = inst.(diff.Applier)
+		applierID = candidate.Registration.ID


Similarly, ensuring that the instance satisfies the expected interface avoids panics when iterating through candidate plugins.

hsiangkao

Hi, it looks good to me, but in order to make the pr more reviewable and backportable, could we fix the bug first and refactor the code into helpers then instead?

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 · 2026-05-04T20:03:02Z

@hsiangkao Sure. I mainly wanted to make sure the patch had a valid test so that backports have less potential to have an issue, however I don't think this will be difficult to backport to 2.3 either way.

This is updated accordingly.

austinvazquez · 2026-05-05T15:46:08Z

Closes #13346

fuweid · 2026-05-06T11:20:23Z

 STEP: Destroying namespace "summary-test-5841" for this suite. @ 05/06/26 07:12:19.242
  << Timeline

  [FAILED] Failed after 15.035s.
  Expected
      <string>: Summary
  to match fields: {
  [.Pods[summary-test-5841::stats-busybox-1].Containers[busybox-container].CPU:
  	Expected
  	    <string>: CPUStats
  	to match fields: {
  	.UsageNanoCores:
  		Expected
  		    <uint64>: 1001721289
  		to be <=
  		    <float64>: 1e+09
  	}
  	, .Pods[summary-test-5841::stats-busybox-0].Containers[busybox-container].CPU:
  	Expected
  	    <string>: CPUStats
  	to match fields: {
  	.UsageNanoCores:
  		Expected
  		    <uint64>: 1004445592
  		to be <=
  		    <float64>: 1e+09
  	}
  	]
  }

hmm it looks like test code issue

AkihiroSuda · 2026-05-10T12:32:26Z

/cherry-pick release/2.3

k8s-infra-cherrypick-robot · 2026-05-10T12:33:12Z

@AkihiroSuda: new pull request created: #13364

Details

In response to this:

/cherry-pick release/2.3

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.

Copilot AI review requested due to automatic review settings May 1, 2026 01:12

k8s-ci-robot added the do-not-merge/work-in-progress label May 1, 2026

github-project-automation Bot added this to Pull Request Review May 1, 2026

github-project-automation Bot moved this to Needs Triage in Pull Request Review May 1, 2026

k8s-ci-robot added the size/L label May 1, 2026

Copilot started reviewing on behalf of cpuguy83 May 1, 2026 01:13 View session

Copilot AI reviewed May 1, 2026

View reviewed changes

Comment thread plugins/transfer/plugin_test.go

Comment thread plugins/transfer/plugin_test.go Outdated

Comment thread plugins/transfer/plugin.go

cpuguy83 force-pushed the fix_erofs_blocking_transfer branch from 0a6e700 to f92fef6 Compare May 1, 2026 01:21

Copilot AI review requested due to automatic review settings May 1, 2026 01:24

cpuguy83 force-pushed the fix_erofs_blocking_transfer branch from f92fef6 to d0d43f4 Compare May 1, 2026 01:24

Copilot started reviewing on behalf of cpuguy83 May 1, 2026 01:24 View session

cpuguy83 force-pushed the fix_erofs_blocking_transfer branch from d0d43f4 to 2591ac9 Compare May 1, 2026 01:26

cpuguy83 marked this pull request as ready for review May 1, 2026 01:28

k8s-ci-robot removed the do-not-merge/work-in-progress label May 1, 2026

cpuguy83 moved this from Needs Triage to Needs Reviewers in Pull Request Review May 1, 2026

dosubot Bot added the kind/bug label May 1, 2026

Copilot AI reviewed May 1, 2026

View reviewed changes

Comment thread plugins/transfer/plugin.go

Comment on lines +127 to +129

if p := ic.Plugins().Get(plugins.SnapshotPlugin, uc.Snapshotter); p != nil {

snExports = p.Meta.Exports

snCapabilities = p.Meta.Capabilities

Comment thread plugins/transfer/plugin_test.go

cpuguy83 changed the title ~~Fix erofs blocking transfer~~ Fix optional EROFS differ setup in transfer plugin May 1, 2026

prakleumas reviewed May 1, 2026

View reviewed changes

samuelkarp approved these changes May 1, 2026

View reviewed changes

hsiangkao approved these changes May 4, 2026

View reviewed changes

github-project-automation Bot moved this from Needs Reviewers to Review In Progress in Pull Request Review May 4, 2026

cpuguy83 added 2 commits May 4, 2026 11:41

Fix optional transfer differ setup

5860534

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Refactor transfer unpack configuration setup

f8a5f8d

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 force-pushed the fix_erofs_blocking_transfer branch from 2591ac9 to f8a5f8d Compare May 4, 2026 18:57

hsiangkao approved these changes May 4, 2026

View reviewed changes

austinvazquez mentioned this pull request May 5, 2026

fix: transfer plugin depends on optional diff plugin #13347

Closed

austinvazquez approved these changes May 5, 2026

View reviewed changes

github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks May 6, 2026

fuweid approved these changes May 6, 2026

View reviewed changes

fuweid added this pull request to the merge queue May 6, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 6, 2026

dmcgowan added this pull request to the merge queue May 6, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 6, 2026

dmcgowan added this pull request to the merge queue May 6, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 6, 2026

fuweid added this pull request to the merge queue May 6, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 6, 2026

Falco20019 mentioned this pull request May 6, 2026

docker-in-docker feature not working on Ubuntu 26.04 devcontainers/features#1633

Closed

This was referenced May 6, 2026

Since 2.3.0, transfer service refuses to initialize without erofs-utils #13346

Closed

docker-in-docker: Feature request - Ability to pin containerd version devcontainers/features#1639

Closed

dmcgowan added this pull request to the merge queue May 7, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 7, 2026

fuweid added this pull request to the merge queue May 8, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 8, 2026

AkihiroSuda added the priority/P0 label May 10, 2026

AkihiroSuda added this pull request to the merge queue May 10, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 10, 2026

AkihiroSuda added this pull request to the merge queue May 10, 2026

Merged via the queue into containerd:main with commit df6b307 May 10, 2026
131 of 146 checks passed

github-project-automation Bot moved this from Review In Progress to Done in Pull Request Review May 10, 2026

k8s-infra-cherrypick-robot mentioned this pull request May 10, 2026

[release/2.3] Fix optional EROFS differ setup in transfer plugin #13364

Merged

austinvazquez added cherry-picked/2.3.x PR commits are cherry picked into release/2.3 branch and removed cherry-pick/2.3.x Change to be cherry picked to release/2.3 labels May 14, 2026

	log.G(ic.Context).Warnf("multiple differs match for platform, set `differ` option to choose, skipping %q", skippedApplier)
	log.G(ic.Context).WithField("skipped_applier", skippedApplier).Warn("multiple differs match for platform, set `differ` option to choose")

		applier = inst.(diff.Applier)
		applierID = candidate.Registration.ID

Conversation

cpuguy83 commented May 1, 2026 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

prakleumas left a comment

Choose a reason for hiding this comment

Uh oh!

prakleumas May 1, 2026

Choose a reason for hiding this comment

Uh oh!

cpuguy83 May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prakleumas May 1, 2026

Choose a reason for hiding this comment

Uh oh!

prakleumas May 1, 2026

Choose a reason for hiding this comment

Uh oh!

prakleumas May 1, 2026

Choose a reason for hiding this comment

Uh oh!

hsiangkao left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented May 4, 2026

Uh oh!

austinvazquez commented May 5, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fuweid commented May 6, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AkihiroSuda commented May 10, 2026

Uh oh!

k8s-infra-cherrypick-robot commented May 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

cpuguy83 commented May 1, 2026 •

edited by thaJeztah

Loading

cpuguy83 May 1, 2026 •

edited

Loading