This seems fine to me. internal/ociwclayer hadn't changed in two years so the original intention of possibly iterating on it doesn't seem as relevant anymore.

@TBBle - I'm not up on my latest golang best practices by why not in pkg/?

Sorry, I must have missed a notification from GitHub. >_<

It's not in pkg because:

• There's not much in pkg/, and all-but-one-file that is in pkg/ are not really public APIs.
• Most of the hcsshim APIs are at the top level, and particularly, the public APIs for layers are just in the top hcsshim namespace.
• It's not clear if we want to migrate things into pkg/, and if not, whether it makes sense to start using it at this stage if the API will be split forever.
• I was hoping someone would object to the current name (my aesthetic sense says it's not great) and we could establish a precedent for the future.

So no good reason. Moving it would be trivial code-wise, but there's more work in the decision than the code.

And naming is one of the two hardest problems in computer science.

@TBBle - So IMO there are legacy parts of this repo that are unfortunately public API we cannot undo. Namely the root go package for Docker. Given we have all learned and evolved over time with golang the best practice is to use the pkg/ for publicly exported packages moving forward right?

And yes LOL, naming and componentization. Re-use, Re-use, Re-ugh. Just copy it...

I agree, and am happy to (trivially) move the APIs to pkg/wclayer or pkg/ociwclayer or pkg/ocilayer. The hard part is... which? We have to be a little careful because we already have ImportLayer and ExportLayer in the top-level API, which bounce through to internal/wclayer.{Import,Export}Layer, which are also used by this code (via internal/wclayer.NewLayer{Writer,Reader}).

So perhaps pkg/wclayer, but rename the APIs to {Import,Export}OCILayerTarStream.

We could also add bounces for the existing (non-deprecated) APIs in the root layers.go to pkg/wclayer, unless those are also APIs we want to reconsider? They're mostly thin wrappers around the RS1 vmcompute.dll, and we have the new computestorage package wrapping the RS5 computestorage.dll.

Note: computestorage is also not in pkg, perhaps it should be? @dcantah?

Note: computestorage is also not in pkg, perhaps it should be? @dcantah?

Yea that should honestly be in pkg, I was just going with what we did for the hcn package. Hcn should ideally be in pkg also but that would be a little more disruptive..

I'm not asking for anything other than the ociwclayer to be in pkg since we are just now moving it. The others cant be moved because that would be breaking changes for people unfortunately.

I wasn't thinking of moving other things for this PR, or in fact moving them at all. I was more thinking about the future of pkg/ociwclayer, and whether that's a good name, or if in fact these functions should be pkg/layer.ExportTarStream rather than pkg/ociwclayer/ExportLayer if-and-when other layer-manipulation things appear in pkg/, either because they're copies of (deprecated) layer.go-exposed APIs, or if they're new APIs reflecting RS5 layer-manipulation APIs.

SemVer 0.8 says there's no promise of API compatibility, so deprecating and getting rid of top-level APIs is feasible, particularly if they're just API moves, since the consumers can just change the API location as part of bumping their consumed version of hcsshim.

Anyway, I've repushed as pkg/ociwclayer for now, simply because it was easy, and I'm not seeing any particular interest apart from mine in renaming them.

Given hcsshim is only v0.8, I'd rather have this API published and consumed in containerd and Docker, and worry about renaming it later, than have this stuck as duplicated code pending higher-level future-looking decision making.

For example, it might make sense for the existing APIs in layer.go (except the deprecated ones) to be recreated in pkg/wclayer, but using the internal/wclayer APIs that expose the Context parameter, and don't take the vestigal DriverInfo parameter. At least for ContainerD, that would simplify the callers of these APIs which are generating DriverInfo in-place from the full path-name.

It'd also make them very similar to the computestorage APIs, where matching APIs exist, which might be helpful for migration as downstream users drop RS1 support.

@TBBle - 100%. I'll let you work that out with the maintainers though to see if they are good with such a change. But I agree moving these as well as adding context's would be a huge improvement with minimal work to update Docker.

@dcantah - Can we merge this?

Overall this change looks good to me especially because it will remove some duplicate code from containerd & moby.

However, I think the functions should be named a little differently. For two reasons:

1. We now have two functions named ImportLayer which do different things.
2. We are currently working on a new type of windows container layers called cim layers. When support for those layers arrive in hcsshim & containerd it would be helpful if the names of these function clearly state that these are for legacy layers.

I feel we can have names like ImportLegacyLayerFromTar & ExportLegacyLayerToTar. What do you think @kevpar & @jstarks?

Should we have a LayerType enum or set of Options? I hate the word "Legacy" lol

Yeah, I think having an enum would be a good idea but it won't make sense to add that enum now when rest of the code for cim layers is not merged. Maybe we can just keep the names of these functions as ImportLayerFromTar and ExportLayerToTar. Then when I create PR for cim suppport I can add the functions named ImportCimLayerFromTar and ExportCimLayerToTar and maybe add a wrapper on top which takes the enum and forwards calls accordingly.

I definitely like the ToTar/FromTar change, that addresses my largest concern with the names. As far as more layer types...

Would they be distinguished by the package, rather than the function name? Are there likely to be type-agnostic APIs, or will the caller always know, and if so, would they be dealing with them in separate code-paths, or will it be basically the same code-path with a local enum value being passed into each call?

I've gone ahead and applied the XXTar suffixes, as those are pretty clearly better. I didn't add the Legacy because it's not very descriptive: They're not Legacy yet (unless I've overlooked something...), and everything is Legacy eventually.

Unrelatedly, I observe that internal/wclayer.NewLayerReader does have both "legacy" and "legacy wrapper" buried in there.

@TBBle - I like the name change. For the context do they really do anything? Its not like we cancel mid write or anything so is it actually likely the context would be canceled before we start the write? I'm 100% in favor of contexts I'm just wondering if we need to make them work throughout the internal methods is all

The contexts came from the containerd version of this code. I'm not sure if these particular contexts can be cancelled mid-stream, but I assume they were added for a reason. I can try and track down later (when I have the code handy) if they are ever usefully cancelled, or if there was another rationale for introducing context here, beyond "It's a long-running operation with a loop which can be interrupted".

It would be more useful if the cancellation could also interrupt the hcsImportLayer/hcsExportLayer calls which are copying the transport-stream data to or from a temp directory, as that seems to be the more-expensive part of these operations. But we don't have that, so it's academic.

I had a look, and it seems to me that a cancel can be generated from a containerd client, and I suspect ctr diff ... and hitting 'control-c' while it was running would replicate that.

So I'd leave the context there for now. I noticed the computestorage APIs take a context, although they don't support cancellation with it, so if-and-when this API adapts to or parallels APIs calling computestorage instead of internal/wclayer, we'd need a context anyway.

@TBBle - Works for me. At a minimum its nice because it adds the proper public api to have the context in the signature. We can always make the contexts actually work later :)