-
Notifications
You must be signed in to change notification settings - Fork 593
runtime: Clarify UTS and mount cleanup on 'delete' #651
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
abf8855 to
083f23e
Compare
|
@dqminh Want to take a look? This needs another maintainer. |
runtime.md
Outdated
| On Linux, [`mounts`](config.md#mounts), [`root`](config.md#root), [`linux.devices`](config-linux.md#devices), and other filesystem changes belong to the container which created the [container mount namespace][container-namespace3]. | ||
| If a container joins an existing mount namespace and applies those settings, deleting the container MUST NOT reverted the settings. | ||
| Similarly [`hostname`](config.md#hostname) changes belong to the container which created the container UTS namespace. | ||
| [`linux.resources`](config-linux.md#control-groups) settings belong to the container which created the associated cgroup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont know, this seems overly specified. I think the concept of "deleting resources owned only by the container" makes more sense, i.e. you dont delete changes to shared resources (i.e. when container make changes to a shared namespace ).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the concept of "deleting resources owned only by the container" makes more sense…
I tried to cover that generically in the preceding paragraph. This paragraph gives some explicit examples to illustrate the concept and avoid ambiguity about who owns a given resource. Do you feel that the preceding paragraph is sufficiently clear without these more-detailed examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleting a container MUST delete the resources that were created during the `create` step.
Resources associated with the container, but not created by this container, MUST NOT be deleted.
For ownership concept, it is reasonably enough for me.
I think its not harmful to have more examples to illustrate the point, but they need to be clearly spelled out as example so they wont be mistook as the only cases that we will support.
WDYT about something like:
In the following example, when the container joins an existing mount namespace on Linux and changes root with the following config.json
{
root: /new/root,
linux: { namespaces: [ {type: mount, path: /proc/1234/ns/mnt} ] },
}
deleting the container MUST not remove the existing mount namespace, and MUST not undo the changes to root
In the following example, when the container joins an existing UTS namespace on Linux and changes hostname with the following config.json
{
hostname: runtimespec,
linux: { namespaes: [{type: uts, path: /proc/1234/ns/uts} ] }
}
deleting the container MUST not remove the existing UTS namespace, and MUST not undo the changes to hostname.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about something like…
I'm fine with a bit more hand-holding like that. To be absolutely clear that these examples were informative (and not adding new normative restrictions), I've reworded them to avoid MUST and split them off into new subsections with 083f23e → 52a5199. I've also added a similar cgroup example so we don't focus exclusively on namespaces. And I've also filled out the configs because I don't like fragments (although we have an existing fragment example here). I personally prefer the compactness of 083f23e, but maybe maintainers prefer 52a5199 or a sweet spot somewhere in between? If you want something in the middle, please give me a list of corners to cut (e.g. “drop the subsections” or “use JSON fragments for the configs”).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
083f23e to
52a5199
Compare
Now that d43fc42 (config-linux: Lift no-tweaking namespace restriction, 2017-01-11, opencontainers#649) allows us to get into this sort of situation. This sort of ownership may also apply to other resources (cgroups?), but we can handle them in follow-up commits. Also drop "Configuration" from the root header. Everything in that file is a configuration. container-namespace3 (instead of container-namespace) supports the single-page, Pandoc-generated file (see e7be40f, Cleanup the spec a bit to remove WG/git text that's not really part of the spec, 2016-11-14, opencontainers#626). Using an informative suggestion was recommended by Dao Quang Minh [1]. I've made the config JSON as small as possible while keeping it valid, but there's still an unfortunate amount of boilerplate there. There is in-flight work to let us at least drop process.args [2]. [1]: opencontainers#651 [2]: opencontainers#620 Signed-off-by: W. Trevor King <[email protected]>
Now that d43fc42 (config-linux: Lift no-tweaking namespace restriction, 2017-01-11, opencontainers#649) allows us to get into this sort of situation. This sort of ownership may also apply to other resources (cgroups?), but we can handle them in follow-up commits. Using an informative suggestion was recommended by Dao Quang Minh [1]. I've made the config JSON as small as possible while keeping it valid, but there's still an unfortunate amount of boilerplate there. There is in-flight work to let us at least drop process.args [2]. The new mount namespace in the UTS example avoids pivoting the host namespace's root. Also drop "Configuration" from the root header. Everything in that file is a configuration. container-namespace3 (instead of container-namespace) supports the single-page, Pandoc-generated file (see e7be40f, Cleanup the spec a bit to remove WG/git text that's not really part of the spec, 2016-11-14, opencontainers#626). [1]: opencontainers#651 [2]: opencontainers#620 Signed-off-by: W. Trevor King <[email protected]>
Expanding on the creater-owns-the-resource cases from the previous commit. Signed-off-by: W. Trevor King <[email protected]>
52a5199 to
eeacccd
Compare
| } | ||
| ``` | ||
|
|
||
| When the example container is deleted, neither removing the preexisting mount namespace nor undoing the pivot into `rootfs` are allowed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think deleting namespace is a bit overly specified, it probably is not true since on Linux, namespace is destroyed when the last process that is a member of the namespace terminates.
So if we join a shared namespace and the original container exits, our container's terminates will delete this namespace, and that's something we should not interfere.
crosbymichael
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just too much over-specifying things and underlying system implementation. Its not needed in the spec.
REJECT
| Deleting a container MUST delete the resources that were created during the `create` step. | ||
| Resources associated with the container, but not created by this container, MUST NOT be altered. | ||
|
|
||
| #### Examples of resource ownership |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why we should have examples like this in the spec at all. Its too much overspecifying things and your descriptions are wrong anyways on how namespaces are "destroyed".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My example I know we have examples in the repo showing off how to populate fields but examples like this describing functionality/implementation don't belong in my opinion.
|
Closing because we should not have implementation specific examples detailing functionality like this in the spec. The original two sentences are enough and provide the context required for runtime authors on their platforms. |
Now that #649 allows us to get into this sort of situation. This sort of ownership may also apply to other resources (cgroups?), but we can handle them in follow-up PRs (or here, I don't mind).
Also drop “Configuration” from the root header. Everything in that file is a configuration.