internal/hcs/schema should probably not be internal, or needs more wrapping functions.

[TBBle]

I was trying to migrate the containerd WCOW layer code from the RS1 hcs API ("github.com/Microsoft/hcsshim") to the RS5 computestorage API ("github.com/Microsoft/hcsshim/computestorage"), and I fell at the very first post: I can't see how to build a LayerData for computestorage.AttachLayerStorageFilter.

Specifically, storage.go has this:

package computestorage

import (
	hcsschema "github.com/Microsoft/hcsshim/internal/hcs/schema2"
)

// LayerData is the data used to describe parent layer information.
type LayerData struct {
	SchemaVersion hcsschema.Version `json:"SchemaVersion,omitempty"`
	Layers        []hcsschema.Layer `json:"Layers,omitempty"`
}

but from outside (i.e. containerd) there's no way to generate either hcsschema.Version nor hcsschema.Layer values, as the types are declared in "github.com/Microsoft/hcsshim/internal/hcs/schema2".

I saw that for hcsschema.Version, there's the handy SchemaV21() in "github.com/Microsoft/hcsshim/internal/hcs/schema2", although that is also currently internal. I can also do the following to work around not being able to name the type.

	result := &computestorage.LayerData{}
	result.SchemaVersion.Major = 2
	result.SchemaVersion.Minor = 1

I'd rather

	result := &computestorage.LayerData{SchemaVersion: hcsschema.SchemaV21()}

though.

I couldn't find any existing wrappers for hcsschema.Layer that aren't also internal, e.g. MountContainerLayers in "github.com/Microsoft/hcsshim/internal/layers" (which would probably be awesome to export as well...). And being a slice, I have to be able to name the type to make or append to it, as far as I know.

So I guess the obvious options going forward are:

• Export MountContainerLayers and similar wrappers for the other computestorage APIs that are wrapping things that require schema access.
• Export some APIs to build the required top-level schema objects, e.g. MakeLayerData(...) (*LayerData, err).
• Recreate (aliasing would be better, if that's possible in Go?) the relevant data structures and constants needed for these APIs. I suspect this was the intended direction, but it was only partly-done.
  • Why is LayerData defined manually, but Layer was exported by code-gen? LayerData is present in the schema. I see the same thing for the other types defined directly in computestorage: ExportLayerOptions, OsLayerOptions and OsLayerType.
• Lift the whole generated schema folder out of internal, as well as exposing any important helpers like SchemaV21().

I've only looked at computestorage, and perhaps the other use-cases are already covered by one of the above approaches, or some other approach that hasn't occurred to me.

I suspect that if I needed to do this now, I could generate the relevant JSON myself (I could run the codegen elsewhere, or just YOLO hand-roll it) and then unmarshall onto the visible LayerData struct, but that seems fairly user-unfriendly.

Side-question: Is https://github.com/MicrosoftDocs/Virtualization-Documentation/tree/master/hyperv-samples/hcs-samples/JSON_files the right source for the OpenAPI specs? All three claim to be "2.4" in their header, but the docs say they should be 2.1, 2.2., and 2.3 for 1809, 1903, and 2004 respectively. (The content looks correct, I just want to make sure I'm looking at the canonical schema specs)

Related side-question: Did anyone write down the swagger-codegen command used to generate the schema? I see lots of edits in the directory history, it's unclear if those were manual or if everyone knew the regenerate command, and just assumes everyone else does too.

Also, sadly #1004 merged before #930 and hence a single file was left in the old location which left me quite confused for a little while. Fix proposed in #1074

[dcantah]

Yea the layer concern is valid... The point of making the binds for computestorage was so we could eventually use all of these in containerd, so it's currently failing it's job somewhat :P

Truth be told, I don't know why the schema IS in internal, seems like it would easier to use this repo to interact with hcs if it was exported (whether that be for hobby or for a Containerd use case like this is mentioning). For much of the docker/v1 hcs code (in the root of the repo) that we rarely change, we just end up assigning the internal types to an exported one at the root anyways, albeit like I said this is only v1 definitions. https://github.com/microsoft/hcsshim/blob/master/container.go#L16-L43

Why is LayerData defined manually, but Layer was exported by code-gen? LayerData is present in the schema. I see the same thing for the other types defined directly in computestorage: ExportLayerOptions, OsLayerOptions and OsLayerType

This was likely an oversight and they should be generated and live in schema2 also.

I'll get back to this in the morning after talking to some folks.

[TBBle]

Sounds good. And I'm glad to see that type-aliases would work, if we wanted to go that way, like schema1. That would definitely be a short fix for my use-case, and since I'm using a hcsshim branch anyway, it would be easy to hack away at to determine if this is actually related to the problem I'm chasing.

And just now I learned the difference in Go between type alias, e.g. type ContainerProperties = schema1.ContainerProperties, and type definition, e..g. type favContextKey string, so thank you for that too. ^_^

[dcantah]

@kevpar @ambarve @anmaxvl @katiewasnothere Anyone have opinions here? Should we just type alias whatever is needed for computestorage like we do at the root of the repo for the v1 schema?

[TBBle]

Something I forgot to mention earlier, but it might be nice if the functional tests were less reliant on the internal tree, so that issues like this would be caught sooner.

It might make sense to disallow internal from inside tests, since that's already set up as a separate module, but I suspect that will reveal a bunch more API work to do. cmd would be worth considering for such a rule too.