-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prune unused artifacts from non-static builds #59
base: main
Are you sure you want to change the base?
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
clean_up_epics_modules $target_paths | ||
remove_static_libs /opt | ||
remove_unused_shared_libs $target_paths |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
4e79a7a
to
cec36cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first two commits are correct and can be merged separately, if you'd like to split them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
base/lnls-prune-artifacts.sh
Outdated
target_paths=$@ | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
targets=$@ | ||
|
||
executables=$(find $targets -type f -executable -print) | ||
libs="$(ldd $executables 2>/dev/null)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
for lib in $epics_libs; do | ||
module=${lib%/lib/*/*lib*so} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know what you think about the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
if [[ -d $module && ! $used_modules =~ "$module" ]]; then | ||
size=$(du -hs $module | cut -f 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
2099c4a
to
0356565
Compare
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
REPONAME: mca | ||
REPONAME: epics/modules/mca |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
# Get their full path | ||
libs=$(echo "$found"| cut -d'>' -f 2 | cut -d' ' -f 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Both
dynamic-link
andno-build
targets generate unnecessarily large IOC images due to the trivialCOPY
done from/opt
. Clean up everything from there and/usr/local
directory before doing theCOPY
to the IOC image.Clean up strategy proposed here is the following:
/opt/$REPONAME
or$RUNDIR
);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.