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

implement ch-image modify #1408

Merged
merged 71 commits into from
Jul 29, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
71 commits
Select commit Hold shift + click to select a range
e8b04c9
draft docs [skip ci]
reidpr Jul 9, 2022
2e1da45
attempt to merge
lucaudill Feb 14, 2024
83beda8
add proper formatting [skip ci]
lucaudill Feb 14, 2024
244d174
interactive modify [skip ci]
lucaudill Feb 26, 2024
1ae7c94
Merge branch 'master' into modify_1400
lucaudill Feb 26, 2024
580b6ec
move modify function, other additions (buggy) [skip ci]
lucaudill Mar 14, 2024
e843576
clean stuff up a bit?
lucaudill Mar 19, 2024
d05460e
add (prototype) completion [skip ci]
lucaudill Mar 19, 2024
1df874f
don't enforce different out image [skip ci]
lucaudill Mar 28, 2024
50f917f
standardize ch-run exit codes, make sure out image is different than …
lucaudill Apr 4, 2024
2f43bc9
fix a bug?
lucaudill Apr 4, 2024
491ce01
update some return codes in CI
lucaudill Apr 4, 2024
6f8014e
update test suite
lucaudill Apr 22, 2024
4996b4e
update more exit statuses
lucaudill Apr 22, 2024
feee043
Merge branch 'master' into modify_1400
lucaudill Apr 22, 2024
e641935
wtf
lucaudill Apr 23, 2024
a0867d4
Merge branch 'modify_1400' of github.com:hpc/charliecloud into modify…
lucaudill Apr 23, 2024
151592f
enable CI ssh
lucaudill Apr 24, 2024
e0bc574
run ci
lucaudill Apr 24, 2024
7d6f66e
oops
lucaudill Apr 24, 2024
2c8b2f7
CI debugging
lucaudill Apr 25, 2024
731cfe8
CI debugging again
lucaudill Apr 25, 2024
fd45f64
CI debugging again pt 2
lucaudill Apr 25, 2024
05dac9b
fix a CI bug
lucaudill Apr 25, 2024
00e2ec6
more ci
lucaudill Apr 25, 2024
de7d63c
uggh
lucaudill Apr 25, 2024
f7b87e5
lets try this
lucaudill Apr 25, 2024
4b84644
fix disabled cache bug
lucaudill Apr 25, 2024
1afc853
update docs [skip ci]
lucaudill Apr 26, 2024
c6ae8a9
weird docker message?
lucaudill Apr 29, 2024
4e61c06
some cleanup [skip ci]
lucaudill Apr 30, 2024
9d74d19
put CI exit statuses in one place
lucaudill May 1, 2024
579ec81
ignore seemingly erroneous SC error
lucaudill May 2, 2024
b37232b
update exit codes and docs
lucaudill May 2, 2024
3742a28
oops 🤦
lucaudill May 6, 2024
6e0ff69
tidy docs [skip ci]
reidpr May 10, 2024
26ce9e6
work on suggestions
lucaudill May 21, 2024
dbbf74d
more work on non-interactive script case
lucaudill Jun 4, 2024
8cb7975
try to debug CI
lucaudill Jun 4, 2024
b859089
hopefully this works
lucaudill Jun 5, 2024
54c2ee8
back to how it used to work?
lucaudill Jun 5, 2024
bb0283e
more interactive shell troubles
lucaudill Jun 5, 2024
6bc35a9
skip mysteriously failing test for now
lucaudill Jun 6, 2024
8a97d8d
work on some suggestions
lucaudill Jun 7, 2024
5be8c3e
more RPM issues?
lucaudill Jun 7, 2024
c53a820
oops, misread failure
lucaudill Jun 7, 2024
1dcbb4d
what happens when I do this
lucaudill Jun 7, 2024
3314546
try removing hidden ci opt
lucaudill Jun 10, 2024
6ec4b26
fully remove automated ci option
lucaudill Jun 10, 2024
b6da358
hopefully this also works in CI
lucaudill Jun 10, 2024
2b96ecc
fix syntax error
lucaudill Jun 10, 2024
ed46d89
check for empty string instead of None
lucaudill Jun 10, 2024
fc5c89a
update comment [skip ci]
lucaudill Jun 11, 2024
a9b1e23
Merge branch 'master' into modify_1400
lucaudill Jun 21, 2024
dc5ed4d
fix interactive case
lucaudill Jun 21, 2024
1593009
Merge branch 'modify_1400' of github.com:hpc/charliecloud into modify…
lucaudill Jun 21, 2024
7c320e7
create new Python module
lucaudill Jul 5, 2024
d3b06fb
add new module to makefile
lucaudill Jul 5, 2024
ce2a620
update fedora spec
lucaudill Jul 5, 2024
7f17900
a few minor suggestions
lucaudill Jul 5, 2024
8d0befc
more suggestions, move some build cli stuff around
lucaudill Jul 15, 2024
1b65df9
fix CI tty error
lucaudill Jul 15, 2024
d4f90f3
add cache validation test
lucaudill Jul 15, 2024
09f78f2
add cache validation for interactive modify
lucaudill Jul 16, 2024
681a39c
more suggestions
lucaudill Jul 18, 2024
2cd6149
fix CI
lucaudill Jul 18, 2024
20e2d91
even more suggestions
lucaudill Jul 19, 2024
32efbe4
couple more suggestions
lucaudill Jul 23, 2024
f4b1ba7
docs + comments
lucaudill Jul 24, 2024
62fe3e2
comment on log level stuff
lucaudill Jul 24, 2024
621b19a
some cleanup
lucaudill Jul 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions bin/ch-completion.bash
Original file line number Diff line number Diff line change
Expand Up @@ -259,14 +259,16 @@ _ch_convert_complete () {
_image_build_opts="-b --bind --build-arg -f --file --force
--force-cmd -n --dry-run --parse-only -t --tag"

_image_modify_opts="-o --out"

_image_common_opts="-a --arch --always-download --auth --break
--cache --cache-large --dependencies -h --help
--no-cache --no-lock --no-xattrs --profile
--rebuild --password-many -q --quiet -s --storage
--tls-no-verify -v --verbose --version --xattrs"

_image_subcommands="build build-cache delete gestalt
import list pull push reset undelete"
_image_subcommands="build build-cache delete gestalt import
list modify pull push reset undelete"

# archs taken from ARCH_MAP in charliecloud.py
_archs="amd64 arm/v5 arm/v6 arm/v7 arm64/v8 386 mips64le ppc64le s390x"
Expand Down Expand Up @@ -374,20 +376,32 @@ _ch_image_complete () {
build-cache)
COMPREPLY=( $(compgen -W "--reset --gc --tree --dot" -- "$cur") )
;;
delete|list)
if [[ "$sub_cmd" == "list" ]]; then
delete|list|modify)
case "$sub_cmd" in
list)
if [[ "$prev" == "--undeletable" || "$prev" == "--undeleteable" || "$prev" == "-u" ]]; then
COMPREPLY=( $(compgen -W "$(_ch_undelete_list "$strg_dir")" -- "$cur") )
return 0
fi
extras+="$extras -l --long -u --undeletable"
extras="$extras -l --long -u --undeletable"
# If “cur” starts with “--undelete,” add “--undeleteable” (the less
# correct version of “--undeletable”) to the list of possible
# completions.
if [[ ${cur::10} == "--undelete" ]]; then
extras="$extras --undeleteable"
fi
fi
;;
modify)
case "$prev" in
-o|--out)
# Can’t complete for this option
COMPREPLY=()
return 0
;;
esac
extras="$extras $_image_modify_opts"
;;
esac
COMPREPLY=( $(compgen -W "$(_ch_list_images "$strg_dir") $extras" -- "$cur") )
__ltrim_colon_completions "$cur"
;;
Expand Down
9 changes: 9 additions & 0 deletions bin/ch-image.py.in
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import charliecloud as ch
import build
import build_cache as bu
import filesystem as fs
import image as im
import misc
import pull
import push
Expand Down Expand Up @@ -278,6 +279,14 @@ def main():
sp.add_argument("image_ref", metavar="IMAGE_REF", nargs="?",
help="print details of this image only")

# modify
sp = ap.add_parser("modify", "foo")
add_opts(sp, build.modify, deps_check=True, stog_init=True)
sp.add_argument("-c", metavar="CMD", action="append", default=[], nargs="*", help="foo")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think you probably want nargs=1 (the default?) and just repeat the option for multiple values (action="append" does this). nargs="+" and nargs="*" can cause problems though I don't recall the details.

sp.add_argument("-o", "--out", metavar="out_image", help="foo", required=True)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If this is required, should it be a second argument rather than an option? That said, it would be better UX if it’s not required.

sp.add_argument("-S", "--shell", metavar="shell", help="foo")
sp.add_argument("image_ref", metavar="IMAGE_REF", help="image to modify")

# pull
sp = ap.add_parser("pull",
"copy image from remote repository to local filesystem")
Expand Down
74 changes: 74 additions & 0 deletions doc/ch-image.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1895,6 +1895,80 @@ functionally identical when re-imported.
will still hit, even if the new image is different.


:code:`modify`
reidpr marked this conversation as resolved.
Show resolved Hide resolved
==============

Interactively edit the specified image.

Synopsis
--------

::

$ ch-image [...] modify [...] TARGET

Description
-----------

This subcommand starts a shell on the image named :code:`TARGET`, in order to
edit the image interactively. It is similar to a :code:`RUN` instruction that
starts an interactive shell. By default, ask the user whether to save changes
when the shell exits.

Options:

:code:`-m MSG`
Use :code:`MSG` to identify the edits to the build cache. That is, if you
run this command twice with the same :code:`TARGET`, the same :code:`-o
DEST`, and the same :code:`MSG`, the second session will overwrite the
first. (Without :code:`-o`, the second session will build atop the first.)
By default, every interactive session is considered different from every
other, as if a random :code:`MSG` were entered.

:code:`-o`, :code:`--out DEST`
Save the results in image named :code:`DEST`, leaving :code:`TARGET`
unchanged.

:code:`-s`, :code:`--shell SHELL`
Start :code:`SHELL` instead of :code:`/bin/sh`.

:code:`-y`, :code:`--yes`
Do not prompt the user to save. Instead, save if the shell exits
successfully, and roll back if it exits unsuccessfully, e.g. by executing
:code:`exit 1`.

.. warning::

This subcommand is rarely needed. Non-interactive build using a Dockerfile
is almost always better, because it preserves the sequence of operations
that created an image. Only use this subcommand if you really know what you
are doing.

Examples
--------

To edit the image :code:`foo`, adding :code:`/opt/lib` to the default shared
library search path, producing image :code:`bar` as the result::

$ ch-image modify -o bar foo
[...]
> emacs /etc/ld.so.conf
[... append line “/opt/lib” to the file ...]
> ldconfig
> exit
Save changes ([y]/n)? y
committing ...
[...]

Equivalently, and almost certainly preferred::

$ cat Dockerfile
FROM foo
RUN echo /opt/lib >> /etc/ld.so.conf
RUN ldconfig
$ ch-image build -t bar -f Dockerfile .


:code:`pull`
============

Expand Down
141 changes: 141 additions & 0 deletions lib/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import os.path
import re
import shutil
import subprocess
import sys

import charliecloud as ch
Expand Down Expand Up @@ -57,6 +58,10 @@ class Environment:
of this is just passed through from the image metadata."""


# Class responsible for traversing the parse tree generated by lark. “Main_Loop”
# visits each node in the parse tree and calls its “__default__” method to
# figure out what to do with the node. This behavior is defined by the parent
# class, “lark.Visitor”, documented here: https://lark-parser.readthedocs.io/en/latest/visitors.html
lucaudill marked this conversation as resolved.
Show resolved Hide resolved
class Main_Loop(lark.Visitor):

__slots__ = ("instruction_total_ct",
Expand All @@ -69,15 +74,28 @@ def __init__(self, *args, **kwargs):
self.instruction_total_ct = 0
super().__init__(*args, **kwargs)

# The main argument of the “__default__” method is “tree”, which is really
# just the current node being visited (the node is called “tree” because it
# represents the root node of the current subtree). The “tree.data” attribute
# gives the “name of the rule or alias” represented by the node. When a
# “lark.Visitor” instance visits a parse tree node, it checks the node’s
# “data” attribute and tries to call its own attribute (e.g. method) of the
# same name. If no such attribute exists, it calls “__default__”. Note that
# in this class, we don’t define any attributes corresponding to the
# Dockerfile instructions that we want to execute, so “__default__” is always
# called when visiting a node. We rely on instruction classes to execute the
# instructions, rather than attributes of this class.
lucaudill marked this conversation as resolved.
Show resolved Hide resolved
def __default__(self, tree):
class_ = tree.data.title() + "_G"
ch.ILLERI(tree.data.title())
if (class_ in globals()):
inst = globals()[class_](tree)
if (self.instruction_total_ct == 0):
if (not (isinstance(inst, Directive_G)
or isinstance(inst, From__G)
or isinstance(inst, Instruction_No_Image))):
ch.FATAL("first instruction must be ARG or FROM")
ch.ILLERI("PREV: %s" % self.inst_prev)
inst.init(self.inst_prev)
# The three announce_maybe() calls are clunky but I couldn’t figure
# out how to avoid the repeats.
Expand Down Expand Up @@ -287,6 +305,129 @@ def unescape(sl):
assert (len(sl) >= 2 and sl[0] == '"' and sl[-1] == '"' and sl[-2:] != '\\"')
return ast.literal_eval(sl)

def modify(cli_):
global cli
cli = cli_

cli.parse_only = False
cli.force = ch.Force_Mode.SECCOMP
lucaudill marked this conversation as resolved.
Show resolved Hide resolved
cli.force_cmd = force.FORCE_CMD_DEFAULT
cli.bind = []
reidpr marked this conversation as resolved.
Show resolved Hide resolved
lucaudill marked this conversation as resolved.
Show resolved Hide resolved
# FIXME: This is super kludgey
cli.tag = cli.out
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can tell argparse to put the value of --out in tag when you declare the command line option. That seems reasonable to me. Or, maybe do the reverse: rename cli.tag to cli.out?


print(cli.image_ref)
ch.ILLERI(cli.c)
ch.ILLERI(type(cli.c))
commands = []
# “Flatten” commands array
for c in cli.c:
commands += c
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The need for this will go away if you remove nargs="*".

src_image = im.Image(im.Reference(cli.image_ref))
out_image = cli.out
if (not src_image.unpack_exist_p):
ch.FATAL("not in storage: %s" % src_image.ref)
if (out_image == str(src_image.ref)):
ch.FATAL("output image must have different name from source (%s)" % src_image.ref)
if (cli.shell is not None):
shell = cli.shell
else:
shell = "/bin/sh"
if not sys.stdin.isatty():
# https://stackoverflow.com/a/17735803
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This idiom is well enough known that you don't need a comment.

# commands from stdin
for line in sys.stdin:
# execute each line (analogous to RUN)
commands.append(line)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This won’t work due to the shell’s complicated rules about line breaks. I’d treat the input as an opaque blob to pass on to the shell’s stdin.

if (commands != []):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder if the -c, interactive stdin, and non-interactive stdin cases are sufficiently different that they should be dispatched to different functions? In any case, we should explain the situation and why the decisions made were made.

tree = modify_tree_make(src_image.ref, commands)

# FIXME: Be more DRY in this section
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah that's probably essential for merging


# Count the number of stages (i.e., FROM instructions)
global image_ct
image_ct = sum(1 for i in tree.children_("from_"))
lucaudill marked this conversation as resolved.
Show resolved Hide resolved

ml = Main_Loop()
if (hasattr(ml, 'visit_topdown')):
ml.visit_topdown(tree)
else:
ml.visit(tree)
if (ml.instruction_total_ct > 0):
if (ml.miss_ct == 0):
ml.inst_prev.checkout()
ml.inst_prev.ready()

else:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should label the branches on which of the three modes we’re in.

# Make sure that shell exists.
#try:
# subprocess.run([ch.CH_BIN + "/ch-run", str(src_image.ref), "--", shell], capture_output=True).check_returncode()
#except subprocess.CalledProcessError as x:
# #print(x.__dict__)
# if ("%s: No such file or directory" % shell in str(x.stderr)):
# ch.FATAL("invalid shell: %s" % shell)

out_image = im.Image(im.Reference(out_image))
# Do something similar to “ch-image import”
out_image.unpack_clear()
out_image.copy_unpacked(src_image)
subprocess.run([ch.CH_BIN + "/ch-run", "--unsafe", "-w",
str(out_image.ref), "--", shell])
# FIXME: metadata history stuff? See misc.import_.
#bu.cache.rollback(src_image.unpack_path)

def modify_tree_make(src_img, cmds):
"""Function that manually constructs a parse tree corresponding to a set of
lucaudill marked this conversation as resolved.
Show resolved Hide resolved
“ch-image modify” commands, as though the commands had been specified in a
Dockerfile. Note that because “ch-image modify” simply executes one or
more commands inside a container, the only Dockerfile instructions we need
to consider are “FROM” and “RUN”. This results in conveniently simple
parse trees of the form:

start
dockerfile
from_
image_ref
IMAGE_REF <image_name>
run
run_shell
LINE_CHUNK <command_1>
[...]
run
run_shell
LINE_CHUNK <command_N>
lucaudill marked this conversation as resolved.
Show resolved Hide resolved

e.g. for the command line

$ ch-image modify -o foo2 -c 'echo foo' -c 'echo bar' -- foo

this function produces the following parse tree

start
dockerfile
from_
image_ref
IMAGE_REF foo
run
run_shell
LINE_CHUNK echo foo
run
run_shell
LINE_CHUNK echo bar
"""
# Children of dockerfile tree
df_children = []
# Metadata attribute. We use this attribute in the “_pretty” method for our
# “Tree” class. Constructing a tree without specifying a “Meta” instance that
# has been given a “line” value will result in the attribute not being present,
# which causes an error when we try to access that attribute. Here we give the
# attribute a debug value of -1 to avoid said errors.
meta = lark.tree.Meta()
meta.line = -1
df_children.append(im.Tree(lark.Token('RULE', 'from_'), [im.Tree(lark.Token('RULE', 'image_ref'),[lark.Token('IMAGE_REF', src_img.name)], meta)], meta))
for cmd in cmds:
df_children.append(im.Tree(lark.Token('RULE', 'run'), [im.Tree(lark.Token('RULE', 'run_shell'),[lark.Token('LINE_CHUNK', cmd)], meta)],meta))
return im.Tree(lark.Token('RULE', 'start'), [im.Tree(lark.Token('RULE','dockerfile'), df_children)], meta)

## Supporting classes ##

Expand Down
3 changes: 3 additions & 0 deletions lib/image.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
import json
import os
import re
import subprocess
import sys
import tarfile

import build_cache as bu
import charliecloud as ch
import filesystem as fs

Expand Down Expand Up @@ -947,3 +949,4 @@ def terminals_cat(self, tname):
"""Return the concatenated values of all child terminals named tname as
a string, with no delimiters. If none, return the empty string."""
return "".join(self.terminals(tname))

lucaudill marked this conversation as resolved.
Show resolved Hide resolved