Skip to content
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.

Sign up for GitHub

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

Thin repositories #798

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

Thin repositories #798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ee0300b
bulk: support thin repository
bapt Nov 4, 2020
68b3911
bulk: implement small_repo
bapt Nov 4, 2020
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
10 changes: 7 additions & 3 deletions src/bin/poudriere-bulk.8
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
.Cm bulk
.Fl a
.Fl j Ar name
.Op Fl CcFIikNnRrSTtvw
.Op Fl CcFIikmNnRrSTtvw
.Op Fl b Ar name
.Op Fl B Ar name
.Op Fl J Ar maxjobs Ns Op Cm \&: Ns Ar prebuildmaxjobs
Expand All @@ -50,7 +50,7 @@
.Cm bulk
.Fl f Ar file Op Oo Fl f Ar file2 Oc Ar ...
.Fl j Ar name
.Op Fl CcFIikNnRrSTtvw
.Op Fl CcFIikmNnRrSTtvw
.Op Fl b Ar name
.Op Fl B Ar name
.Op Fl J Ar maxjobs Ns Op Cm \&: Ns Ar prebuildmaxjobs
Expand All @@ -60,7 +60,7 @@
.Nm poudriere
.Cm bulk
.Fl j Ar name
.Op Fl CcFIikNnRrSTtvw
.Op Fl CcFIikmNnRrSTtvw
.Op Fl b Ar name
.Op Fl B Ar name
.Op Fl J Ar maxjobs Ns Op Cm \&: Ns Ar prebuildmaxjobs
Expand Down Expand Up @@ -216,6 +216,10 @@ when using
Do not skip dependent ports on findings.
This flag is automatically set when using
.Fl at .
.It Fl m
Build a minimal repository or thin repository which only contains the packages
that has been listed to be built, note that this option is incompatible with
Copy link
Member

Choose a reason for hiding this comment

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

s/has/have/
s/built, note/built.\nNote/

.Fl a .
.It Fl N
Do not build a package repository when the build is completed.
.It Fl n
Expand Down
21 changes: 20 additions & 1 deletion src/share/poudriere/bulk.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ Options:
-j name -- Run only on the given jail
-k -- When doing testing with -t, don't consider failures as
fatal; don't skip dependent ports on findings.
-m -- minimal repository, only create a repository with the listed
packages, incompatible with -a.
-M -- medium repository, only create a repository with the listed
packages and their runtime dependencies, incompatible with -a.
Comment on lines +58 to +61
Copy link
Member

Choose a reason for hiding this comment

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

My default reaction to new flags in PRs is to suggest or ask if this could be a hook instead. For this particular feature it seems like it could be entirely a hook in pkgclean. We could even provide this as an example hook.

-N -- Do not build package repository when build completed
-n -- Dry-run. Show what will be done, but do not build
any packages.
Expand Down Expand Up @@ -88,14 +92,16 @@ CLEAN=0
CLEAN_LISTED=0
DRY_RUN=0
ALL=0
THIN_REPO=0
SMALL_REPO=0
BUILD_REPO=1
INTERACTIVE_MODE=0
OVERLAYS=""
. ${SCRIPTPREFIX}/common.sh

[ $# -eq 0 ] && usage

while getopts "ab:B:CcFf:iIj:J:knNO:p:RrSTtvwz:" FLAG; do
while getopts "ab:B:CcFf:iIj:J:kmnMNO:p:RrSTtvwz:" FLAG; do
case "${FLAG}" in
a)
ALL=1
Expand Down Expand Up @@ -140,6 +146,13 @@ while getopts "ab:B:CcFf:iIj:J:knNO:p:RrSTtvwz:" FLAG; do
k)
PORTTESTING_FATAL=no
;;
M)
SMALL_REPO=1
THIN_REPO=1
;;
m)
THIN_REPO=1
;;
N)
BUILD_REPO=0
;;
Expand Down Expand Up @@ -193,6 +206,12 @@ while getopts "ab:B:CcFf:iIj:J:knNO:p:RrSTtvwz:" FLAG; do
esac
done

if [ ${ALL} -eq 1 -a ${SMALL_REPO} -eq 1 ]; then
err 1 "incompatible options: both -a and -M are provided"
fi
if [ ${ALL} -eq 1 -a ${THIN_REPO} -eq 1 ]; then
err 1 "incompatible options: both -a and -m are provided"
fi
if [ ${ALL} -eq 1 -a ${CLEAN_LISTED} -eq 1 ]; then
CLEAN=1
CLEAN_LISTED=0
Expand Down
74 changes: 67 additions & 7 deletions src/share/poudriere/common.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2647,6 +2647,12 @@ jail_start() {
# do_portbuild_mounts depends on PACKAGES being set.
# May already be set for pkgclean
: ${PACKAGES:=${POUDRIERE_DATA:?}/packages/${MASTERNAME}}
if [ ${SMALL_REPO} -eq 1 ]; then
THIN_EXT=-small
Copy link
Contributor

Choose a reason for hiding this comment

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

It's confusing that the options are minimal and medium, but the extensions are -thin and -small. At a minimum the documentation should mention this. Ideally things should lineup throughout.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes but minimal and medium are bad names, think and small are nice, but we can't use proper switches for them, I am open to any suggestion for renaming that

Copy link
Member

Choose a reason for hiding this comment

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

I think we should spell out what these are rather than "thin" or "small". Too bad -f/-F is taken as -f[ilter] makes sense to me.

-m all # default
-m listed
-m recursive / runtime # This only excludes build dependencies right?

listed/recursive/runtime don't make me(user) think about what "small" means.
There's also terms like 'leaf' and 'automatic' that describe these.

p.s. I cringe every time we add any new flag as it complicates the interface and makes it harder to clean up the interface later.

else
THIN_EXT=-thin
fi
: ${THIN_PACKAGES:=${POUDRIERE_DATA:?}/packages/${MASTERNAME}${THIN_EXT}}
mkdir -p "${PACKAGES:?}/"
was_a_bulk_run && stash_packages
do_portbuild_mounts ${tomnt} ${name} ${ptname} ${setname}
Expand Down Expand Up @@ -7809,15 +7815,69 @@ sign_pkg() {
fi
}

add_pkg_to_repo() {
Copy link
Member

Choose a reason for hiding this comment

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

needs quotes around pathed vars

local pkgname=$1
local target=$2
Copy link
Member

Choose a reason for hiding this comment

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

local dep

[ -f ${target}/${pkgname}.${PKG_EXT} ] && return
if [ ${SMALL_REPO} -eq 1 ]; then
for dep in $(injail ${PKG_BIN} info -qd -F /packages/All/${pkgname}.${PKG_EXT}); do
Copy link
Member

Choose a reason for hiding this comment

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

We have this information in some metadata files that could avoid the need to use pkg here. Related to that I think this functionality makes sense in pkgclean which generates the same metadata. Moving towards that method could wait until later though as I think it brings some risk/complexity to the build itself and I have some pending stuff to push around it all.

add_pkg_to_repo ${dep} ${target}
done
fi
cp ${PACKAGES}/All/${pkgname}.${PKG_EXT} ${target}/
}

build_thin_repo() {
# Try to be as atomic as possible in recreating the new thin repo
Copy link
Member

Choose a reason for hiding this comment

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

I've aggregated some comments for future lines here.

I don't think it's important to be atomic here as there is already a mechanism for it on by default. ATOMIC_PACKAGE_REPOSITORY. The directory being acted on here shouldn't be the real repository in that case.

# ls -al /poudriere/data/packages/exp-11amd64-commit-test
total 88
drwxr-xr-x   7 root  wheel  16 May 13 15:01 ./
drwxr-x--x  38 root  wheel  43 Jun  4 14:58 ../
lrwxr-xr-x   1 root  wheel  18 Oct 13  2017 .buildname@ -> .latest/.buildname
lrwxr-xr-x   1 root  wheel  20 Oct 13  2017 .jailversion@ -> .latest/.jailversion
lrwxr-xr-x   1 root  wheel  16 May 13 15:01 .latest@ -> .real_1589407304
drwxr-xr-x   4 root  wheel  10 Apr 22  2020 .real_1587540631/
drwxr-xr-x   4 root  wheel  10 Apr 25  2020 .real_1587845826/
drwxr-xr-x   4 root  wheel  10 May  4  2020 .real_1588616190/
drwxr-xr-x   4 root  wheel  10 May 13 14:58 .real_1589407113/
drwxr-xr-x   4 root  wheel  10 May 13 15:01 .real_1589407304/
lrwxr-xr-x   1 root  wheel  11 Mar  7  2017 All@ -> .latest/All
lrwxr-xr-x   1 root  wheel  14 Mar  7  2017 Latest@ -> .latest/Latest
lrwxr-xr-x   1 root  wheel  19 Mar  7  2017 digests.txz@ -> .latest/digests.txz
lrwxr-xr-x   1 root  wheel  17 Mar 23  2020 meta.conf@ -> .latest/meta.conf
lrwxr-xr-x   1 root  wheel  16 Mar  7  2017 meta.txz@ -> .latest/meta.txz
lrwxr-xr-x   1 root  wheel  23 Mar  7  2017 packagesite.txz@ -> .latest/packagesite.txz

There's not much reason to disable that feature. We disabled on the cluster systems because pkgsync expects a certain hardlink count. That could be reworked.

The way it works is, for example, bulk currently copies (cp -al hardlinks) the .latest into a .building. Then runs build_repo against that directory. Then later in bulk.sh commit_packages is ran which flips everything around so .building becomes the new real/latest directory.

Lots of ways to go about this but say for this feature we keep the All -> .latest symlinks all alone. Add a /thin or /small dir that symlinks into a .real_TS_filtered directory. Then on client systems you add /thin to the pkg repo name. Generating the directory would just be copying (hardlinks) the .building directory to .filter and removing unexpected packages (which sounds like pkgclean which is why I think most of this should be there).

git grep -F .building has some of the relevant places.

mkdir -p ${THIN_PACKAGES}/All.new
Copy link
Member

Choose a reason for hiding this comment

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

What if this directory is leftover from a previous build? Should rm -rf here probably. But as noted earlier I don't think we need to do this .new and .old thing anyhow.

while mapfile_read_loop "all_pkgs" \
_pkgname _originspec _rdep _ignore; do
if [ "${_rdep}" = "listed" ] ; then
add_pkg_to_repo ${_pkgname} \
${THIN_PACKAGES}/All.new
fi
done
Comment on lines +7833 to +7839
Copy link
Member

Choose a reason for hiding this comment

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

for pkgname in $(listed_pkgnames); do
    add_pkg_to_repo "${pkgname}" "${THIN_PACKAGES}/All.new"

if [ ${SMALL_REPO} -eq 1 ]; then
cp ${PACKAGES}/All/pkg-*.txz ${THIN_PACKAGES}/All.new
fi
if [ -d "${THIN_PACKAGES}/Latest" ]; then
rm -rf "${THIN_PACKAGES}/Latest"
if [ ${SMALL_REPO} -eq 1 ]; then
mkdir ${THIN_PACKAGES}/Latest
cp -RP ${PACKAGES}/Latest/pkg.${PKG_EXT} ${THIN_PACKAGES}/Latest
fi
fi
if [ -d "${THIN_PACKAGES}/All" ]; then
mv ${THIN_PACKAGES}/All ${THIN_PACKAGES}/All.old
fi
mv ${THIN_PACKAGES}/All.new ${THIN_PACKAGES}/All
if [ -d "${THIN_PACKAGES}/All" ]; then
rm -rf ${THIN_PACKAGES}/All.old
fi
Comment on lines +7850 to +7856
Copy link
Member

Choose a reason for hiding this comment

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

As noted earlier I don't think this careful swap is needed, or if it is we should use a symlink anyway.

}

build_repo() {
local origin
local origin packages

msg "Creating pkg repository"
if [ ${THIN_REPO} -eq 1 ]; then
msg "Creating thin pkg repository"
packages=${THIN_PACKAGES}
else
msg "Creating pkg repository"
packages=${PACKAGES}
fi
Comment on lines +7862 to +7868
Copy link
Member

Choose a reason for hiding this comment

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

This confuses me as a user. If I do a --thin build it will update packages in both directories but only the 'thin' one has its pkg files updated. There's no clear path to resolve that or to filter an existing repository down to the small/thing (hint: pkgclean).

[ ${DRY_RUN} -eq 1 ] && return 0
bset status "pkgrepo:"
ensure_pkg_installed force_extract || \
err 1 "Unable to extract pkg."
run_hook pkgrepo sign "${PACKAGES}" "${PKG_REPO_SIGNING_KEY}" \
if [ ${THIN_REPO} -eq 1 ]; then
build_thin_repo
# only overwrite the packages repo with the thin one
# after having extracted pkg because pkg might not
# be on the thin repo
PACKAGES=${THIN_PACKAGES} mount_packages -o ro
fi
run_hook pkgrepo sign "${packages}" "${PKG_REPO_SIGNING_KEY}" \
"${PKG_REPO_FROM_HOST:-no}" "${PKG_REPO_META_FILE}"
if [ -r "${PKG_REPO_META_FILE:-/nonexistent}" ]; then
PKG_META="-m /tmp/pkgmeta"
Expand Down Expand Up @@ -7846,14 +7906,14 @@ build_repo() {
-o /tmp/packages ${PKG_META} /packages \
${SIGNING_COMMAND:+signing_command: ${SIGNING_COMMAND}}
fi
cp ${MASTERMNT}/tmp/packages/* ${PACKAGES}/
cp ${MASTERMNT}/tmp/packages/* ${packages}/

# Sign the ports-mgmt/pkg package for bootstrap
if [ -e "${PACKAGES}/Latest/pkg.${PKG_EXT}" ]; then
if [ -e "${packages}/Latest/pkg.${PKG_EXT}" ]; then
if [ -n "${SIGNING_COMMAND}" ]; then
sign_pkg fingerprint "${PACKAGES}/Latest/pkg.${PKG_EXT}"
sign_pkg fingerprint "${packages}/Latest/pkg.${PKG_EXT}"
elif [ -n "${PKG_REPO_SIGNING_KEY}" ]; then
sign_pkg pubkey "${PACKAGES}/Latest/pkg.${PKG_EXT}"
sign_pkg pubkey "${packages}/Latest/pkg.${PKG_EXT}"
fi
fi
}
Expand Down