rkt: implement generic no volume policy #3102
Conversation
This patch implements a generic no volume policy instead of a custom, per stage1, behavior based on a docker2aci specific app annotation. This also avoids stage1 having to read the app image manifest but just uses the pod manifest as its unique source for app setup. Since the behavior on unspecified volumes is not defined in the appc/spec but left to the ACE, the idea is to use a pod.RuntimeApp annotation to define the no volume policy per runtime app (instead of extending the appc PodManifest with rkt specific fields). The annotation is called `coreos.com/rkt/annotations/novolumepolicy` and can have two possible values: * `empty`: create an empty, per app, volume. * `empty-copy`: create an empty, per app, volume and copy image contents. `empty` is the default if the annotation doesn't exists. rkt, in the stage0, will check if the annotation is already provided by the pod manifest and, in such case, only validate it. If not provided and the image has been converted using docker2aci then an annotation with value `empty-copy` will be defined. This has the additional effect to letting the user define (and override for docker2aci converted images) the no volume policy for every app (with this patch only by providing a --pod-manifest, in future, if needed, a per app command line flag could be added) Note that only pod.RuntimeApp annotations are checked for `coreos.com/rkt/annotations/novolumepolicy` and not a app manifest annotation (this, to correctly work, requires rkt#3100). **Notes** Before this patch an user provided pod manifest was only validated without changing it, now, instead, it's also enhanced.
|
Do we actually have usecases for all those cases? If not, I think this could be a good moment for some cleaning and uniforming the behaviors. The safest option would be to validate the pod manifest and just abort if volumes are not matching. However it seems that Docker is behaving in a different way (described here), which we are trying to mimic. As such, I think it makes sense to clarify in appc that mountpoints not covered by any volumes have the |
|
Also, semaphore is failing because KVM doesn't have the concept of stage1 return value: #2777 |
|
There may be actually more to it. According to appc pod spec: |
|
@lucab That's an interesting point. My view, given also the next sentence This change raises a question:
Then we should define, as you said above, if just one no volume policy is enough or different policies should be available ( Related to this point the question is: how to, if needed, make the user choose the no volume policy (per app). Do like in this patch providing a runtimeApp annotation (for user specified podManifests) and a cli option or other solutions? |
I think this should be the way to go to reify incomplete/template manifests.
If k8s is not relying on this implicit behavior, we should stick to the spec and properly validate that all mountpoints are covered by a volume. Otherwise, this should be made explicit somewhere and whatever is producing the runtime manifest for us should be adapted first. /cc @euank for insights
This looks like a corner case enough, that I would avoid exposing another knob just for it. Just to have the possibility of getting a docker-like behavior we can discuss about a kind= |
|
@sgotti bumping this to the next release. |
|
ping @jonboulle It looks like last time we encountered it (#2315 (comment)) we special-cased it, and it's now coming back. Proposal here is to have another special casing, and I feel this will come back at a later point. Given that this kind of guest-filled volumes are there in the wild and we have to provide compatibility for it, can we capture this semantics in appc? |
This patch implements a generic no volume policy instead of a custom, per
stage1, behavior based on a docker2aci specific app annotation.
This also avoids stage1 having to read the app image manifest but just uses the
pod manifest as its unique source for app setup.
Since the behavior on unspecified volumes is not defined in the appc/spec but
left to the ACE, the idea is to use a pod.RuntimeApp annotation to define the
no volume policy per runtime app (instead of extending the appc PodManifest
with rkt specific fields).
The annotation is called
coreos.com/rkt/annotations/novolumepolicyandcan have two possible values:
empty: create an empty, per app, volume.empty-copy: create an empty, per app, volume and copy image contents.emptyis the default if the annotation doesn't exists.rkt, in the stage0, will check if the annotation is already provided by the pod
manifest and, in such case, only validate it.
If not provided and the image has been converted using docker2aci then an
annotation with value
empty-copywill be defined.This has the additional effect to let the user define (and override for
docker2aci converted images) the no volume policy for every app (with this
patch only by providing a --pod-manifest, in future, if needed, a per app
command line flag could be added)
Note that only pod.RuntimeApp annotations are checked for
coreos.com/rkt/annotations/novolumepolicyand not an app manifestannotation (this, to correctly work, requires #3100).
Notes
Before this patch an user provided pod manifest was only validated without
changing it, now, instead, it's also enhanced.
Questions
nonebe implemented to define the fly behavior?emptyandempty-copybehaviors (if it this makes sense).This fixes one part of #3098.