Skip to content

c8d: mark stargz as requiring reference-counted mounts#45781

Merged
thaJeztah merged 1 commit intomoby:masterfrom
neersighted:c8d_stargz_refcount
Jun 21, 2023
Merged

c8d: mark stargz as requiring reference-counted mounts#45781
thaJeztah merged 1 commit intomoby:masterfrom
neersighted:c8d_stargz_refcount

Conversation

@neersighted
Copy link
Member

- What I did

The stargz snapshotter cannot be re-mounted, so the reference-counted path must be used.

- How to verify it

docker cp with the stargz snapshotter.

The stargz snapshotter cannot be re-mounted, so the reference-counted
path must be used.

Co-authored-by: Djordje Lukic <djordje.lukic@docker.com>
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
@neersighted neersighted added this to the 25.0.0 milestone Jun 20, 2023
@neersighted neersighted added area/storage Image Storage kind/bugfix PR's that fix bugs containerd-integration Issues and PRs related to containerd integration process/cherry-pick labels Jun 20, 2023
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


// List of known filesystems that can't be re-mounted or have shared layers
var refCountedFileSystems = []string{"overlayfs", "zfs", "fuse-overlayfs"}
var refCountedFileSystems = []string{"fuse-overlayfs", "overlayfs", "stargz", "zfs"}
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we need a tracking ticket for this hard-coded list;

  • is this information that the snapshotter itself could provide?
  • If we must hard-code, do we need to add more snapshotters to this list?

Copy link
Member Author

Choose a reason for hiding this comment

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

I also wonder if we should just go the route of only using the refcounted method, myself.

Copy link
Member

Choose a reason for hiding this comment

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

Good one; perhaps yes. Can you create a tracking ticket for further discussion? (or a draft PR that removes it, which could also help with the discussion)

Copy link
Contributor

Choose a reason for hiding this comment

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

Going with the refcount in all cases seems good to me too.

Also, If re-mount not being supported is a norm, rather than exception, we could go with the reverse - disabling ref counting only for snapshotters that are known to support re-mount and have been tested.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, these are the kind of things I'd love to either see centralised (in containerd library code), or to have an API to request the information from the snapshotter (or some other way to feature-detect). Especially because the list of possible snapshotters is "unlimited", so we can't really make any assumptions beyond the ones we happen to know of.

but perhaps @kzys and @AkihiroSuda have thoughts on that as well

Copy link
Member

@AkihiroSuda AkihiroSuda Jun 21, 2023

Choose a reason for hiding this comment

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

Yes, this information should be exposed to the plugin API for containerd v2.0.
We can just use the hard-coded list for existing containerd releases.

Copy link
Member

Choose a reason for hiding this comment

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

@thaJeztah
Copy link
Member

@vvoland @AkihiroSuda ptal 🤗

@thaJeztah
Copy link
Member

thanks! let me bring this one in 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/storage Image Storage 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.

5 participants