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

syscontainers: do not delete modified files with --system-package=no #1131

Closed
wants to merge 4 commits into from
Closed

syscontainers: do not delete modified files with --system-package=no #1131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
540ec99
syscontainers: do not delete modified files with --system-package=no
giuseppe Nov 17, 2017
1eac52e
fixup! syscontainers: do not delete modified files with --system-pack…
giuseppe Nov 23, 2017
8181d73
syscontainers: print only existing files, not directories
giuseppe Nov 24, 2017
73d9191
fixup! syscontainers: do not delete modified files with --system-pack…
giuseppe Nov 27, 2017
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
53 changes: 41 additions & 12 deletions Atomic/rpm_host_install.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from . import rpmwriter
import tempfile
import shutil
import hashlib

RPM_NAME_PREFIX = "atomic-container"

Expand All @@ -12,16 +13,20 @@ class RPMHostInstall(object):
@staticmethod
def copyfile(src, dest):
if os.path.isdir(src):
# add the directory only if it is empty, so we don't create directories that
# add the directory only if it is empty, so we don't delete directories that
Copy link
Contributor

Choose a reason for hiding this comment

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

Q, hmm, I don't quite get ... how "delete" got involved in this function, can you explain it a bit :).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we keep track of what files/directories we are adding. We add the directory only if it is not already present.
In this way when we uninstall the container we will not delete directories that were already present when we first installed the container

# weren't added by us. Anyway a non empty directory would be created by
# its children.
if len(os.listdir(src)) == 0:
os.mkdir(dest)
return True
elif os.path.islink(src):
linkto = os.readlink(src)
os.symlink(linkto, dest)
return True
else:
shutil.copy2(src, dest)
return True
return False

@staticmethod
def _do_rename_path(path, rename_files):
Expand All @@ -33,12 +38,31 @@ def _do_rename_path(path, rename_files):
return path

@staticmethod
def rm_add_files_to_host(old_installed_files, exports, prefix="/", files_template=None, values=None, rename_files=None):
def file_checksum(path, blocksize=(1<<20)):
if not os.path.exists(path) or os.path.isdir(path):
return "0"

h = hashlib.sha256()
with open(path, "rb") as f:
while True:
chunk = f.read(blocksize)
if chunk == None or len(chunk) == 0:
break
h.update(chunk)
return h.hexdigest()

@staticmethod
def rm_add_files_to_host(old_installed_files_checksum, exports, prefix="/", files_template=None, values=None, rename_files=None):
# if any file was installed on the host delete it
if old_installed_files:
for i in old_installed_files:
if old_installed_files_checksum:
for path, checksum in old_installed_files_checksum.items():
new_checksum = RPMHostInstall.file_checksum(path)
if new_checksum != checksum:
# Do not delete the file if it was modified.
util.write_out("Will not delete %s as it was manually modified." % path, lf="\n")
continue
try:
os.remove(i)
os.remove(path)
except OSError:
pass

Expand All @@ -49,7 +73,7 @@ def rm_add_files_to_host(old_installed_files, exports, prefix="/", files_templat

# if there is a directory hostfs/ under exports, copy these files to the host file system.
hostfs = os.path.join(exports, "hostfs")
new_installed_files = []
new_installed_files_checksum = {}
if os.path.exists(hostfs):
for root, dirs, files in os.walk(hostfs):
rel_root_path = os.path.relpath(root, hostfs)
Expand All @@ -66,23 +90,27 @@ def rm_add_files_to_host(old_installed_files, exports, prefix="/", files_templat
dest_path = os.path.join(prefix or "/", os.path.relpath(rel_dest_path, "/"))

if os.path.exists(dest_path):
util.write_out("File %s already exists." % dest_path, lf="\n")
if os.path.isfile(dest_path):
util.write_out("File %s already exists" % os.path.normpath(dest_path), lf="\n")
continue

if not os.path.exists(os.path.dirname(dest_path)):
os.makedirs(os.path.dirname(dest_path))

created = False
if rel_dest_path in templates_set:
with open(src_file, 'r') as src_file_obj:
data = src_file_obj.read()
util.write_template(src_file, data, values or {}, dest_path)
shutil.copystat(src_file, dest_path)
created = True
else:
RPMHostInstall.copyfile(src_file, dest_path)
created = RPMHostInstall.copyfile(src_file, dest_path)

new_installed_files.append(rel_dest_path)
new_installed_files.sort() # just for an aesthetic reason in the info file output
if created:
new_installed_files_checksum[rel_dest_path] = RPMHostInstall.file_checksum(dest_path)

return new_installed_files
return new_installed_files_checksum


@staticmethod
Expand Down Expand Up @@ -175,7 +203,8 @@ def generate_rpm(name, image_id, labels, exports, destination, values=None, inst
rootfs = os.path.join(rpm_content, "usr/lib/containers/atomic", name)
os.makedirs(rootfs)
if installed_files is None:
installed_files = RPMHostInstall.rm_add_files_to_host(None, exports, rpm_content, files_template=installed_files_template, values=values, rename_files=rename_files)
checksums = RPMHostInstall.rm_add_files_to_host(None, exports, rpm_content, files_template=installed_files_template, values=values, rename_files=rename_files)
installed_files = checksums.keys()
rpm_root = RPMHostInstall.generate_rpm_from_rootfs(destination, temp_dir, name, image_id, labels, include_containers_file=False, installed_files=installed_files, defaultversion=defaultversion)
rpm_file = RPMHostInstall.find_rpm(rpm_root)
if rpm_file:
Expand Down
40 changes: 26 additions & 14 deletions Atomic/syscontainers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,7 +526,7 @@ def _resolve_remote_path(self, remote_path):
raise ValueError("The container's rootfs is set to remote, but the remote rootfs does not exist")
return real_path

def _checkout(self, repo, name, img, deployment, upgrade, values=None, destination=None, extract_only=False, remote=None, prefix=None, installed_files=None, system_package='no'):
def _checkout(self, repo, name, img, deployment, upgrade, values=None, destination=None, extract_only=False, remote=None, prefix=None, installed_files_checksum=None, system_package='no'):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing to do in this PR but this method is getting a bit out of hand with it's arguments!

destination = destination or os.path.join(self._get_system_checkout_path(), "{}.{}".format(name, deployment))
unitfileout, tmpfilesout = self._get_systemd_destination_files(name, prefix)

Expand All @@ -536,8 +536,8 @@ def _checkout(self, repo, name, img, deployment, upgrade, values=None, destinati
raise ValueError("The file %s already exists." % f)

try:
return self._do_checkout(repo, name, img, upgrade, deployment, values, destination, unitfileout, tmpfilesout, extract_only, remote, prefix, installed_files=installed_files,
system_package=system_package)
return self._do_checkout(repo, name, img, upgrade, deployment, values, destination, unitfileout, tmpfilesout, extract_only, remote, prefix,
installed_files_checksum=installed_files_checksum, system_package=system_package)
except (GLib.Error, ValueError, OSError, subprocess.CalledProcessError, KeyboardInterrupt) as e:
try:
if not extract_only and not upgrade:
Expand Down Expand Up @@ -707,7 +707,7 @@ def _canonicalize_location(self, destination):


def _do_checkout(self, repo, name, img, upgrade, deployment, values, destination, unitfileout,
tmpfilesout, extract_only, remote, prefix=None, installed_files=None, system_package='no'):
tmpfilesout, extract_only, remote, prefix=None, installed_files_checksum=None, system_package='no'):
"""
Actually do the checkout.

Expand Down Expand Up @@ -886,10 +886,11 @@ def _do_checkout(self, repo, name, img, upgrade, deployment, values, destination
labels = {k.lower() : v for k, v in img_obj.get('Labels', {}).items()}
(rpm_installed, rpm_file, _) = RPMHostInstall.generate_rpm(name, image_id, labels, exports, destination, values=values, installed_files_template=installed_files_template, rename_files=rename_files, defaultversion=deployment)
if rpm_installed or system_package == 'absent':
new_installed_files = []
new_installed_files_checksum = {}
else:
new_installed_files = RPMHostInstall.rm_add_files_to_host(installed_files, exports, prefix or "/", files_template=installed_files_template, values=values, rename_files=rename_files)
new_installed_files_checksum = RPMHostInstall.rm_add_files_to_host(installed_files_checksum, exports, prefix or "/", files_template=installed_files_template, values=values, rename_files=rename_files)

new_installed_files = list(new_installed_files_checksum.keys())
try:
with open(os.path.join(destination, "info"), 'w') as info_file:
info = {"image" : img,
Expand All @@ -899,6 +900,7 @@ def _do_checkout(self, repo, name, img, upgrade, deployment, values, destination
"values" : values,
"has-container-service" : has_container_service,
"installed-files": new_installed_files,
"installed-files-checksum": new_installed_files_checksum,
"installed-files-template": installed_files_template,
"rename-installed-files" : rename_files,
"rpm-installed" : rpm_installed,
Expand Down Expand Up @@ -1077,7 +1079,10 @@ def update_container(self, name, setvalues=None, rebase=None):
image = rebase or info["image"]
values = info["values"]
revision = info["revision"] if "revision" in info else None
installed_files = info["installed-files"] if "installed-files" in info else None
installed_files_checksum = info["installed-files-checksum"] if "installed-files-checksum" in info else None
if installed_files_checksum is None:
installed_files = info["installed-files"] if "installed-files" in info else None
installed_files_checksum = {k : "" for k in installed_files}
rpm_installed = info["rpm-installed"] if "rpm-installed" in info else None
system_package = info["system-package"] if "system-package" in info else None
runtime = info["runtime"] if "runtime" in info else None
Expand Down Expand Up @@ -1110,7 +1115,8 @@ def update_container(self, name, setvalues=None, rebase=None):
self._runtime_from_info_file = runtime
if system_package is None:
system_package = 'yes' if rpm_installed else 'no'
self._checkout(repo, name, image, next_deployment, True, values, remote=self.args.remote, installed_files=installed_files, system_package=system_package)
self._checkout(repo, name, image, next_deployment, True, values, remote=self.args.remote,
installed_files_checksum=installed_files_checksum, system_package=system_package)
return

def rollback(self, name):
Expand All @@ -1131,7 +1137,10 @@ def rollback(self, name):
with open(os.path.join(self._get_system_checkout_path(), name, "info"), "r") as info_file:
info = json.loads(info_file.read())
rpm_installed = info["rpm-installed"] if "rpm-installed" in info else None
installed_files = info["installed-files"] if "installed-files" in info and rpm_installed is None else None
installed_files_checksum = info["installed-files-checksum"] if "installed-files-checksum" in info else None
if installed_files_checksum is None:
installed_files = info["installed-files"] if "installed-files" in info else None
installed_files_checksum = {k : "" for k in installed_files}
installed_files_template = info["installed-files-template"] if "installed-files-template" in info and rpm_installed is None else None
has_container_service = info["has-container-service"] if "has-container-service" in info else True
rename_files = info["rename-installed-files"] if "rename-installed-files" in info else None
Expand Down Expand Up @@ -1164,8 +1173,8 @@ def rollback(self, name):
if (os.path.exists(tmpfiles)):
shutil.copyfile(tmpfiles, tmpfilesout)

if installed_files:
RPMHostInstall.rm_add_files_to_host(installed_files, os.path.join(destination, "rootfs/exports"), files_template=installed_files_template, rename_files=rename_files)
if installed_files_checksum:
RPMHostInstall.rm_add_files_to_host(installed_files_checksum, os.path.join(destination, "rootfs/exports"), files_template=installed_files_template, rename_files=rename_files)

os.unlink(path)
os.symlink(destination, path)
Expand Down Expand Up @@ -1597,9 +1606,12 @@ def uninstall(self, name):
installed_files = None
with open(os.path.join(checkout, name, "info"), 'r') as info_file:
info = json.loads(info_file.read())
installed_files = info["installed-files"] if "installed-files" in info else None
if installed_files:
RPMHostInstall.rm_add_files_to_host(installed_files, None)
installed_files_checksum = info["installed-files-checksum"] if "installed-files-checksum" in info else None
installed_files = info["installed-files"] if "installed-files" in info else []
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: seems like the assignment of checksum here is a bit inconsistent with other implementations,

 installed_files_checksum = info["installed-files-checksum"] if "installed-files-checksum" in info else None
        if installed_files_checksum is None:
            installed_files = info["installed-files"] if "installed-files" in info else None
            installed_files_checksum = {k : "" for k in installed_files}

any reasons for that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it is done a bit differently so if installed_files_checksum is not present (for containers installed before this patch), we assume it is empty:

installed_files_checksum = {k: "" for k in installed_files}

if installed_files_checksum == None:
installed_files_checksum = {k: "" for k in installed_files}
if installed_files_checksum:
RPMHostInstall.rm_add_files_to_host(installed_files_checksum, None)

if rpm_installed:
try:
Expand Down
9 changes: 9 additions & 0 deletions tests/integration/test_system_containers_rpm.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ teardown () {
${ATOMIC} uninstall --storage ostree atomic-test-system-hostfs || true
rm -rf /etc/systemd/system/atomic-test-system-*.service /etc/tmpfiles.d/atomic-test-system-*.conf
ostree --repo=${ATOMIC_OSTREE_REPO} refs --delete ociimage &> /dev/null || true
rm -f /usr/local/lib/secret-message
}
trap teardown EXIT

Expand Down Expand Up @@ -149,3 +150,11 @@ for i in /usr/local/lib/renamed-atomic-test-system-hostfs /usr/local/lib/secret-
do
test -e $i
done

echo "This message will not be deleted" > /usr/local/lib/secret-message

ATOMIC_OSTREE_TEST_FORCE_IMAGE_ID=NEW-ID ${ATOMIC} containers update atomic-test-system-hostfs

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Do you think it is better to also include a check to see if the error message is there to notify the user?
I.e: We check if Do not delete xxx as it was manually modified is outputted?

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 do that no?

There is:

+assert_matches "This message will not be deleted" /usr/local/lib/secret-message

test -e /usr/local/lib/secret-message

assert_matches "This message will not be deleted" /usr/local/lib/secret-message