Closed sameo closed 7 years ago
cc @mcastelino @sboeuf @jodh-intel @grahamwhaley
@sboeuf
Did I miss something explaining that we MUST implement this in virtcontainers ?
No you did not miss anything and this could be done by only modifying utils/oci.go
. However, I did not want to do something OCI specific and I wanted the code to be generic and specification agnostic. I also realized it would be good to have a generic asset handling API for virtcontainers.
So it makes the code a little complex but I think it's cleaner.
Also, one thing I was considering is adding private fields to HypervisorConfig
for not overwriting the public paths passed from above. I think it would be cleaner.
Well, I think the problem is to have two ways to setup the same thing from virtcontainers perspective. We should not give the choice between either the Pod annotations, or the HypervisorConfig structure to the consumer of virtcontainers, otherwise we're just bringing confusion IMO. Now, if you say we want to modify HypervisorConfig so that all its fields are private, then the annotations make sense since this would be the only way to setup the kernel/image. But there's one issue about that, we cannot make those fields private because this structure is stored to our filesystem through a json marshalling and it needs those fields to be public.
Now, if you say we want to modify HypervisorConfig so that all its fields are private, then the annotations make sense since this would be the only way to setup the kernel/image. But there's one issue about that, we cannot make those fields private because this structure is stored to our filesystem through a json marshalling and it needs those fields to be public.
Oh, don't get me wrong: I don't want to have all HypervisorConfig
fields private, only the ones we'd use to set assets not defined by the caller directly.
So I fully agree things could be done from the oci utils package or from the runtime directly, and it would make sense if the definition of alternative assets would be part of the OCI spec (Which may eventually become reality). In that case, we'd just use the OCI spec and translate that into an HypervisorConfig
setting.
But here I'm looking at the problem from a k8s point of view where the only thing we're going to be able to pass in to the runtime or virtcontainers are pod annotations. We can decide to translate those into HypervisorConfig
fields in several places. In generalization order:
virtcontainers/pkg/oci
: This becomes OCI specificvirtcontainers/assets.go
: This becomes virtcontainers specificSo by adding a customAssets []asset
field to HypervisorConfig
or even to PodConfig
, we have a generic implementation (not OCI or CC specific) and it is transparent to the user: There is still only one way to pass a configured set of assets (through HypervisorConfig
) and Annotations would only allow the higher layers (Docker CLI, k8s) to overwrite the runtime configured assets.
@sameo so you're saying that you want something generic for those annotations, but as of today, k8s relies on CRI which relies on OCI. That's why I consider this has to be handled at the OCI level and not at the virtcontainers level. But I think I understand that you want to be able to have any orchestrator using those annotations in order to give specific info to virtcontainers. The problem that I see here is that all orchestrations use a runtime (and hence a specification file). This means that we are creating a generic thing, but in practice, we will have to translate those annotations to PodConfig annotations in every runtime implementation.
You have some golint errors. Let's also wait for @jodh-intel review now that you have addressed his comments. LGTM
I'll merge this one once the 17.04 CI is fixed.
Based on the pod config annotations, we change the underlying hypervisor configuration. Pod config annotations can carry a kernel, image, hypervisor and firmware paths. Those annotations, if valid, will supersede the hypervisor configuration paths. If not valid, virtcontainers will error out.
Optionally, asset SHA-512 hashes can be passed through pod annotations as well. In that case virtcontainers will verify that the asset binary matches the hash annotation and error out when it does not.
More details on the architecture in #439
Fixes #439
TODO:
pkg/oci/utils.go
to convert an OCI annotations into aPodConfig
one for assets