Codecov Report

Merging #3055 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3055      +/-   ##
==========================================
+ Coverage   44.75%   44.83%   +0.07%     
==========================================
  Files         113      113              
  Lines       12513    12531      +18     
==========================================
+ Hits         5600     5618      +18     
  Misses       6057     6057              
  Partials      856      856

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4355a2a...75f1838. Read the comment docs.

The options need to be filtered. The pass through should be done based on whether the metadata sees labels that should apply to the backend and not just to the namespace. In the design issue I mentioned doing this based on prefixing, such as containerd.io/snapshot/*. The backend snapshotter is considered a shared resource across namespaces, we need to prevent namespace specific data leaking into that layer.

Makes sense. I didn't see #2907 before this, looks like I just hit the same problem. I'll rework it a bit to make it upstream-friendly.

@dmcgowan was busy doing other things recently, but I'll get back to this now. Just to clarify, you just want a simple filter func that does a strings.HasPrefix for containerd.io/snapshot/ for all keys and creates a new map and passes that in?

@ehotinger yes, I believe you are on the right track; @dmcgowan can confirm. We also need to document these prefixes somewhere so this design is clear to implementers/users.

Yeah just let me know what you guys want and I can add in the filter + rebase.

@ehotinger - Took me a minute to capture @dmcgowan's ask here. I believe to satisfy what @estesp said about documentation and to satisfy only forwarding containerd.io/snapshot/ labels to a snapshots.Snapshotter we make the following changes:

Update: https://github.com/containerd/containerd/blob/master/snapshots/snapshotter.go#L92
To say something like

        // Labels for a snapshot
        //
        // Note: that only labels prefixed with `containerd.io/snapshot/` will be forwarded to the
        // snapshotter's `Prepare`, `View`, or `Commit` calls.
	Labels  map[string]string `json:",omitempty"`

And then in:
https://github.com/containerd/containerd/blob/master/metadata/snapshot.go#L344
https://github.com/containerd/containerd/blob/master/metadata/snapshot.go#L346
https://github.com/containerd/containerd/blob/master/metadata/snapshot.go#L450

We only forward snapshots.WithLabels(toSnapshotterLabels(base.Labels)) based on the following:

func toSnapshotterLabels(labels map[string]string) map[string]string {
    m := make(map[string]string)
    for k, v := range labels {
        if strings.HasPrefix(k, "containerd.io/snapshot/") {
            m[k] = v
        }
    }
    return m
}

So basically, persist all labels, forward only prefix containerd.io/snapshot/ to a snapshotter impl and document on the snapshotter interface which prefix is forwarded.

@jterry75 yeah I'm not sure still. The opts are being passed along with Parent, Labels, Created, Updated, all of these are useful to consume. And maybe more stuff in the future. So... maybe if an opt contains at least one label that has the magic prefix then include the opt it in a new slice and pass that through?

/cc @estesp + @dmcgowan

@jterry75 yeah I'm not sure still. The opts are being passed along with Parent, Labels, Created, Updated, all of these are useful to consume. And maybe more stuff in the future. So... maybe if an opt contains at least one label that has the magic prefix then include the opt it in a new slice and pass that through?

/cc @estesp + @dmcgowan

The interface does not allow passing the full spec. I think the way to do that would be to add the additional opts With* to snapshotter.go or you could add one WithInfo and pass a blob through in total.

Build succeeded.

• containerd-build-arm64 : SUCCESS in 8m 33s (non-voting)

@ehotinger - Did you forget to add your changes? This is the same commit force-pushed ?

@jterry75 I force-pushed just to do a rebase initially. 😃

I force-pushed again now with the changes.

Edit: forgot docs. Will add those in a bit and update.

Build succeeded.

• containerd-build-arm64 : SUCCESS in 8m 00s (non-voting)

@estesp - PTAL

Test failure seems unrelated/flaky

Test failure seems unrelated/flaky

Error is for sure not related to this change. It failed in cri test

@crosbymichael - How do you feel about this one as well?

Code looks good, the only thing I would change is the terminology. fowarded/passthrought to just inherit because that is what is actually happening.

@crosbymichael I changed the terminology to "inherited" across the board, it seemed to fit better than "inherit" to me; PTAL - let me know if anything can be improved. Naming things is hard. 😄

Build succeeded.

• containerd-build-arm64 : SUCCESS in 8m 30s (non-voting)

LGTM