Skip to content

Add shared content label to namespaces#6660

Merged
estesp merged 1 commit intocontainerd:mainfrom
henry118:shared-ns
Mar 15, 2022
Merged

Add shared content label to namespaces#6660
estesp merged 1 commit intocontainerd:mainfrom
henry118:shared-ns

Conversation

@henry118
Copy link
Copy Markdown
Member

@henry118 henry118 commented Mar 10, 2022

Re-introducing the shareable namespace feature

Related PR: #5043

This PR is a re-implementation on the shareable namespace feature that was originally introduced by #5043, which was reverted due to a bug identified during testing.

The new implementation follows and extends the semantic of the existing content sharing policy, by allowing the content of a namespace (if configured as "shareable") to be accessible from other namespaces even "content_sharing_policy" is set to "isolated".

For example:

$ cat config.toml
[plugins]
  [plugins.bolt]
    content_sharing_policy = "isolated"
    
// 0. label the shared namespace
$ ctr ns label shared containerd.io/namespace.shareable=true
    
// 1. pull image into shared namespace
$ ctr -n shared i pull docker.io/library/hello-world:latest

// 2. pull the same image into test namespace, this should be much quicker
//    due to content sharing
$ ctr -n test i pull docker.io/library/hello-world:latest

// 3. run container in test namespace
$ ctr -n test run --rm docker.io/library/hello-world:latest demo

Signed-off-by: Henry Wang [email protected]

@k8s-ci-robot
Copy link
Copy Markdown

k8s-ci-robot commented Mar 10, 2022

Hi @henry118. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@henry118 henry118 mentioned this pull request Mar 10, 2022
Closed
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 10, 2022

Build succeeded.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 11, 2022

Build succeeded.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 12, 2022

Build succeeded.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Mar 12, 2022

Build succeeded.

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Mar 15, 2022

A few comments:

  • Whether this feature should piggyback off the "isolated" policy...
    I went back and forth on this one but think it is probably fine the way it is. Since "shared" is the default policy already and this requires explicitly setting the label. Generally, we don't consider it a point of security weakness to make these changes over the API. Even with namespace enforcement, this would only allow an API to give up hash verified data to another namespace, not access anything new.

  • Efficiency
    This won't effect the "shared" default policy. For "isolate" the impact still looks minimal, especially since it is only on open writer. There is a potential for caching the shared namespaces in the future but the improvement is likely minimal, although could save some IO for larger databases. The measuring factor there might be more lock time, since it is performed during a global write lock.

Overall, this PR LGTM

estesp
estesp approved these changes Mar 15, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 58bae86 into containerd:main Mar 15, 2022
@henry118 henry118 deleted the shared-ns branch March 15, 2022 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@estesp estesp estesp approved these changes

Assignees

@estesp estesp

Labels

needs-ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@henry118 @k8s-ci-robot @dmcgowan @estesp