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

Wrap local calls to the content and lease service #44091

Merged
merged 1 commit into from Sep 6, 2022

Conversation

rumpl
Copy link
Member

@rumpl rumpl commented Sep 6, 2022

- What I did

Fixed the issue where the calls to the content store or the leases manager
would fail because the context doesn't define a namespace.

Wrapped all the calls to the local content and lease services to add
the default namespace in the context.

The wrapper sets the default namespace in the context if none is
provided, this is needed because we are calling these services directly
and not trough GRPC that has an interceptor to set the default namespace
to all calls.

The error is hidden currently because the manifestMatchesPlatform doesn't
return an error if there is one, this PR changes that and now returns an error.
See this comment to see the error that was previously hidden.

- How I did it

Implemented content.Store and leases.Manager.

- How to verify it

- Description for the changelog

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

@thaJeztah
Copy link
Member

Looks like this test-helper avoided this case by setting a namespace on the context;

ctx := namespaces.WithNamespace(context.Background(), t.Name())
images := &imageStoreWithLease{Store: is, ns: t.Name(), leases: metadata.NewLeaseManager(mdb)}

Slightly confused by that code though;

  • imageStoreWithLease has a ns property (so a default namespace)
  • but that test-utility ALSO sets the namespace on the context (why both?)

Haven't fully wrapped my head around it, but wondering now if that test is correct or also should be switching to the wrapped one (depends on what we're testing there I guess). Slight complication in that case is that the wrapped store is in the daemon/ package, not daemon/images

@rumpl
Copy link
Member Author

rumpl commented Sep 6, 2022

There are two ways we are calling containerd:

  • using the client over GRPC, in this case there is a GRPC interceptor that sets the default namespace if the current context doesn't have any.
  • directly by creating local services, in this case there is nothing setting the default namespace, my change fixes this.

I'm not sure about the test but in any case the manifestMatchesPlatform uses a context.TODO() so nothing you would do in the test would set the namespace since there are some cases where this call calls the leases directly and not over GRPC, see this comment

daemon/content.go Outdated Show resolved Hide resolved
daemon/content.go Outdated Show resolved Hide resolved
@thaJeztah
Copy link
Member

Both good suggestions, @corhere!

@rumpl wdyt? worth updating before merging?

The wrapper sets the default namespace in the context if none is
provided, this is needed because we are calling these services directly
and not trough GRPC that has an interceptor to set the default namespace
to all calls.

Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
@rumpl
Copy link
Member Author

rumpl commented Sep 6, 2022

@thaJeztah yeah, I'll update 👍

@rumpl rumpl force-pushed the fix-local-context branch 2 times, most recently from 5987e03 to 8789066 Compare September 6, 2022 15:41
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/images containerd-integration Issues and PRs related to containerd integration kind/bugfix PR's that fix bugs process/cherry-picked status/4-merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants