Prune unused artifacts from non-static builds #59

henriquesimoes · 2024-04-16T18:09:47Z

Both dynamic-link and no-build targets generate unnecessarily large IOC images due to the trivial COPY done from /opt. Clean up everything from there and /usr/local directory before doing the COPY to the IOC image.

Clean up strategy proposed here is the following:

Remove entirely any module that does not have a shared library linked against binaries under the IOC target directories (i.e. /opt/$REPONAME or $RUNDIR);
Prune directories from used modules and EPICS base not containing shared libraries, EPICS databases or autosave requirement files;
Remove any shared library not linked against binaries under IOC target directories;
Remove all statically linked libraries;

For pruning module directories, another approach that could be followed is to delegate this responsibility to install_module. This way, we can have contextualized pruning for modules. On the other hand, this cannot be as aggressive as implemented in this proposal, as build-dependent files (the ones defining the build-system itself, for instance) must exist until the IOC build is complete.

I have tested this with ad-aravis-epics-ioc, and it shrinks the image size from 1.33GB down to 322MB.

This should be further tested and extensively reviewed, so that no artifact is removed unexpectedly.

henriquesimoes · 2024-04-16T18:12:16Z

I'm waiting for #57 to be merged to rebase it. As of now, this would mean another step is introduced which removes all IOCs installed in the base image.

henriquesimoes

I've added comments to points that I wasn't sure what is the best way to deal with, so that we can discuss if there are relevant or not and how to improve, if that's the case.

In general, I feel like I could have written helper functions that could be more easily shared among the steps. But that's not a concrete thing to propose anything. Let me know if you feel the same.

henriquesimoes · 2024-04-17T12:27:56Z

base/Dockerfile

@@ -66,3 +66,5 @@ RUN ./install_area_detector.sh

 COPY install_motor.sh .
 RUN ./install_motor.sh
+
+COPY lnls-prune-artifacts.sh /usr/local/bin/lnls-prune-artifacts


I've added this last so that image cache is not invalidated every time for nothing. It can be moved to be side-by-side with lnls-run after the script ready to be merged.

base/lnls-prune-artifacts.sh

henriquesimoes · 2024-04-17T12:39:35Z

base/lnls-prune-artifacts.sh

+    for candidate in $(find $module -type d); do
+        [ -d $candidate ] || continue
+
+        if [[ ! $keep_paths =~ "$candidate".* ]]; then
+            size=$(du -hs $candidate | cut -f 1)
+
+            printf "Removing directory '$candidate' ($size)...\n"
+            rm -rf $candidate
+        fi


In this loop, I'm removing only directories as a whole if no important file (listed in keep_paths) is stored in it.

Another approach is to do this in a file basis, which could prune modules even further. I'm not sure if that's the best way to handle it, though.

base/lnls-prune-artifacts.sh

henriquesimoes · 2024-04-17T12:48:16Z

base/lnls-prune-artifacts.sh

+clean_up_epics_modules $target_paths
+remove_static_libs /opt
+remove_unused_shared_libs $target_paths


I'm not sure if remove_static_libs should receive an argument or not. Their companion functions receive directories which contain important binaries whose dependencies must be preserved. On the other hand, remove_static_libs currently receives the directories to prune the libraries off.

Maybe both of them should take /opt (and /usr/local?) from a shared constant variable instead.

Opinions on that?

ericonr

The first two commits are correct and can be merged separately, if you'd like to split them.

ericonr

The Dockerfile parts of the third commit are ok.

Commit message nits:

It's not the caching that's the issue, it's simply how container images are layered:

The clean up phase must be executed in the build stage (before the final
copy), otherwise the COPY layer would still make the image size big.
This implies `no-build` target needs to copy from a pruned version of
the base image, which is a new stage.

ericonr · 2024-08-29T15:00:56Z

base/lnls-prune-artifacts.sh

+target_paths=$@
+
+


I don't think this variable needs to be set; the function call at the end can simply use $@ itself.

Putting this at the start of the script makes it look like you are going to manipulate the array in the middle of it.

Alright. I did so to document its meaning somehow. I'm fine with passing this directly to remove_unused_epics_modules though.

base/lnls-prune-artifacts.sh

ericonr · 2024-08-29T15:22:57Z

base/lnls-prune-artifacts.sh

+    targets=$@
+
+    executables=$(find $targets -type f -executable -print)
+    libs="$(ldd $executables 2>/dev/null)"


ldd on musl doesn't check all arguments, it simply ignores all but the first one.

We should either explicitly loop or add a comment about depending on this behavior.

We should either explicitly loop or add a comment about depending on this behavior.

Is there any good reason to support musl's ldd? I'll add the comment for now.

base/lnls-prune-artifacts.sh

ericonr · 2024-08-29T15:47:16Z

base/lnls-prune-artifacts.sh

+    for lib in $epics_libs; do
+        module=${lib%/lib/*/*lib*so}


Use epics_lib or even something other than lib. Otherwise the substitution gets confusing with the repeated lib, and it's confusing enough already.

Add a comment explaining the purpose of expression (an example would be enough).

Let me know what you think about the comment.

I dropped this logic entirely. We cannot rely on the fact that module libraries are under lib/<architecture>, because OPCUA breaks this. Instead, I'm currently iterating over all parent directories of used libraries and removing them the list of all modules.

This way, we can correctly get the module top directory regardless of its location inside such module.

For motor modules, we still rely on filtering out all targets to make sure the module is not removed.

base/lnls-prune-artifacts.sh

ericonr · 2024-08-29T15:59:18Z

base/lnls-prune-artifacts.sh

+        if [[ -d $module && ! $used_modules =~ "$module" ]]; then
+            size=$(du -hs $module | cut -f 1)


Why is -d $module required? Shouldn't they always exist? Or is this covering some case like an AreaDetector or motor submodule being in the list after the super-repository has already been cleaned? I think this should be explained somewhere.

I find =~ somewhat non obvious to parse. And it means "motor" won't be removed if "motorX" is in used modules, right? I think it might make sense to print the list into a file (in /tmp, which can be removed afterwards), so it's split by newlines, and use grep to match the whole line.

Or is this covering some case like an AreaDetector or motor submodule being in the list after the super-repository has already been cleaned? I think this should be explained somewhere.

That's exactly the case. We might have cleaned them up already.

I find =~ somewhat non obvious to parse. And it means "motor" won't be removed if "motorX" is in used modules, right?

Yes, and it can't be removed. I agree the expression is confusing.

I think it might make sense to print the list into a file (in /tmp, which can be removed afterwards), so it's split by newlines, and use grep to match the whole line.

Okay. Let me try it out.

And it means "motor" won't be removed if "motorX" is in used modules

Because of such issues with prefix matching, I moved to checking the each level of the path at a time, with whole line matching instead. This turned out to be handy for the following patches as well.

Still, don't feel like my implementation explores all bash-ims it could.

Remove EPICS modules that do not have a dynamic library linked to any executables in the target IOC. This shrinks the final image size by removing useless artifacts. We introduce to REPONAME the semantics of specifying the path where relevant binaries are, when no-build target is used, so that the proper clean-up can happen in such cases as well. Previously, REPONAME was not used at all. The clean up phase must be executed in the build stage (before the final copy), otherwise the COPY layer would still make the image size big. This implies `no-build` target needs to copy from a pruned version of the base image, which is a new stage. The list of modules to be removed is taken from the RELEASE file, so that it contains a valid mapping of all modules that have been installed. EPICS base is assumed to be always needed, even though some binaries in it can certainly be thrown away given the linkage of the IOC (for instance, PVAccess binaries when the IOC uses only Channel Access). Linkage information is taken from ldd(1), which assumes the inspected binaries are safe to be potentially executed. It is assumed we are using glibc implementation, which provides a straightforward way to query all binaries at once.

Static libraries are not used neither during compile time nor runtime for non-static builds. On the other hand, they take up a large amount of storage: about 312MB for EPICS base and 222MB for modules. Remove them all right before finishing the build stages, avoiding their copy to IOC images. All AR archives are assumed to be static libraries, which should be a good assumption in GNU/Linux. Other static artifacts, such as object files, are correctly removed by the build system clean target and are not handled explictly here for simplification. A lint script for the build system can be implemented in the future if this eventually becomes false.

Shared libraries may be installed either in /opt or /usr/local/lib but may not be used by the target binaries from the IOC image. Remove them during the prune phase before copying both directories to resulting image to shrink the final image size. Both actual versioned binary and its symbolic links are removed to keep the filesystem consistent.

Modules may contain several artifacts, including configuration files, graphical interface files and other repository artifacts that do not need to be in the IOC image. Remove them all except the ones containing EPICS database (`.db` and `.template`) or autosave requirement (`.req`) files, besides shared libraries. Binaries directory (`bin`) is also removed, as only $REPONAME and $RUNDIR should contain target executables, which are filtered out from the list.

EPICS base has rather large configuration files for build, and other repository files, which are not needed in the IOC images. Prune them after building the IOCs, shrinking about 40MB the final image size. Prune is performed with the same script as modules, which discards all executables in `bin` (~15MB), as well as Perl scripts. This should be fine considering that `static-link` target also does not preserve EPICS binaries in the resulting image.

henriquesimoes

In general, I'm starting to think that we should provide a way to explicitly skip a pruning step, possibly for a given target directory.

This would give users some tooling for managing corner cases, without relying on immediate fixes in lnls-prune-artifacts.

henriquesimoes · 2024-10-18T13:40:14Z

images/docker-compose-mca.yml

-        REPONAME: mca
+        REPONAME: epics/modules/mca


To be honest, I don't like very much the idea of reusing REPONAME. It seems awkward to me. I'd rather have them specified by a variable whose name makes sense at all.

I did so because it was easier to test other things first. Let me know what you think.

henriquesimoes · 2024-10-18T13:42:34Z

base/lnls-prune-artifacts.sh

+    for module in $(filter_out_dirs "$unused_modules" "$keep_dirs"); do
+        # if we already removed it because of its top-level repository or
+        # because it is an IOC, move on to the next.
+        [ ! -d $module ] && continue
+
+        size=$(du -hs $module | cut -f 1)
+
+        echo "Removing module '$module' ($size)..."
+        rm -rf $module
+    done


This loop will misbehave if we turn out having modules with empty spaces.

I kept this as a loop with such requirement because we mostly control such paths. Let me know what you think about this.

henriquesimoes · 2024-10-18T13:44:15Z

base/lnls-prune-artifacts.sh

+    while read -r executable; do
+        read -r -N 4 trailer < "$executable"
+
+        # Output only ELF binaries
+        if [ "$trailer" = $'\x7fELF' ]; then
+            echo $executable
+        fi
+    done < <(find $targets -type f -executable)


We must loop this way here because they might be file names with spaces. Indeed, MCA is such a representative of such scenario.

Because of that, I wonder if I haven't assumed to much elsewhere regarding file names.

Also, would this deserve a comment or is this clear enough?

henriquesimoes · 2024-10-18T13:46:54Z

base/lnls-prune-artifacts.sh

+    # Get their full path
+    libs=$(echo "$found"| cut -d'>' -f 2 | cut -d' ' -f 2)


I feel like there is a better way to do this. I though abort grep -o, but it would require a pattern for any library path, including soname handling. I'm not sure if that turns out to be better.

henriquesimoes requested review from ericonr and guirodrigueslima April 16, 2024 18:09

henriquesimoes marked this pull request as draft April 16, 2024 18:10

henriquesimoes changed the title ~~Prune unused artifacts from IOC images~~ Prune unused artifacts from non-static builds Apr 16, 2024

henriquesimoes mentioned this pull request Apr 16, 2024

Opcua IOC #57

Merged

henriquesimoes linked an issue Apr 17, 2024 that may be closed by this pull request

Prune unused modules from dynamic-link #19

Open

henriquesimoes commented Apr 17, 2024

View reviewed changes

ericonr mentioned this pull request Apr 26, 2024

ioc: copy locally installed software from build stages. #55

Open

henriquesimoes mentioned this pull request May 3, 2024

images: add OPCUA image. #61

Merged

henriquesimoes force-pushed the prune branch 2 times, most recently from 4e79a7a to cec36cb Compare June 5, 2024 15:46

ericonr reviewed Aug 29, 2024

View reviewed changes

henriquesimoes force-pushed the prune branch from cec36cb to cfd1f04 Compare October 14, 2024 13:07

henriquesimoes mentioned this pull request Oct 14, 2024

Remove Git repositories from base images #82

Merged

henriquesimoes force-pushed the prune branch 2 times, most recently from 2099c4a to 0356565 Compare October 15, 2024 16:44

henriquesimoes added 5 commits October 17, 2024 18:25

henriquesimoes force-pushed the prune branch from 0356565 to a0fcdf2 Compare October 18, 2024 13:17

henriquesimoes commented Oct 18, 2024

View reviewed changes

henriquesimoes requested a review from ericonr October 18, 2024 13:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prune unused artifacts from non-static builds #59

Prune unused artifacts from non-static builds #59

henriquesimoes commented Apr 16, 2024

henriquesimoes commented Apr 16, 2024

henriquesimoes left a comment

henriquesimoes Apr 17, 2024 •

edited

Loading

henriquesimoes Apr 17, 2024

henriquesimoes Apr 17, 2024

ericonr left a comment

ericonr left a comment

ericonr Aug 29, 2024

henriquesimoes Oct 14, 2024

ericonr Aug 29, 2024

henriquesimoes Oct 14, 2024

ericonr Aug 29, 2024

henriquesimoes Oct 15, 2024

henriquesimoes Oct 18, 2024

ericonr Aug 29, 2024

henriquesimoes Oct 14, 2024

henriquesimoes Oct 18, 2024

henriquesimoes left a comment

henriquesimoes Oct 18, 2024

henriquesimoes Oct 18, 2024

henriquesimoes Oct 18, 2024

henriquesimoes Oct 18, 2024

		if [[ -d $module && ! $used_modules =~ "$module" ]]; then
		size=$(du -hs $module \| cut -f 1)

		# Get their full path
		libs=$(echo "$found"\| cut -d'>' -f 2 \| cut -d' ' -f 2)

		target_paths=$@

Prune unused artifacts from non-static builds #59

Are you sure you want to change the base?

Prune unused artifacts from non-static builds #59

Conversation

henriquesimoes commented Apr 16, 2024

henriquesimoes commented Apr 16, 2024

henriquesimoes left a comment

Choose a reason for hiding this comment

henriquesimoes Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericonr left a comment

Choose a reason for hiding this comment

ericonr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henriquesimoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henriquesimoes Apr 17, 2024 •

edited

Loading