Allow systemcontainers to install, run, and uninstall in one go by ashcrow · Pull Request #952 · projectatomic/atomic

ashcrow · 2017-03-24T14:46:58Z

This change adds a new optional label called atomic.run. When set to once the image will be extracted, run, and cleaned up. This currently requires the use of --system.

The idea behind this new option is that there are cases where a system container would not be a full fledged service on a host. For example, the openshift-ansible installer isn't a service, but a containerized installer.

Example:

$ sudo atomic pull --storage ostree docker:testcontainer:latest
$ sudo atomic install --system testcontainer:latest
Extracting to /ostree/repo/tmp/atomic-container/15926/rootfs
HI
$ ls /ostree/repo/tmp/atomic-container/15926/rootfs
ls: cannot access '/ostree/repo/tmp/atomic-container/15926/rootfs': No such file or directory
$

/cc @sdodson @tbielawa

giuseppe · 2017-03-24T15:28:32Z

Atomic/syscontainers.py

+                util.call(start_command)
+            finally:
+                # Uninstall the image post run
+                self.uninstall(name)


when I firstly suggested this I forgot we had an extract. Sorry :)

At some point we will need an OSTree temporary storage (also to fix mount) but for now I think it would be better if we can do something like:

try: self.extract(img, tmpdir) generate_systemd_directives(...) os.chdir("$tmpdir/rootfs") util.call(...) finally: shutil.rmtree(tmp)

tmpdir is better created under the OSTree repository so we are sure to use hard links and the checkout is faster. The _try_ostree_mount function in mount.py does that (we will need to copy/move it to syscontainers). Anyway, I'll have to fix this for the RPM stuff as well so feel free to ignore this part.

What do you think?

I see in your TODO list that there is to create the runc directives for --user. That should be already done by generate_systemd_startstop_directives now to use bwrap-oci so there should not be anything to do for this.

What do you think?

SGTM. I'll update this afternoon.

I see in your TODO list that there is to create the runc directives for --user. That should be already done by generate_systemd_startstop_directives now to use bwrap-oci so there should not be anything to do for this.

Perfect!

rhatdan · 2017-03-24T18:54:02Z

We keep making changes to system containers that force the user to have some kind of knowledge of how to install the image, can we remove the option and just put something like a label into the container to do a --oneshot? Force the image developer to define how to install his image and allow the atomic command to react. I want the same thing for --system.

If I do
atomic install foobar

And foobar is intended to be a --oneshot --system container I will be prone to forget those options.

ashcrow · 2017-03-24T19:49:44Z

@rhatdan that's a really good point. If --system is going to make that change --oneshot should be done that was as well. @giuseppe if you're on board with this as well we can work on this together if you'd like.

giuseppe · 2017-03-26T11:31:34Z

@ashcrow, sure! A container that is designed to be "oneshot" probably makes no sense at all to have it normally installed so a label that indicates this behaviour can make this easier for the user.

ashcrow · 2017-03-27T13:32:48Z

@giuseppe I'll pull the option code out of this PR, take a look at how labels are used, and see if I can't put something together. I'll probably bug you a bit 😆

ashcrow · 2017-03-27T16:02:59Z

I created an atomic.run label which, if used and set to once will extract, run, and clean up after the extract.

Labels

LABEL "Name"="atomic-test-system"\
      "atomic.run"="once"\
      "atomic.type"="system"

Run

$ sudo python atomic install --system o:latest
Extracting to /tmp/oIg9Ulv/rootfs
HI
$ ls /tmp/oIg9Ulv/rootfs
ls: cannot access '/tmp/oIg9Ulv/rootfs': No such file or directory

Question

@giuseppe I noticed that using atomic.type as system didn't react the same was as using the --system option. Is this by design?

giuseppe · 2017-03-27T19:16:21Z

@ashcrow Nice!

We should probably add a check for the type of container also for already pulled images, I guess you are pulling it directly from the local Docker engine? We do that now for images pulled from a registry.

I've added a fixup commit here: https://github.com/giuseppe/atomic/tree/oneshot

I have not tested it, just wanted to share it quickly.

I've done some small changes, also added the possibility of generating the configuration from the config.json.template file and support for systemd tmpfiles, if the container needs to bind directories/files not present on the file system when it starts.

Some code is deliberately duplicated in the new function, as it will save some merge conflicts with #949.

I also like the new path for _run_once as in the future I'd like we treat these containers separately: we will have a "layers storage" that we can use for mounting images and at the same time for running one shot containers since they are managed by atomic, we can setup the overlayfs for mounting the layers, and proceed with the cleanup once it is done.

Another change is to use a tmpdirectory under the OSTree repository, so we use hard links instead of copying the files. I think at this point all the features we need to support are there.

What do you think?

ashcrow · 2017-03-27T19:34:43Z

@giuseppe I'll pull in and test.

yuqi-zhang · 2017-03-27T20:36:51Z

@ashcrow with regards to "atomic.type=system" vs "install --system" there are some discrepancies. I added a PR here to address some issues: #954

ashcrow · 2017-03-27T21:03:16Z

@yuqi-zhang thanks for the info!

@giuseppe I'm thinking that inspect_system_image won't end up being enough. The failures CI is finding is related to image names such as "docker:atomic-test-system:latest" which the inspect_system_image doesn't seem made for. Any pointers before I dig deeper in the code?

giuseppe · 2017-03-27T23:45:09Z

@ashcrow let's add another exception for now not image.startswith('dockertar:/') and not (image.startswith("docker:") and image.count(':') > 1).

_install has the code for pulling the image and then use the canonical name for the image, we can move the new code there once #949 gets merged and we don't end up in merge conflicts.

giuseppe · 2017-03-28T09:22:22Z

I've added another fixup commit that solves the issue and a few other things. There is only one missing part now, and that we need to set the correct values or the installation with template files will fail. Now this code is in _do_checkout .

This requires some refactoring as the code in common must be moved out of _do_checkout, but I'd prefer if we do this after: #949 as it changes a lot of stuff around there.

The next failure in the tests is fixed by 10ba9dc (which is part of #949). No idea how this could get broken.

giuseppe · 2017-03-28T10:05:15Z

once 949 gets merged, we can use this commit here to refactor out the generate values function and use from here as well:
https://github.com/giuseppe/atomic/tree/move-read-values

ashcrow · 2017-03-28T18:00:21Z

Local testing worked as expected.

giuseppe · 2017-03-28T19:02:47Z

yes, I've done some testing as well and it worked for me (except handling template files). We should probably add the oneshot image to the testing images as well.

ashcrow · 2017-03-28T19:04:23Z

@giuseppe I'll add that shortly. I'll also stage a Labels document for the atomic-system-containers repo.

giuseppe · 2017-03-28T19:06:52Z

@ashcrow thanks, could the image include a config.json.template file instead of config.json?

ashcrow · 2017-03-28T19:10:01Z

@giuseppe the test image? Sure.

rh-atomic-bot · 2017-03-28T19:28:05Z

☔ The latest upstream changes (presumably 1b216a4) made this pull request unmergeable. Please resolve the merge conflicts.

ashcrow · 2017-03-28T20:01:36Z

Added a Dockerfile for the change and rebased.

ashcrow · 2017-03-28T20:58:47Z

I'll add to integration tests an look at your added patch in the morning @giuseppe.

ashcrow · 2017-04-07T18:39:57Z

This PR is ready for review.

giuseppe · 2017-04-10T13:28:59Z

LGTM

ashcrow · 2017-04-11T14:09:35Z

/cc @baude

baude · 2017-04-11T14:14:27Z

Atomic/syscontainers.py

            if not os.path.exists(rootfs):
                os.makedirs(rootfs)
-
+        # XXX


Yes, good catch. Will update.

baude · 2017-04-11T14:17:06Z

Atomic/syscontainers.py


    def install(self, image, name):
+        """
+        External container install logic.


I know we are not outwardly changing at of the interfaces to the users which usually prompts someone to do updates a docs/ file. I'm thinking however it might be worth noting this change and the labels in the docs somwhere.

Right now there are 3 (or more?) places that document labels. I'm planning on adding it in the Atomic System Containers repo and, as it is used, should propagate to wherever becomes the proper place. Or at least that is the theory.

baude · 2017-04-11T14:19:13Z

@ashcrow @giuseppe This might be out of scope but I would really love a mechanism for systemcontainers that would allow us to copy files from the system container to the host filesystem during install. This seems like the right place for something like that. Not sure.

When the atomic.run is provided and set to "once" the install method will extract, run, and clean up after the container. This change also splits install logic into an internal and external method.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

ashcrow · 2017-04-11T14:25:52Z

Removed the leftover and rebased (for good measure).

⬆️

rhatdan · 2017-04-12T21:36:21Z

@baude Ready to merge?

baude · 2017-04-13T18:32:23Z

@rh-atomic-bot r+ 6887eab

rh-atomic-bot · 2017-04-13T18:32:27Z

⌛ Testing commit 6887eab with merge 5b5dffa...

Closes: #952 Approved by: baude

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #952 Approved by: baude

Closes: #952 Approved by: baude

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #952 Approved by: baude

rh-atomic-bot · 2017-04-13T19:00:05Z

☀️ Test successful - status-redhatci
Approved by: baude
Pushing 5b5dffa to master...

See projectatomic/atomic#952

See projectatomic/atomic#952 Closes: #58 Approved by: yuqi-zhang

After projectatomic/atomic#952 it's now possible to add a label to an image atomic.run=once so that when installed as a system container it will be run immediately with runc (no systemd involved) and then uninstalled. This use case matches containerized Ansible playbooks very well, so we add that label in playbook2image so that the usage experience of packaged playbooks run as system containers matches standalone container runs.

ashcrow force-pushed the oneshot branch from fc307b7 to 70c6abf Compare March 24, 2017 14:48

giuseppe reviewed Mar 24, 2017

View reviewed changes

ashcrow force-pushed the oneshot branch from db270a8 to d4d23bb Compare March 27, 2017 16:01

ashcrow force-pushed the oneshot branch 2 times, most recently from 5fba4fd to 4889f33 Compare March 27, 2017 18:35

giuseppe mentioned this pull request Mar 28, 2017

Fixes for system containers generate RPM #949

Closed

ashcrow force-pushed the oneshot branch from af9af42 to cd16243 Compare March 28, 2017 15:05

ashcrow mentioned this pull request Mar 28, 2017

doc: Add LABELS.md projectatomic/atomic-system-containers#50

Closed

ashcrow force-pushed the oneshot branch from cd16243 to da34262 Compare March 28, 2017 20:01

ashcrow force-pushed the oneshot branch from da34262 to e8bad8e Compare March 28, 2017 20:36

ashcrow force-pushed the oneshot branch from 11ee58c to 7440081 Compare April 4, 2017 20:59

baude reviewed Apr 11, 2017

View reviewed changes

ashcrow and others added 5 commits April 11, 2017 10:23

syscontainers: Add run once logic to install

479412d

When the atomic.run is provided and set to "once" the install method will extract, run, and clean up after the container. This change also splits install logic into an internal and external method.

syscontainers: Add run once test Dockerfile and files

f2c8993

syscontainers: refactor out values generation to a function

056c652

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

systemcontainers: fill default values when using oneshot

0c91233

tests: test oneshot system containers with values

6887eab

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

ashcrow force-pushed the oneshot branch from 7440081 to 6887eab Compare April 11, 2017 14:25

rh-atomic-bot pushed a commit that referenced this pull request Apr 13, 2017

syscontainers: Add run once test Dockerfile and files

67ebafb

Closes: #952 Approved by: baude

rh-atomic-bot pushed a commit that referenced this pull request Apr 13, 2017

syscontainers: refactor out values generation to a function

ee13843

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #952 Approved by: baude

rh-atomic-bot pushed a commit that referenced this pull request Apr 13, 2017

systemcontainers: fill default values when using oneshot

ccfe992

Closes: #952 Approved by: baude

rh-atomic-bot pushed a commit that referenced this pull request Apr 13, 2017

tests: test oneshot system containers with values

5b5dffa

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com> Closes: #952 Approved by: baude

rh-atomic-bot closed this in 58bc76c Apr 13, 2017

ashcrow added a commit to ashcrow/atomic-system-containers that referenced this pull request Apr 13, 2017

doc: Add atomic.run to LABELS.md

44e0e24

See projectatomic/atomic#952

ashcrow mentioned this pull request Apr 13, 2017

doc: Add atomic.run to LABELS.md projectatomic/atomic-system-containers#58

Closed

rh-atomic-bot pushed a commit to projectatomic/atomic-system-containers that referenced this pull request Apr 13, 2017

doc: Add atomic.run to LABELS.md

944774f

See projectatomic/atomic#952 Closes: #58 Approved by: yuqi-zhang

codificat mentioned this pull request May 3, 2017

System container support openshift/playbook2image#3

Merged

Conversation

ashcrow commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

giuseppe Mar 24, 2017

Choose a reason for hiding this comment

Uh oh!

ashcrow Mar 24, 2017

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Mar 24, 2017

Uh oh!

ashcrow commented Mar 24, 2017

Uh oh!

giuseppe commented Mar 26, 2017

Uh oh!

ashcrow commented Mar 27, 2017

Uh oh!

ashcrow commented Mar 27, 2017

Labels

Run

Question

Uh oh!

giuseppe commented Mar 27, 2017

Uh oh!

ashcrow commented Mar 27, 2017

Uh oh!

yuqi-zhang commented Mar 27, 2017

Uh oh!

ashcrow commented Mar 27, 2017

Uh oh!

giuseppe commented Mar 27, 2017

Uh oh!

giuseppe commented Mar 28, 2017

Uh oh!

giuseppe commented Mar 28, 2017

Uh oh!

ashcrow commented Mar 28, 2017

Uh oh!

giuseppe commented Mar 28, 2017

Uh oh!

ashcrow commented Mar 28, 2017

Uh oh!

giuseppe commented Mar 28, 2017

Uh oh!

ashcrow commented Mar 28, 2017

Uh oh!

rh-atomic-bot commented Mar 28, 2017

Uh oh!

ashcrow commented Mar 28, 2017

Uh oh!

ashcrow commented Mar 28, 2017

Uh oh!

ashcrow commented Apr 7, 2017

Uh oh!

giuseppe commented Apr 10, 2017

Uh oh!

ashcrow commented Apr 11, 2017

Uh oh!

baude Apr 11, 2017

Choose a reason for hiding this comment

Uh oh!

ashcrow Apr 11, 2017

Choose a reason for hiding this comment

Uh oh!

baude Apr 11, 2017

Choose a reason for hiding this comment

Uh oh!

ashcrow Apr 11, 2017

Choose a reason for hiding this comment

Uh oh!

baude commented Apr 11, 2017

Uh oh!

ashcrow commented Apr 11, 2017

Uh oh!

rhatdan commented Apr 12, 2017

Uh oh!

baude commented Apr 13, 2017

Uh oh!

rh-atomic-bot commented Apr 13, 2017

ashcrow commented Mar 24, 2017 •

edited

Loading