Skip to content
This repository has been archived by the owner on Oct 2, 2024. It is now read-only.

ch-run: run oci bundles #1870

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

ch-run: run oci bundles #1870

wants to merge 14 commits into from

Conversation

kchilleri
Copy link
Contributor

@kchilleri kchilleri commented Mar 21, 2024

If option --oci or --o is provided/enabled and the image path is the path to an OCI bundle containing a directory named rootfs and a spec file config.json then parse the JSON file and distribute the arguments to the corresponding Charliecloud container arguments.

Example:

kchilleri@dev:/usr/local/src/charliecloud$ ls ~/examples/redis
config.json  rootfs
kchilleri@dev:/usr/local/src/charliecloud$ ls ~/examples/redis/rootfs/
bin  boot  data  dev  etc  home  lib  media  mnt  opt  proc  root  run  sbin  srv  sys  tmp  usr  var
kchilleri@dev:/usr/local/src/charliecloud$ cat ~/examples/redis/config.json 
{
        "ociVersion": "1.0.2-dev",
        "process": {
                "terminal": true,
                "user": {}, 

                "args": [
                        "sh", "redis-server", "--bind", "0.0.0.0"
                ],
                "env": [
                        "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
                        "TERM=xterm"
                ],
                "cwd": "/",
                "capabilities": {
                        "bounding": [
                                "CAP_AUDIT_WRITE",
                                "CAP_KILL",
                                "CAP_NET_BIND_SERVICE"
                        ],
...
kchilleri@dev:/usr/local/src/charliecloud$ ch-run --oci ~/examples/redis -- true

closes #1754

@kchilleri kchilleri linked an issue Mar 21, 2024 that may be closed by this pull request
@kchilleri kchilleri requested a review from reidpr March 21, 2024 22:33
Copy link
Collaborator

@reidpr reidpr left a comment

Choose a reason for hiding this comment

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

Nice work.

I’d like to also see documentation for this. In particular, as written, the PR ignores config.json. We’ll need to justify this choice. Looking briefly over the config.json spec, I do see a lot of things that correspond to ch-run arguments, as well as a lot of stuff that we can’t support, e.g. complex UID/GID mappings. We’ll need to decide what to do with all that.

There are quite a few JSON libraries for C, so we could consider those. A Python wrapper to process config.json would also be a reasonable choice.

bin/ch-run.c Outdated Show resolved Hide resolved
bin/ch-run.c Outdated Show resolved Hide resolved
bin/ch_core.c Outdated Show resolved Hide resolved
bin/ch_core.c Outdated Show resolved Hide resolved
bin/ch_core.h Outdated Show resolved Hide resolved
@reidpr
Copy link
Collaborator

reidpr commented Mar 25, 2024

e.g. this one is BSD licensed and recommends “just drop json.c and json.h into your project”.

@reidpr
Copy link
Collaborator

reidpr commented Mar 25, 2024

this one similarly

@j-ogas
Copy link
Contributor

j-ogas commented Mar 26, 2024

FWIW, we should embrace JSON parsing sooner, rather than later. I suspect it would be very useful for things like oci-hooks for GPU injection.

@reidpr
Copy link
Collaborator

reidpr commented Mar 27, 2024

@j-ogas do you have an opinion on whether it lives in ch-run or a Python wrapper?

@j-ogas
Copy link
Contributor

j-ogas commented Apr 1, 2024

@j-ogas do you have an opinion on whether it lives in ch-run or a Python wrapper?

Python may offer the path of the least resistance. Aside from that, not really.

Copy link
Collaborator

@reidpr reidpr left a comment

Choose a reason for hiding this comment

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

Krishna, great work here! I think we might want to alter the approach a bit, and some suggestions in-line on that, but this is a solid start that avoids some common pitfalls.

bin/cjson-parser.c Outdated Show resolved Hide resolved
bin/cjson-parser.c Outdated Show resolved Hide resolved
bin/cjson-parser.c Outdated Show resolved Hide resolved
bin/cjson-parser.c Outdated Show resolved Hide resolved
bin/ch-run.c Show resolved Hide resolved
bin/ch-run.c Show resolved Hide resolved
bin/ch-run.c Show resolved Hide resolved
bin/ch-run.c Outdated Show resolved Hide resolved
bin/ch-run.c Outdated Show resolved Hide resolved
bin/ch-run.c Show resolved Hide resolved
bin/ch-run.c Show resolved Hide resolved
@kchilleri kchilleri requested a review from reidpr June 4, 2024 21:25
Copy link
Collaborator

@reidpr reidpr left a comment

Choose a reason for hiding this comment

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

Nice progress! Discussed offline.

@kchilleri kchilleri requested a review from reidpr June 10, 2024 19:30
@kchilleri
Copy link
Contributor Author

TO DO:

  1. Add documentation
  2. Perhaps more error checking

Copy link
Collaborator

@reidpr reidpr left a comment

Choose a reason for hiding this comment

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

Solid! Looks like the actual logic isn’t too complicated.

The major missing feature might be priority (defaults, environment variables, cmd line, now config.json). One way to approach that might be to create an arguments struct for each of those, then merge.

I think the path forward on this might be to table it for a little while until my CDI stuff is merged — there’s some duplicate code.

@@ -0,0 +1,872 @@
# Makefile.in generated by automake 1.16.5 from Makefile.am.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Makefiles are generated, so shouldn’t include in commit.

Comment on lines +597 to +599
case 'o': // --oci
args->c.type = IMG_OCI_BUNDLE;
break;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We’ll want to auto-detect that it’s a bundle rather than adding an option. In image_type() I think it would be a straightforward addition to the directory case.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

run OCI bundles
3 participants