Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

referrer: fix panic on enable_referrer_detect enabled #459

Merged
merged 2 commits into from
May 16, 2023

Conversation

imeoer
Copy link
Collaborator

@imeoer imeoer commented Apr 26, 2023

The panic occurs when enable_referrer_detect = true is enabled in config.toml:

panic: interface conversion: docker.dockerFetcher is not remotes.ReferrersFetcher:
missing method FetchReferrers runtime/debug.Stack()

Fixed it by replacing all github.com/containerd/containerd/remotes package to
github.com/containerd/nydus-snapshotter/pkg/remote/remotes in remote package.

Fix #458

@codecov-commenter
Copy link

codecov-commenter commented Apr 26, 2023

Codecov Report

Merging #459 (c2502eb) into main (70599d9) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #459   +/-   ##
=======================================
  Coverage   37.31%   37.31%           
=======================================
  Files          59       59           
  Lines        6960     6960           
=======================================
  Hits         2597     2597           
  Misses       4052     4052           
  Partials      311      311           
Impacted Files Coverage Δ
pkg/remote/remotes/docker/auth/fetch.go 16.52% <ø> (ø)
pkg/remote/remotes/docker/authorizer.go 77.77% <ø> (ø)
pkg/remote/remotes/docker/config/hosts.go 53.53% <ø> (ø)
pkg/remote/remotes/docker/converter.go 0.00% <ø> (ø)
pkg/remote/remotes/docker/pusher.go 52.72% <ø> (ø)
pkg/remote/remotes/docker/resolver.go 58.99% <ø> (ø)

@changweige
Copy link
Member

Can we have an E2E test of the OCI referrer feature too?

@changweige
Copy link
Member

Say an error likes the issue #458 tells it's fragile

@changweige
Copy link
Member

@imeoer Is it possible to add a test alongside this PR? If you don't have bandwidth right now, it's fine for me to merge this fix only now

The panic occurs when `enable_referrer_detect = true` is enabled in config.toml:

```
panic: interface conversion: docker.dockerFetcher is not remotes.ReferrersFetcher:
missing method FetchReferrers runtime/debug.Stack()
```

Fixed it by replacing all `github.com/containerd/containerd/remotes` package to
`github.com/containerd/nydus-snapshotter/pkg/remote/remotes` in `remote` package.

Fix containerd#458

Signed-off-by: Yan Song <imeoer@linux.alibaba.com>
@imeoer imeoer force-pushed the fix_referrer branch 3 times, most recently from 5db686d to b72251b Compare May 15, 2023 08:44
Signed-off-by: Yan Song <imeoer@linux.alibaba.com>
@imeoer
Copy link
Collaborator Author

imeoer commented May 15, 2023

@imeoer Is it possible to add a test alongside this PR? If you don't have bandwidth right now, it's fine for me to merge this fix only now

Added, PTAL.

KEY="${1}"
VALUE="${2}"

sed -i "s/\($KEY *= *\).*/\1$VALUE/" "${SNAPSHOTTER_CONFIG}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works but not generic since it ignores the toml hierarchy. It's fine for now

@changweige changweige merged commit 38d9301 into containerd:main May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Referrer detection causes crash in v0.8.0
4 participants