rkt: experimental support for pod sandbox #3318
Conversation
Since 'rkt stop' is idempotent, 'rkt stop' on an already stopped pod should not return error.
When the pod is stopped, don't call app-add/app-rm entrypoint because there is not much we can/need to do when the whole pod is stopped.
Also add annotations, startedAt in the json definition 8000 of the pod.
This enables injecting user-defined pod level labels and annotations.
This documents and autodetects stage1 mutable cabability detection. It is needed for stage1 images which do not support mutable operations, and retains backwards compatibility with old stage1 images.
… flags. Add these flags for 'rkt app add', 'rkt run', 'rkt prepare'.
It will be used in more places.
If we try to stop an app that doesn't have a service file (either because it was not started or because the starting failed) we should give an appropriate error message. Also, since `app rm` calls the stop entrypoint, we should identify this error and continue removing the app.
Also removed unneccessary flag conflict checks for --pod-manifest v.s. other flags (--name, --environment, etc) because they are already taken care of since they all need to follow an image.
Currently we set up the app's service during app-start which is not the right place. This implements the proper logic to set up systemd service files, as well as any remounts of cgroups during app-add.
This removes some duplicated code in AddApp which was previously copied over.
This introduces a dedicated RmConfig synchronous to all the other *App methods.
This specifies the synchronization of pod mutation operations using a file lock.
|
+1 on moving back to master, perhaps with app subcommands hidden behind a On 26 October 2016 at 14:54, Sergiusz Urbaniak notifications@github.com
|
|
CI failure is a known flake in |
Squashed commit of the following: commit 0b348ab71a2b3a5a576a1129458487fb957f0e43 Merge: ce991fc 2ef0885 Author: Casey Callendrello <c1@caseyc.net> Date: Fri Oct 7 15:08:00 2016 +0200 Merge remote-tracking branch 'origin/cri' into cri-volumes commit ce991fcf1bbce5fb22b519e88eb0f0cfd6dbaca6 Author: Casey Callendrello <c1@caseyc.net> Date: Tue Oct 4 20:24:59 2016 +0200 WIP: support app volumes commit dca86e7 Merge: 081300c b9ddcc6 Author: Casey Callendrello <c1@caseyc.net> Date: Tue Oct 4 12:23:06 2016 +0200 Merge remote-tracking branch 'origin/cri' into cri-volumes commit 081300c Author: Alban Crequy <alban@kinvolk.io> Date: Thu Sep 22 12:45:32 2016 +0200 cri: mount volumes in the app This requires systemd with the following fix: systemd/systemd#4152
Fixes partially rkt#3242
|
Regarding hiding those app subcommands, cobra already has a simple Hmm ... I think this is "good enough" for hiding them, or do we additionally want an activation env var? @lucab |
|
@s-urbaniak I'd suggest to mark the whole app subcommand as hidden and behind an activation env flag until we are comfortably happy with the UX and we have some tests around it. |
This also introduces a common.IsExperimentEnabled function for querying experiments in rkt.
|
+1 On 1 November 2016 at 09:31, Luca Bruno notifications@github.com wrote:
|
|
|
||
| #### Arguments | ||
|
|
||
| `start $OPTIONS UUID APPNAME ENTERENTRYPOINT PID` |
There was a problem hiding this comment.
$OPTIONS should be documented (they are mostly enterexec related). Moreover, there is a TODO entry to make the remaining flag named one (instead of positional arguments).
|
|
||
| If the annotation is not present, `false` is assumed. | ||
|
|
||
| ### Versioning |
There was a problem hiding this comment.
Version interface should also be bumped once we agree on entrypoints interface.
| @@ -102,6 +102,13 @@ func PodManifestPath(root string) string { | |||
| return filepath.Join(root, "pod") | |||
| } | |||
|
|
|||
| // PodManifestLockPath returns the path in root to the Pod Manifest lock file. | |||
| // This must be different from the PodManifestPath since mutations on the pod manifest file | |||
There was a problem hiding this comment.
What is "this" here? Are we talking about the fact that the lockfile must be a new specific one and we can't re-use one of the other lock types we have?
There was a problem hiding this comment.
Ok, I guessed so but the comment may be a bit more explicit as casual readers won't have the whole context in mind.
There was a problem hiding this comment.
agreed, as a self-note, I should add more rationale in this section of the pod-lifecycle.md https://github.com/coreos/rkt/pull/3318/files#diff-7f4a1c7974386fb54394c2fd1f91e9d9R44 document
| @@ -95,11 +96,6 @@ func newApp(ra *schema.RuntimeApp, podManifest *schema.PodManifest, pod *pkgPod. | |||
| }) | |||
| } | |||
|
|
|||
| // Generate annotations. | |||
| for _, anno := range ra.Annotations { | |||
| app.Annotations[anno.Name.String()] = anno.Value | |||
There was a problem hiding this comment.
@squeed aren't we loosing app-level annotations here (not speaking about user ones, but the ones coming with the image itself)?
There was a problem hiding this comment.
The change was introduced in 348facd, where App.Annotations was renamed to App.CRIAnnotations, and later to App.UserAnnotations.
We are setting them directly without iteration now here https://github.com/s-urbaniak/rkt/blob/crisync/lib/app.go#L64
There was a problem hiding this comment.
Ok, thanks for digging through history. I think we should double-check this later and verify that we are not messing between internal annotations and user annotations at any point, but from a quick glance this should be fine.
| @@ -139,7 +135,7 @@ func appState(app *App, pod *pkgPod.Pod) error { | |||
| } | |||
|
|
|||
| app.State = AppStateCreated | |||
| createdAt := fi.ModTime().UnixNano() | |||
There was a problem hiding this comment.
@lucab Good catch, the change happens before CRI decides on using nano seconds.
We need to change it back to unix nano.
There was a problem hiding this comment.
yeah, this breaks https://github.com/coreos/rkt/pull/3318/files#diff-6e87d9abadd3d0ece359c1a0d15ef2b5R47 which is expecting nano
| cmdAppStart = &cobra.Command{ | ||
| Use: "start UUID --app=NAME", | ||
| Short: "Start an app in a pod", | ||
| Long: `Start appz!`, |
| return 1 | ||
| } | ||
|
|
||
| if flagAppName == "" { |
There was a problem hiding this comment.
Should app-start implement the "if there is only one app, use that" semantic that we have in other places (like enter)?
There was a problem hiding this comment.
We could, that sounds like a nice addition. If we intend the app subcommands to be first-class for end-users, I'm all for this feature. For the current use-case of machine-only usage (rktlet) I'd say it is even good to err out.
| return 1 | ||
| } | ||
|
|
||
| // TODO(yifan): Print yamls. |
There was a problem hiding this comment.
I'd say is a non-TODO, for the moment, unless some k8s consumer needs it.
| cmdAppStop = &cobra.Command{ | ||
| Use: "stop UUID --app=NAME", | ||
| Short: "Stop an app in a pod", | ||
| Long: `Stop appz!`, |
| _, appStartOk := s1m.Annotations.Get(appStartEntrypoint) | ||
| _, appStopOk := s1m.Annotations.Get(appStopEntrypoint) | ||
|
|
||
| return appRmOk && appStartOk && appStopOk, nil |
There was a problem hiding this comment.
This seems to be lacking a check on "app/add" entrypoint.
This reintroduces nanosecond units for reporting the app state.
|
LGTM. There are clearly still a lot of comments and TODO to be addressed, but new features are gated behind an experimental flag and we didn't introduce any regression. So I think we can move forward here by landing this and addressing concerns in followup PRs. @s-urbaniak @jonboulle @squeed can you have a review pass on this and check if you spot any blockers? |
|
There was some concern about the experimental interface changes not being documented well enough, so I put together s-urbaniak#4 to start polishing and documenting the app and attach entrypoints, PTAL. Once that PR is merged, anything else blocking this? |
This commit introduces and unifies "crossing entrypoints" for app and attach commands, defining an experimental interface v5 (yet to be finalized).
stage1: document experimental interface v5
|
LGTM |
|
👍 |
|
All green, merging. |
This PR merge the
cribranch back into master, introducing an experimentalappsubcommands. This is the second sync-point for CRI tasks (the first one was #3218).This introduces the concept of a "pod sandbox", i.e. a mutable pod where applications can be added/started/stopped/removed while the pod is already running.
This feature and the corresponding v5 stage interface are currently marked as experimental, and will undergo a stabilization process in the next release cycles. This also applies at the CLI level, where the
appsubcommand is gated by default behind an environment flag.Closes: #3135
Closes: #3136
Closes: #3137
Closes: #3153