Skip to content
This repository has been archived by the owner on Feb 24, 2020. It is now read-only.

*: expose application-level operations #2932

Merged
merged 1 commit into from
Aug 11, 2016

Conversation

tmrts
Copy link
Contributor

@tmrts tmrts commented Jul 14, 2016

To facilitate kubernetes#25899, new API extensions are proposed after taking the feedback in #2867 into account.

cc @euank @yifan-gu @philips @jonboulle @coreos/rkt-maintainers

so that it is ready to be started by the stage1 supervisor (e.g. systemd for the default flavor).

For example the operations would be as follows for the default rkt stage1 flavor:
- Prepare the application rootfs and its service files given an application image/images.
Copy link
Contributor

@sgotti sgotti Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This step is done by different stages:

  • stage0: Prepare the application rootfs: it can be: direct rendering or mount overlayfs over the treestore
  • stage1: Prepare the application service files given an application image/images.

@sgotti
Copy link
Contributor

sgotti commented Jul 14, 2016

@tmrts I putted some observation in the first commit. They are valid also for the updated commits. Should I recopy them?

Additionally:

  • Now the rkt run/prepare accepts a pod manifest instead of the various command line options (this is used by rktnetes). The podmanifest can override the image manifest app part (https://github.com/appc/spec/blob/master/spec/pods.md), I think there'll be the need to do something similar for the single app to be used by rktnetes. @yifan-gu ?

@tmrts
Copy link
Contributor Author

tmrts commented Jul 14, 2016

@sgotti thank you for the prompt review, I'm updating the proposal according to those. You don't have to copy the comments, they are not lost.

Keep the constructive comments coming 😄

```

## rkt app eject
Ejects an application image into a running pod, meaning that the traces of it are removed from the pod.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are traces concretely?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be better reworded as leftover resources in the context of garbage collection

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So traces are removed during eject, or leftover during eject and require a separate GC? I don't quite follow

Copy link
Member

@alban alban Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • the app rootfs (in /opt/stage/appname)
  • some mounts from volumes for the apps (such as in /opt/stage/appname/data)
  • some other mounts (such as in /opt/stage/appname/dev/null)
  • the systemd .service files (appname.service, reaper-appname.service)
  • some misc files (/rkt/myapp.env, /rkt/status...)
  • anything else?

Those can be removed synchronously when running rkt app eject. We should not need a GC for apps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll update the document accordingly


`coreos.com/rkt/stage1/app/inject`

1. prepare the application intended for injection, its rootfs and service files or else.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structure of this document is confusing; this section is ostensibly about entrypoints but this behaviour here is clearly pre-entrypoint. Can you just combine them for now, e.g. have one "app inject" section that describes what the stage0 and stage1 each do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to separate the implementation and the subcommand API, I'll make the boundary more visible

@jonboulle jonboulle changed the title stage1: expose application operations *: expose application-level operations Jul 14, 2016
```

## rkt app stop
Stops a running application gracefully.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean to stop it gracefully?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I think I found it below (such confusion could be mitigated by https://github.com/coreos/rkt/pull/2932/files#r70785013)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a note pointing to the explanation

@jonboulle
Copy link
Contributor

Since this is a significant change in pod architecture and lifecycle, I'd like to see some language around what it means for an app to be in a pod (e.g. is an injected pod identical as apps that are already in the pod, in terms of environment etc? do we expect semantic differences between them? how does it affect shutdown lifecycle of a pod?)

```

## rkt app exec
Executes a command inside an application.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does inside an application mean? (Can probably steal language from rkt exec for this - since AIUI this should be semantically identical)

Copy link
Contributor Author

@tmrts tmrts Jul 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to correct it as application container, did you mean to say rkt enter?

@tmrts
Copy link
Contributor Author

tmrts commented Jul 14, 2016

@jonboulle the plan is to have injected apps that are identical to the normal apps inside a pod and it shouldn't affect the shutdown workflow (but we might need to reconsider terminating empty pods)

@tmrts
Copy link
Contributor Author

tmrts commented Jul 14, 2016

I addressed every comment so far PTAL @sgotti @jonboulle @alban

@tmrts tmrts force-pushed the stage1/app-level-operations branch 3 times, most recently from 25041b3 to 713e7f8 Compare July 14, 2016 14:45
`coreos.com/rkt/stage1/app/add`

1. Receive the rootfs, the manifest and the volumes of the prepared app.
2. `coreos.com/rkt/stage1/add` entrypoint is resolved via annotations found within `/var/lib/rkt/pods/run/$uuid/stage1/manifest`.
Copy link
Contributor

@sgotti sgotti Aug 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't pass the app manifest to a pod but I'll maintain the current implementation where only the pod manifest is used (since you don't need the full app manifest but just the app part and this one is copied from the app manifest to the pod manifest by the stage0 prepare).

Also if a draft I think this is an important point to define and, as suggested by @alban here I'll go for updating (atomically) the pod manifest adding/removing the apps.

(This will also make things like #3039 working without making this proposal appc only)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @alban on his comment, I must've missed it when reworking the proposal, thanks!

@tmrts tmrts force-pushed the stage1/app-level-operations branch from f212098 to 0dee9c0 Compare August 10, 2016 09:23
@tmrts tmrts force-pushed the stage1/app-level-operations branch from 0dee9c0 to a642c6c Compare August 10, 2016 10:24
@sgotti
Copy link
Contributor

sgotti commented Aug 10, 2016

I also think this is in a good shape for starting the implementation. Many additional questions and changes will probably emerge during the development (for example how to handle atomic state transitions, handle failed/killed app prepare/rm, concurrent operations on the same app etc...).

@tmrts
Copy link
Contributor Author

tmrts commented Aug 10, 2016

@sgotti thanks, this doc will change a lot during the implementation I agree.

@alban feel free to review and merge since others @euank and @yifan-gu already LGTM'd it

**Note:** Not every pod will be injectable and it will be enabled through an option.

## rkt app rm
Removes an application image into a running pod, meaning that the leftover resources are removed from the pod.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it require the app to be stopped before (with rkt app stop)?

Copy link
Contributor Author

@tmrts tmrts Aug 11, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alban
Copy link
Member

alban commented Aug 11, 2016

Thanks! I agree with the discussion above, the other additional questions can be addressed during the implementation.

LGTM

@alban alban merged commit 38875d8 into rkt:master Aug 11, 2016
@tmrts tmrts deleted the stage1/app-level-operations branch August 11, 2016 11:52
@tmrts
Copy link
Contributor Author

tmrts commented Aug 11, 2016

@alban thanks I also answered the questions.

Thanks for the reviews @euank @yifan-gu @alban @lucab @sgotti 👏

s-urbaniak pushed a commit to s-urbaniak/rkt that referenced this pull request Aug 31, 2016
This adds a new subcommand `app init` to create an initial empty pod.

Follows rkt#2932
s-urbaniak pushed a commit to s-urbaniak/rkt that referenced this pull request Aug 31, 2016
This adds a new subcommand `app init` to create an initial empty pod.

Follows rkt#2932
s-urbaniak pushed a commit to s-urbaniak/rkt that referenced this pull request Sep 1, 2016
This adds a new subcommand `app init` to create an initial empty pod.

Follows rkt#2932
s-urbaniak pushed a commit to s-urbaniak/rkt that referenced this pull request Sep 1, 2016
This adds a new subcommand `app init` to create an initial empty pod.

Follows rkt#2932
s-urbaniak pushed a commit to s-urbaniak/rkt that referenced this pull request Sep 1, 2016
This adds a new subcommand `app sandbox` to create an initial empty pod.

Follows rkt#2932
alepuccetti pushed a commit to kinvolk/rkt that referenced this pull request Sep 6, 2016
This adds a new subcommand `app sandbox` to create an initial empty pod.

Follows rkt#2932
antoni pushed a commit to intelsdi-x/rkt that referenced this pull request Sep 9, 2016
This adds a new subcommand `app sandbox` to create an initial empty pod.

Follows rkt#2932
Wilybald pushed a commit to Wilybald/rkt that referenced this pull request Sep 11, 2016
This adds a new subcommand `app sandbox` to create an initial empty pod.

Follows rkt#2932
@lucab lucab unassigned euank and yifan-gu Apr 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants